On 11/26/19 3:43 PM, Arnd Bergmann wrote:
On Mon, Nov 25, 2019 at 3:40 PM Hans Verkuil hverkuil@xs4all.nl wrote:
On 11/11/19 9:38 PM, Arnd Bergmann wrote:
switch (cmd) {+#ifdef CONFIG_COMPAT_32BIT_TIME
case VIDIOC_DQEVENT_TIME32: {struct v4l2_event_time32 ev32;struct v4l2_event *ev = parg;memcpy(&ev32, ev, offsetof(struct v4l2_event, timestamp));ev32.timestamp.tv_sec = ev->timestamp.tv_sec;ev32.timestamp.tv_nsec = ev->timestamp.tv_nsec;memcpy(&ev32.id, &ev->id, sizeof(*ev) - offsetof(struct v4l2_event, id));This looks dangerous: due to 64-bit alignment requirements the v4l2_event struct may end with a 4-byte hole at the end of the struct, which you do not want to copy to ev32.
I think it is safer to just copy id and reserved separately:
ev32.id = ev->id; memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));Actually I think it's that's also bad: The padding in *ev must already be cleared here (otherwise there is a leak of stack data in the kernel already), so *not* copying the padding requires at least adding a memset upfront.
I would do the per-member copy like I did for v4l2_buffer in my other reply:
struct v4l2_event *ev = parg; struct v4l2_event_time32 ev32 = { .type = ev->type, .pending = ev->pending, .sequence = ev->sequence, .timestamp.tv_sec = ev->timestamp.tv_sec, .timestamp.tv_nsec = ev->timestamp.tv_nsec, .id = ev->id, }; memcpy(ev32.u, ev->u, sizeof(ev->u)); memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved)); if (copy_to_user(arg, &ev32, sizeof(ev32))) return -EFAULT;Unfortunately this is a little uglier because it still requires the two memcpy() for the arrays, but I think it's good enough.
I agree.
Hmm, can't you do .u = ev->u ? Or is that not allowed by this syntax?
Any other ideas? Let me know if I should do a memset() plus individual member copy instead.
I think we have to, unless you have a better solution. This is leaking information from the holes in the struct.
if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))return -ENOIOCTLCMD;rval = v4l2_event_dequeue(vfh, &ev, file->f_flags & O_NONBLOCK);memcpy(ev32, &ev, offsetof(struct v4l2_event, timestamp));ev32->timestamp.tv_sec = ev.timestamp.tv_sec;ev32->timestamp.tv_nsec = ev.timestamp.tv_nsec;memcpy(&ev32->id, &ev.id,sizeof(ev) - offsetof(struct v4l2_event, id));Ditto.
Using the corresponding code here as well.
#define V4L2_EVENT_SUB_FL_SEND_INITIAL (1 << 0) #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK (1 << 1)
@@ -2486,6 +2515,7 @@ struct v4l2_create_buffers { #define VIDIOC_S_DV_TIMINGS _IOWR('V', 87, struct v4l2_dv_timings) #define VIDIOC_G_DV_TIMINGS _IOWR('V', 88, struct v4l2_dv_timings) #define VIDIOC_DQEVENT _IOR('V', 89, struct v4l2_event) +#define VIDIOC_DQEVENT_TIME32 _IOR('V', 89, struct v4l2_event_time32)
Shouldn't this be under #ifdef __KERNEL__?
And should this be in the public header at all? media/v4l2-ioctl.h might be a better place.
Done.
Arnd
Regards,
Hans