All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv2] v4l2-ioctl.c: fix incorrect error path
@ 2022-03-21 11:03 Hans Verkuil
  2022-03-21 11:53 ` Laurent Pinchart
  0 siblings, 1 reply; 4+ messages in thread
From: Hans Verkuil @ 2022-03-21 11:03 UTC (permalink / raw)
  To: Linux Media Mailing List

If allocating array_buf fails, or copying data from userspace into that
buffer fails, then just free memory and return the error. Don't attempt
to call video_put_user() since there is no point.

If writing the array back to userspace fails, then don't go to out_array_args,
instead just continue with the regular code that just returns the error
unless 'always_copy' is set. That flag was just completely ignored when
writing back the array data.

Update the VIDIOC_G/S/TRY_EXT_CTRLS ioctls to set the ALWAYS_COPY flag
since they now need it.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
Changes since v1:
- drop the now unused out_array_args label
- set the INFO_FL_ALWAYS_COPY for the VIDIOC_G/S/TRY_EXT_CTRLS ioctls
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 96e307fe3aab..3eb8f5f0958d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2816,9 +2816,9 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO(VIDIOC_S_PRIORITY, v4l_s_priority, v4l_print_u32, INFO_FL_PRIO),
 	IOCTL_INFO(VIDIOC_G_SLICED_VBI_CAP, v4l_g_sliced_vbi_cap, v4l_print_sliced_vbi_cap, INFO_FL_CLEAR(v4l2_sliced_vbi_cap, type)),
 	IOCTL_INFO(VIDIOC_LOG_STATUS, v4l_log_status, v4l_print_newline, 0),
-	IOCTL_INFO(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
-	IOCTL_INFO(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL),
-	IOCTL_INFO(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
+	IOCTL_INFO(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
+	IOCTL_INFO(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
+	IOCTL_INFO(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
 	IOCTL_INFO(VIDIOC_ENUM_FRAMESIZES, v4l_stub_enum_framesizes, v4l_print_frmsizeenum, INFO_FL_CLEAR(v4l2_frmsizeenum, pixel_format)),
 	IOCTL_INFO(VIDIOC_ENUM_FRAMEINTERVALS, v4l_stub_enum_frameintervals, v4l_print_frmivalenum, INFO_FL_CLEAR(v4l2_frmivalenum, height)),
 	IOCTL_INFO(VIDIOC_G_ENC_INDEX, v4l_stub_g_enc_index, v4l_print_enc_idx, 0),
@@ -3311,7 +3311,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
 		array_buf = kvmalloc(array_size, GFP_KERNEL);
 		err = -ENOMEM;
 		if (array_buf == NULL)
-			goto out_array_args;
+			goto out;
 		err = -EFAULT;
 		if (in_compat_syscall())
 			err = v4l2_compat_get_array_args(file, array_buf,
@@ -3321,7 +3321,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
 			err = copy_from_user(array_buf, user_ptr, array_size) ?
 								-EFAULT : 0;
 		if (err)
-			goto out_array_args;
+			goto out;
 		*kernel_ptr = array_buf;
 	}

@@ -3353,7 +3353,6 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
 		} else if (copy_to_user(user_ptr, array_buf, array_size)) {
 			err = -EFAULT;
 		}
-		goto out_array_args;
 	}
 	/*
 	 * Some ioctls can return an error, but still have valid
@@ -3362,7 +3361,6 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
 	if (err < 0 && !always_copy)
 		goto out;

-out_array_args:
 	if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
 		err = -EFAULT;
 out:
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCHv2] v4l2-ioctl.c: fix incorrect error path
  2022-03-21 11:03 [PATCHv2] v4l2-ioctl.c: fix incorrect error path Hans Verkuil
@ 2022-03-21 11:53 ` Laurent Pinchart
  2022-03-21 12:34   ` Hans Verkuil
  0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2022-03-21 11:53 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Linux Media Mailing List

Hi Hans,

Thank you for the patch.

On Mon, Mar 21, 2022 at 12:03:30PM +0100, Hans Verkuil wrote:
> If allocating array_buf fails, or copying data from userspace into that
> buffer fails, then just free memory and return the error. Don't attempt
> to call video_put_user() since there is no point.
> 
> If writing the array back to userspace fails, then don't go to out_array_args,
> instead just continue with the regular code that just returns the error
> unless 'always_copy' is set. That flag was just completely ignored when
> writing back the array data.
> 
> Update the VIDIOC_G/S/TRY_EXT_CTRLS ioctls to set the ALWAYS_COPY flag
> since they now need it.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Changes since v1:
> - drop the now unused out_array_args label
> - set the INFO_FL_ALWAYS_COPY for the VIDIOC_G/S/TRY_EXT_CTRLS ioctls
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 96e307fe3aab..3eb8f5f0958d 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2816,9 +2816,9 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO(VIDIOC_S_PRIORITY, v4l_s_priority, v4l_print_u32, INFO_FL_PRIO),
>  	IOCTL_INFO(VIDIOC_G_SLICED_VBI_CAP, v4l_g_sliced_vbi_cap, v4l_print_sliced_vbi_cap, INFO_FL_CLEAR(v4l2_sliced_vbi_cap, type)),
>  	IOCTL_INFO(VIDIOC_LOG_STATUS, v4l_log_status, v4l_print_newline, 0),
> -	IOCTL_INFO(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
> -	IOCTL_INFO(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL),
> -	IOCTL_INFO(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
> +	IOCTL_INFO(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
> +	IOCTL_INFO(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
> +	IOCTL_INFO(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
>  	IOCTL_INFO(VIDIOC_ENUM_FRAMESIZES, v4l_stub_enum_framesizes, v4l_print_frmsizeenum, INFO_FL_CLEAR(v4l2_frmsizeenum, pixel_format)),
>  	IOCTL_INFO(VIDIOC_ENUM_FRAMEINTERVALS, v4l_stub_enum_frameintervals, v4l_print_frmivalenum, INFO_FL_CLEAR(v4l2_frmivalenum, height)),
>  	IOCTL_INFO(VIDIOC_G_ENC_INDEX, v4l_stub_g_enc_index, v4l_print_enc_idx, 0),
> @@ -3311,7 +3311,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>  		array_buf = kvmalloc(array_size, GFP_KERNEL);
>  		err = -ENOMEM;
>  		if (array_buf == NULL)
> -			goto out_array_args;
> +			goto out;
>  		err = -EFAULT;
>  		if (in_compat_syscall())
>  			err = v4l2_compat_get_array_args(file, array_buf,
> @@ -3321,7 +3321,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>  			err = copy_from_user(array_buf, user_ptr, array_size) ?
>  								-EFAULT : 0;
>  		if (err)
> -			goto out_array_args;
> +			goto out;
>  		*kernel_ptr = array_buf;
>  	}
> 
> @@ -3353,7 +3353,6 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>  		} else if (copy_to_user(user_ptr, array_buf, array_size)) {
>  			err = -EFAULT;
>  		}
> -		goto out_array_args;
>  	}
>  	/*
>  	 * Some ioctls can return an error, but still have valid
> @@ -3362,7 +3361,6 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>  	if (err < 0 && !always_copy)
>  		goto out;
> 
> -out_array_args:
>  	if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
>  		err = -EFAULT;

Could we turn this into

  	if (err >= 0 || always_copy) {
  		if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
  			err = -EFAULT;
	}

to avoid the goto ?

With this,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I also wonder if we should skip the copy to user for the array args when
an error occurs (unless always_copy is set). What do you think ? It's
not a candidate for this patch of course.

>  out:

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCHv2] v4l2-ioctl.c: fix incorrect error path
  2022-03-21 11:53 ` Laurent Pinchart
@ 2022-03-21 12:34   ` Hans Verkuil
  2022-03-21 12:37     ` Laurent Pinchart
  0 siblings, 1 reply; 4+ messages in thread
From: Hans Verkuil @ 2022-03-21 12:34 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Linux Media Mailing List

On 21/03/2022 12:53, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Mon, Mar 21, 2022 at 12:03:30PM +0100, Hans Verkuil wrote:
>> If allocating array_buf fails, or copying data from userspace into that
>> buffer fails, then just free memory and return the error. Don't attempt
>> to call video_put_user() since there is no point.
>>
>> If writing the array back to userspace fails, then don't go to out_array_args,
>> instead just continue with the regular code that just returns the error
>> unless 'always_copy' is set. That flag was just completely ignored when
>> writing back the array data.
>>
>> Update the VIDIOC_G/S/TRY_EXT_CTRLS ioctls to set the ALWAYS_COPY flag
>> since they now need it.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> Changes since v1:
>> - drop the now unused out_array_args label
>> - set the INFO_FL_ALWAYS_COPY for the VIDIOC_G/S/TRY_EXT_CTRLS ioctls
>> ---
>>  drivers/media/v4l2-core/v4l2-ioctl.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 96e307fe3aab..3eb8f5f0958d 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -2816,9 +2816,9 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>>  	IOCTL_INFO(VIDIOC_S_PRIORITY, v4l_s_priority, v4l_print_u32, INFO_FL_PRIO),
>>  	IOCTL_INFO(VIDIOC_G_SLICED_VBI_CAP, v4l_g_sliced_vbi_cap, v4l_print_sliced_vbi_cap, INFO_FL_CLEAR(v4l2_sliced_vbi_cap, type)),
>>  	IOCTL_INFO(VIDIOC_LOG_STATUS, v4l_log_status, v4l_print_newline, 0),
>> -	IOCTL_INFO(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
>> -	IOCTL_INFO(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL),
>> -	IOCTL_INFO(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
>> +	IOCTL_INFO(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
>> +	IOCTL_INFO(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
>> +	IOCTL_INFO(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
>>  	IOCTL_INFO(VIDIOC_ENUM_FRAMESIZES, v4l_stub_enum_framesizes, v4l_print_frmsizeenum, INFO_FL_CLEAR(v4l2_frmsizeenum, pixel_format)),
>>  	IOCTL_INFO(VIDIOC_ENUM_FRAMEINTERVALS, v4l_stub_enum_frameintervals, v4l_print_frmivalenum, INFO_FL_CLEAR(v4l2_frmivalenum, height)),
>>  	IOCTL_INFO(VIDIOC_G_ENC_INDEX, v4l_stub_g_enc_index, v4l_print_enc_idx, 0),
>> @@ -3311,7 +3311,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>>  		array_buf = kvmalloc(array_size, GFP_KERNEL);
>>  		err = -ENOMEM;
>>  		if (array_buf == NULL)
>> -			goto out_array_args;
>> +			goto out;
>>  		err = -EFAULT;
>>  		if (in_compat_syscall())
>>  			err = v4l2_compat_get_array_args(file, array_buf,
>> @@ -3321,7 +3321,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>>  			err = copy_from_user(array_buf, user_ptr, array_size) ?
>>  								-EFAULT : 0;
>>  		if (err)
>> -			goto out_array_args;
>> +			goto out;
>>  		*kernel_ptr = array_buf;
>>  	}
>>
>> @@ -3353,7 +3353,6 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>>  		} else if (copy_to_user(user_ptr, array_buf, array_size)) {
>>  			err = -EFAULT;
>>  		}
>> -		goto out_array_args;
>>  	}
>>  	/*
>>  	 * Some ioctls can return an error, but still have valid
>> @@ -3362,7 +3361,6 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>>  	if (err < 0 && !always_copy)
>>  		goto out;
>>
>> -out_array_args:
>>  	if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
>>  		err = -EFAULT;
> 
> Could we turn this into
> 
>   	if (err >= 0 || always_copy) {
>   		if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
>   			err = -EFAULT;
> 	}
> 
> to avoid the goto ?
> 
> With this,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I also wonder if we should skip the copy to user for the array args when
> an error occurs (unless always_copy is set). What do you think ? It's
> not a candidate for this patch of course.

Actually, that's a very good point and I think it belongs to this patch as well.
It's really wrong to copy back data on error unless always_copy is set.

But in that case I prefer to keep the goto: the 'if (err < 0 && !always_copy)'
just moves up to before the 'if (has_array_args) {':

That part of the patch will then look like this:

@@ -3339,6 +3339,13 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
                        trace_v4l2_qbuf(video_devdata(file)->minor, parg);
        }

+       /*
+        * Some ioctls can return an error, but still have valid
+        * results that must be returned.
+        */
+       if (err < 0 && !always_copy)
+               goto out;
+
        if (has_array_args) {
                *kernel_ptr = (void __force *)user_ptr;
                if (in_compat_syscall()) {
@@ -3353,16 +3360,8 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
                } else if (copy_to_user(user_ptr, array_buf, array_size)) {
                        err = -EFAULT;
                }
-               goto out_array_args;
        }
-       /*
-        * Some ioctls can return an error, but still have valid
-        * results that must be returned.
-        */
-       if (err < 0 && !always_copy)
-               goto out;

-out_array_args:
        if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
                err = -EFAULT;
 out:

Regards,

	Hans

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCHv2] v4l2-ioctl.c: fix incorrect error path
  2022-03-21 12:34   ` Hans Verkuil
@ 2022-03-21 12:37     ` Laurent Pinchart
  0 siblings, 0 replies; 4+ messages in thread
From: Laurent Pinchart @ 2022-03-21 12:37 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Linux Media Mailing List

Hi Hans,

On Mon, Mar 21, 2022 at 01:34:05PM +0100, Hans Verkuil wrote:
> On 21/03/2022 12:53, Laurent Pinchart wrote:
> > On Mon, Mar 21, 2022 at 12:03:30PM +0100, Hans Verkuil wrote:
> >> If allocating array_buf fails, or copying data from userspace into that
> >> buffer fails, then just free memory and return the error. Don't attempt
> >> to call video_put_user() since there is no point.
> >>
> >> If writing the array back to userspace fails, then don't go to out_array_args,
> >> instead just continue with the regular code that just returns the error
> >> unless 'always_copy' is set. That flag was just completely ignored when
> >> writing back the array data.
> >>
> >> Update the VIDIOC_G/S/TRY_EXT_CTRLS ioctls to set the ALWAYS_COPY flag
> >> since they now need it.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >> Changes since v1:
> >> - drop the now unused out_array_args label
> >> - set the INFO_FL_ALWAYS_COPY for the VIDIOC_G/S/TRY_EXT_CTRLS ioctls
> >> ---
> >>  drivers/media/v4l2-core/v4l2-ioctl.c | 12 +++++-------
> >>  1 file changed, 5 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> index 96e307fe3aab..3eb8f5f0958d 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> @@ -2816,9 +2816,9 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
> >>  	IOCTL_INFO(VIDIOC_S_PRIORITY, v4l_s_priority, v4l_print_u32, INFO_FL_PRIO),
> >>  	IOCTL_INFO(VIDIOC_G_SLICED_VBI_CAP, v4l_g_sliced_vbi_cap, v4l_print_sliced_vbi_cap, INFO_FL_CLEAR(v4l2_sliced_vbi_cap, type)),
> >>  	IOCTL_INFO(VIDIOC_LOG_STATUS, v4l_log_status, v4l_print_newline, 0),
> >> -	IOCTL_INFO(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
> >> -	IOCTL_INFO(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL),
> >> -	IOCTL_INFO(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL),
> >> +	IOCTL_INFO(VIDIOC_G_EXT_CTRLS, v4l_g_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
> >> +	IOCTL_INFO(VIDIOC_S_EXT_CTRLS, v4l_s_ext_ctrls, v4l_print_ext_controls, INFO_FL_PRIO | INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
> >> +	IOCTL_INFO(VIDIOC_TRY_EXT_CTRLS, v4l_try_ext_ctrls, v4l_print_ext_controls, INFO_FL_CTRL | INFO_FL_ALWAYS_COPY),
> >>  	IOCTL_INFO(VIDIOC_ENUM_FRAMESIZES, v4l_stub_enum_framesizes, v4l_print_frmsizeenum, INFO_FL_CLEAR(v4l2_frmsizeenum, pixel_format)),
> >>  	IOCTL_INFO(VIDIOC_ENUM_FRAMEINTERVALS, v4l_stub_enum_frameintervals, v4l_print_frmivalenum, INFO_FL_CLEAR(v4l2_frmivalenum, height)),
> >>  	IOCTL_INFO(VIDIOC_G_ENC_INDEX, v4l_stub_g_enc_index, v4l_print_enc_idx, 0),
> >> @@ -3311,7 +3311,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> >>  		array_buf = kvmalloc(array_size, GFP_KERNEL);
> >>  		err = -ENOMEM;
> >>  		if (array_buf == NULL)
> >> -			goto out_array_args;
> >> +			goto out;
> >>  		err = -EFAULT;
> >>  		if (in_compat_syscall())
> >>  			err = v4l2_compat_get_array_args(file, array_buf,
> >> @@ -3321,7 +3321,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> >>  			err = copy_from_user(array_buf, user_ptr, array_size) ?
> >>  								-EFAULT : 0;
> >>  		if (err)
> >> -			goto out_array_args;
> >> +			goto out;
> >>  		*kernel_ptr = array_buf;
> >>  	}
> >>
> >> @@ -3353,7 +3353,6 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> >>  		} else if (copy_to_user(user_ptr, array_buf, array_size)) {
> >>  			err = -EFAULT;
> >>  		}
> >> -		goto out_array_args;
> >>  	}
> >>  	/*
> >>  	 * Some ioctls can return an error, but still have valid
> >> @@ -3362,7 +3361,6 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
> >>  	if (err < 0 && !always_copy)
> >>  		goto out;
> >>
> >> -out_array_args:
> >>  	if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
> >>  		err = -EFAULT;
> > 
> > Could we turn this into
> > 
> >   	if (err >= 0 || always_copy) {
> >   		if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
> >   			err = -EFAULT;
> > 	}
> > 
> > to avoid the goto ?
> > 
> > With this,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > I also wonder if we should skip the copy to user for the array args when
> > an error occurs (unless always_copy is set). What do you think ? It's
> > not a candidate for this patch of course.
> 
> Actually, that's a very good point and I think it belongs to this patch as well.
> It's really wrong to copy back data on error unless always_copy is set.
> 
> But in that case I prefer to keep the goto: the 'if (err < 0 && !always_copy)'
> just moves up to before the 'if (has_array_args) {':
> 
> That part of the patch will then look like this:
> 
> @@ -3339,6 +3339,13 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>                         trace_v4l2_qbuf(video_devdata(file)->minor, parg);
>         }
> 
> +       /*
> +        * Some ioctls can return an error, but still have valid
> +        * results that must be returned.
> +        */
> +       if (err < 0 && !always_copy)
> +               goto out;
> +
>         if (has_array_args) {
>                 *kernel_ptr = (void __force *)user_ptr;
>                 if (in_compat_syscall()) {
> @@ -3353,16 +3360,8 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>                 } else if (copy_to_user(user_ptr, array_buf, array_size)) {
>                         err = -EFAULT;
>                 }
> -               goto out_array_args;
>         }
> -       /*
> -        * Some ioctls can return an error, but still have valid
> -        * results that must be returned.
> -        */
> -       if (err < 0 && !always_copy)
> -               goto out;
> 
> -out_array_args:
>         if (video_put_user((void __user *)arg, parg, cmd, orig_cmd))
>                 err = -EFAULT;
>  out:

No objection.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-21 12:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-21 11:03 [PATCHv2] v4l2-ioctl.c: fix incorrect error path Hans Verkuil
2022-03-21 11:53 ` Laurent Pinchart
2022-03-21 12:34   ` Hans Verkuil
2022-03-21 12:37     ` Laurent Pinchart

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.