ACK: [PATCH][OEM-5.6] media: v4l: ioctl: Fix memory leak in video_usercopy

[Kleber Souza]

On 08.04.21 21:17, Tim Gardner wrote:
> From: Sakari Ailus <sakari.ailus at linux.intel.com>
> 
> CVE-2021-30002
> 
> commit fb18802a338b36f675a388fc03d2aa504a0d0899 upstream.
> 
> 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 at kernel.org>
> Reported-by: syzbot+1115e79c8df6472c612b at syzkaller.appspotmail.com
> Fixes: d14e6d76ebf7 ("[media] v4l: Add multi-planar ioctl handling code")
> Cc: stable at vger.kernel.org
> Signed-off-by: Sakari Ailus <sakari.ailus at linux.intel.com>
> Acked-by: Arnd Bergmann <arnd at arndb.de>
> Acked-by: Hans Verkuil <hverkuil-cisco at xs4all.nl>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei at kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (cherry picked from commit 5400770e31e8b80efc25b4c1d619361255174d11 linux-5.10.y)
> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>

Thanks

> ---
>   drivers/media/v4l2-core/v4l2-ioctl.c | 19 +++++++------------
>   1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index aaf83e254272..eee040ea0694 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3182,7 +3182,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;
> @@ -3217,20 +3217,14 @@ 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 (copy_from_user(mbuf, user_ptr, array_size))
> +		if (copy_from_user(array_buf, user_ptr, array_size))
>   			goto out_array_args;
> -		*kernel_ptr = mbuf;
> +		*kernel_ptr = array_buf;
>   	}
>   
>   	/* Handles IOCTL */
> @@ -3249,7 +3243,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>   
>   	if (has_array_args) {
>   		*kernel_ptr = (void __force *)user_ptr;
> -		if (copy_to_user(user_ptr, mbuf, array_size))
> +		if (copy_to_user(user_ptr, array_buf, array_size))
>   			err = -EFAULT;
>   		goto out_array_args;
>   	}
> @@ -3264,6 +3258,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>   	if (video_put_user((void __user *)arg, parg, orig_cmd))
>   		err = -EFAULT;
>   out:
> +	kvfree(array_buf);
>   	kvfree(mbuf);
>   	return err;
>   }
>