Re: [PATCH] udmabuf: Fix a potential (and unlikely) access to unallocated memory

Re: [PATCH] udmabuf: Fix a potential (and unlikely) access to unallocated memory

[Gustavo A. R. Silva]

On 9/18/23 12:46, Christophe JAILLET wrote:
> If 'list_limit' is set to a very high value, 'lsize' computation could
> overflow if 'head.count' is big enough.
> 
> In such a case, udmabuf_create() will access to memory beyond 'list'.
> 
> Use size_mul() to saturate the value, and have memdup_user() fail.
> 
> Fixes: fbb0de795078 ("Add udmabuf misc device")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/dma-buf/udmabuf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index c40645999648..fb4c4b5b3332 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -314,13 +314,13 @@ 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;
> +	size_t lsize;
>   
>   	if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
>   		return -EFAULT;
>   	if (head.count > list_limit)
>   		return -EINVAL;
> -	lsize = sizeof(struct udmabuf_create_item) * head.count;
> +	lsize = size_mul(sizeof(struct udmabuf_create_item), head.count);
>   	list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
>   	if (IS_ERR(list))
>   		return PTR_ERR(list);

How about this, and we get rid of `lsize`:

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index c40645999648..5cf9d849aaa8 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -314,14 +314,13 @@ 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;

         if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
                 return -EFAULT;
         if (head.count > list_limit)
                 return -EINVAL;
-       lsize = sizeof(struct udmabuf_create_item) * head.count;
-       list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
+       list = memdup_user((void __user *)(arg + sizeof(head)),
+                          size_mul(sizeof(*list), head.count));
         if (IS_ERR(list))
                 return PTR_ERR(list);


--
Gustavo

[PATCH] udmabuf: Fix a potential (and unlikely) access to unallocated memory

[Christophe JAILLET]

If 'list_limit' is set to a very high value, 'lsize' computation could
overflow if 'head.count' is big enough.

In such a case, udmabuf_create() will access to memory beyond 'list'.

Use size_mul() to saturate the value, and have memdup_user() fail.

Fixes: fbb0de795078 ("Add udmabuf misc device")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/dma-buf/udmabuf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index c40645999648..fb4c4b5b3332 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -314,13 +314,13 @@ 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;
+	size_t lsize;
 
 	if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
 		return -EFAULT;
 	if (head.count > list_limit)
 		return -EINVAL;
-	lsize = sizeof(struct udmabuf_create_item) * head.count;
+	lsize = size_mul(sizeof(struct udmabuf_create_item), head.count);
 	list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
 	if (IS_ERR(list))
 		return PTR_ERR(list);
-- 
2.34.1

Re: [PATCH] udmabuf: Fix a potential (and unlikely) access to unallocated memory

[Christophe JAILLET]

Le 18/09/2023 à 05:10, Gustavo A. R. Silva a écrit :
> 
> 
> On 9/18/23 12:46, Christophe JAILLET wrote:
>> If 'list_limit' is set to a very high value, 'lsize' computation could
>> overflow if 'head.count' is big enough.
>>
>> In such a case, udmabuf_create() will access to memory beyond 'list'.
>>
>> Use size_mul() to saturate the value, and have memdup_user() fail.
>>
>> Fixes: fbb0de795078 ("Add udmabuf misc device")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/dma-buf/udmabuf.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
>> index c40645999648..fb4c4b5b3332 100644
>> --- a/drivers/dma-buf/udmabuf.c
>> +++ b/drivers/dma-buf/udmabuf.c
>> @@ -314,13 +314,13 @@ 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;
>> +    size_t lsize;
>>       if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
>>           return -EFAULT;
>>       if (head.count > list_limit)
>>           return -EINVAL;
>> -    lsize = sizeof(struct udmabuf_create_item) * head.count;
>> +    lsize = size_mul(sizeof(struct udmabuf_create_item), head.count);
>>       list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
>>       if (IS_ERR(list))
>>           return PTR_ERR(list);
> 
> How about this, and we get rid of `lsize`:

Keeping or removing lsize is mostly a matter of taste, I think.

Using sizeof(*list) is better.

Let see if there are some other comments, and I'll send a v2.

Thanks for the feed-back.

CJ

> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index c40645999648..5cf9d849aaa8 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -314,14 +314,13 @@ 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;
> 
>          if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
>                  return -EFAULT;
>          if (head.count > list_limit)
>                  return -EINVAL;
> -       lsize = sizeof(struct udmabuf_create_item) * head.count;
> -       list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
> +       list = memdup_user((void __user *)(arg + sizeof(head)),
> +                          size_mul(sizeof(*list), head.count));
>          if (IS_ERR(list))
>                  return PTR_ERR(list);
> 
> 
> -- 
> Gustavo
> 

Re: [PATCH] udmabuf: Fix a potential (and unlikely) access to unallocated memory

[Kees Cook]

On Mon, Sep 18, 2023 at 09:22:44PM +0200, Christophe JAILLET wrote:
> Le 18/09/2023 à 05:10, Gustavo A. R. Silva a écrit :
> > 
> > 
> > On 9/18/23 12:46, Christophe JAILLET wrote:
> > > If 'list_limit' is set to a very high value, 'lsize' computation could
> > > overflow if 'head.count' is big enough.
> > > 
> > > In such a case, udmabuf_create() will access to memory beyond 'list'.
> > > 
> > > Use size_mul() to saturate the value, and have memdup_user() fail.
> > > 
> > > Fixes: fbb0de795078 ("Add udmabuf misc device")
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > >   drivers/dma-buf/udmabuf.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> > > index c40645999648..fb4c4b5b3332 100644
> > > --- a/drivers/dma-buf/udmabuf.c
> > > +++ b/drivers/dma-buf/udmabuf.c
> > > @@ -314,13 +314,13 @@ 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;
> > > +    size_t lsize;
> > >       if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
> > >           return -EFAULT;
> > >       if (head.count > list_limit)
> > >           return -EINVAL;
> > > -    lsize = sizeof(struct udmabuf_create_item) * head.count;
> > > +    lsize = size_mul(sizeof(struct udmabuf_create_item), head.count);
> > >       list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
> > >       if (IS_ERR(list))
> > >           return PTR_ERR(list);
> > 
> > How about this, and we get rid of `lsize`:
> 
> Keeping or removing lsize is mostly a matter of taste, I think.

I'm on the fence, but kind of lean towards keeping lsize, but I think
it's fine either way.

> Using sizeof(*list) is better.

That I agree with, yes.

> Let see if there are some other comments, and I'll send a v2.

I note that this looks like a use-case for the very recently proposed
memdup_array_user():
https://lore.kernel.org/all/ACD75DAA-AF42-486C-B44B-9272EF302E3D@kernel.org/

(i.e. a built-in size_mul)

-Kees

-- 
Kees Cook