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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
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
