2021-03-18 13:45:20

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 1/2] media: v4l2-core: ignore native time32 ioctls on 64-bit

From: Arnd Bergmann <[email protected]>

Syzbot found that passing ioctl command 0xc0505609 into a 64-bit
kernel from a 32-bit process causes uninitialized kernel memory to
get passed to drivers instead of the user space data:

BUG: KMSAN: uninit-value in check_array_args drivers/media/v4l2-core/v4l2-ioctl.c:3041 [inline]
BUG: KMSAN: uninit-value in video_usercopy+0x1631/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3315
CPU: 0 PID: 19595 Comm: syz-executor.4 Not tainted 5.11.0-rc7-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:79 [inline]
dump_stack+0x21c/0x280 lib/dump_stack.c:120
kmsan_report+0xfb/0x1e0 mm/kmsan/kmsan_report.c:118
__msan_warning+0x5f/0xa0 mm/kmsan/kmsan_instr.c:197
check_array_args drivers/media/v4l2-core/v4l2-ioctl.c:3041 [inline]
video_usercopy+0x1631/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3315
video_ioctl2+0x9f/0xb0 drivers/media/v4l2-core/v4l2-ioctl.c:3391
v4l2_ioctl+0x255/0x290 drivers/media/v4l2-core/v4l2-dev.c:360
v4l2_compat_ioctl32+0x2c6/0x370 drivers/media/v4l2-core/v4l2-compat-ioctl32.c:1248
__do_compat_sys_ioctl fs/ioctl.c:842 [inline]
__se_compat_sys_ioctl+0x53d/0x1100 fs/ioctl.c:793
__ia32_compat_sys_ioctl+0x4a/0x70 fs/ioctl.c:793
do_syscall_32_irqs_on arch/x86/entry/common.c:79 [inline]
__do_fast_syscall_32+0x102/0x160 arch/x86/entry/common.c:141
do_fast_syscall_32+0x6a/0xc0 arch/x86/entry/common.c:166
do_SYSENTER_32+0x73/0x90 arch/x86/entry/common.c:209
entry_SYSENTER_compat_after_hwframe+0x4d/0x5c

The time32 commands are defined but were never meant to be called on
64-bit machines, as those have always used time64 interfaces. I missed
this in my patch that introduced the time64 handling on 32-bit platforms.

The problem in this case is the mismatch of one function checking for
the numeric value of the command and another function checking for the
type of process (native vs compat) instead, with the result being that
for this combination, nothing gets copied into the buffer at all.

Avoid this by only trying to convert the time32 commands when running
on a 32-bit kernel where these are defined in a meaningful way.

Fixes: 577c89b0ce72 ("media: v4l2-core: fix v4l2_buffer handling for time64 ABI")
Reported-by: [email protected]
Tested-by: Hans Verkuil <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
---
This patch adds two more changes than the version that Hans tested
---
drivers/media/v4l2-core/v4l2-ioctl.c | 6 +++---
drivers/media/v4l2-core/v4l2-subdev.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 31d1342e61e8..2b1bb68dc27f 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3115,7 +3115,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
static unsigned int video_translate_cmd(unsigned int cmd)
{
switch (cmd) {
-#ifdef CONFIG_COMPAT_32BIT_TIME
+#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
case VIDIOC_DQEVENT_TIME32:
return VIDIOC_DQEVENT;
case VIDIOC_QUERYBUF_TIME32:
@@ -3169,7 +3169,7 @@ static int video_get_user(void __user *arg, void *parg,
err = v4l2_compat_get_user(arg, parg, cmd);
} else {
switch (cmd) {
-#ifdef CONFIG_COMPAT_32BIT_TIME
+#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
case VIDIOC_QUERYBUF_TIME32:
case VIDIOC_QBUF_TIME32:
case VIDIOC_DQBUF_TIME32:
@@ -3224,7 +3224,7 @@ static int video_put_user(void __user *arg, void *parg,
return v4l2_compat_put_user(arg, parg, cmd);

switch (cmd) {
-#ifdef CONFIG_COMPAT_32BIT_TIME
+#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
case VIDIOC_DQEVENT_TIME32: {
struct v4l2_event *ev = parg;
struct v4l2_event_time32 ev32;
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 336133dbc759..9f5573d3b857 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -428,7 +428,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)

return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);

-#ifdef CONFIG_COMPAT_32BIT_TIME
+#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
case VIDIOC_DQEVENT_TIME32: {
struct v4l2_event_time32 *ev32 = arg;
struct v4l2_event ev = { };
--
2.29.2


2021-03-18 13:46:34

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 2/2] media: v4l2-core: explicitly clear ioctl input data

From: Arnd Bergmann <[email protected]>

As seen from a recent syzbot bug report, mistakes in the compat ioctl
implementation can lead to uninitialized kernel stack data getting used
as input for driver ioctl handlers.

The reported bug is now fixed, but it's possible that other related
bugs are still present or get added in the future. As the drivers need
to check user input already, the possible impact is fairly low, but it
might still cause an information leak.

To be on the safe side, always clear the entire ioctl buffer before
calling the conversion handler functions that are meant to initialize
them.

Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++------------
1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 2b1bb68dc27f..6cec92d0972c 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3164,12 +3164,23 @@ static int video_get_user(void __user *arg, void *parg,

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) {
+ return -EFAULT;
+
+ /* zero out anything we don't copy from userspace */
+ if (n < _IOC_SIZE(real_cmd))
+ memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);
+
+ return 0;
+ }
+
+ /* zero out whole buffer first to deal with missing emulation */
+ memset(parg, 0, _IOC_SIZE(real_cmd));
+
+ if (in_compat_syscall())
+ return v4l2_compat_get_user(arg, parg, cmd);
+
#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
+ switch (cmd) {
case VIDIOC_QUERYBUF_TIME32:
case VIDIOC_QBUF_TIME32:
case VIDIOC_DQBUF_TIME32:
@@ -3182,28 +3193,24 @@ static int video_get_user(void __user *arg, void *parg,

*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,
+ .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
- }
}
+#endif

- /* 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;
}

--
2.29.2

2021-03-18 15:05:11

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH 1/2] media: v4l2-core: ignore native time32 ioctls on 64-bit

Hi Arnd,

Thank you for the patch.

On Thu, Mar 18, 2021 at 02:43:18PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> Syzbot found that passing ioctl command 0xc0505609 into a 64-bit
> kernel from a 32-bit process causes uninitialized kernel memory to
> get passed to drivers instead of the user space data:
>
> BUG: KMSAN: uninit-value in check_array_args drivers/media/v4l2-core/v4l2-ioctl.c:3041 [inline]
> BUG: KMSAN: uninit-value in video_usercopy+0x1631/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3315
> CPU: 0 PID: 19595 Comm: syz-executor.4 Not tainted 5.11.0-rc7-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:79 [inline]
> dump_stack+0x21c/0x280 lib/dump_stack.c:120
> kmsan_report+0xfb/0x1e0 mm/kmsan/kmsan_report.c:118
> __msan_warning+0x5f/0xa0 mm/kmsan/kmsan_instr.c:197
> check_array_args drivers/media/v4l2-core/v4l2-ioctl.c:3041 [inline]
> video_usercopy+0x1631/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3315
> video_ioctl2+0x9f/0xb0 drivers/media/v4l2-core/v4l2-ioctl.c:3391
> v4l2_ioctl+0x255/0x290 drivers/media/v4l2-core/v4l2-dev.c:360
> v4l2_compat_ioctl32+0x2c6/0x370 drivers/media/v4l2-core/v4l2-compat-ioctl32.c:1248
> __do_compat_sys_ioctl fs/ioctl.c:842 [inline]
> __se_compat_sys_ioctl+0x53d/0x1100 fs/ioctl.c:793
> __ia32_compat_sys_ioctl+0x4a/0x70 fs/ioctl.c:793
> do_syscall_32_irqs_on arch/x86/entry/common.c:79 [inline]
> __do_fast_syscall_32+0x102/0x160 arch/x86/entry/common.c:141
> do_fast_syscall_32+0x6a/0xc0 arch/x86/entry/common.c:166
> do_SYSENTER_32+0x73/0x90 arch/x86/entry/common.c:209
> entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
>
> The time32 commands are defined but were never meant to be called on
> 64-bit machines, as those have always used time64 interfaces. I missed
> this in my patch that introduced the time64 handling on 32-bit platforms.
>
> The problem in this case is the mismatch of one function checking for
> the numeric value of the command and another function checking for the
> type of process (native vs compat) instead, with the result being that
> for this combination, nothing gets copied into the buffer at all.
>
> Avoid this by only trying to convert the time32 commands when running
> on a 32-bit kernel where these are defined in a meaningful way.
>
> Fixes: 577c89b0ce72 ("media: v4l2-core: fix v4l2_buffer handling for time64 ABI")
> Reported-by: [email protected]
> Tested-by: Hans Verkuil <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>

v4l2_event vs. v4l2_event32 vs. v4l2_event_time32 vs.
v4l2_event32_time32 is a bit confusing. Do I understand correctly that
the code below runs for the non-compat path, thus native userspace
(32-bit on 32-bit machines, 64-bit on 64-bit machines), and handles the
case of a native userspace using a 32-bit time ? If so it indeed doesn't
make sense for 64-bit machines.

Reviewed-by: Laurent Pinchart <[email protected]>

> ---
> This patch adds two more changes than the version that Hans tested
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 6 +++---
> drivers/media/v4l2-core/v4l2-subdev.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 31d1342e61e8..2b1bb68dc27f 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3115,7 +3115,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
> static unsigned int video_translate_cmd(unsigned int cmd)
> {
> switch (cmd) {
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_DQEVENT_TIME32:
> return VIDIOC_DQEVENT;
> case VIDIOC_QUERYBUF_TIME32:
> @@ -3169,7 +3169,7 @@ static int video_get_user(void __user *arg, void *parg,
> err = v4l2_compat_get_user(arg, parg, cmd);
> } else {
> switch (cmd) {
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_QUERYBUF_TIME32:
> case VIDIOC_QBUF_TIME32:
> case VIDIOC_DQBUF_TIME32:
> @@ -3224,7 +3224,7 @@ static int video_put_user(void __user *arg, void *parg,
> return v4l2_compat_put_user(arg, parg, cmd);
>
> switch (cmd) {
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_DQEVENT_TIME32: {
> struct v4l2_event *ev = parg;
> struct v4l2_event_time32 ev32;
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 336133dbc759..9f5573d3b857 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -428,7 +428,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>
> return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
>
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_DQEVENT_TIME32: {
> struct v4l2_event_time32 *ev32 = arg;
> struct v4l2_event ev = { };

--
Regards,

Laurent Pinchart

2021-03-18 15:13:49

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH 2/2] media: v4l2-core: explicitly clear ioctl input data

Hi Arnd,

Thank you for the patch.

On Thu, Mar 18, 2021 at 02:43:19PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> As seen from a recent syzbot bug report, mistakes in the compat ioctl
> implementation can lead to uninitialized kernel stack data getting used
> as input for driver ioctl handlers.
>
> The reported bug is now fixed, but it's possible that other related
> bugs are still present or get added in the future. As the drivers need
> to check user input already, the possible impact is fairly low, but it
> might still cause an information leak.
>
> To be on the safe side, always clear the entire ioctl buffer before
> calling the conversion handler functions that are meant to initialize
> them.
>
> Signed-off-by: Arnd Bergmann <[email protected]>

Reviewed-by: Laurent Pinchart <[email protected]>

> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++------------
> 1 file changed, 29 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 2b1bb68dc27f..6cec92d0972c 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3164,12 +3164,23 @@ static int video_get_user(void __user *arg, void *parg,
>
> 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) {
> + return -EFAULT;
> +
> + /* zero out anything we don't copy from userspace */
> + if (n < _IOC_SIZE(real_cmd))
> + memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);
> +
> + return 0;
> + }
> +
> + /* zero out whole buffer first to deal with missing emulation */
> + memset(parg, 0, _IOC_SIZE(real_cmd));
> +
> + if (in_compat_syscall())
> + return v4l2_compat_get_user(arg, parg, cmd);
> +
> #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> + switch (cmd) {
> case VIDIOC_QUERYBUF_TIME32:
> case VIDIOC_QBUF_TIME32:
> case VIDIOC_DQBUF_TIME32:
> @@ -3182,28 +3193,24 @@ static int video_get_user(void __user *arg, void *parg,
>
> *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,
> + .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
> - }
> }
> +#endif
>
> - /* 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;
> }
>

--
Regards,

Laurent Pinchart

2021-03-18 15:27:05

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 1/2] media: v4l2-core: ignore native time32 ioctls on 64-bit

On Thu, Mar 18, 2021 at 4:00 PM Laurent Pinchart
<[email protected]> wrote:
> On Thu, Mar 18, 2021 at 02:43:18PM +0100, Arnd Bergmann wrote:
>
> v4l2_event vs. v4l2_event32 vs. v4l2_event_time32 vs.
> v4l2_event32_time32 is a bit confusing. Do I understand correctly that
> the code below runs for the non-compat path, thus native userspace
> (32-bit on 32-bit machines, 64-bit on 64-bit machines), and handles the
> case of a native userspace using a 32-bit time ? If so it indeed doesn't
> make sense for 64-bit machines.

Yes, that's correct. The way the structures are defined, we have
all four combinations of native/compat pointer and time64/time32
timestamps, but the combination of native pointer with time32
stamps only makes sense for 32-bit users.

> Reviewed-by: Laurent Pinchart <[email protected]>

Thanks,

Arnd

2021-03-21 08:52:39

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH 1/2] media: v4l2-core: ignore native time32 ioctls on 64-bit

Hi Arnd,

On 18/03/2021 14:43, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> Syzbot found that passing ioctl command 0xc0505609 into a 64-bit
> kernel from a 32-bit process causes uninitialized kernel memory to
> get passed to drivers instead of the user space data:
>
> BUG: KMSAN: uninit-value in check_array_args drivers/media/v4l2-core/v4l2-ioctl.c:3041 [inline]
> BUG: KMSAN: uninit-value in video_usercopy+0x1631/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3315
> CPU: 0 PID: 19595 Comm: syz-executor.4 Not tainted 5.11.0-rc7-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:79 [inline]
> dump_stack+0x21c/0x280 lib/dump_stack.c:120
> kmsan_report+0xfb/0x1e0 mm/kmsan/kmsan_report.c:118
> __msan_warning+0x5f/0xa0 mm/kmsan/kmsan_instr.c:197
> check_array_args drivers/media/v4l2-core/v4l2-ioctl.c:3041 [inline]
> video_usercopy+0x1631/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3315
> video_ioctl2+0x9f/0xb0 drivers/media/v4l2-core/v4l2-ioctl.c:3391
> v4l2_ioctl+0x255/0x290 drivers/media/v4l2-core/v4l2-dev.c:360
> v4l2_compat_ioctl32+0x2c6/0x370 drivers/media/v4l2-core/v4l2-compat-ioctl32.c:1248
> __do_compat_sys_ioctl fs/ioctl.c:842 [inline]
> __se_compat_sys_ioctl+0x53d/0x1100 fs/ioctl.c:793
> __ia32_compat_sys_ioctl+0x4a/0x70 fs/ioctl.c:793
> do_syscall_32_irqs_on arch/x86/entry/common.c:79 [inline]
> __do_fast_syscall_32+0x102/0x160 arch/x86/entry/common.c:141
> do_fast_syscall_32+0x6a/0xc0 arch/x86/entry/common.c:166
> do_SYSENTER_32+0x73/0x90 arch/x86/entry/common.c:209
> entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
>
> The time32 commands are defined but were never meant to be called on
> 64-bit machines, as those have always used time64 interfaces. I missed
> this in my patch that introduced the time64 handling on 32-bit platforms.
>
> The problem in this case is the mismatch of one function checking for
> the numeric value of the command and another function checking for the
> type of process (native vs compat) instead, with the result being that
> for this combination, nothing gets copied into the buffer at all.
>
> Avoid this by only trying to convert the time32 commands when running
> on a 32-bit kernel where these are defined in a meaningful way.
>
> Fixes: 577c89b0ce72 ("media: v4l2-core: fix v4l2_buffer handling for time64 ABI")
> Reported-by: [email protected]
> Tested-by: Hans Verkuil <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> This patch adds two more changes than the version that Hans tested
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 6 +++---
> drivers/media/v4l2-core/v4l2-subdev.c | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 31d1342e61e8..2b1bb68dc27f 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3115,7 +3115,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
> static unsigned int video_translate_cmd(unsigned int cmd)
> {
> switch (cmd) {
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_DQEVENT_TIME32:
> return VIDIOC_DQEVENT;
> case VIDIOC_QUERYBUF_TIME32:
> @@ -3169,7 +3169,7 @@ static int video_get_user(void __user *arg, void *parg,
> err = v4l2_compat_get_user(arg, parg, cmd);
> } else {
> switch (cmd) {
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_QUERYBUF_TIME32:
> case VIDIOC_QBUF_TIME32:
> case VIDIOC_DQBUF_TIME32:
> @@ -3224,7 +3224,7 @@ static int video_put_user(void __user *arg, void *parg,
> return v4l2_compat_put_user(arg, parg, cmd);
>
> switch (cmd) {
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_DQEVENT_TIME32: {
> struct v4l2_event *ev = parg;
> struct v4l2_event_time32 ev32;
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 336133dbc759..9f5573d3b857 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -428,7 +428,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>
> return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
>
> -#ifdef CONFIG_COMPAT_32BIT_TIME
> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
> case VIDIOC_DQEVENT_TIME32: {
> struct v4l2_event_time32 *ev32 = arg;
> struct v4l2_event ev = { };
>

This chunk doesn't apply since there is no '#ifdef CONFIG_COMPAT_32BIT_TIME' in
either the mainline kernel or the media_tree master branch.

Are we missing a patch for v4l2-subdev.c?

Regards,

Hans

2021-03-25 07:45:58

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH 1/2] media: v4l2-core: ignore native time32 ioctls on 64-bit

On 21/03/2021 09:50, Hans Verkuil wrote:
> Hi Arnd,

<snip>

>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 336133dbc759..9f5573d3b857 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -428,7 +428,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>
>> return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
>>
>> -#ifdef CONFIG_COMPAT_32BIT_TIME
>> +#if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
>> case VIDIOC_DQEVENT_TIME32: {
>> struct v4l2_event_time32 *ev32 = arg;
>> struct v4l2_event ev = { };
>>
>
> This chunk doesn't apply since there is no '#ifdef CONFIG_COMPAT_32BIT_TIME' in
> either the mainline kernel or the media_tree master branch.
>
> Are we missing a patch for v4l2-subdev.c?

Ping!

Hans

>
> Regards,
>
> Hans
>