usb/sound/bcd2000: warning in bcd2000_init_device

usb/sound/bcd2000: warning in bcd2000_init_device

[Andrey Konovalov]

Hi!

I've got the following report while fuzzing the kernel with syzkaller.

On commit e19b205be43d11bff638cad4487008c48d21c103 (4.14-rc2).

It seems that there's no check of the endpoint type.

usb 1-1: BOGUS urb xfer, pipe 1 != type 3
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449
usb_submit_urb+0xf8a/0x11d0
Modules linked in:
CPU: 0 PID: 1846 Comm: kworker/0:2 Not tainted
4.14.0-rc2-42613-g1488251d1a98 #238
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: usb_hub_wq hub_event
task: ffff880064296300 task.stack: ffff8800643b0000
RIP: 0010:usb_submit_urb+0xf8a/0x11d0 drivers/usb/core/urb.c:448
RSP: 0018:ffff8800643b6140 EFLAGS: 00010286
RAX: 0000000000000029 RBX: ffff880063842400 RCX: 0000000000000000
RDX: 0000000000000029 RSI: ffffffff85a58800 RDI: ffffed000c876c1a
RBP: ffff8800643b6240 R08: 1ffff1000c876ac0 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 1ffff1000c876c2f
R13: 0000000000000003 R14: 0000000000000001 R15: ffff88006325c768
FS:  0000000000000000(0000) GS:ffff88006c800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fa2a51c0000 CR3: 000000006acc8000 CR4: 00000000000006f0
Call Trace:
 bcd2000_init_device sound/usb/bcd2000/bcd2000.c:289
 bcd2000_init_midi sound/usb/bcd2000/bcd2000.c:345
 bcd2000_probe+0xe64/0x19e0 sound/usb/bcd2000/bcd2000.c:406
 usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
 really_probe drivers/base/dd.c:413
 driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
 __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
 bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
 __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
 device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
 bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
 device_add+0xd0b/0x1660 drivers/base/core.c:1835
 usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
 generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
 usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
 really_probe drivers/base/dd.c:413
 driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
 __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
 bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
 __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
 device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
 bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
 device_add+0xd0b/0x1660 drivers/base/core.c:1835
 usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
 hub_port_connect drivers/usb/core/hub.c:4903
 hub_port_connect_change drivers/usb/core/hub.c:5009
 port_event drivers/usb/core/hub.c:5115
 hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
 process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
 worker_thread+0x221/0x1850 kernel/workqueue.c:2253
 kthread+0x3a1/0x470 kernel/kthread.c:231
 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
Code: 48 8b 85 30 ff ff ff 48 8d b8 98 00 00 00 e8 6e 91 8b ff 45 89
e8 44 89 f1 4c 89 fa 48 89 c6 48 c7 c7 c0 5a c8 85 e8 10 50 dd fd <0f>
ff e9 9b f7 ff ff e8 ba cf 26 fe e9 80 f7 ff ff e8 a0 a5 f4
---[ end trace bad127706d5fe2d6 ]---

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Mon, 25 Sep 2017 14:39:51 +0200,
Andrey Konovalov wrote:
> 
> Hi!
> 
> I've got the following report while fuzzing the kernel with syzkaller.
> 
> On commit e19b205be43d11bff638cad4487008c48d21c103 (4.14-rc2).
> 
> It seems that there's no check of the endpoint type.
> 
> usb 1-1: BOGUS urb xfer, pipe 1 != type 3
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449

How is this bug triggered?  As it's syzkaller with QEMU, it looks
hitting an inconsistent state the driver didn't expect (it sets the
fixed endpoint), then USB-core detects the inconsistency and spews the
kernel warning with stack trace.  If so, it's no serious problem as it
appears.

Suppose my guess is right, I'm not sure what's the best way to fix
this.  Certainly we can add more sanity check in the caller side.
OTOH, I find the reaction of USB core too aggressive, it's not
necessary to be dev_WARN() but a normal dev_err().
Or I might be looking at a wrong place?

Adding USB guys to Cc for hearing their comments.


thanks,

Takashi


> usb_submit_urb+0xf8a/0x11d0
> Modules linked in:
> CPU: 0 PID: 1846 Comm: kworker/0:2 Not tainted
> 4.14.0-rc2-42613-g1488251d1a98 #238
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Workqueue: usb_hub_wq hub_event
> task: ffff880064296300 task.stack: ffff8800643b0000
> RIP: 0010:usb_submit_urb+0xf8a/0x11d0 drivers/usb/core/urb.c:448
> RSP: 0018:ffff8800643b6140 EFLAGS: 00010286
> RAX: 0000000000000029 RBX: ffff880063842400 RCX: 0000000000000000
> RDX: 0000000000000029 RSI: ffffffff85a58800 RDI: ffffed000c876c1a
> RBP: ffff8800643b6240 R08: 1ffff1000c876ac0 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 1ffff1000c876c2f
> R13: 0000000000000003 R14: 0000000000000001 R15: ffff88006325c768
> FS:  0000000000000000(0000) GS:ffff88006c800000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fa2a51c0000 CR3: 000000006acc8000 CR4: 00000000000006f0
> Call Trace:
>  bcd2000_init_device sound/usb/bcd2000/bcd2000.c:289
>  bcd2000_init_midi sound/usb/bcd2000/bcd2000.c:345
>  bcd2000_probe+0xe64/0x19e0 sound/usb/bcd2000/bcd2000.c:406
>  usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
>  really_probe drivers/base/dd.c:413
>  driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
>  __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
>  bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
>  __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
>  device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
>  bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
>  device_add+0xd0b/0x1660 drivers/base/core.c:1835
>  usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
>  generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
>  usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
>  really_probe drivers/base/dd.c:413
>  driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
>  __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
>  bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
>  __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
>  device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
>  bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
>  device_add+0xd0b/0x1660 drivers/base/core.c:1835
>  usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
>  hub_port_connect drivers/usb/core/hub.c:4903
>  hub_port_connect_change drivers/usb/core/hub.c:5009
>  port_event drivers/usb/core/hub.c:5115
>  hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
>  process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
>  worker_thread+0x221/0x1850 kernel/workqueue.c:2253
>  kthread+0x3a1/0x470 kernel/kthread.c:231
>  ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
> Code: 48 8b 85 30 ff ff ff 48 8d b8 98 00 00 00 e8 6e 91 8b ff 45 89
> e8 44 89 f1 4c 89 fa 48 89 c6 48 c7 c7 c0 5a c8 85 e8 10 50 dd fd <0f>
> ff e9 9b f7 ff ff e8 ba cf 26 fe e9 80 f7 ff ff e8 a0 a5 f4
> ---[ end trace bad127706d5fe2d6 ]---
> 

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Mon, 25 Sep 2017 14:39:51 +0200,
Andrey Konovalov wrote:
> 
> Hi!
> 
> I've got the following report while fuzzing the kernel with syzkaller.
> 
> On commit e19b205be43d11bff638cad4487008c48d21c103 (4.14-rc2).
> 
> It seems that there's no check of the endpoint type.
> 
> usb 1-1: BOGUS urb xfer, pipe 1 != type 3
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449

How is this bug triggered?  As it's syzkaller with QEMU, it looks
hitting an inconsistent state the driver didn't expect (it sets the
fixed endpoint), then USB-core detects the inconsistency and spews the
kernel warning with stack trace.  If so, it's no serious problem as it
appears.

Suppose my guess is right, I'm not sure what's the best way to fix
this.  Certainly we can add more sanity check in the caller side.
OTOH, I find the reaction of USB core too aggressive, it's not
necessary to be dev_WARN() but a normal dev_err().
Or I might be looking at a wrong place?

Adding USB guys to Cc for hearing their comments.


thanks,

Takashi


> usb_submit_urb+0xf8a/0x11d0
> Modules linked in:
> CPU: 0 PID: 1846 Comm: kworker/0:2 Not tainted
> 4.14.0-rc2-42613-g1488251d1a98 #238
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Workqueue: usb_hub_wq hub_event
> task: ffff880064296300 task.stack: ffff8800643b0000
> RIP: 0010:usb_submit_urb+0xf8a/0x11d0 drivers/usb/core/urb.c:448
> RSP: 0018:ffff8800643b6140 EFLAGS: 00010286
> RAX: 0000000000000029 RBX: ffff880063842400 RCX: 0000000000000000
> RDX: 0000000000000029 RSI: ffffffff85a58800 RDI: ffffed000c876c1a
> RBP: ffff8800643b6240 R08: 1ffff1000c876ac0 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 1ffff1000c876c2f
> R13: 0000000000000003 R14: 0000000000000001 R15: ffff88006325c768
> FS:  0000000000000000(0000) GS:ffff88006c800000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fa2a51c0000 CR3: 000000006acc8000 CR4: 00000000000006f0
> Call Trace:
>  bcd2000_init_device sound/usb/bcd2000/bcd2000.c:289
>  bcd2000_init_midi sound/usb/bcd2000/bcd2000.c:345
>  bcd2000_probe+0xe64/0x19e0 sound/usb/bcd2000/bcd2000.c:406
>  usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
>  really_probe drivers/base/dd.c:413
>  driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
>  __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
>  bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
>  __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
>  device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
>  bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
>  device_add+0xd0b/0x1660 drivers/base/core.c:1835
>  usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
>  generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
>  usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
>  really_probe drivers/base/dd.c:413
>  driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
>  __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
>  bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
>  __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
>  device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
>  bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
>  device_add+0xd0b/0x1660 drivers/base/core.c:1835
>  usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
>  hub_port_connect drivers/usb/core/hub.c:4903
>  hub_port_connect_change drivers/usb/core/hub.c:5009
>  port_event drivers/usb/core/hub.c:5115
>  hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
>  process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
>  worker_thread+0x221/0x1850 kernel/workqueue.c:2253
>  kthread+0x3a1/0x470 kernel/kthread.c:231
>  ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
> Code: 48 8b 85 30 ff ff ff 48 8d b8 98 00 00 00 e8 6e 91 8b ff 45 89
> e8 44 89 f1 4c 89 fa 48 89 c6 48 c7 c7 c0 5a c8 85 e8 10 50 dd fd <0f>
> ff e9 9b f7 ff ff e8 ba cf 26 fe e9 80 f7 ff ff e8 a0 a5 f4
> ---[ end trace bad127706d5fe2d6 ]---
> 

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Alan Stern]

On Tue, 3 Oct 2017, Takashi Iwai wrote:

> On Mon, 25 Sep 2017 14:39:51 +0200,
> Andrey Konovalov wrote:
> > 
> > Hi!
> > 
> > I've got the following report while fuzzing the kernel with syzkaller.
> > 
> > On commit e19b205be43d11bff638cad4487008c48d21c103 (4.14-rc2).
> > 
> > It seems that there's no check of the endpoint type.
> > 
> > usb 1-1: BOGUS urb xfer, pipe 1 != type 3
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449
> 
> How is this bug triggered?  As it's syzkaller with QEMU, it looks
> hitting an inconsistent state the driver didn't expect (it sets the
> fixed endpoint), then USB-core detects the inconsistency and spews the
> kernel warning with stack trace.  If so, it's no serious problem as it
> appears.
> 
> Suppose my guess is right, I'm not sure what's the best way to fix
> this.  Certainly we can add more sanity check in the caller side.
> OTOH, I find the reaction of USB core too aggressive, it's not
> necessary to be dev_WARN() but a normal dev_err().
> Or I might be looking at a wrong place?

It's a dev_WARN because it indicates a potentially serious error in the 
driver: The driver has submitted an interrupt URB to a bulk endpoint.  
That may not sound bad, but the same check gets triggered if a driver 
submits a bulk URB to an isochronous endpoint, or any other invalid 
combination.

Most likely the explanation here is that the driver doesn't bother to
check the endpoint type because it expects the endpoint will always be
interrupt.  But that is not a safe strategy.  USB devices and their
firmware should not be trusted unnecessarily.

The best fix is, like you said, to add a sanity check in the caller.

Alan Stern

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Alan Stern]

On Tue, 3 Oct 2017, Takashi Iwai wrote:

> On Mon, 25 Sep 2017 14:39:51 +0200,
> Andrey Konovalov wrote:
> > 
> > Hi!
> > 
> > I've got the following report while fuzzing the kernel with syzkaller.
> > 
> > On commit e19b205be43d11bff638cad4487008c48d21c103 (4.14-rc2).
> > 
> > It seems that there's no check of the endpoint type.
> > 
> > usb 1-1: BOGUS urb xfer, pipe 1 != type 3
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449
> 
> How is this bug triggered?  As it's syzkaller with QEMU, it looks
> hitting an inconsistent state the driver didn't expect (it sets the
> fixed endpoint), then USB-core detects the inconsistency and spews the
> kernel warning with stack trace.  If so, it's no serious problem as it
> appears.
> 
> Suppose my guess is right, I'm not sure what's the best way to fix
> this.  Certainly we can add more sanity check in the caller side.
> OTOH, I find the reaction of USB core too aggressive, it's not
> necessary to be dev_WARN() but a normal dev_err().
> Or I might be looking at a wrong place?

It's a dev_WARN because it indicates a potentially serious error in the 
driver: The driver has submitted an interrupt URB to a bulk endpoint.  
That may not sound bad, but the same check gets triggered if a driver 
submits a bulk URB to an isochronous endpoint, or any other invalid 
combination.

Most likely the explanation here is that the driver doesn't bother to
check the endpoint type because it expects the endpoint will always be
interrupt.  But that is not a safe strategy.  USB devices and their
firmware should not be trusted unnecessarily.

The best fix is, like you said, to add a sanity check in the caller.

Alan Stern

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Tue, 03 Oct 2017 16:21:57 +0200,
Alan Stern wrote:
> 
> On Tue, 3 Oct 2017, Takashi Iwai wrote:
> 
> > On Mon, 25 Sep 2017 14:39:51 +0200,
> > Andrey Konovalov wrote:
> > > 
> > > Hi!
> > > 
> > > I've got the following report while fuzzing the kernel with syzkaller.
> > > 
> > > On commit e19b205be43d11bff638cad4487008c48d21c103 (4.14-rc2).
> > > 
> > > It seems that there's no check of the endpoint type.
> > > 
> > > usb 1-1: BOGUS urb xfer, pipe 1 != type 3
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449
> > 
> > How is this bug triggered?  As it's syzkaller with QEMU, it looks
> > hitting an inconsistent state the driver didn't expect (it sets the
> > fixed endpoint), then USB-core detects the inconsistency and spews the
> > kernel warning with stack trace.  If so, it's no serious problem as it
> > appears.
> > 
> > Suppose my guess is right, I'm not sure what's the best way to fix
> > this.  Certainly we can add more sanity check in the caller side.
> > OTOH, I find the reaction of USB core too aggressive, it's not
> > necessary to be dev_WARN() but a normal dev_err().
> > Or I might be looking at a wrong place?
> 
> It's a dev_WARN because it indicates a potentially serious error in the 
> driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> That may not sound bad, but the same check gets triggered if a driver 
> submits a bulk URB to an isochronous endpoint, or any other invalid 
> combination.
> 
> Most likely the explanation here is that the driver doesn't bother to
> check the endpoint type because it expects the endpoint will always be
> interrupt.  But that is not a safe strategy.  USB devices and their
> firmware should not be trusted unnecessarily.
> 
> The best fix is, like you said, to add a sanity check in the caller.

OK, but then do we have some handy helper for the check?
As other bug reports by syzkaller suggest, there are a few other
drivers that do the same, submitting a urb with naive assumption of
the fixed EP for specific devices.  In the end we'll need to put the
very same checks there in multiple places.


thanks,

Takashi

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Tue, 03 Oct 2017 16:21:57 +0200,
Alan Stern wrote:
> 
> On Tue, 3 Oct 2017, Takashi Iwai wrote:
> 
> > On Mon, 25 Sep 2017 14:39:51 +0200,
> > Andrey Konovalov wrote:
> > > 
> > > Hi!
> > > 
> > > I've got the following report while fuzzing the kernel with syzkaller.
> > > 
> > > On commit e19b205be43d11bff638cad4487008c48d21c103 (4.14-rc2).
> > > 
> > > It seems that there's no check of the endpoint type.
> > > 
> > > usb 1-1: BOGUS urb xfer, pipe 1 != type 3
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 1846 at drivers/usb/core/urb.c:449
> > 
> > How is this bug triggered?  As it's syzkaller with QEMU, it looks
> > hitting an inconsistent state the driver didn't expect (it sets the
> > fixed endpoint), then USB-core detects the inconsistency and spews the
> > kernel warning with stack trace.  If so, it's no serious problem as it
> > appears.
> > 
> > Suppose my guess is right, I'm not sure what's the best way to fix
> > this.  Certainly we can add more sanity check in the caller side.
> > OTOH, I find the reaction of USB core too aggressive, it's not
> > necessary to be dev_WARN() but a normal dev_err().
> > Or I might be looking at a wrong place?
> 
> It's a dev_WARN because it indicates a potentially serious error in the 
> driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> That may not sound bad, but the same check gets triggered if a driver 
> submits a bulk URB to an isochronous endpoint, or any other invalid 
> combination.
> 
> Most likely the explanation here is that the driver doesn't bother to
> check the endpoint type because it expects the endpoint will always be
> interrupt.  But that is not a safe strategy.  USB devices and their
> firmware should not be trusted unnecessarily.
> 
> The best fix is, like you said, to add a sanity check in the caller.

OK, but then do we have some handy helper for the check?
As other bug reports by syzkaller suggest, there are a few other
drivers that do the same, submitting a urb with naive assumption of
the fixed EP for specific devices.  In the end we'll need to put the
very same checks there in multiple places.


thanks,

Takashi

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Alan Stern]

On Tue, 3 Oct 2017, Takashi Iwai wrote:

> > It's a dev_WARN because it indicates a potentially serious error in the 
> > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > That may not sound bad, but the same check gets triggered if a driver 
> > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > combination.
> > 
> > Most likely the explanation here is that the driver doesn't bother to
> > check the endpoint type because it expects the endpoint will always be
> > interrupt.  But that is not a safe strategy.  USB devices and their
> > firmware should not be trusted unnecessarily.
> > 
> > The best fix is, like you said, to add a sanity check in the caller.
> 
> OK, but then do we have some handy helper for the check?
> As other bug reports by syzkaller suggest, there are a few other
> drivers that do the same, submitting a urb with naive assumption of
> the fixed EP for specific devices.  In the end we'll need to put the
> very same checks there in multiple places.

Perhaps we could add a helper routine that would take a list of 
expected endpoint types and check that the actual endpoints match the 
types.  But of course, all the drivers you're talking about would have 
to add a call to this helper routine.

Alan Stern

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Alan Stern]

On Tue, 3 Oct 2017, Takashi Iwai wrote:

> > It's a dev_WARN because it indicates a potentially serious error in the 
> > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > That may not sound bad, but the same check gets triggered if a driver 
> > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > combination.
> > 
> > Most likely the explanation here is that the driver doesn't bother to
> > check the endpoint type because it expects the endpoint will always be
> > interrupt.  But that is not a safe strategy.  USB devices and their
> > firmware should not be trusted unnecessarily.
> > 
> > The best fix is, like you said, to add a sanity check in the caller.
> 
> OK, but then do we have some handy helper for the check?
> As other bug reports by syzkaller suggest, there are a few other
> drivers that do the same, submitting a urb with naive assumption of
> the fixed EP for specific devices.  In the end we'll need to put the
> very same checks there in multiple places.

Perhaps we could add a helper routine that would take a list of 
expected endpoint types and check that the actual endpoints match the 
types.  But of course, all the drivers you're talking about would have 
to add a call to this helper routine.

Alan Stern

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Greg Kroah-Hartman]

On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> On Tue, 3 Oct 2017, Takashi Iwai wrote:
> 
> > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > That may not sound bad, but the same check gets triggered if a driver 
> > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > combination.
> > > 
> > > Most likely the explanation here is that the driver doesn't bother to
> > > check the endpoint type because it expects the endpoint will always be
> > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > firmware should not be trusted unnecessarily.
> > > 
> > > The best fix is, like you said, to add a sanity check in the caller.
> > 
> > OK, but then do we have some handy helper for the check?
> > As other bug reports by syzkaller suggest, there are a few other
> > drivers that do the same, submitting a urb with naive assumption of
> > the fixed EP for specific devices.  In the end we'll need to put the
> > very same checks there in multiple places.
> 
> Perhaps we could add a helper routine that would take a list of 
> expected endpoint types and check that the actual endpoints match the 
> types.  But of course, all the drivers you're talking about would have 
> to add a call to this helper routine.

We have almost this type of function, usb_find_common_endpoints(),
what's wrong with using that?  Johan has already swept the tree and
added a lot of these checks, odds are no one looked at the sound/
subdir...

thanks,

greg k-h

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Tue, 03 Oct 2017 19:42:21 +0200,
Greg Kroah-Hartman wrote:
> 
> On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> > On Tue, 3 Oct 2017, Takashi Iwai wrote:
> > 
> > > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > > That may not sound bad, but the same check gets triggered if a driver 
> > > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > > combination.
> > > > 
> > > > Most likely the explanation here is that the driver doesn't bother to
> > > > check the endpoint type because it expects the endpoint will always be
> > > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > > firmware should not be trusted unnecessarily.
> > > > 
> > > > The best fix is, like you said, to add a sanity check in the caller.
> > > 
> > > OK, but then do we have some handy helper for the check?
> > > As other bug reports by syzkaller suggest, there are a few other
> > > drivers that do the same, submitting a urb with naive assumption of
> > > the fixed EP for specific devices.  In the end we'll need to put the
> > > very same checks there in multiple places.
> > 
> > Perhaps we could add a helper routine that would take a list of 
> > expected endpoint types and check that the actual endpoints match the 
> > types.  But of course, all the drivers you're talking about would have 
> > to add a call to this helper routine.
> 
> We have almost this type of function, usb_find_common_endpoints(),
> what's wrong with using that?  Johan has already swept the tree and
> added a lot of these checks, odds are no one looked at the sound/
> subdir...

Well, what I had in my mind is just a snippet from usb_submit_urb(),
something like:

bool usb_sanity_check_urb_pipe(struct urb *urb)
{
	struct usb_host_endpoint *ep;
	int xfertype;
	static const int pipetypes[4] = {
		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
	};

	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
	xfertype = usb_endpoint_type(&ep->desc);
	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
}

And calling this before usb_submit_urb() in each place that assigns
the fixed EP as device-specific quirks.
Does it make sense?


thanks,

Takashi

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Greg Kroah-Hartman]

On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> On Tue, 03 Oct 2017 19:42:21 +0200,
> Greg Kroah-Hartman wrote:
> > 
> > On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> > > On Tue, 3 Oct 2017, Takashi Iwai wrote:
> > > 
> > > > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > > > That may not sound bad, but the same check gets triggered if a driver 
> > > > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > > > combination.
> > > > > 
> > > > > Most likely the explanation here is that the driver doesn't bother to
> > > > > check the endpoint type because it expects the endpoint will always be
> > > > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > > > firmware should not be trusted unnecessarily.
> > > > > 
> > > > > The best fix is, like you said, to add a sanity check in the caller.
> > > > 
> > > > OK, but then do we have some handy helper for the check?
> > > > As other bug reports by syzkaller suggest, there are a few other
> > > > drivers that do the same, submitting a urb with naive assumption of
> > > > the fixed EP for specific devices.  In the end we'll need to put the
> > > > very same checks there in multiple places.
> > > 
> > > Perhaps we could add a helper routine that would take a list of 
> > > expected endpoint types and check that the actual endpoints match the 
> > > types.  But of course, all the drivers you're talking about would have 
> > > to add a call to this helper routine.
> > 
> > We have almost this type of function, usb_find_common_endpoints(),
> > what's wrong with using that?  Johan has already swept the tree and
> > added a lot of these checks, odds are no one looked at the sound/
> > subdir...
> 
> Well, what I had in my mind is just a snippet from usb_submit_urb(),
> something like:
> 
> bool usb_sanity_check_urb_pipe(struct urb *urb)
> {
> 	struct usb_host_endpoint *ep;
> 	int xfertype;
> 	static const int pipetypes[4] = {
> 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> 	};
> 
> 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> 	xfertype = usb_endpoint_type(&ep->desc);
> 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> }
> 
> And calling this before usb_submit_urb() in each place that assigns
> the fixed EP as device-specific quirks.
> Does it make sense?

Yes, kind of, but checking the endpoint type/direction is what you are
expecting it to be as you "know" what the type should be for each
driver as it is unique.

Anyway, a "real" patch might make more sense to me.

thanks,

greg k-h

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Wed, 04 Oct 2017 09:52:36 +0200,
Greg Kroah-Hartman wrote:
> 
> On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> > On Tue, 03 Oct 2017 19:42:21 +0200,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> > > > On Tue, 3 Oct 2017, Takashi Iwai wrote:
> > > > 
> > > > > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > > > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > > > > That may not sound bad, but the same check gets triggered if a driver 
> > > > > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > > > > combination.
> > > > > > 
> > > > > > Most likely the explanation here is that the driver doesn't bother to
> > > > > > check the endpoint type because it expects the endpoint will always be
> > > > > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > > > > firmware should not be trusted unnecessarily.
> > > > > > 
> > > > > > The best fix is, like you said, to add a sanity check in the caller.
> > > > > 
> > > > > OK, but then do we have some handy helper for the check?
> > > > > As other bug reports by syzkaller suggest, there are a few other
> > > > > drivers that do the same, submitting a urb with naive assumption of
> > > > > the fixed EP for specific devices.  In the end we'll need to put the
> > > > > very same checks there in multiple places.
> > > > 
> > > > Perhaps we could add a helper routine that would take a list of 
> > > > expected endpoint types and check that the actual endpoints match the 
> > > > types.  But of course, all the drivers you're talking about would have 
> > > > to add a call to this helper routine.
> > > 
> > > We have almost this type of function, usb_find_common_endpoints(),
> > > what's wrong with using that?  Johan has already swept the tree and
> > > added a lot of these checks, odds are no one looked at the sound/
> > > subdir...
> > 
> > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > something like:
> > 
> > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > {
> > 	struct usb_host_endpoint *ep;
> > 	int xfertype;
> > 	static const int pipetypes[4] = {
> > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > 	};
> > 
> > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > 	xfertype = usb_endpoint_type(&ep->desc);
> > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > }
> > 
> > And calling this before usb_submit_urb() in each place that assigns
> > the fixed EP as device-specific quirks.
> > Does it make sense?
> 
> Yes, kind of, but checking the endpoint type/direction is what you are
> expecting it to be as you "know" what the type should be for each
> driver as it is unique.

Yes, it can be simplified, but if we want a common helper function,
this style would have an advantage that it can be used generically for
all drivers.

> Anyway, a "real" patch might make more sense to me.

I can cook up a patch if you find it a good idea to add such a common
function to usb core side.  OTOH, if each driver should open-code this
in each place, I can work on that, too.  Which would you prefer?


thanks,

Takashi

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Wed, 04 Oct 2017 09:52:36 +0200,
Greg Kroah-Hartman wrote:
> 
> On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> > On Tue, 03 Oct 2017 19:42:21 +0200,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> > > > On Tue, 3 Oct 2017, Takashi Iwai wrote:
> > > > 
> > > > > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > > > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > > > > That may not sound bad, but the same check gets triggered if a driver 
> > > > > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > > > > combination.
> > > > > > 
> > > > > > Most likely the explanation here is that the driver doesn't bother to
> > > > > > check the endpoint type because it expects the endpoint will always be
> > > > > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > > > > firmware should not be trusted unnecessarily.
> > > > > > 
> > > > > > The best fix is, like you said, to add a sanity check in the caller.
> > > > > 
> > > > > OK, but then do we have some handy helper for the check?
> > > > > As other bug reports by syzkaller suggest, there are a few other
> > > > > drivers that do the same, submitting a urb with naive assumption of
> > > > > the fixed EP for specific devices.  In the end we'll need to put the
> > > > > very same checks there in multiple places.
> > > > 
> > > > Perhaps we could add a helper routine that would take a list of 
> > > > expected endpoint types and check that the actual endpoints match the 
> > > > types.  But of course, all the drivers you're talking about would have 
> > > > to add a call to this helper routine.
> > > 
> > > We have almost this type of function, usb_find_common_endpoints(),
> > > what's wrong with using that?  Johan has already swept the tree and
> > > added a lot of these checks, odds are no one looked at the sound/
> > > subdir...
> > 
> > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > something like:
> > 
> > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > {
> > 	struct usb_host_endpoint *ep;
> > 	int xfertype;
> > 	static const int pipetypes[4] = {
> > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > 	};
> > 
> > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > 	xfertype = usb_endpoint_type(&ep->desc);
> > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > }
> > 
> > And calling this before usb_submit_urb() in each place that assigns
> > the fixed EP as device-specific quirks.
> > Does it make sense?
> 
> Yes, kind of, but checking the endpoint type/direction is what you are
> expecting it to be as you "know" what the type should be for each
> driver as it is unique.

Yes, it can be simplified, but if we want a common helper function,
this style would have an advantage that it can be used generically for
all drivers.

> Anyway, a "real" patch might make more sense to me.

I can cook up a patch if you find it a good idea to add such a common
function to usb core side.  OTOH, if each driver should open-code this
in each place, I can work on that, too.  Which would you prefer?


thanks,

Takashi

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Greg Kroah-Hartman]

On Wed, Oct 04, 2017 at 10:08:24AM +0200, Takashi Iwai wrote:
> On Wed, 04 Oct 2017 09:52:36 +0200,
> Greg Kroah-Hartman wrote:
> > 
> > On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> > > On Tue, 03 Oct 2017 19:42:21 +0200,
> > > Greg Kroah-Hartman wrote:
> > > > 
> > > > On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> > > > > On Tue, 3 Oct 2017, Takashi Iwai wrote:
> > > > > 
> > > > > > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > > > > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > > > > > That may not sound bad, but the same check gets triggered if a driver 
> > > > > > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > > > > > combination.
> > > > > > > 
> > > > > > > Most likely the explanation here is that the driver doesn't bother to
> > > > > > > check the endpoint type because it expects the endpoint will always be
> > > > > > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > > > > > firmware should not be trusted unnecessarily.
> > > > > > > 
> > > > > > > The best fix is, like you said, to add a sanity check in the caller.
> > > > > > 
> > > > > > OK, but then do we have some handy helper for the check?
> > > > > > As other bug reports by syzkaller suggest, there are a few other
> > > > > > drivers that do the same, submitting a urb with naive assumption of
> > > > > > the fixed EP for specific devices.  In the end we'll need to put the
> > > > > > very same checks there in multiple places.
> > > > > 
> > > > > Perhaps we could add a helper routine that would take a list of 
> > > > > expected endpoint types and check that the actual endpoints match the 
> > > > > types.  But of course, all the drivers you're talking about would have 
> > > > > to add a call to this helper routine.
> > > > 
> > > > We have almost this type of function, usb_find_common_endpoints(),
> > > > what's wrong with using that?  Johan has already swept the tree and
> > > > added a lot of these checks, odds are no one looked at the sound/
> > > > subdir...
> > > 
> > > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > > something like:
> > > 
> > > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > > {
> > > 	struct usb_host_endpoint *ep;
> > > 	int xfertype;
> > > 	static const int pipetypes[4] = {
> > > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > > 	};
> > > 
> > > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > > 	xfertype = usb_endpoint_type(&ep->desc);
> > > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > > }
> > > 
> > > And calling this before usb_submit_urb() in each place that assigns
> > > the fixed EP as device-specific quirks.
> > > Does it make sense?
> > 
> > Yes, kind of, but checking the endpoint type/direction is what you are
> > expecting it to be as you "know" what the type should be for each
> > driver as it is unique.
> 
> Yes, it can be simplified, but if we want a common helper function,
> this style would have an advantage that it can be used generically for
> all drivers.
> 
> > Anyway, a "real" patch might make more sense to me.
> 
> I can cook up a patch if you find it a good idea to add such a common
> function to usb core side.  OTOH, if each driver should open-code this
> in each place, I can work on that, too.  Which would you prefer?

A common function is good, open-coding is bad :)

Try it with a driver or two to see what it looks like?

thanks,

greg k-h

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Greg Kroah-Hartman]

On Wed, Oct 04, 2017 at 10:08:24AM +0200, Takashi Iwai wrote:
> On Wed, 04 Oct 2017 09:52:36 +0200,
> Greg Kroah-Hartman wrote:
> > 
> > On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> > > On Tue, 03 Oct 2017 19:42:21 +0200,
> > > Greg Kroah-Hartman wrote:
> > > > 
> > > > On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> > > > > On Tue, 3 Oct 2017, Takashi Iwai wrote:
> > > > > 
> > > > > > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > > > > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > > > > > That may not sound bad, but the same check gets triggered if a driver 
> > > > > > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > > > > > combination.
> > > > > > > 
> > > > > > > Most likely the explanation here is that the driver doesn't bother to
> > > > > > > check the endpoint type because it expects the endpoint will always be
> > > > > > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > > > > > firmware should not be trusted unnecessarily.
> > > > > > > 
> > > > > > > The best fix is, like you said, to add a sanity check in the caller.
> > > > > > 
> > > > > > OK, but then do we have some handy helper for the check?
> > > > > > As other bug reports by syzkaller suggest, there are a few other
> > > > > > drivers that do the same, submitting a urb with naive assumption of
> > > > > > the fixed EP for specific devices.  In the end we'll need to put the
> > > > > > very same checks there in multiple places.
> > > > > 
> > > > > Perhaps we could add a helper routine that would take a list of 
> > > > > expected endpoint types and check that the actual endpoints match the 
> > > > > types.  But of course, all the drivers you're talking about would have 
> > > > > to add a call to this helper routine.
> > > > 
> > > > We have almost this type of function, usb_find_common_endpoints(),
> > > > what's wrong with using that?  Johan has already swept the tree and
> > > > added a lot of these checks, odds are no one looked at the sound/
> > > > subdir...
> > > 
> > > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > > something like:
> > > 
> > > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > > {
> > > 	struct usb_host_endpoint *ep;
> > > 	int xfertype;
> > > 	static const int pipetypes[4] = {
> > > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > > 	};
> > > 
> > > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > > 	xfertype = usb_endpoint_type(&ep->desc);
> > > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > > }
> > > 
> > > And calling this before usb_submit_urb() in each place that assigns
> > > the fixed EP as device-specific quirks.
> > > Does it make sense?
> > 
> > Yes, kind of, but checking the endpoint type/direction is what you are
> > expecting it to be as you "know" what the type should be for each
> > driver as it is unique.
> 
> Yes, it can be simplified, but if we want a common helper function,
> this style would have an advantage that it can be used generically for
> all drivers.
> 
> > Anyway, a "real" patch might make more sense to me.
> 
> I can cook up a patch if you find it a good idea to add such a common
> function to usb core side.  OTOH, if each driver should open-code this
> in each place, I can work on that, too.  Which would you prefer?

A common function is good, open-coding is bad :)

Try it with a driver or two to see what it looks like?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Johan Hovold]

On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> On Tue, 03 Oct 2017 19:42:21 +0200,
> Greg Kroah-Hartman wrote:
> > 
> > On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> > > On Tue, 3 Oct 2017, Takashi Iwai wrote:
> > > 
> > > > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > > > That may not sound bad, but the same check gets triggered if a driver 
> > > > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > > > combination.
> > > > > 
> > > > > Most likely the explanation here is that the driver doesn't bother to
> > > > > check the endpoint type because it expects the endpoint will always be
> > > > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > > > firmware should not be trusted unnecessarily.
> > > > > 
> > > > > The best fix is, like you said, to add a sanity check in the caller.
> > > > 
> > > > OK, but then do we have some handy helper for the check?
> > > > As other bug reports by syzkaller suggest, there are a few other
> > > > drivers that do the same, submitting a urb with naive assumption of
> > > > the fixed EP for specific devices.  In the end we'll need to put the
> > > > very same checks there in multiple places.
> > > 
> > > Perhaps we could add a helper routine that would take a list of 
> > > expected endpoint types and check that the actual endpoints match the 
> > > types.  But of course, all the drivers you're talking about would have 
> > > to add a call to this helper routine.
> > 
> > We have almost this type of function, usb_find_common_endpoints(),
> > what's wrong with using that?  Johan has already swept the tree and
> > added a lot of these checks, odds are no one looked at the sound/
> > subdir...

Yeah, I only swept the tree for instances were a missing endpoint could
lead to a NULL-deref. This is not the case here were the endpoint
addresses are hardcoded in the driver.

I also never got around to applying the new helper outside of
drivers/usb.

> Well, what I had in my mind is just a snippet from usb_submit_urb(),
> something like:
> 
> bool usb_sanity_check_urb_pipe(struct urb *urb)
> {
> 	struct usb_host_endpoint *ep;
> 	int xfertype;
> 	static const int pipetypes[4] = {
> 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> 	};
> 
> 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> 	xfertype = usb_endpoint_type(&ep->desc);
> 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> }
> 
> And calling this before usb_submit_urb() in each place that assigns
> the fixed EP as device-specific quirks.
> Does it make sense?

Not really. Your driver should not even bind to an interface which lacks
the expected endpoints (rather than check this at a potentially later
point in time when URBs are submitted).

The new helper which Greg mentioned would allow this to implemented with
just a few lines of code. Just add it to bcd2000_init_midi() or similar.  

Thanks,
Johan

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Wed, 04 Oct 2017 11:24:42 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> > On Tue, 03 Oct 2017 19:42:21 +0200,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> > > > On Tue, 3 Oct 2017, Takashi Iwai wrote:
> > > > 
> > > > > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > > > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > > > > That may not sound bad, but the same check gets triggered if a driver 
> > > > > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > > > > combination.
> > > > > > 
> > > > > > Most likely the explanation here is that the driver doesn't bother to
> > > > > > check the endpoint type because it expects the endpoint will always be
> > > > > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > > > > firmware should not be trusted unnecessarily.
> > > > > > 
> > > > > > The best fix is, like you said, to add a sanity check in the caller.
> > > > > 
> > > > > OK, but then do we have some handy helper for the check?
> > > > > As other bug reports by syzkaller suggest, there are a few other
> > > > > drivers that do the same, submitting a urb with naive assumption of
> > > > > the fixed EP for specific devices.  In the end we'll need to put the
> > > > > very same checks there in multiple places.
> > > > 
> > > > Perhaps we could add a helper routine that would take a list of 
> > > > expected endpoint types and check that the actual endpoints match the 
> > > > types.  But of course, all the drivers you're talking about would have 
> > > > to add a call to this helper routine.
> > > 
> > > We have almost this type of function, usb_find_common_endpoints(),
> > > what's wrong with using that?  Johan has already swept the tree and
> > > added a lot of these checks, odds are no one looked at the sound/
> > > subdir...
> 
> Yeah, I only swept the tree for instances were a missing endpoint could
> lead to a NULL-deref. This is not the case here were the endpoint
> addresses are hardcoded in the driver.
> 
> I also never got around to applying the new helper outside of
> drivers/usb.
> 
> > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > something like:
> > 
> > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > {
> > 	struct usb_host_endpoint *ep;
> > 	int xfertype;
> > 	static const int pipetypes[4] = {
> > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > 	};
> > 
> > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > 	xfertype = usb_endpoint_type(&ep->desc);
> > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > }
> > 
> > And calling this before usb_submit_urb() in each place that assigns
> > the fixed EP as device-specific quirks.
> > Does it make sense?
> 
> Not really. Your driver should not even bind to an interface which lacks
> the expected endpoints (rather than check this at a potentially later
> point in time when URBs are submitted).

The endpoint may exist but it may be invalid, as the problem is
triggered by a VM.  It doesn't parse but tries a fixed EP as it's no
compliant device.

> The new helper which Greg mentioned would allow this to implemented with
> just a few lines of code. Just add it to bcd2000_init_midi() or similar.  

Could you give an example?  Then I can ask Andrey whether such a call
really addresses the issue.


thanks,

Takashi

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Wed, 04 Oct 2017 11:24:42 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> > On Tue, 03 Oct 2017 19:42:21 +0200,
> > Greg Kroah-Hartman wrote:
> > > 
> > > On Tue, Oct 03, 2017 at 12:50:08PM -0400, Alan Stern wrote:
> > > > On Tue, 3 Oct 2017, Takashi Iwai wrote:
> > > > 
> > > > > > It's a dev_WARN because it indicates a potentially serious error in the 
> > > > > > driver: The driver has submitted an interrupt URB to a bulk endpoint.  
> > > > > > That may not sound bad, but the same check gets triggered if a driver 
> > > > > > submits a bulk URB to an isochronous endpoint, or any other invalid 
> > > > > > combination.
> > > > > > 
> > > > > > Most likely the explanation here is that the driver doesn't bother to
> > > > > > check the endpoint type because it expects the endpoint will always be
> > > > > > interrupt.  But that is not a safe strategy.  USB devices and their
> > > > > > firmware should not be trusted unnecessarily.
> > > > > > 
> > > > > > The best fix is, like you said, to add a sanity check in the caller.
> > > > > 
> > > > > OK, but then do we have some handy helper for the check?
> > > > > As other bug reports by syzkaller suggest, there are a few other
> > > > > drivers that do the same, submitting a urb with naive assumption of
> > > > > the fixed EP for specific devices.  In the end we'll need to put the
> > > > > very same checks there in multiple places.
> > > > 
> > > > Perhaps we could add a helper routine that would take a list of 
> > > > expected endpoint types and check that the actual endpoints match the 
> > > > types.  But of course, all the drivers you're talking about would have 
> > > > to add a call to this helper routine.
> > > 
> > > We have almost this type of function, usb_find_common_endpoints(),
> > > what's wrong with using that?  Johan has already swept the tree and
> > > added a lot of these checks, odds are no one looked at the sound/
> > > subdir...
> 
> Yeah, I only swept the tree for instances were a missing endpoint could
> lead to a NULL-deref. This is not the case here were the endpoint
> addresses are hardcoded in the driver.
> 
> I also never got around to applying the new helper outside of
> drivers/usb.
> 
> > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > something like:
> > 
> > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > {
> > 	struct usb_host_endpoint *ep;
> > 	int xfertype;
> > 	static const int pipetypes[4] = {
> > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > 	};
> > 
> > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > 	xfertype = usb_endpoint_type(&ep->desc);
> > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > }
> > 
> > And calling this before usb_submit_urb() in each place that assigns
> > the fixed EP as device-specific quirks.
> > Does it make sense?
> 
> Not really. Your driver should not even bind to an interface which lacks
> the expected endpoints (rather than check this at a potentially later
> point in time when URBs are submitted).

The endpoint may exist but it may be invalid, as the problem is
triggered by a VM.  It doesn't parse but tries a fixed EP as it's no
compliant device.

> The new helper which Greg mentioned would allow this to implemented with
> just a few lines of code. Just add it to bcd2000_init_midi() or similar.  

Could you give an example?  Then I can ask Andrey whether such a call
really addresses the issue.


thanks,

Takashi

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Johan Hovold]

On Wed, Oct 04, 2017 at 12:04:06PM +0200, Takashi Iwai wrote:
> On Wed, 04 Oct 2017 11:24:42 +0200, Johan Hovold wrote:
> > On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:

> > > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > > something like:
> > > 
> > > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > > {
> > > 	struct usb_host_endpoint *ep;
> > > 	int xfertype;
> > > 	static const int pipetypes[4] = {
> > > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > > 	};
> > > 
> > > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > > 	xfertype = usb_endpoint_type(&ep->desc);
> > > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > > }
> > > 
> > > And calling this before usb_submit_urb() in each place that assigns
> > > the fixed EP as device-specific quirks.
> > > Does it make sense?
> > 
> > Not really. Your driver should not even bind to an interface which lacks
> > the expected endpoints (rather than check this at a potentially later
> > point in time when URBs are submitted).
> 
> The endpoint may exist but it may be invalid, as the problem is
> triggered by a VM.  It doesn't parse but tries a fixed EP as it's no
> compliant device.

Yes, that's why a driver should verify that the endpoints it expects are
indeed present (and of the right type) already at probe.

In Andrey's fuzzing it's triggered by in a VM using the dummy_hcd
driver, but this could just as well be a (malicious) physical device
with unexpected descriptors.

> > The new helper which Greg mentioned would allow this to implemented with
> > just a few lines of code. Just add it to bcd2000_init_midi() or similar.  
> 
> Could you give an example?  Then I can ask Andrey whether such a call
> really addresses the issue.

If you grep for usb_find_common_endpoints you'll find a few examples
of how that function may be used (e.g. in drivers/usb/misc/usblcd.c).

The helper iterates of the endpoint descriptors of an interface
alt-setting and returns a descriptor for each requested type if found.
After a vetting of our current drivers I concluded that this would
cover the needs of the vast majority of drivers.

So for the driver in question you'd only need to add something like:

	struct usb_endpoint_descriptor *int_in, *int_out;
	int ret;

	ret = usb_find_common_endpoints(interface->cur_altsetting,
					NULL, NULL, &int_in, &int_out);
	if (ret) {
		dev_err(&interface->dev, "required endpoints not found\n");
		return -ENODEV;
	}

Then you can use int_in->bEndpointAddress etc. when initialising your
URBs.

Johan

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Wed, 04 Oct 2017 12:23:11 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 04, 2017 at 12:04:06PM +0200, Takashi Iwai wrote:
> > On Wed, 04 Oct 2017 11:24:42 +0200, Johan Hovold wrote:
> > > On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> 
> > > > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > > > something like:
> > > > 
> > > > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > > > {
> > > > 	struct usb_host_endpoint *ep;
> > > > 	int xfertype;
> > > > 	static const int pipetypes[4] = {
> > > > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > > > 	};
> > > > 
> > > > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > > > 	xfertype = usb_endpoint_type(&ep->desc);
> > > > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > > > }
> > > > 
> > > > And calling this before usb_submit_urb() in each place that assigns
> > > > the fixed EP as device-specific quirks.
> > > > Does it make sense?
> > > 
> > > Not really. Your driver should not even bind to an interface which lacks
> > > the expected endpoints (rather than check this at a potentially later
> > > point in time when URBs are submitted).
> > 
> > The endpoint may exist but it may be invalid, as the problem is
> > triggered by a VM.  It doesn't parse but tries a fixed EP as it's no
> > compliant device.
> 
> Yes, that's why a driver should verify that the endpoints it expects are
> indeed present (and of the right type) already at probe.
> 
> In Andrey's fuzzing it's triggered by in a VM using the dummy_hcd
> driver, but this could just as well be a (malicious) physical device
> with unexpected descriptors.
> 
> > > The new helper which Greg mentioned would allow this to implemented with
> > > just a few lines of code. Just add it to bcd2000_init_midi() or similar.  
> > 
> > Could you give an example?  Then I can ask Andrey whether such a call
> > really addresses the issue.
> 
> If you grep for usb_find_common_endpoints you'll find a few examples
> of how that function may be used (e.g. in drivers/usb/misc/usblcd.c).
> 
> The helper iterates of the endpoint descriptors of an interface
> alt-setting and returns a descriptor for each requested type if found.
> After a vetting of our current drivers I concluded that this would
> cover the needs of the vast majority of drivers.
> 
> So for the driver in question you'd only need to add something like:
> 
> 	struct usb_endpoint_descriptor *int_in, *int_out;
> 	int ret;
> 
> 	ret = usb_find_common_endpoints(interface->cur_altsetting,
> 					NULL, NULL, &int_in, &int_out);
> 	if (ret) {
> 		dev_err(&interface->dev, "required endpoints not found\n");
> 		return -ENODEV;
> 	}
> 
> Then you can use int_in->bEndpointAddress etc. when initialising your
> URBs.

OK, but in our cases, it's not about using the returned one but
checking whether it's the expected address, right?  The device is
non-compliant and that's the reason the driver takes the fixed EP.

In anyway, the check will be shortly before the URB submission because
the EP is often determined a late stage of probe, as most of errors
happened for the MIDI interface that are device-specific.


thanks,

Takashi

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Wed, 04 Oct 2017 12:23:11 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 04, 2017 at 12:04:06PM +0200, Takashi Iwai wrote:
> > On Wed, 04 Oct 2017 11:24:42 +0200, Johan Hovold wrote:
> > > On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> 
> > > > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > > > something like:
> > > > 
> > > > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > > > {
> > > > 	struct usb_host_endpoint *ep;
> > > > 	int xfertype;
> > > > 	static const int pipetypes[4] = {
> > > > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > > > 	};
> > > > 
> > > > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > > > 	xfertype = usb_endpoint_type(&ep->desc);
> > > > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > > > }
> > > > 
> > > > And calling this before usb_submit_urb() in each place that assigns
> > > > the fixed EP as device-specific quirks.
> > > > Does it make sense?
> > > 
> > > Not really. Your driver should not even bind to an interface which lacks
> > > the expected endpoints (rather than check this at a potentially later
> > > point in time when URBs are submitted).
> > 
> > The endpoint may exist but it may be invalid, as the problem is
> > triggered by a VM.  It doesn't parse but tries a fixed EP as it's no
> > compliant device.
> 
> Yes, that's why a driver should verify that the endpoints it expects are
> indeed present (and of the right type) already at probe.
> 
> In Andrey's fuzzing it's triggered by in a VM using the dummy_hcd
> driver, but this could just as well be a (malicious) physical device
> with unexpected descriptors.
> 
> > > The new helper which Greg mentioned would allow this to implemented with
> > > just a few lines of code. Just add it to bcd2000_init_midi() or similar.  
> > 
> > Could you give an example?  Then I can ask Andrey whether such a call
> > really addresses the issue.
> 
> If you grep for usb_find_common_endpoints you'll find a few examples
> of how that function may be used (e.g. in drivers/usb/misc/usblcd.c).
> 
> The helper iterates of the endpoint descriptors of an interface
> alt-setting and returns a descriptor for each requested type if found.
> After a vetting of our current drivers I concluded that this would
> cover the needs of the vast majority of drivers.
> 
> So for the driver in question you'd only need to add something like:
> 
> 	struct usb_endpoint_descriptor *int_in, *int_out;
> 	int ret;
> 
> 	ret = usb_find_common_endpoints(interface->cur_altsetting,
> 					NULL, NULL, &int_in, &int_out);
> 	if (ret) {
> 		dev_err(&interface->dev, "required endpoints not found\n");
> 		return -ENODEV;
> 	}
> 
> Then you can use int_in->bEndpointAddress etc. when initialising your
> URBs.

OK, but in our cases, it's not about using the returned one but
checking whether it's the expected address, right?  The device is
non-compliant and that's the reason the driver takes the fixed EP.

In anyway, the check will be shortly before the URB submission because
the EP is often determined a late stage of probe, as most of errors
happened for the MIDI interface that are device-specific.


thanks,

Takashi

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Johan Hovold]

On Wed, Oct 04, 2017 at 12:41:55PM +0200, Takashi Iwai wrote:
> On Wed, 04 Oct 2017 12:23:11 +0200,
> Johan Hovold wrote:
> > 
> > On Wed, Oct 04, 2017 at 12:04:06PM +0200, Takashi Iwai wrote:
> > > On Wed, 04 Oct 2017 11:24:42 +0200, Johan Hovold wrote:
> > > > On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> > 
> > > > > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > > > > something like:
> > > > > 
> > > > > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > > > > {
> > > > > 	struct usb_host_endpoint *ep;
> > > > > 	int xfertype;
> > > > > 	static const int pipetypes[4] = {
> > > > > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > > > > 	};
> > > > > 
> > > > > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > > > > 	xfertype = usb_endpoint_type(&ep->desc);
> > > > > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > > > > }
> > > > > 
> > > > > And calling this before usb_submit_urb() in each place that assigns
> > > > > the fixed EP as device-specific quirks.
> > > > > Does it make sense?
> > > > 
> > > > Not really. Your driver should not even bind to an interface which lacks
> > > > the expected endpoints (rather than check this at a potentially later
> > > > point in time when URBs are submitted).
> > > 
> > > The endpoint may exist but it may be invalid, as the problem is
> > > triggered by a VM.  It doesn't parse but tries a fixed EP as it's no
> > > compliant device.
> > 
> > Yes, that's why a driver should verify that the endpoints it expects are
> > indeed present (and of the right type) already at probe.
> > 
> > In Andrey's fuzzing it's triggered by in a VM using the dummy_hcd
> > driver, but this could just as well be a (malicious) physical device
> > with unexpected descriptors.
> > 
> > > > The new helper which Greg mentioned would allow this to implemented with
> > > > just a few lines of code. Just add it to bcd2000_init_midi() or similar.  
> > > 
> > > Could you give an example?  Then I can ask Andrey whether such a call
> > > really addresses the issue.
> > 
> > If you grep for usb_find_common_endpoints you'll find a few examples
> > of how that function may be used (e.g. in drivers/usb/misc/usblcd.c).
> > 
> > The helper iterates of the endpoint descriptors of an interface
> > alt-setting and returns a descriptor for each requested type if found.
> > After a vetting of our current drivers I concluded that this would
> > cover the needs of the vast majority of drivers.
> > 
> > So for the driver in question you'd only need to add something like:
> > 
> > 	struct usb_endpoint_descriptor *int_in, *int_out;
> > 	int ret;
> > 
> > 	ret = usb_find_common_endpoints(interface->cur_altsetting,
> > 					NULL, NULL, &int_in, &int_out);
> > 	if (ret) {
> > 		dev_err(&interface->dev, "required endpoints not found\n");
> > 		return -ENODEV;
> > 	}
> > 
> > Then you can use int_in->bEndpointAddress etc. when initialising your
> > URBs.
> 
> OK, but in our cases, it's not about using the returned one but
> checking whether it's the expected address, right?  The device is
> non-compliant and that's the reason the driver takes the fixed EP.

There's nothing preventing you from verifying that the returned
descriptors have the expected addresses if tightening the constraints
this ways makes sense for your application.

Or you can implement your own sanity checks, just do it at probe.

But note that you'd introduce NULL-deref that can be triggered by a
malicious device with your outlined helper above, as

	ep = usb_pipe_endpoint(urb->dev, urb->pipe);

will be NULL when the corresponding descriptor is missing.

> In anyway, the check will be shortly before the URB submission because
> the EP is often determined a late stage of probe, as most of errors
> happened for the MIDI interface that are device-specific.

As long as you do the check during probe and refuse to bind to a
non-compliant device you should be fine. Some drivers do not submit URBs
until the user tries to use whatever interface the driver exposes (e.g.
when opening a character device), which IMO is too late for such sanity
checks.

Johan

Re: usb/sound/bcd2000: warning in bcd2000_init_device

[Takashi Iwai]

On Wed, 04 Oct 2017 14:03:25 +0200,
Johan Hovold wrote:
> 
> On Wed, Oct 04, 2017 at 12:41:55PM +0200, Takashi Iwai wrote:
> > On Wed, 04 Oct 2017 12:23:11 +0200,
> > Johan Hovold wrote:
> > > 
> > > On Wed, Oct 04, 2017 at 12:04:06PM +0200, Takashi Iwai wrote:
> > > > On Wed, 04 Oct 2017 11:24:42 +0200, Johan Hovold wrote:
> > > > > On Wed, Oct 04, 2017 at 08:10:59AM +0200, Takashi Iwai wrote:
> > > 
> > > > > > Well, what I had in my mind is just a snippet from usb_submit_urb(),
> > > > > > something like:
> > > > > > 
> > > > > > bool usb_sanity_check_urb_pipe(struct urb *urb)
> > > > > > {
> > > > > > 	struct usb_host_endpoint *ep;
> > > > > > 	int xfertype;
> > > > > > 	static const int pipetypes[4] = {
> > > > > > 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
> > > > > > 	};
> > > > > > 
> > > > > > 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> > > > > > 	xfertype = usb_endpoint_type(&ep->desc);
> > > > > > 	return usb_pipetype(urb->pipe) != pipetypes[xfertype];
> > > > > > }
> > > > > > 
> > > > > > And calling this before usb_submit_urb() in each place that assigns
> > > > > > the fixed EP as device-specific quirks.
> > > > > > Does it make sense?
> > > > > 
> > > > > Not really. Your driver should not even bind to an interface which lacks
> > > > > the expected endpoints (rather than check this at a potentially later
> > > > > point in time when URBs are submitted).
> > > > 
> > > > The endpoint may exist but it may be invalid, as the problem is
> > > > triggered by a VM.  It doesn't parse but tries a fixed EP as it's no
> > > > compliant device.
> > > 
> > > Yes, that's why a driver should verify that the endpoints it expects are
> > > indeed present (and of the right type) already at probe.
> > > 
> > > In Andrey's fuzzing it's triggered by in a VM using the dummy_hcd
> > > driver, but this could just as well be a (malicious) physical device
> > > with unexpected descriptors.
> > > 
> > > > > The new helper which Greg mentioned would allow this to implemented with
> > > > > just a few lines of code. Just add it to bcd2000_init_midi() or similar.  
> > > > 
> > > > Could you give an example?  Then I can ask Andrey whether such a call
> > > > really addresses the issue.
> > > 
> > > If you grep for usb_find_common_endpoints you'll find a few examples
> > > of how that function may be used (e.g. in drivers/usb/misc/usblcd.c).
> > > 
> > > The helper iterates of the endpoint descriptors of an interface
> > > alt-setting and returns a descriptor for each requested type if found.
> > > After a vetting of our current drivers I concluded that this would
> > > cover the needs of the vast majority of drivers.
> > > 
> > > So for the driver in question you'd only need to add something like:
> > > 
> > > 	struct usb_endpoint_descriptor *int_in, *int_out;
> > > 	int ret;
> > > 
> > > 	ret = usb_find_common_endpoints(interface->cur_altsetting,
> > > 					NULL, NULL, &int_in, &int_out);
> > > 	if (ret) {
> > > 		dev_err(&interface->dev, "required endpoints not found\n");
> > > 		return -ENODEV;
> > > 	}
> > > 
> > > Then you can use int_in->bEndpointAddress etc. when initialising your
> > > URBs.
> > 
> > OK, but in our cases, it's not about using the returned one but
> > checking whether it's the expected address, right?  The device is
> > non-compliant and that's the reason the driver takes the fixed EP.
> 
> There's nothing preventing you from verifying that the returned
> descriptors have the expected addresses if tightening the constraints
> this ways makes sense for your application.

OK.

> Or you can implement your own sanity checks, just do it at probe.
> 
> But note that you'd introduce NULL-deref that can be triggered by a
> malicious device with your outlined helper above, as
> 
> 	ep = usb_pipe_endpoint(urb->dev, urb->pipe);
> 
> will be NULL when the corresponding descriptor is missing.

Yes, if we do that.  But I'm working on a version using
usb_find_*_endpoint*() instead.

> > In anyway, the check will be shortly before the URB submission because
> > the EP is often determined a late stage of probe, as most of errors
> > happened for the MIDI interface that are device-specific.
> 
> As long as you do the check during probe and refuse to bind to a
> non-compliant device you should be fine. Some drivers do not submit URBs
> until the user tries to use whatever interface the driver exposes (e.g.
> when opening a character device), which IMO is too late for such sanity
> checks.

Right, and this won't be a problem as the issue is triggered before
the actual device registration (ALSA has a staged registration
scheme).


thanks,

Takashi