From 8dbcc3fa387c16abfc88a628cf528c439c91b332 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Fri, 30 Oct 2020 17:55:22 +0100 Subject: [PATCH] media: v4l2: prepare compat-ioctl rework The v4l2-compat-ioctl32() currently takes an extra round trip through user space pointers when converting the data structure formats. In particular, this involves using the compat_alloc_user_space() and copy_in_user() helpers that often lead to worse compat handlers compared to using in_compat_syscall() checks when copying the data. The native implementation already gained a simpler method to deal with the conversion for the time32 conversion. Hook into the same places to provide a location for reading and writing user space data from inside of the generic video_usercopy() helper. Hans Verkuil rewrote the video_get_user() function here to simplify the zeroing of the extra input fields and fixed a couple of bugs in the original implementation. [hverkuil: fix: CHECK: Please don't use multiple blank lines] Co-developed-by: Hans Verkuil Signed-off-by: Hans Verkuil Signed-off-by: Arnd Bergmann Signed-off-by: Mauro Carvalho Chehab --- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 53 ++++++ drivers/media/v4l2-core/v4l2-ioctl.c | 157 ++++++++++-------- include/media/v4l2-ioctl.h | 10 ++ 3 files changed, 154 insertions(+), 66 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index a99e82ec9ab6..7c939d5c5232 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -1414,6 +1414,59 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64, #define VIDIOC_G_OUTPUT32 _IOR ('V', 46, s32) #define VIDIOC_S_OUTPUT32 _IOWR('V', 47, s32) +unsigned int v4l2_compat_translate_cmd(unsigned int cmd) +{ + switch (cmd) { + } + return cmd; +} + +int v4l2_compat_get_user(void __user *arg, void *parg, unsigned int cmd) +{ + switch (cmd) { + } + return 0; +} + +int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) +{ + switch (cmd) { + } + return 0; +} + +int v4l2_compat_get_array_args(struct file *file, void *mbuf, + void __user *user_ptr, size_t array_size, + unsigned int cmd, void *arg) +{ + int err = 0; + + switch (cmd) { + default: + if (copy_from_user(mbuf, user_ptr, array_size)) + err = -EFAULT; + break; + } + + return err; +} + +int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr, + void *mbuf, size_t array_size, + unsigned int cmd, void *arg) +{ + int err = 0; + + switch (cmd) { + default: + if (copy_to_user(user_ptr, mbuf, array_size)) + err = -EFAULT; + break; + } + + return err; +} + /** * alloc_userspace() - Allocates a 64-bits userspace pointer compatible * for calling the native 64-bits version of an ioctl. diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index eeff398fbdcc..b8be61a09776 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -8,6 +8,7 @@ * Mauro Carvalho Chehab (version 2) */ +#include #include #include #include @@ -3104,14 +3105,18 @@ static unsigned int video_translate_cmd(unsigned int cmd) return VIDIOC_PREPARE_BUF; #endif } + if (in_compat_syscall()) + return v4l2_compat_translate_cmd(cmd); return cmd; } -static int video_get_user(void __user *arg, void *parg, unsigned int cmd, +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 */ @@ -3119,73 +3124,82 @@ static int video_get_user(void __user *arg, void *parg, unsigned int cmd, return 0; } - 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; + /* + * 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; - 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; + if (flags & INFO_FL_CLEAR_MASK) + n = (flags & INFO_FL_CLEAR_MASK) >> 16; + *always_copy = flags & INFO_FL_ALWAYS_COPY; } -#endif - default: - /* - * 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 (cmd == real_cmd) { if (copy_from_user(parg, (void __user *)arg, n)) - return -EFAULT; + 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; - /* zero out anything we don't copy from userspace */ - if (n < _IOC_SIZE(cmd)) - memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n); - break; + 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, unsigned int cmd) +static int video_put_user(void __user *arg, void *parg, + unsigned int real_cmd, unsigned int cmd) { if (!(_IOC_DIR(cmd) & _IOC_READ)) return 0; + if (cmd == real_cmd) { + /* Copy results into user buffer */ + if (copy_to_user(arg, parg, _IOC_SIZE(cmd))) + return -EFAULT; + return 0; + } + + if (in_compat_syscall()) + return v4l2_compat_put_user(arg, parg, cmd); + switch (cmd) { #ifdef CONFIG_COMPAT_32BIT_TIME case VIDIOC_DQEVENT_TIME32: { @@ -3236,11 +3250,6 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd) break; } #endif - default: - /* Copy results into user buffer */ - if (copy_to_user(arg, parg, _IOC_SIZE(cmd))) - return -EFAULT; - break; } return 0; @@ -3274,8 +3283,8 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, parg = mbuf; } - err = video_get_user((void __user *)arg, parg, orig_cmd, - &always_copy); + err = video_get_user((void __user *)arg, parg, cmd, + orig_cmd, &always_copy); if (err) goto out; } @@ -3297,7 +3306,14 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, if (NULL == mbuf) goto out_array_args; err = -EFAULT; - if (copy_from_user(mbuf, user_ptr, array_size)) + if (in_compat_syscall()) + err = v4l2_compat_get_array_args(file, mbuf, user_ptr, + array_size, orig_cmd, + parg); + else + err = copy_from_user(mbuf, user_ptr, array_size) ? + -EFAULT : 0; + if (err) goto out_array_args; *kernel_ptr = mbuf; } @@ -3318,8 +3334,17 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, if (has_array_args) { *kernel_ptr = (void __force *)user_ptr; - 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; } /* @@ -3330,7 +3355,7 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, goto out; out_array_args: - if (video_put_user((void __user *)arg, parg, orig_cmd)) + if (video_put_user((void __user *)arg, parg, cmd, orig_cmd)) err = -EFAULT; out: kvfree(mbuf); diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h index 86878fba332b..edb733f21604 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -686,6 +686,16 @@ long int v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg); #endif +unsigned int v4l2_compat_translate_cmd(unsigned int cmd); +int v4l2_compat_get_user(void __user *arg, void *parg, unsigned int cmd); +int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd); +int v4l2_compat_get_array_args(struct file *file, void *mbuf, + void __user *user_ptr, size_t array_size, + unsigned int cmd, void *arg); +int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr, + void *mbuf, size_t array_size, + unsigned int cmd, void *arg); + /** * typedef v4l2_kioctl - Typedef used to pass an ioctl handler. *