Re: [PATCH 1/1] v4l: ioctl: Fix memory leak in video_usercopy

[Sakari Ailus]

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@xxxxxxxxxx>
> > Reported-by: syzbot+1115e79c8df6472c612b@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Fixes: d14e6d76ebf7 ("[media] v4l: Add multi-planar ioctl handling code")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxxx>

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