Re: [PATCH] dma-buf: udmabuf: avoid list copy size overflow

[Christian König]

On 6/24/26 14:52, Yousef Alhouseen wrote:
> UDMABUF_CREATE_LIST copies an array whose element count comes from
> userspace. The count is compared against list_limit, but list_limit is a
> signed module parameter while the count is u32.

We should probably just drop the sign from the module parameter instead.

I don't see an use case for negative values here.

Regards,
Christian.

> 
> If the limit is raised too far or made negative, that comparison no
> longer bounds the count to a range where sizeof(*list) * count fits in
> the u32 temporary used for the copy length. A wrapped copy length lets
> memdup_user() copy fewer entries than udmabuf_create() subsequently
> walks, leading to out-of-bounds reads from the copied list.
> 
> Take a positive snapshot of the module limit and use memdup_array_user()
> so the multiplication is checked before copying.
> 
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@xxxxxxxxx>
> ---
>  drivers/dma-buf/udmabuf.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index bced421c0..b4078ec84 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -469,14 +469,15 @@ static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg)
>         struct udmabuf_create_list head;
>         struct udmabuf_create_item *list;
>         int ret = -EINVAL;
> -       u32 lsize;
> +       int limit;
> 
>         if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
>                 return -EFAULT;
> -       if (head.count > list_limit)
> +       limit = READ_ONCE(list_limit);
> +       if (!head.count || limit <= 0 || head.count > limit)
>                 return -EINVAL;
> -       lsize = sizeof(struct udmabuf_create_item) * head.count;
> -       list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
> +       list = memdup_array_user((void __user *)(arg + sizeof(head)),
> +                                head.count, sizeof(*list));
>         if (IS_ERR(list))
>                 return PTR_ERR(list);
> 
> --
> 2.54.0
>