Hi Christian, Agreed. I sent a follow-up that makes list_limit unsigned and keeps the checked array copy path.
Thanks, Yousef 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. > > 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 > >
