Hi Arnd,
On 11/06/2021 17:22, Arnd Bergmann wrote:
> On Fri, Jun 11, 2021 at 2:05 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>> On 10/06/2021 23:43, Arnd Bergmann wrote:
>>> @@ -3122,12 +3122,23 @@ static int video_get_user(void __user *arg, void *parg,
>>>
>>> if (cmd == real_cmd) {
>>> if (copy_from_user(parg, (void __user *)arg, n))
>>> - err = -EFAULT;
>>> - } else if (in_compat_syscall()) {
>>> - err = v4l2_compat_get_user(arg, parg, cmd);
>>> - } else {
>>> - switch (cmd) {
>>> + return -EFAULT;
>>> +
>>> + /* zero out anything we don't copy from userspace */
>>> + if (n < _IOC_SIZE(real_cmd))
>>> + memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);
>>
>> This should always happen, not just when cmd == real_cmd.
>
> Ok, got it. I was trying to simplify this, but I went a little too far, so
> in the case of VIDIOC_QUERYBUF_TIME32 I dropped the final
> clearing of the extra data, leaving the user data in place.
>
>> The comment is a bit misleading: besides zeroing what isn't copied from
>> userspace, it also zeroes copied fields based on INFO_FL_CLEAR_MASK.
>
> I'm not following here, isn't that the same? We copy 'n' bytes, and then we
> clear 'size - n' bytes, which is everything that wasn't copied.
>
>> With this change that no longer happens and v4l2-compliance starts complaining.
>>
>>> +
>>> + return 0;
>>> + }
>>> +
>>> + /* zero out whole buffer first to deal with missing emulation */
>>> + memset(parg, 0, _IOC_SIZE(real_cmd));
>>> +
>>> + if (in_compat_syscall())
>>> + return v4l2_compat_get_user(arg, parg, cmd);
>>> +
>>> #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
>>> + switch (cmd) {
>>> case VIDIOC_QUERYBUF_TIME32:
>>> case VIDIOC_QBUF_TIME32:
>>> case VIDIOC_DQBUF_TIME32:
>>
>> The 'case' statements need to be indented one tab less.
>
> It seems this is no longer needed when I go back to having the switch()
> inside the else{}.
>
>>> @@ -3140,28 +3151,24 @@ static int video_get_user(void __user *arg, void *parg,
>>>
>>> *vb = (struct v4l2_buffer) {
>>> .index = vb32.index,
>>> - .type = vb32.type,
>>> - .bytesused = vb32.bytesused,
>>> - .flags = vb32.flags,
>>> - .field = vb32.field,
>>> - .timestamp.tv_sec = vb32.timestamp.tv_sec,
>>> - .timestamp.tv_usec = vb32.timestamp.tv_usec,
>>> - .timecode = vb32.timecode,
>>> - .sequence = vb32.sequence,
>>> - .memory = vb32.memory,
>>> - .m.userptr = vb32.m.userptr,
>>> - .length = vb32.length,
>>> - .request_fd = vb32.request_fd,
>>> + .type = vb32.type,
>>> + .bytesused = vb32.bytesused,
>>> + .flags = vb32.flags,
>>> + .field = vb32.field,
>>> + .timestamp.tv_sec = vb32.timestamp.tv_sec,
>>> + .timestamp.tv_usec = vb32.timestamp.tv_usec,
>>> + .timecode = vb32.timecode,
>>> + .sequence = vb32.sequence,
>>> + .memory = vb32.memory,
>>> + .m.userptr = vb32.m.userptr,
>>> + .length = vb32.length,
>>> + .request_fd = vb32.request_fd,
>>
>> Can you put these whitespace changes in a separate patch?
>
> Sure.
>
>> I ended up with this code, and then my tests passed:
>>
>> if (cmd == real_cmd) {
>> if (copy_from_user(parg, (void __user *)arg, n))
>> return -EFAULT;
>> } else if (in_compat_syscall()) {
>> memset(parg, 0, n);
>> err = v4l2_compat_get_user(arg, parg, cmd);
>> } else {
>> #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
>> memset(parg, 0, n);
>> switch (cmd) {
>> case VIDIOC_QUERYBUF_TIME32:
>> case VIDIOC_QBUF_TIME32:
>> case VIDIOC_DQBUF_TIME32:
>> case VIDIOC_PREPARE_BUF_TIME32: {
>> struct v4l2_buffer_time32 vb32;
>> struct v4l2_buffer *vb = parg;
>>
>> if (copy_from_user(&vb32, arg, sizeof(vb32)))
>> return -EFAULT;
>>
>> *vb = (struct v4l2_buffer) {
>> .index = vb32.index,
>> .type = vb32.type,
>> .bytesused = vb32.bytesused,
>> .flags = vb32.flags,
>> .field = vb32.field,
>> .timestamp.tv_sec = vb32.timestamp.tv_sec,
>> .timestamp.tv_usec = vb32.timestamp.tv_usec,
>> .timecode = vb32.timecode,
>> .sequence = vb32.sequence,
>> .memory = vb32.memory,
>> .m.userptr = vb32.m.userptr,
>> .length = vb32.length,
>> .request_fd = vb32.request_fd,
>> };
>> break;
>> }
>> }
>> #endif
>> }
>>
>> /* zero out anything we don't copy from userspace */
>> if (!err && n < _IOC_SIZE(real_cmd))
>> memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);
>>
>> return err;
>
> Ok, so this version just adds the two memset(), without any other
> changes. That is clearly the safest change, and I'll send it like this
> in v3.
>
>> That said, I also ran the regression tests on a i686 VM, and there I got a
>> bunch of failures, but that was *without* your patches, so I think something
>> unrelated broke. I'll have to dig more into this in the next few days.
>>
>> But I wanted to get this out first, since this patch is clearly wrong.
>
> Thanks a lot for taking a look and giving it an initial test. I have
> updated the git tree at
>
> git://git.kernel.org:/pub/scm/linux/kernel/git/arnd/playground.git
> playground/v4l2-compat-ioctl
>
> with the changes you pointed out. Let me know when you have found
> out what was going on in the VM guest, and I'll send it as v3 or integrate
> additional fixes that you find necessary.
Please post v3: one issue I had turned out to be due to a misconfiguration
causing some of the tests to run out of memory.
The other issue is with the X32 ABI: that is currently failing, with or
without your series. I'm not sure why yet, from what I remember this worked fine
last time I tested. But my memory may be faulty, since it is so long ago.
Regards,
Hans
>
> Arnd
>
