On Wed, 24 Jun 2026 14:58:58 +0200
Christian König <[email protected]> wrote:
> 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.
Does anything sanity-check the module parameter?
David
>
> 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 <[email protected]>
> > ---
> > 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
> >
>
>
