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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7043f179-b670-db3c-3ab0-a1f3e991add9@embeddedor.com>
Date:   Sun, 17 Sep 2023 21:10:05 -0600
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Gerd Hoffmann <kraxel@...hat.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian König <christian.koenig@....com>,
        Daniel Vetter <daniel.vetter@...ll.ch>
Cc:     linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH] udmabuf: Fix a potential (and unlikely) access to
 unallocated memory



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@...adoo.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