Re: [alsa-devel] [PATCH] ALSA: usb-audio: Fix gpf in snd_usb_pipe_sanity_check

Re: [alsa-devel] [PATCH] ALSA: usb-audio: Fix gpf in snd_usb_pipe_sanity_check

[Takashi Iwai]

On Tue, 30 Jul 2019 11:24:36 +0200,
Hillf Danton wrote:
> 
> 
> syzbot found the following crash on:
> 
> HEAD commit:    7f7867ff usb-fuzzer: main usb gadget fuzzer driver
> git tree:       https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=12befdc8600000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=792eb47789f57810
> dashboard link: https://syzkaller.appspot.com/bug?extid=d59c4387bfb6eced94e2
> compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16efc49fa00000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=13245854600000
> 
> usb 1-1: New USB device found, idVendor=07fd, idProduct=0004, bcdDevice=d5.ac
> usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
> usb 1-1: config 0 descriptor??
> usb 1-1: string descriptor 0 read error: -71
> usb 1-1: Waiting for MOTU Microbook II to boot up...
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] SMP KASAN
> CPU: 1 PID: 21 Comm: kworker/1:1 Not tainted 5.3.0-rc2+ #23
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: usb_hub_wq hub_event
> RIP: 0010:snd_usb_pipe_sanity_check+0x80/0x130 sound/usb/helper.c:75
> Code: 48 c1 ea 03 80 3c 02 00 0f 85 b3 00 00 00 48 8b 6d 00 c1 eb 1e 48 b8  
> 00 00 00 00 00 fc ff df 48 8d 7d 03 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48  
> 89 fa 83 e2 07 38 d0 7f 04 84 c0 75 7b 48 b8 00 00
> RSP: 0018:ffff8881da2f7010 EFLAGS: 00010246
> RAX: dffffc0000000000 RBX: 0000000000000001 RCX: ffffffff8484d252
> RDX: 0000000000000000 RSI: ffffffff8484d26c RDI: 0000000000000003
> RBP: 0000000000000000 R08: ffff8881da22e000 R09: ffffed103b665d58
> R10: ffffed103b665d57 R11: ffff8881db32eabf R12: 0000000000000000
> R13: ffff8881d400ba80 R14: 1ffff1103b45ee06 R15: ffff8881c79244a0
> FS:  0000000000000000(0000) GS:ffff8881db300000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f31b2a87000 CR3: 00000001d3fd4000 CR4: 00000000001406e0
> Call Trace:
>   snd_usb_motu_microbookii_communicate.constprop.0+0xa0/0x2fb  sound/usb/quirks.c:1007
>   snd_usb_motu_microbookii_boot_quirk sound/usb/quirks.c:1051 [inline]
>   snd_usb_apply_boot_quirk.cold+0x163/0x370 sound/usb/quirks.c:1280
>   usb_audio_probe+0x2ec/0x2010 sound/usb/card.c:576
>   usb_probe_interface+0x305/0x7a0 drivers/usb/core/driver.c:361
>   really_probe+0x281/0x650 drivers/base/dd.c:548
>   driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
>   __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
>   bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
>   __device_attach+0x217/0x360 drivers/base/dd.c:882
>   bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
>   device_add+0xae6/0x16f0 drivers/base/core.c:2114
>   usb_set_configuration+0xdf6/0x1670 drivers/usb/core/message.c:2023
>   generic_probe+0x9d/0xd5 drivers/usb/core/generic.c:210
>   usb_probe_device+0x99/0x100 drivers/usb/core/driver.c:266
>   really_probe+0x281/0x650 drivers/base/dd.c:548
>   driver_probe_device+0x101/0x1b0 drivers/base/dd.c:709
>   __device_attach_driver+0x1c2/0x220 drivers/base/dd.c:816
>   bus_for_each_drv+0x15c/0x1e0 drivers/base/bus.c:454
>   __device_attach+0x217/0x360 drivers/base/dd.c:882
>   bus_probe_device+0x1e4/0x290 drivers/base/bus.c:514
>   device_add+0xae6/0x16f0 drivers/base/core.c:2114
>   usb_new_device.cold+0x6a4/0xe79 drivers/usb/core/hub.c:2536
>   hub_port_connect drivers/usb/core/hub.c:5098 [inline]
>   hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
>   port_event drivers/usb/core/hub.c:5359 [inline]
>   hub_event+0x1b5c/0x3640 drivers/usb/core/hub.c:5441
>   process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
>   worker_thread+0x96/0xe20 kernel/workqueue.c:2415
>   kthread+0x318/0x420 kernel/kthread.c:255
>   ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> Modules linked in:
> [ end trace 41e8577a8c48635e ]

You don't have to copy the whole these texts at all.
In general, it'd suffice to point out the dashboard URL, and if the
stack trace is mandatory, drop the useless hex numbers and just show
the significant part of the stack trace.

> It was introduced in commit 801ebf1043ae for checking pipe and endpoint
> types. It is fixed by adding a check of the ep pointer in question.
> 
> Reported-by: syzbot <syzbot+d59c4387bfb6eced94e2@syzkaller.appspotmail.com>
> Fixes: commit 801ebf1043ae ("ALSA: usb-audio: Sanity checks for each pipe and EP types")

Drop "commit" word.

> Cc: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Hillf Danton <dhanton@sina.com>
> ---
> This is to make syzbot happy for now and in long run we can make
> snd_usb_pipe_sanity_check() available outside sound/usb by making
> usb_urb_ep_type_check() a wrapper of the former. We will revisit
> sound/usb once when things in the usb/core get in place.

Actually I expected to apply the "long-term" fix now.  The same kind
of fix was already submitted from me (<s5hlfwn376e.wl-tiwai@suse.de>),
but I didn't merge it because working on the usb core helper would be
a saner solution.

If the usb core helper change would take more time, I'm going to merge
this workaround for now with corrections.


thanks,

Takashi

Re: [alsa-devel] [PATCH] ALSA: usb-audio: Fix gpf in snd_usb_pipe_sanity_check

[Takashi Iwai]

On Tue, 30 Jul 2019 12:07:56 +0200,
Takashi Iwai wrote:
> 
> On Tue, 30 Jul 2019 11:24:36 +0200,
> Hillf Danton wrote:
> > Signed-off-by: Hillf Danton <dhanton@sina.com>

BTW, is this address a typo?


Takashi

Re: [alsa-devel] [PATCH] ALSA: usb-audio: Fix gpf in snd_usb_pipe_sanity_check

[Takashi Iwai]

On Tue, 30 Jul 2019 13:28:56 +0200,
Hillf Danton wrote:
> 
> 
> On Tue, 30 Jul 2019 18:08:02 +0800 Takashi Iwai wrote:
> >
> > You don't have to copy the whole these texts at all.
> > In general, it'd suffice to point out the dashboard URL, and if the
> > stack trace is mandatory, drop the useless hex numbers and just show
> > the significant part of the stack trace.
> >
> I am used to give readers as much info as appropriate, and learning that
> an URL is enough in cases like this one. I will send you the pruned
> version next time.

That's fine, I already edited the commit message in my side.

> > > It was introduced in commit 801ebf1043ae for checking pipe and endpoint
> > > types. It is fixed by adding a check of the ep pointer in question.
> > >
> > > Reported-by: syzbot <syzbot+d59c4387bfb6eced94e2@syzkaller.appspotmail.com>
> > > Fixes: commit 801ebf1043ae ("ALSA: usb-audio: Sanity checks for each pipe and EP types")
> >
> > Drop "commit" word.
> >
> Yes, Sir.
> 
> > > Cc: Andrey Konovalov <andreyknvl@google.com>
> > > Signed-off-by: Hillf Danton <dhanton@sina.com>
> > > ---
> There is a typo, s/dhanton/hdanton/, as you pointed out in another mail.

Good, also corrected.

> > > This is to make syzbot happy for now and in long run we can make
> > > snd_usb_pipe_sanity_check() available outside sound/usb by making
> > > usb_urb_ep_type_check() a wrapper of the former. We will revisit
> > > sound/usb once when things in the usb/core get in place.
> >
> > Actually I expected to apply the "long-term" fix now.
> 
> There is change in usb/core included in that fix as you see, and
> according to the rule, one fix a patch, it is better and simpler
> IMO to fix the sound part first.

Yeah, that's a right approach, too.

What I expected was a patch series, containing two changes: one to
add/modify the USB core helper and another to apply it for usb-audio.
But it's fine to move that direction after addressing the usb-audio
problem quickly, of course.

> > The same kind
> > of fix was already submitted from me (<s5hlfwn376e.wl-tiwai@suse.de>),
> > but I didn't merge it because working on the usb core helper would be
> > a saner solution.
> >
> Feel free to merge that, Sir.

Heh, you did it right, so let's merge yours now.

Thanks!


Takashi