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

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

[Yousef Alhouseen]

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.

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@gmail.com>
---
 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

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@gmail.com>
> ---
>  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
> 

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

[Yousef Alhouseen]

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"
<christian.koenig@amd.com> 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 <alhouseenyousef@gmail.com>
> > ---
> > 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
> >

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

[David Laight]

On Wed, 24 Jun 2026 14:58:58 +0200
Christian König <christian.koenig@amd.com> 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 <alhouseenyousef@gmail.com>
> > ---
> >  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
> >   
> 
> 

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

[Christian König]

On 6/26/26 13:31, David Laight wrote:
> On Wed, 24 Jun 2026 14:58:58 +0200
> Christian König <christian.koenig@amd.com> 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?

Do we need to? I mean shooting into your own foot is supposed to hurt.

Christian.

> 
> 	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 <alhouseenyousef@gmail.com>
>>> ---
>>>  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
>>>   
>>
>>
>