| Message ID | c27f3bf4-63d8-4fb5-ac82-09e3cd19f61c@rowland.harvard.edu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2240fed37afbcdb5e8b627bc7ad986891100e05d |
| Headers | show |
| Series | USB: hub: Ignore non-compliant devices with too many configs or interfaces | expand |
On Wed, Jan 22, 2025 at 02:26:17PM -0500, Alan Stern wrote: > Robert Morris created a test program which can cause > usb_hub_to_struct_hub() to dereference a NULL or inappropriate > pointer: > > Oops: general protection fault, probably for non-canonical address > 0xcccccccccccccccc: 0000 [#1] SMP DEBUG_PAGEALLOC PTI > CPU: 7 UID: 0 PID: 117 Comm: kworker/7:1 Not tainted 6.13.0-rc3-00017-gf44d154d6e3d #14 > Hardware name: FreeBSD BHYVE/BHYVE, BIOS 14.0 10/17/2021 > Workqueue: usb_hub_wq hub_event > RIP: 0010:usb_hub_adjust_deviceremovable+0x78/0x110 > ... > Call Trace: > <TASK> > ? die_addr+0x31/0x80 > ? exc_general_protection+0x1b4/0x3c0 > ? asm_exc_general_protection+0x26/0x30 > ? usb_hub_adjust_deviceremovable+0x78/0x110 > hub_probe+0x7c7/0xab0 > usb_probe_interface+0x14b/0x350 > really_probe+0xd0/0x2d0 > ? __pfx___device_attach_driver+0x10/0x10 > __driver_probe_device+0x6e/0x110 > driver_probe_device+0x1a/0x90 > __device_attach_driver+0x7e/0xc0 > bus_for_each_drv+0x7f/0xd0 > __device_attach+0xaa/0x1a0 > bus_probe_device+0x8b/0xa0 > device_add+0x62e/0x810 > usb_set_configuration+0x65d/0x990 > usb_generic_driver_probe+0x4b/0x70 > usb_probe_device+0x36/0xd0 > > The cause of this error is that the device has two interfaces, and the > hub driver binds to interface 1 instead of interface 0, which is where > usb_hub_to_struct_hub() looks. > > We can prevent the problem from occurring by refusing to accept hub > devices that violate the USB spec by having more than one > configuration or interface. > > Reported-and-tested-by: Robert Morris <rtm@csail.mit.edu> > Closes: https://lore.kernel.org/linux-usb/95564.1737394039@localhost/ > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > --- Greg: I originally didn't mark this patch for the -stable kernels because it doesn't really fix a bug. But on the other hand, it does protect against a possible DOS attack from a malicious device, so perhaps it does belong in the -stable kernels. I'll leave the decision up to you. Alan Stern > > drivers/usb/core/hub.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > Index: usb-devel/drivers/usb/core/hub.c > =================================================================== > --- usb-devel.orig/drivers/usb/core/hub.c > +++ usb-devel/drivers/usb/core/hub.c > @@ -1848,6 +1848,17 @@ static int hub_probe(struct usb_interfac > hdev = interface_to_usbdev(intf); > > /* > + * The USB 2.0 spec prohibits hubs from having more than one > + * configuration or interface, and we rely on this prohibition. > + * Refuse to accept a device that violates it. > + */ > + if (hdev->descriptor.bNumConfigurations > 1 || > + hdev->actconfig->desc.bNumInterfaces > 1) { > + dev_err(&intf->dev, "Invalid hub with more than one config or interface\n"); > + return -EINVAL; > + } > + > + /* > * Set default autosuspend delay as 0 to speedup bus suspend, > * based on the below considerations: > *
On Mon, Feb 03, 2025 at 10:35:42AM -0500, Alan Stern wrote: > On Wed, Jan 22, 2025 at 02:26:17PM -0500, Alan Stern wrote: > > Robert Morris created a test program which can cause > > usb_hub_to_struct_hub() to dereference a NULL or inappropriate > > pointer: > > > > Oops: general protection fault, probably for non-canonical address > > 0xcccccccccccccccc: 0000 [#1] SMP DEBUG_PAGEALLOC PTI > > CPU: 7 UID: 0 PID: 117 Comm: kworker/7:1 Not tainted 6.13.0-rc3-00017-gf44d154d6e3d #14 > > Hardware name: FreeBSD BHYVE/BHYVE, BIOS 14.0 10/17/2021 > > Workqueue: usb_hub_wq hub_event > > RIP: 0010:usb_hub_adjust_deviceremovable+0x78/0x110 > > ... > > Call Trace: > > <TASK> > > ? die_addr+0x31/0x80 > > ? exc_general_protection+0x1b4/0x3c0 > > ? asm_exc_general_protection+0x26/0x30 > > ? usb_hub_adjust_deviceremovable+0x78/0x110 > > hub_probe+0x7c7/0xab0 > > usb_probe_interface+0x14b/0x350 > > really_probe+0xd0/0x2d0 > > ? __pfx___device_attach_driver+0x10/0x10 > > __driver_probe_device+0x6e/0x110 > > driver_probe_device+0x1a/0x90 > > __device_attach_driver+0x7e/0xc0 > > bus_for_each_drv+0x7f/0xd0 > > __device_attach+0xaa/0x1a0 > > bus_probe_device+0x8b/0xa0 > > device_add+0x62e/0x810 > > usb_set_configuration+0x65d/0x990 > > usb_generic_driver_probe+0x4b/0x70 > > usb_probe_device+0x36/0xd0 > > > > The cause of this error is that the device has two interfaces, and the > > hub driver binds to interface 1 instead of interface 0, which is where > > usb_hub_to_struct_hub() looks. > > > > We can prevent the problem from occurring by refusing to accept hub > > devices that violate the USB spec by having more than one > > configuration or interface. > > > > Reported-and-tested-by: Robert Morris <rtm@csail.mit.edu> > > Closes: https://lore.kernel.org/linux-usb/95564.1737394039@localhost/ > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > > > --- > > Greg: > > I originally didn't mark this patch for the -stable kernels because it > doesn't really fix a bug. But on the other hand, it does protect > against a possible DOS attack from a malicious device, so perhaps it > does belong in the -stable kernels. > > I'll leave the decision up to you. We need to protect from malicious USB devices as we have a whole history of that being an exploit vector for systems, with the most recent ones happening just a few months ago, allowing anyone full access to a locked Android device by just plugging in a device with some specially crafted USB configuration headers. So let's try our best here where we can. Ideally the Android exploit shouldn't have even attempted to read the bad headers at the driver level, but this fix is at the hub level where I think all userspace "don't bind a driver unless we know it is ok" seem to ignore, so it's a good fix. thanks, greg k-h
Index: usb-devel/drivers/usb/core/hub.c =================================================================== --- usb-devel.orig/drivers/usb/core/hub.c +++ usb-devel/drivers/usb/core/hub.c @@ -1848,6 +1848,17 @@ static int hub_probe(struct usb_interfac hdev = interface_to_usbdev(intf); /* + * The USB 2.0 spec prohibits hubs from having more than one + * configuration or interface, and we rely on this prohibition. + * Refuse to accept a device that violates it. + */ + if (hdev->descriptor.bNumConfigurations > 1 || + hdev->actconfig->desc.bNumInterfaces > 1) { + dev_err(&intf->dev, "Invalid hub with more than one config or interface\n"); + return -EINVAL; + } + + /* * Set default autosuspend delay as 0 to speedup bus suspend, * based on the below considerations: *
Robert Morris created a test program which can cause usb_hub_to_struct_hub() to dereference a NULL or inappropriate pointer: Oops: general protection fault, probably for non-canonical address 0xcccccccccccccccc: 0000 [#1] SMP DEBUG_PAGEALLOC PTI CPU: 7 UID: 0 PID: 117 Comm: kworker/7:1 Not tainted 6.13.0-rc3-00017-gf44d154d6e3d #14 Hardware name: FreeBSD BHYVE/BHYVE, BIOS 14.0 10/17/2021 Workqueue: usb_hub_wq hub_event RIP: 0010:usb_hub_adjust_deviceremovable+0x78/0x110 ... Call Trace: <TASK> ? die_addr+0x31/0x80 ? exc_general_protection+0x1b4/0x3c0 ? asm_exc_general_protection+0x26/0x30 ? usb_hub_adjust_deviceremovable+0x78/0x110 hub_probe+0x7c7/0xab0 usb_probe_interface+0x14b/0x350 really_probe+0xd0/0x2d0 ? __pfx___device_attach_driver+0x10/0x10 __driver_probe_device+0x6e/0x110 driver_probe_device+0x1a/0x90 __device_attach_driver+0x7e/0xc0 bus_for_each_drv+0x7f/0xd0 __device_attach+0xaa/0x1a0 bus_probe_device+0x8b/0xa0 device_add+0x62e/0x810 usb_set_configuration+0x65d/0x990 usb_generic_driver_probe+0x4b/0x70 usb_probe_device+0x36/0xd0 The cause of this error is that the device has two interfaces, and the hub driver binds to interface 1 instead of interface 0, which is where usb_hub_to_struct_hub() looks. We can prevent the problem from occurring by refusing to accept hub devices that violate the USB spec by having more than one configuration or interface. Reported-and-tested-by: Robert Morris <rtm@csail.mit.edu> Closes: https://lore.kernel.org/linux-usb/95564.1737394039@localhost/ Signed-off-by: Alan Stern <stern@rowland.harvard.edu> --- drivers/usb/core/hub.c | 11 +++++++++++ 1 file changed, 11 insertions(+)