Re: [syzbot] KMSAN: uninit-value in video_usercopy (2)

[Arnd Bergmann]

On Tue, Mar 16, 2021 at 3:02 PM Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
> On Tue, Mar 16, 2021 at 2:56 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Tue, Mar 16, 2021 at 11:44 AM Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
> > > On Tue, Mar 16, 2021 at 11:18 AM syzbot
> > > <syzbot+142888ffec98ab194028@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > Hello,
> > > >
> > > > syzbot found the following issue on:
> > > >
> > > > HEAD commit:    29ad81a1 arch/x86: add missing include to sparsemem.h
> >
> > This tree seems to be missing fb18802a338b ("media: v4l: ioctl: Fix memory
> > leak in video_usercopy"), which rewrote that function partly and might
> > fix the problem.
> >
> > > > Local variable ----sbuf@video_usercopy created at:
> > > >  video_usercopy+0xaa/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3285
> > > >  video_usercopy+0xaa/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3285
> > > > =====================================================
> > > > =====================================================
> > > > BUG: KMSAN: uninit-value in check_fmt+0x864/0x1070 drivers/media/v4l2-core/v4l2-ioctl.c:963
> > > > CPU: 0 PID: 19595 Comm: syz-executor.4 Tainted: G    B             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_fmt+0x864/0x1070 drivers/media/v4l2-core/v4l2-ioctl.c:963
> > > >  v4l_prepare_buf+0xbf/0x1d0 drivers/media/v4l2-core/v4l2-ioctl.c:2107
> > > >  __video_do_ioctl+0x15cd/0x1d20 drivers/media/v4l2-core/v4l2-ioctl.c:2993
> > > >  video_usercopy+0x2313/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3345
> > > >  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
> > > > RIP: 0023:0xf7fec549
> > > > Code: 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00
> > > > RSP: 002b:00000000f55e65fc EFLAGS: 00000296 ORIG_RAX: 0000000000000036
> > > > RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00000000c050565d
> > > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> > > > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > > > R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> > > >
> > > > Local variable ----sbuf@video_usercopy created at:
> > > >  video_usercopy+0xaa/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3285
> > > >  video_usercopy+0xaa/0x3d30 drivers/media/v4l2-core/v4l2-ioctl.c:3285
> > > > =====================================================
> > >
> > > I did not get to the very bottom of this, but I looked at this a bit.
> > > It seems to be related to some unfortunate interaction of compat
> > > syscall and CONFIG_COMPAT_32BIT_TIME. It seems that in this case
> > > nothing at all is copied from userspace because cmd gets messed up or
> > > something. Perhaps VIDIOC_QUERYBUF is translated into
> > > VIDIOC_QUERYBUF_TIME32 instead of VIDIOC_QUERYBUF32_TIME32 and then
> > > this gets into compat syscall path and v4l2_compat_get_user does not
> > > recognize the command, copies nothing but returns 0.
> >
> > User space would be calling VIDIOC_QUERYBUF32_TIME32 here,
> > if it's built against glibc, though with a musl based user space, you
> > would get called with VIDIOC_QUERYBUF32.
>
> Or somebody fetching somebody else's credit card number will be
> calling VIDIOC_QUERYBUF_TIME32 directly ;)

Ah of course, I forgot the ioctl command may already be fuzzed here.

When I look at
https://syzkaller.appspot.com/text?tag=CrashLog&x=12bd0e3ad00000

I see 0xc0585609, which would be a VIDIOC_QUERYBUF with
size=0x58, which is the native ioctl, not the compat one. This
is something we didn't expect to get passed into the compat ioctl
handler, but should of course handle gracefully

If the command were to get is the 64-bit version of
VIDIOC_QUERYBUF_TIME32 (0xc0505609), then it gets converted to
VIDIOC_QUERYBUF by video_translate_cmd().
If it's VIDIOC_QUERYBUF, it stays that way.

It does break down in v4l2_compat_get_user() when we get
called with VIDIOC_QUERYBUF_TIME32, since that leads
to not copying at all, as you guessed.

I think this should fix the case of passing
VIDIOC_QUERYBUF_TIME32:

--- 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:

       Arnd