Re: [PATCHv2 13/13] v4l2-compat-ioctl32.c: refactor, fix security bug in compat ioctl32
On 01/30/18 12:46, Sakari Ailus wrote:
> Hi Hans,
>
> Thanks for the update. Please see a few additional comments below.
>
> On Tue, Jan 30, 2018 at 11:27:01AM +0100, Hans Verkuil wrote:
> ...
>> @@ -891,30 +1057,53 @@ static long do_video_ioctl(struct file *file,
>> unsigned int cmd, unsigned long ar
>> case VIDIOC_STREAMOFF:
>> case VIDIOC_S_INPUT:
>> case VIDIOC_S_OUTPUT:
>> -err = get_user(karg.vi, (s32 __user *)up);
>> +err = alloc_userspace(sizeof(unsigned int), 0, _native);
>> +if (!err && assign_in_user((unsigned int __user *)up_native,
>> + (compat_uint_t __user *)up))
>> +err = -EFAULT;
>> compatible_arg = 0;
>> break;
>>
>> case VIDIOC_G_INPUT:
>> case VIDIOC_G_OUTPUT:
>> +err = alloc_userspace(sizeof(unsigned int), 0,
>> + _native);
>
> Fits on a single line.
Changed.
>
>> compatible_arg = 0;
>> break;
>>
>> case VIDIOC_G_EDID:
>> case VIDIOC_S_EDID:
>> -err = get_v4l2_edid32(, up);
>> +err = alloc_userspace(sizeof(struct v4l2_edid), 0, _native);
>> +if (!err)
>> +err = get_v4l2_edid32(up_native, up);
>> compatible_arg = 0;
>> break;
>>
>> case VIDIOC_G_FMT:
>> case VIDIOC_S_FMT:
>> case VIDIOC_TRY_FMT:
>> -err = get_v4l2_format32(, up);
>> +err = bufsize_v4l2_format(up, _space);
>> +if (!err)
>> +err = alloc_userspace(sizeof(struct v4l2_format),
>> + aux_space, _native);
>> +if (!err) {
>> +aux_buf = up_native + sizeof(struct v4l2_format);
>> +err = get_v4l2_format32(up_native, up,
>> +aux_buf, aux_space);
>> +}
>> compatible_arg = 0;
>> break;
>>
>> case VIDIOC_CREATE_BUFS:
>> -err = get_v4l2_create32(, up);
>> +err = bufsize_v4l2_create(up, _space);
>> +if (!err)
>> +err = alloc_userspace(sizeof(struct
>> v4l2_create_buffers),
>> +aux_space, _native);
>> +if (!err) {
>> +aux_buf = up_native + sizeof(struct
>> v4l2_create_buffers);
>
> A few lines over 80 characters. It's not a lot but I see no reason to avoid
> wrapping them either.
I'm not changing this. It looks really ugly if I split it up.
>
>> +err = get_v4l2_create32(up_native, up,
>> +aux_buf, aux_space);
>> +}
>> compatible_arg = 0;
>> break;
>>
>
> The above can be addressed later, right now this isn't a priority.
>
> Acked-by: Sakari Ailus
>
Thanks!
Hans
Re: [PATCHv2 13/13] v4l2-compat-ioctl32.c: refactor, fix security bug in compat ioctl32
Hi Hans,
Thanks for the update. Please see a few additional comments below.
On Tue, Jan 30, 2018 at 11:27:01AM +0100, Hans Verkuil wrote:
...
> @@ -891,30 +1057,53 @@ static long do_video_ioctl(struct file *file, unsigned
> int cmd, unsigned long ar
> case VIDIOC_STREAMOFF:
> case VIDIOC_S_INPUT:
> case VIDIOC_S_OUTPUT:
> - err = get_user(karg.vi, (s32 __user *)up);
> + err = alloc_userspace(sizeof(unsigned int), 0, _native);
> + if (!err && assign_in_user((unsigned int __user *)up_native,
> +(compat_uint_t __user *)up))
> + err = -EFAULT;
> compatible_arg = 0;
> break;
>
> case VIDIOC_G_INPUT:
> case VIDIOC_G_OUTPUT:
> + err = alloc_userspace(sizeof(unsigned int), 0,
> + _native);
Fits on a single line.
> compatible_arg = 0;
> break;
>
> case VIDIOC_G_EDID:
> case VIDIOC_S_EDID:
> - err = get_v4l2_edid32(, up);
> + err = alloc_userspace(sizeof(struct v4l2_edid), 0, _native);
> + if (!err)
> + err = get_v4l2_edid32(up_native, up);
> compatible_arg = 0;
> break;
>
> case VIDIOC_G_FMT:
> case VIDIOC_S_FMT:
> case VIDIOC_TRY_FMT:
> - err = get_v4l2_format32(, up);
> + err = bufsize_v4l2_format(up, _space);
> + if (!err)
> + err = alloc_userspace(sizeof(struct v4l2_format),
> + aux_space, _native);
> + if (!err) {
> + aux_buf = up_native + sizeof(struct v4l2_format);
> + err = get_v4l2_format32(up_native, up,
> + aux_buf, aux_space);
> + }
> compatible_arg = 0;
> break;
>
> case VIDIOC_CREATE_BUFS:
> - err = get_v4l2_create32(, up);
> + err = bufsize_v4l2_create(up, _space);
> + if (!err)
> + err = alloc_userspace(sizeof(struct
> v4l2_create_buffers),
> + aux_space, _native);
> + if (!err) {
> + aux_buf = up_native + sizeof(struct
> v4l2_create_buffers);
A few lines over 80 characters. It's not a lot but I see no reason to avoid
wrapping them either.
> + err = get_v4l2_create32(up_native, up,
> + aux_buf, aux_space);
> + }
> compatible_arg = 0;
> break;
>
The above can be addressed later, right now this isn't a priority.
Acked-by: Sakari Ailus
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
