* 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