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

[Hans Verkuil <hverkuil@xs4all.nl>]

On 11/26/19 2:50 PM, Arnd Bergmann wrote:
> On Mon, Nov 25, 2019 at 3:57 PM Hans Verkuil <hverkuil@xs4all.nl> 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,
>                 };

That looks clean.

> 
>                 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.

How about something like this in videodev2.h:

Split off the ugly kernel timeval definition in a separate struct:

#ifdef __KERNEL__
	/* match glibc timeval64 format */
	struct __kernel_v4l2_timeval {
		long long	tv_sec;
# if defined(__sparc__) && defined(__arch64__)
		int		tv_usec;
		int		__pad;
# else
		long long	tv_usec;
# endif
	};
#endif

Then use that in the struct v4l2_buffer definition:

struct v4l2_buffer {
...
#ifdef __KERNEL__
 	struct __kernel_v4l2_timeval timestamp;
#else
 	struct timeval		     timestamp;
#endif

That keeps struct v4l2_buffer fairly clean. And it also makes it
possible to have a bit more extensive documentation for the
struct __kernel_v4l2_timeval without polluting the actual struct
v4l2_buffer definition.

The videodev2.h header is something users of the API look at a
lot and having this really ugly kernel timestamp in there is
not acceptably IMHO. But splitting it off should work.

> 
>>> +#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
> 

Regards,

	Hans