lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 11 Jun 2021 17:22:56 +0200
From:   Arnd Bergmann <arnd@...nel.org>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        "Lad, Prabhakar" <prabhakar.csengg@...il.com>,
        Eduardo Valentin <edubezval@...il.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Vaibhav Gupta <vaibhavgupta40@...il.com>,
        Liu Shixin <liushixin2@...wei.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        linux-staging@...ts.linux.dev
Subject: Re: [PATCH v2 2/7] media: v4l2-core: explicitly clear ioctl input data

On Fri, Jun 11, 2021 at 2:05 PM Hans Verkuil <hverkuil-cisco@...all.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.

     Arnd

Powered by blists - more mailing lists