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

[Hans Verkuil <hverkuil@xs4all.nl>]

On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> The v4l2_buffer structure contains a 'struct timeval' member that is
> defined by the user space C library, creating an ABI incompatibility
> when that gets updated to a 64-bit time_t.
> 
> As in v4l2_event, handle this with a special case in video_put_user()
> and video_get_user() to replace the memcpy there.
> 
> Since the structure also contains a pointer, there are now two
> native versions (on 32-bit systems) as well as two compat versions
> (on 64-bit systems), which unfortunately complicates the compat
> handler quite a bit.
> 
> Duplicating the existing handlers for the new types is a safe
> conversion for now, but unfortunately this may turn into a
> maintenance burden later. A larger-scale rework of the
> compat code might be a better alternative, but is out of scope
> of the y2038 work.
> 
> Sparc64 needs a special case because of their special suseconds_t
> definition.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 57 ++++++++++++++++++++++++++--
>  include/uapi/linux/videodev2.h       | 45 ++++++++++++++++++++++
>  2 files changed, 98 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 1de939d11628..4ae1bcaec3fa 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -474,10 +474,10 @@ static void v4l_print_buffer(const void *arg, bool write_only)
>  	const struct v4l2_plane *plane;
>  	int i;
>  
> -	pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
> -			p->timestamp.tv_sec / 3600,
> -			(int)(p->timestamp.tv_sec / 60) % 60,
> -			(int)(p->timestamp.tv_sec % 60),
> +	pr_cont("%02d:%02d:%02d.%09ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
> +			(int)p->timestamp.tv_sec / 3600,
> +			((int)p->timestamp.tv_sec / 60) % 60,
> +			((int)p->timestamp.tv_sec % 60),
>  			(long)p->timestamp.tv_usec,
>  			p->index,
>  			prt_names(p->type, v4l2_type_names), p->request_fd,
> @@ -3014,6 +3014,14 @@ static unsigned int video_translate_cmd(unsigned int cmd)
>  #ifdef CONFIG_COMPAT_32BIT_TIME
>  	case VIDIOC_DQEVENT_TIME32:
>  		return VIDIOC_DQEVENT;
> +	case VIDIOC_QUERYBUF_TIME32:
> +		return VIDIOC_QUERYBUF;
> +	case VIDIOC_QBUF_TIME32:
> +		return VIDIOC_QBUF;
> +	case VIDIOC_DQBUF_TIME32:
> +		return VIDIOC_DQBUF;
> +	case VIDIOC_PREPARE_BUF_TIME32:
> +		return VIDIOC_PREPARE_BUF;
>  #endif
>  	}
>  
> @@ -3032,6 +3040,30 @@ static int video_get_user(void __user *arg, void *parg, unsigned int cmd,
>  	}
>  
>  	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.

> +
> +		if (cmd == VIDIOC_QUERYBUF_TIME32)
> +			memset(&vb->length, 0, sizeof(*vb) -
> +			       offsetof(struct v4l2_buffer, length));
> +
> +		break;
> +	}
> +#endif
>  	default:
>  		/*
>  		 * In some cases, only a few fields are used as input,
> @@ -3080,6 +3112,23 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
>  			return -EFAULT;
>  		break;
>  	}
> +	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;
> +
> +		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.

> +
> +		if (copy_to_user(arg, &vb32, sizeof(vb32)))
> +			return -EFAULT;
> +		break;
> +	}
>  #endif
>  	default:
>  		/*  Copy results into user buffer  */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 1d2553d4ed5b..f05c54d63f96 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -990,7 +990,47 @@ struct v4l2_buffer {
>  	__u32			bytesused;
>  	__u32			flags;
>  	__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.

> +#else
>  	struct timeval		timestamp;
> +#endif
> +	struct v4l2_timecode	timecode;
> +	__u32			sequence;
> +
> +	/* memory location */
> +	__u32			memory;
> +	union {
> +		__u32           offset;
> +		unsigned long   userptr;
> +		struct v4l2_plane *planes;
> +		__s32		fd;
> +	} m;
> +	__u32			length;
> +	__u32			reserved2;
> +	union {
> +		__s32		request_fd;
> +		__u32		reserved;
> +	};
> +};
> +
> +#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?

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

>  #define VIDIOC_G_FBUF		 _IOR('V', 10, struct v4l2_framebuffer)
>  #define VIDIOC_S_FBUF		 _IOW('V', 11, struct v4l2_framebuffer)
>  #define VIDIOC_OVERLAY		 _IOW('V', 14, int)
>  #define VIDIOC_QBUF		_IOWR('V', 15, struct v4l2_buffer)
> +#define VIDIOC_QBUF_TIME32	_IOWR('V', 15, struct v4l2_buffer_time32)
>  #define VIDIOC_EXPBUF		_IOWR('V', 16, struct v4l2_exportbuffer)
>  #define VIDIOC_DQBUF		_IOWR('V', 17, struct v4l2_buffer)
> +#define VIDIOC_DQBUF_TIME32	_IOWR('V', 17, struct v4l2_buffer_time32)
>  #define VIDIOC_STREAMON		 _IOW('V', 18, int)
>  #define VIDIOC_STREAMOFF	 _IOW('V', 19, int)
>  #define VIDIOC_G_PARM		_IOWR('V', 21, struct v4l2_streamparm)
> @@ -2520,6 +2564,7 @@ struct v4l2_create_buffers {
>  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
>  #define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
>  #define VIDIOC_PREPARE_BUF	_IOWR('V', 93, struct v4l2_buffer)
> +#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32)
>  #define VIDIOC_G_SELECTION	_IOWR('V', 94, struct v4l2_selection)
>  #define VIDIOC_S_SELECTION	_IOWR('V', 95, struct v4l2_selection)
>  #define VIDIOC_DECODER_CMD	_IOWR('V', 96, struct v4l2_decoder_cmd)
> 

Regards,

	Hans