Re: [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI

[Arnd Bergmann]

On Mon, Nov 25, 2019 at 3:57 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 11/11/19 9:38 PM, Arnd Bergmann wrote:

> >       switch (cmd) {
> > +#ifdef COMPAT_32BIT_TIME
> > +     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;
> > +
> > +             memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
> > +             vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
> > +             vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
> > +             memcpy(&vb->timecode, &vb32.timecode,
> > +                    sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>
> I have similar concerns as with dqevent about whether this memcpy is the right approach.
> Unless you can prove with a utility like pahole that this memcpy is safe.

This is the video_get_user() function, so the input data comes from user
space and gets copied into the kernel, which has to check each field for
validity already, so I think this is safe regardless of the padding (which
exists before the 64-bit timestamp on 32-bit architectures). The fields
match because the definition of all members other than the timeval is
the same.

On the other hand, I agree it's not obvious from the code why this
is correct. I've changed my copy to this version below now, do you like
that better?

                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,
                        .memory         = vb32.memory,
                        .m.userptr      = vb32.usercopy,
                        .length         = vb32.length,
                        .request_fd     = vb32.request_fd,
                };

                if (cmd == VIDIOC_QUERYBUF_TIME32)
                        memset(&vb->length, 0, sizeof(*vb) -
                               offsetof(struct v4l2_buffer, length));

This way, all padding is zeroed out, and it's obvious to human
readers that each field gets set in the correct location.

> > +             memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
> > +             vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
> > +             vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
> > +             memcpy(&vb32.timecode, &vb->timecode,
> > +                    sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>
> Ditto.

This is my new version:

                struct v4l2_buffer *vb = parg;
                struct v4l2_buffer_time32 vb32 = {
                        .index          = vb->index,
                        .type           = vb->type,
                        .bytesused      = vb->bytesused,
                        .flags          = vb->flags,
                        .field          = vb->field,
                        .timestamp.tv_sec       = vb->timestamp.tv_sec,
                        .timestamp.tv_usec      = vb->timestamp.tv_usec,
                        .timecode       = vb->timecode,
                        .memory         = vb->memory,
                        .m.userptr      = vb->usercopy,
                        .length         = vb->length,
                        .request_fd     = vb->request_fd,
                };

                if (copy_to_user(arg, &vb32, sizeof(vb32)))
                        return -EFAULT;

> >       __u32                   field;
> > +#ifdef __KERNEL__
> > +     /* match glibc timeval64 format */
> > +     struct {
> > +             long long       tv_sec;
> > +# if defined(__sparc__) && defined(__arch64__)
> > +             int             tv_usec;
> > +             int             __pad;
> > +# else
> > +             long long       tv_usec;
> > +# endif
> > +     } timestamp;
>
> Ewww!
>
> Are there more places where this is needed? If so, then I very much prefer
> that a __kernel_timeval struct is defined somewhere, with appropriate
> comments.

I was trying hard to avoid adding a modern version of timeval, because
all new code should be encouraged to use __kernel_timespec instead.

There are not many users of timeval in the uapi, and this is the last one
after the others all got invididual treatment.

Usually what I would do is to have a kernel-internal type based
on timespec or u64, and then define three uapi types:
old native (based on __kernel_old_timeval), old compat (using
old_timeval32) and the new type with 64-bit time_t.

The problem with v4l2_buffer is that it includes another
compat-incompatible field (m.userptr) and that it's passed
between kernel functions, so then I'd probably need five variants
of it in total, and it would slow down the common case (64-bit
native) because it would require an extra copy.

I can try a few more things here, but I don't expect to find anything
much better than this.

> > +#ifdef __KERNEL__
> > +struct v4l2_buffer_time32 {
> > +     __u32                   index;
> > +     __u32                   type;
> > +     __u32                   bytesused;
> > +     __u32                   flags;
> > +     __u32                   field;
> > +     struct old_timeval32    timestamp;
> >       struct v4l2_timecode    timecode;
> >       __u32                   sequence;
> >
> > @@ -1009,6 +1049,7 @@ struct v4l2_buffer {
> >               __u32           reserved;
> >       };
> >  };
> > +#endif
>
> Can this be moved to v4l2-ioctls.h?

done.

> >  #ifndef __KERNEL__
> >  /**
> > @@ -2446,12 +2487,15 @@ struct v4l2_create_buffers {
> >  #define VIDIOC_S_FMT         _IOWR('V',  5, struct v4l2_format)
> >  #define VIDIOC_REQBUFS               _IOWR('V',  8, struct v4l2_requestbuffers)
> >  #define VIDIOC_QUERYBUF              _IOWR('V',  9, struct v4l2_buffer)
> > +#define VIDIOC_QUERYBUF_TIME32       _IOWR('V',  9, struct v4l2_buffer_time32)
>
> And all these should be moved there as well.

done.

      Arnd