* [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy
@ 2020-12-20 11:06 Sakari Ailus
2020-12-20 11:09 ` Hans Verkuil
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Sakari Ailus @ 2020-12-20 11:06 UTC (permalink / raw)
To: linux-media
Cc: Arnd Bergmann, syzbot, Arnd Bergmann, Hans Verkuil,
Laurent Pinchart, linux-kernel, Mauro Carvalho Chehab,
syzkaller-bugs
When an IOCTL with argument size larger than 128 that also used array
arguments were handled, two memory allocations were made but alas, only
the latter one of them was released. This happened because there was only
a single local variable to hold such a temporary allocation.
Fix this by adding separate variables to hold the pointers to the
temporary allocations.
Reported-by: Arnd Bergmann <arnd@kernel.org>
Reported-by: syzbot+1115e79c8df6472c612b@syzkaller.appspotmail.com
Fixes: d14e6d76ebf7 ("[media] v4l: Add multi-planar ioctl handling code")
Cc: stable@vger.kernel.org
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-ioctl.c | 31 +++++++++++++---------------
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 3198abdd538ce..f42a779948779 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3283,7 +3283,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
v4l2_kioctl func)
{
char sbuf[128];
- void *mbuf = NULL;
+ void *mbuf = NULL, *array_buf = NULL;
void *parg = (void *)arg;
long err = -EINVAL;
bool has_array_args;
@@ -3318,27 +3318,21 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
has_array_args = err;
if (has_array_args) {
- /*
- * When adding new types of array args, make sure that the
- * parent argument to ioctl (which contains the pointer to the
- * array) fits into sbuf (so that mbuf will still remain
- * unused up to here).
- */
- mbuf = kvmalloc(array_size, GFP_KERNEL);
+ array_buf = kvmalloc(array_size, GFP_KERNEL);
err = -ENOMEM;
- if (NULL == mbuf)
+ if (array_buf == NULL)
goto out_array_args;
err = -EFAULT;
if (in_compat_syscall())
- err = v4l2_compat_get_array_args(file, mbuf, user_ptr,
- array_size, orig_cmd,
- parg);
+ err = v4l2_compat_get_array_args(file, array_buf,
+ user_ptr, array_size,
+ orig_cmd, parg);
else
- err = copy_from_user(mbuf, user_ptr, array_size) ?
+ err = copy_from_user(array_buf, user_ptr, array_size) ?
-EFAULT : 0;
if (err)
goto out_array_args;
- *kernel_ptr = mbuf;
+ *kernel_ptr = array_buf;
}
/* Handles IOCTL */
@@ -3360,12 +3354,14 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
if (in_compat_syscall()) {
int put_err;
- put_err = v4l2_compat_put_array_args(file, user_ptr, mbuf,
- array_size, orig_cmd,
+ put_err = v4l2_compat_put_array_args(file, user_ptr,
+ array_buf,
+ array_size,
+ orig_cmd,
parg);
if (put_err)
err = put_err;
- } else if (copy_to_user(user_ptr, mbuf, array_size)) {
+ } else if (copy_to_user(user_ptr, array_buf, array_size)) {
err = -EFAULT;
}
goto out_array_args;
@@ -3381,6 +3377,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
err = -EFAULT;
out:
+ kvfree(array_buf);
kvfree(mbuf);
return err;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy
2020-12-20 11:06 [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy Sakari Ailus
@ 2020-12-20 11:09 ` Hans Verkuil
2020-12-20 11:36 ` Arnd Bergmann
2020-12-20 14:15 ` Laurent Pinchart
2 siblings, 0 replies; 6+ messages in thread
From: Hans Verkuil @ 2020-12-20 11:09 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: Arnd Bergmann, syzbot, Arnd Bergmann, Laurent Pinchart,
linux-kernel, Mauro Carvalho Chehab, syzkaller-bugs
On 20/12/2020 12:06, Sakari Ailus wrote:
> When an IOCTL with argument size larger than 128 that also used array
> arguments were handled, two memory allocations were made but alas, only
> the latter one of them was released. This happened because there was only
> a single local variable to hold such a temporary allocation.
>
> Fix this by adding separate variables to hold the pointers to the
> temporary allocations.
>
> Reported-by: Arnd Bergmann <arnd@kernel.org>
> Reported-by: syzbot+1115e79c8df6472c612b@syzkaller.appspotmail.com
> Fixes: d14e6d76ebf7 ("[media] v4l: Add multi-planar ioctl handling code")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Regards,
Hans
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 31 +++++++++++++---------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 3198abdd538ce..f42a779948779 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3283,7 +3283,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> v4l2_kioctl func)
> {
> char sbuf[128];
> - void *mbuf = NULL;
> + void *mbuf = NULL, *array_buf = NULL;
> void *parg = (void *)arg;
> long err = -EINVAL;
> bool has_array_args;
> @@ -3318,27 +3318,21 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> has_array_args = err;
>
> if (has_array_args) {
> - /*
> - * When adding new types of array args, make sure that the
> - * parent argument to ioctl (which contains the pointer to the
> - * array) fits into sbuf (so that mbuf will still remain
> - * unused up to here).
> - */
> - mbuf = kvmalloc(array_size, GFP_KERNEL);
> + array_buf = kvmalloc(array_size, GFP_KERNEL);
> err = -ENOMEM;
> - if (NULL == mbuf)
> + if (array_buf == NULL)
> goto out_array_args;
> err = -EFAULT;
> if (in_compat_syscall())
> - err = v4l2_compat_get_array_args(file, mbuf, user_ptr,
> - array_size, orig_cmd,
> - parg);
> + err = v4l2_compat_get_array_args(file, array_buf,
> + user_ptr, array_size,
> + orig_cmd, parg);
> else
> - err = copy_from_user(mbuf, user_ptr, array_size) ?
> + err = copy_from_user(array_buf, user_ptr, array_size) ?
> -EFAULT : 0;
> if (err)
> goto out_array_args;
> - *kernel_ptr = mbuf;
> + *kernel_ptr = array_buf;
> }
>
> /* Handles IOCTL */
> @@ -3360,12 +3354,14 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> if (in_compat_syscall()) {
> int put_err;
>
> - put_err = v4l2_compat_put_array_args(file, user_ptr, mbuf,
> - array_size, orig_cmd,
> + put_err = v4l2_compat_put_array_args(file, user_ptr,
> + array_buf,
> + array_size,
> + orig_cmd,
> parg);
> if (put_err)
> err = put_err;
> - } else if (copy_to_user(user_ptr, mbuf, array_size)) {
> + } else if (copy_to_user(user_ptr, array_buf, array_size)) {
> err = -EFAULT;
> }
> goto out_array_args;
> @@ -3381,6 +3377,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
> err = -EFAULT;
> out:
> + kvfree(array_buf);
> kvfree(mbuf);
> return err;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy
2020-12-20 11:06 [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy Sakari Ailus
2020-12-20 11:09 ` Hans Verkuil
@ 2020-12-20 11:36 ` Arnd Bergmann
2020-12-20 11:51 ` Sakari Ailus
2020-12-20 14:15 ` Laurent Pinchart
2 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2020-12-20 11:36 UTC (permalink / raw)
To: Sakari Ailus
Cc: Linux Media Mailing List, syzbot, Arnd Bergmann, Hans Verkuil,
Laurent Pinchart, linux-kernel, Mauro Carvalho Chehab,
syzkaller-bugs
On Sun, Dec 20, 2020 at 12:06 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> When an IOCTL with argument size larger than 128 that also used array
> arguments were handled, two memory allocations were made but alas, only
> the latter one of them was released. This happened because there was only
> a single local variable to hold such a temporary allocation.
>
> Fix this by adding separate variables to hold the pointers to the
> temporary allocations.
>
> Reported-by: Arnd Bergmann <arnd@kernel.org>
> Reported-by: syzbot+1115e79c8df6472c612b@syzkaller.appspotmail.com
> Fixes: d14e6d76ebf7 ("[media] v4l: Add multi-planar ioctl handling code")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
> out:
> + kvfree(array_buf);
> kvfree(mbuf);
I think it would make sense to change mbuf back to kzalloc()/kfree
after this, since the size of the ioctl argument has an upper bound
of 2^_IOC_SIZEBITS (16KB), which does not need the vmalloc
path, unlike the array args.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy
2020-12-20 11:36 ` Arnd Bergmann
@ 2020-12-20 11:51 ` Sakari Ailus
0 siblings, 0 replies; 6+ messages in thread
From: Sakari Ailus @ 2020-12-20 11:51 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linux Media Mailing List, syzbot, Arnd Bergmann, Hans Verkuil,
Laurent Pinchart, linux-kernel, Mauro Carvalho Chehab,
syzkaller-bugs
On Sun, Dec 20, 2020 at 12:36:23PM +0100, Arnd Bergmann wrote:
> On Sun, Dec 20, 2020 at 12:06 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > When an IOCTL with argument size larger than 128 that also used array
> > arguments were handled, two memory allocations were made but alas, only
> > the latter one of them was released. This happened because there was only
> > a single local variable to hold such a temporary allocation.
> >
> > Fix this by adding separate variables to hold the pointers to the
> > temporary allocations.
> >
> > Reported-by: Arnd Bergmann <arnd@kernel.org>
> > Reported-by: syzbot+1115e79c8df6472c612b@syzkaller.appspotmail.com
> > Fixes: d14e6d76ebf7 ("[media] v4l: Add multi-planar ioctl handling code")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
Thanks!
>
> > out:
> > + kvfree(array_buf);
> > kvfree(mbuf);
>
> I think it would make sense to change mbuf back to kzalloc()/kfree
> after this, since the size of the ioctl argument has an upper bound
> of 2^_IOC_SIZEBITS (16KB), which does not need the vmalloc
> path, unlike the array args.
Good point. I can send a patch for that, too.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy
2020-12-20 11:06 [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy Sakari Ailus
2020-12-20 11:09 ` Hans Verkuil
2020-12-20 11:36 ` Arnd Bergmann
@ 2020-12-20 14:15 ` Laurent Pinchart
2020-12-20 20:03 ` Sakari Ailus
2 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2020-12-20 14:15 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Arnd Bergmann, syzbot, Arnd Bergmann, Hans Verkuil,
linux-kernel, Mauro Carvalho Chehab, syzkaller-bugs
Hi Sakari,
Thank you for the patch.
On Sun, Dec 20, 2020 at 01:06:51PM +0200, Sakari Ailus wrote:
> When an IOCTL with argument size larger than 128 that also used array
> arguments were handled, two memory allocations were made but alas, only
> the latter one of them was released.
Alas, this fills my heart with sorrow indeed :-)
> This happened because there was only
> a single local variable to hold such a temporary allocation.
>
> Fix this by adding separate variables to hold the pointers to the
> temporary allocations.
>
> Reported-by: Arnd Bergmann <arnd@kernel.org>
> Reported-by: syzbot+1115e79c8df6472c612b@syzkaller.appspotmail.com
> Fixes: d14e6d76ebf7 ("[media] v4l: Add multi-planar ioctl handling code")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 31 +++++++++++++---------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 3198abdd538ce..f42a779948779 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3283,7 +3283,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> v4l2_kioctl func)
> {
> char sbuf[128];
> - void *mbuf = NULL;
> + void *mbuf = NULL, *array_buf = NULL;
> void *parg = (void *)arg;
> long err = -EINVAL;
> bool has_array_args;
> @@ -3318,27 +3318,21 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> has_array_args = err;
>
> if (has_array_args) {
> - /*
> - * When adding new types of array args, make sure that the
> - * parent argument to ioctl (which contains the pointer to the
> - * array) fits into sbuf (so that mbuf will still remain
> - * unused up to here).
> - */
> - mbuf = kvmalloc(array_size, GFP_KERNEL);
> + array_buf = kvmalloc(array_size, GFP_KERNEL);
> err = -ENOMEM;
> - if (NULL == mbuf)
> + if (array_buf == NULL)
> goto out_array_args;
> err = -EFAULT;
> if (in_compat_syscall())
> - err = v4l2_compat_get_array_args(file, mbuf, user_ptr,
> - array_size, orig_cmd,
> - parg);
> + err = v4l2_compat_get_array_args(file, array_buf,
> + user_ptr, array_size,
> + orig_cmd, parg);
> else
> - err = copy_from_user(mbuf, user_ptr, array_size) ?
> + err = copy_from_user(array_buf, user_ptr, array_size) ?
> -EFAULT : 0;
> if (err)
> goto out_array_args;
> - *kernel_ptr = mbuf;
> + *kernel_ptr = array_buf;
> }
>
> /* Handles IOCTL */
> @@ -3360,12 +3354,14 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> if (in_compat_syscall()) {
> int put_err;
>
> - put_err = v4l2_compat_put_array_args(file, user_ptr, mbuf,
> - array_size, orig_cmd,
> + put_err = v4l2_compat_put_array_args(file, user_ptr,
> + array_buf,
> + array_size,
> + orig_cmd,
> parg);
orig_cmd and pargs would fit on the same line if you want to.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> if (put_err)
> err = put_err;
> - } else if (copy_to_user(user_ptr, mbuf, array_size)) {
> + } else if (copy_to_user(user_ptr, array_buf, array_size)) {
> err = -EFAULT;
> }
> goto out_array_args;
> @@ -3381,6 +3377,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
> err = -EFAULT;
> out:
> + kvfree(array_buf);
> kvfree(mbuf);
> return err;
> }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy
2020-12-20 14:15 ` Laurent Pinchart
@ 2020-12-20 20:03 ` Sakari Ailus
0 siblings, 0 replies; 6+ messages in thread
From: Sakari Ailus @ 2020-12-20 20:03 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Arnd Bergmann, syzbot, Arnd Bergmann, Hans Verkuil,
linux-kernel, Mauro Carvalho Chehab, syzkaller-bugs
Hi Laurent,
On Sun, Dec 20, 2020 at 04:15:07PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Sun, Dec 20, 2020 at 01:06:51PM +0200, Sakari Ailus wrote:
> > When an IOCTL with argument size larger than 128 that also used array
> > arguments were handled, two memory allocations were made but alas, only
> > the latter one of them was released.
>
> Alas, this fills my heart with sorrow indeed :-)
>
> > This happened because there was only
> > a single local variable to hold such a temporary allocation.
> >
> > Fix this by adding separate variables to hold the pointers to the
> > temporary allocations.
> >
> > Reported-by: Arnd Bergmann <arnd@kernel.org>
> > Reported-by: syzbot+1115e79c8df6472c612b@syzkaller.appspotmail.com
> > Fixes: d14e6d76ebf7 ("[media] v4l: Add multi-planar ioctl handling code")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-ioctl.c | 31 +++++++++++++---------------
> > 1 file changed, 14 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 3198abdd538ce..f42a779948779 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -3283,7 +3283,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> > v4l2_kioctl func)
> > {
> > char sbuf[128];
> > - void *mbuf = NULL;
> > + void *mbuf = NULL, *array_buf = NULL;
> > void *parg = (void *)arg;
> > long err = -EINVAL;
> > bool has_array_args;
> > @@ -3318,27 +3318,21 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> > has_array_args = err;
> >
> > if (has_array_args) {
> > - /*
> > - * When adding new types of array args, make sure that the
> > - * parent argument to ioctl (which contains the pointer to the
> > - * array) fits into sbuf (so that mbuf will still remain
> > - * unused up to here).
> > - */
> > - mbuf = kvmalloc(array_size, GFP_KERNEL);
> > + array_buf = kvmalloc(array_size, GFP_KERNEL);
> > err = -ENOMEM;
> > - if (NULL == mbuf)
> > + if (array_buf == NULL)
> > goto out_array_args;
> > err = -EFAULT;
> > if (in_compat_syscall())
> > - err = v4l2_compat_get_array_args(file, mbuf, user_ptr,
> > - array_size, orig_cmd,
> > - parg);
> > + err = v4l2_compat_get_array_args(file, array_buf,
> > + user_ptr, array_size,
> > + orig_cmd, parg);
> > else
> > - err = copy_from_user(mbuf, user_ptr, array_size) ?
> > + err = copy_from_user(array_buf, user_ptr, array_size) ?
> > -EFAULT : 0;
> > if (err)
> > goto out_array_args;
> > - *kernel_ptr = mbuf;
> > + *kernel_ptr = array_buf;
> > }
> >
> > /* Handles IOCTL */
> > @@ -3360,12 +3354,14 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> > if (in_compat_syscall()) {
> > int put_err;
> >
> > - put_err = v4l2_compat_put_array_args(file, user_ptr, mbuf,
> > - array_size, orig_cmd,
> > + put_err = v4l2_compat_put_array_args(file, user_ptr,
> > + array_buf,
> > + array_size,
> > + orig_cmd,
> > parg);
>
> orig_cmd and pargs would fit on the same line if you want to.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Thanks!
I'll send v2.
>
> > if (put_err)
> > err = put_err;
> > - } else if (copy_to_user(user_ptr, mbuf, array_size)) {
> > + } else if (copy_to_user(user_ptr, array_buf, array_size)) {
> > err = -EFAULT;
> > }
> > goto out_array_args;
> > @@ -3381,6 +3377,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> > if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
> > err = -EFAULT;
> > out:
> > + kvfree(array_buf);
> > kvfree(mbuf);
> > return err;
> > }
>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-20 20:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-20 11:06 [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy Sakari Ailus
2020-12-20 11:09 ` Hans Verkuil
2020-12-20 11:36 ` Arnd Bergmann
2020-12-20 11:51 ` Sakari Ailus
2020-12-20 14:15 ` Laurent Pinchart
2020-12-20 20:03 ` Sakari Ailus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).