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

[Arnd Bergmann]

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.

     Arnd