[alsa-devel] usb/sound/bcd2000: warning in bcd2000_init_device
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 ]---
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 ]---
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
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
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
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
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
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
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
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
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
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
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
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
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
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
participants (5)
-
Alan Stern -
Andrey Konovalov -
Greg Kroah-Hartman -
Johan Hovold -
Takashi Iwai