Hi Arnd,
On 17/09/2020 17:28, Arnd Bergmann wrote:
> I have a series to remove all uses of compat_alloc_user_space()
> and copy_in_user() from the kernel, this is the part of it that
> involves the v4l2 compat code.
>
> The resulting code is significantly shorter and arguably more
> readable, but I have not done any testing beyond compilation on
> it, so at the minimum this first needs to pass the test suite
> for both native and compat users space.
>
> Given the complexity of the code, I am fairly sure that there
> are bugs I missed.
>
> Please review and test if possible.
While testing I found a lot of bugs. Below is a patch that sits on top
of your series and fixes all the bugs:
- put_v4l2_standard32: typo: p64->index -> p64->id
- get_v4l2_plane32: typo: p64 -> plane32
typo: duplicate bytesused: the 2nd should be 'length'
- put_v4l2_plane32: typo: duplicate bytesused: the 2nd should be 'length'
- get_v4l2_buffer32/get_v4l2_buffer32_time32: missing compat_ptr for vb32.m.userptr
- get_v4l2_buffer32/get_v4l2_buffer32_time32: drop querybuf bool
- put_v4l2_buffer32/put_v4l2_buffer32_time32: add uintptr_t cast for vb->m.userptr
- get_v4l2_ext_controls32: typo: error_idx -> request_fd
- put_v4l2_ext_controls32: typo: error_idx -> request_fd
- v4l2_compat_translate_cmd: missing case VIDIOC_CREATE_BUFS32
- v4l2_compat_translate_cmd: #ifdef CONFIG_COMPAT_32BIT_TIME for
case VIDIOC_DQEVENT32_TIME32 and return VIDIOC_DQEVENT
- v4l2_compat_put_user: #ifdef CONFIG_COMPAT_32BIT_TIME for case VIDIOC_DQEVENT32_TIME32
- v4l2_compat_get_array_args: typo: p64 -> b64
- v4l2_compat_put_array_args: typo: p64 -> b64
- video_get_user: make sure INFO_FL_CLEAR_MASK is honored, also when in_compat_syscall()
- video_usercopy: err = v4l2_compat_put_array_args overwrote the original ioctl err value.
Handle this correctly.
I've tested this as well with the y2038 compat mode (i686 with 64-bit time_t)
and that too works fine.
Regards,
Hans
Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
---
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index d20c36ab01b4..e8c823776de0 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -258,7 +258,7 @@ static int put_v4l2_standard32(struct v4l2_standard *p64,
struct v4l2_standard32 __user *p32)
{
if (put_user(p64->index, &p32->index) ||
- put_user(p64->index, &p32->id) ||
+ put_user(p64->id, &p32->id) ||
copy_to_user(p32->name, p64->name, sizeof(p32->name)) ||
copy_to_user(&p32->frameperiod, &p64->frameperiod,
sizeof(p32->frameperiod)) ||
@@ -358,10 +358,10 @@ static int get_v4l2_plane32(struct v4l2_plane *p64,
memset(p64, 0, sizeof(*p64));
*p64 = (struct v4l2_plane) {
- .bytesused = p64->bytesused,
- .length = p64->bytesused,
+ .bytesused = plane32.bytesused,
+ .length = plane32.length,
.m = m,
- .data_offset = p64->data_offset,
+ .data_offset = plane32.data_offset,
};
return 0;
@@ -376,7 +376,7 @@ static int put_v4l2_plane32(struct v4l2_plane *p64,
memset(&plane32, 0, sizeof(plane32));
plane32 = (struct v4l2_plane32) {
.bytesused = p64->bytesused,
- .length = p64->bytesused,
+ .length = p64->length,
.data_offset = p64->data_offset,
};
@@ -400,8 +400,7 @@ static int put_v4l2_plane32(struct v4l2_plane *p64,
}
static int get_v4l2_buffer32(struct v4l2_buffer *vb,
- struct v4l2_buffer32 __user *arg,
- bool querybuf)
+ struct v4l2_buffer32 __user *arg)
{
struct v4l2_buffer32 vb32;
@@ -431,7 +430,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer *vb,
vb->m.offset = vb32.m.offset;
break;
case V4L2_MEMORY_USERPTR:
- vb->m.userptr = vb32.m.userptr;
+ vb->m.userptr = (unsigned long)compat_ptr(vb32.m.userptr);
break;
case V4L2_MEMORY_DMABUF:
vb->m.fd = vb32.m.fd;
@@ -442,16 +441,12 @@ static int get_v4l2_buffer32(struct v4l2_buffer *vb,
vb->m.planes = (void __force *)
compat_ptr(vb32.m.planes);
- if (querybuf)
- vb->request_fd = 0;
-
return 0;
}
#ifdef CONFIG_COMPAT_32BIT_TIME
static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb,
- struct v4l2_buffer32_time32 __user *arg,
- bool querybuf)
+ struct v4l2_buffer32_time32 __user *arg)
{
struct v4l2_buffer32_time32 vb32;
@@ -479,7 +474,7 @@ static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb,
vb->m.offset = vb32.m.offset;
break;
case V4L2_MEMORY_USERPTR:
- vb->m.userptr = vb32.m.userptr;
+ vb->m.userptr = (unsigned long)compat_ptr(vb32.m.userptr);
break;
case V4L2_MEMORY_DMABUF:
vb->m.fd = vb32.m.fd;
@@ -489,8 +484,6 @@ static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb,
if (V4L2_TYPE_IS_MULTIPLANAR(vb->type))
vb->m.planes = (void __force *)
compat_ptr(vb32.m.planes);
- if (querybuf)
- vb->request_fd = 0;
return 0;
}
@@ -524,7 +517,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *vb,
vb32.m.offset = vb->m.offset;
break;
case V4L2_MEMORY_USERPTR:
- vb32.m.userptr = vb->m.userptr;
+ vb32.m.userptr = (uintptr_t)(vb->m.userptr);
break;
case V4L2_MEMORY_DMABUF:
vb32.m.fd = vb->m.fd;
@@ -568,7 +561,7 @@ static int put_v4l2_buffer32_time32(struct v4l2_buffer *vb,
vb32.m.offset = vb->m.offset;
break;
case V4L2_MEMORY_USERPTR:
- vb32.m.userptr = vb->m.userptr;
+ vb32.m.userptr = (uintptr_t)(vb->m.userptr);
break;
case V4L2_MEMORY_DMABUF:
vb32.m.fd = vb->m.fd;
@@ -722,7 +715,7 @@ static int get_v4l2_ext_controls32(struct v4l2_ext_controls *p64,
.which = ec32.which,
.count = ec32.count,
.error_idx = ec32.error_idx,
- .request_fd = ec32.error_idx,
+ .request_fd = ec32.request_fd,
.reserved[0] = ec32.reserved[0],
.controls = (void __force *)compat_ptr(ec32.controls),
};
@@ -740,7 +733,7 @@ static int put_v4l2_ext_controls32(struct v4l2_ext_controls *p64,
.which = p64->which,
.count = p64->count,
.error_idx = p64->error_idx,
- .request_fd = p64->error_idx,
+ .request_fd = p64->request_fd,
.reserved[0] = p64->reserved[0],
.controls = (uintptr_t)p64->controls,
};
@@ -910,8 +903,10 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd)
return VIDIOC_QBUF;
case VIDIOC_DQBUF32:
return VIDIOC_DQBUF;
+ case VIDIOC_CREATE_BUFS32:
+ return VIDIOC_CREATE_BUFS;
case VIDIOC_G_EXT_CTRLS32:
- return VIDIOC_G_EXT_CTRLS;
+ return VIDIOC_G_EXT_CTRLS;
case VIDIOC_S_EXT_CTRLS32:
return VIDIOC_S_EXT_CTRLS;
case VIDIOC_TRY_EXT_CTRLS32:
@@ -929,8 +924,10 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd)
#ifdef CONFIG_X86_64
case VIDIOC_DQEVENT32:
return VIDIOC_DQEVENT;
+#ifdef CONFIG_COMPAT_32BIT_TIME
case VIDIOC_DQEVENT32_TIME32:
- return VIDIOC_DQEVENT_TIME32;
+ return VIDIOC_DQEVENT;
+#endif
#endif
}
return cmd;
@@ -951,15 +948,13 @@ int v4l2_compat_get_user(void __user *arg, void *parg, unsigned int cmd)
case VIDIOC_QBUF32_TIME32:
case VIDIOC_DQBUF32_TIME32:
case VIDIOC_PREPARE_BUF32_TIME32:
- return get_v4l2_buffer32_time32(parg, arg,
- cmd == VIDIOC_QUERYBUF32_TIME32);
+ return get_v4l2_buffer32_time32(parg, arg);
#endif
case VIDIOC_QUERYBUF32:
case VIDIOC_QBUF32:
case VIDIOC_DQBUF32:
case VIDIOC_PREPARE_BUF32:
- return get_v4l2_buffer32(parg, arg,
- cmd == VIDIOC_QUERYBUF32);
+ return get_v4l2_buffer32(parg, arg);
case VIDIOC_G_EXT_CTRLS32:
case VIDIOC_S_EXT_CTRLS32:
@@ -1025,8 +1020,10 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd)
#ifdef CONFIG_X86_64
case VIDIOC_DQEVENT32:
return put_v4l2_event32(parg, arg);
+#ifdef CONFIG_COMPAT_32BIT_TIME
case VIDIOC_DQEVENT32_TIME32:
return put_v4l2_event32_time32(parg, arg);
+#endif
#endif
}
return 0;
@@ -1073,9 +1070,10 @@ int v4l2_compat_get_array_args(struct file *file, void *mbuf,
struct v4l2_buffer *b64 = arg;
struct v4l2_plane *p64 = mbuf;
struct v4l2_plane32 __user *p32 = user_ptr;
- u32 num_planes = p64->length;
if (V4L2_TYPE_IS_MULTIPLANAR(b64->type)) {
+ u32 num_planes = b64->length;
+
if (num_planes == 0)
return 0;
@@ -1161,9 +1159,10 @@ int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr,
struct v4l2_buffer *b64 = arg;
struct v4l2_plane *p64 = mbuf;
struct v4l2_plane32 __user *p32 = user_ptr;
- u32 num_planes = p64->length;
if (V4L2_TYPE_IS_MULTIPLANAR(b64->type)) {
+ u32 num_planes = b64->length;
+
if (num_planes == 0)
return 0;
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 114eaa79b6f5..aacb13b8f0a9 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3133,7 +3133,8 @@ static int video_get_user(void __user *arg, void *parg,
unsigned int real_cmd, unsigned int cmd,
bool *always_copy)
{
- unsigned int n = _IOC_SIZE(cmd);
+ unsigned int n = _IOC_SIZE(real_cmd);
+ int err = 0;
if (!(_IOC_DIR(cmd) & _IOC_WRITE)) {
/* read-only ioctl */
@@ -3141,70 +3142,64 @@ static int video_get_user(void __user *arg, void *parg,
return 0;
}
- if (cmd == real_cmd) {
- /*
- * In some cases, only a few fields are used as input,
- * i.e. when the app sets "index" and then the driver
- * fills in the rest of the structure for the thing
- * with that index. We only need to copy up the first
- * non-input field.
- */
- if (v4l2_is_known_ioctl(cmd)) {
- u32 flags = v4l2_ioctls[_IOC_NR(cmd)].flags;
-
- if (flags & INFO_FL_CLEAR_MASK)
- n = (flags & INFO_FL_CLEAR_MASK) >> 16;
- *always_copy = flags & INFO_FL_ALWAYS_COPY;
- }
-
- if (copy_from_user(parg, (void __user *)arg, n))
- return -EFAULT;
+ /*
+ * In some cases, only a few fields are used as input,
+ * i.e. when the app sets "index" and then the driver
+ * fills in the rest of the structure for the thing
+ * with that index. We only need to copy up the first
+ * non-input field.
+ */
+ if (v4l2_is_known_ioctl(real_cmd)) {
+ u32 flags = v4l2_ioctls[_IOC_NR(real_cmd)].flags;
- /* zero out anything we don't copy from userspace */
- if (n < _IOC_SIZE(cmd))
- memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
- return 0;
+ if (flags & INFO_FL_CLEAR_MASK)
+ n = (flags & INFO_FL_CLEAR_MASK) >> 16;
+ *always_copy = flags & INFO_FL_ALWAYS_COPY;
}
- if (in_compat_syscall())
- return v4l2_compat_get_user(arg, parg, cmd);
-
- switch (cmd) {
+ 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) {
#ifdef CONFIG_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;
-
- *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,
- };
-
- if (cmd == VIDIOC_QUERYBUF_TIME32)
- vb->request_fd = 0;
-
- 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;
+
+ 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
+ }
}
- return 0;
+
+ /* 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;
}
static int video_put_user(void __user *arg, void *parg,
@@ -3357,12 +3352,17 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
if (has_array_args) {
*kernel_ptr = (void __force *)user_ptr;
- if (in_compat_syscall())
- err = v4l2_compat_put_array_args(file, user_ptr, mbuf,
- array_size, orig_cmd,
- parg);
- else if (copy_to_user(user_ptr, mbuf, array_size))
+ if (in_compat_syscall()) {
+ int put_err;
+
+ put_err = v4l2_compat_put_array_args(file, user_ptr, mbuf,
+ array_size, orig_cmd,
+ parg);
+ if (put_err)
+ err = put_err;
+ } else if (copy_to_user(user_ptr, mbuf, array_size)) {
err = -EFAULT;
+ }
goto out_array_args;
}
/*
