Re: [PATCH v2 2/7] media: v4l2-core: explicitly clear ioctl input data

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

Hi Arnd,

On 11/06/2021 17:22, Arnd Bergmann wrote:
> On Fri, Jun 11, 2021 at 2:05 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> 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
>