| Message ID | 3e37f05c7593f1016f0a46de188b3357cbbd0c0b.1695060389.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | udmabuf: Fix a potential (and unlikely) access to unallocated memory | expand |
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
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 >
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
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);
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(-)