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

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

[Sakari Ailus]

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 32 ++++++++++++----------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 3198abdd538c..9906b41004e9 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,13 @@ 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,
-							     parg);
+			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 +3376,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.29.2

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

[Bingbu Cao]

Sakari,

On 12/21/20 4:11 AM, 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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 32 ++++++++++++----------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 3198abdd538c..9906b41004e9 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)

if (!array_buf)
?

>  			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,13 @@ 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,
> -							     parg);
> +			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 +3376,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;
>  }
> 

-- 
Best regards,
Bingbu Cao

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

[Bingbu Cao]

On 1/14/21 12:50 PM, Bingbu Cao wrote:
> Sakari,
> 
> On 12/21/20 4:11 AM, 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>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 32 ++++++++++++----------------
>>  1 file changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 3198abdd538c..9906b41004e9 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)
> 
> if (!array_buf)
> ?
> 
Please ignore my previous comment, as the patch was landed. :)
....
-- 
Best regards,
Bingbu Cao