USB hub code can dereference NULL hub and hub->ports

The attached program, which acts via usbip as a USB device or hub, causes my linux machines to dereference some NULL pointers in drivers/usb/core/hub.c. These are places where udev->maxchild > 0, but either usb_hub_to_struct_hub(udev) returns NULL, or the returned hub has hub->ports == NULL.

This is one such place:

static void recursively_mark_NOTATTACHED(struct usb_device *udev)

{
        struct usb_hub *hub = usb_hub_to_struct_hub(udev);
        int i;

        for (i = 0; i < udev->maxchild; ++i) {
                if (hub->ports[i]->child)

And this:

static void hub_disconnect_children(struct usb_device *udev)

{
        struct usb_hub *hub = usb_hub_to_struct_hub(udev);
        int i;

        /* Free up all the children before we remove this device */
        for (i = 0; i < udev->maxchild; i++) {
                if (hub->ports[i]->child)

This can see NULL hub->ports:

void usb_hub_adjust_deviceremovable(struct usb_device *hdev,
struct usb_hub_descriptor *desc)

{
        struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
        enum usb_port_connect_type connect_type;
        int i;

        if (!hub)
                return;

        if (!hub_is_superspeed(hdev)) {
                for (i = 1; i <= hdev->maxchild; i++) {
                        struct usb_port *port_dev = hub->ports[i - 1];

This can see a NULL hub:

static int hub_set_address(struct usb_device *udev, int devnum)

{
        int retval;
        unsigned int timeout_ms = USB_CTRL_SET_TIMEOUT;
        struct usb_hcd *hcd = bus_to_hcd(udev->bus);
        struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);

        if (hub->hdev->quirks & USB_QUIRK_SHORT_SET_ADDRESS_REQ_TIMEOUT)

I've attached a demo that runs into some of these NULL dereferences. It depends on being able to run usbip (and modeprobe vhci-hcd).

# uname -a
Linux xxx 6.13.0-rc3-00017-gf44d154d6e3d #14 SMP Mon Jan 20 04:52:59 EST 2025 x86_64 x86_64 x86_64 GNU/Linux
# cc usbhub11b.c
# ./a.out
...
hub 1-1:1.16: bad descriptor, ignoring hub
hub 1-1:1.16: probe with driver hub failed with error -5
BUG: kernel NULL pointer dereference, address: 0000000000000250
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: Oops: 0000 [#1] SMP DEBUG_PAGEALLOC PTI
CPU: 8 UID: 0 PID: 302 Comm: kworker/8:1 Not tainted 6.13.0-rc3-00017-gf44d154d6
e3d #14
Hardware name: FreeBSD BHYVE/BHYVE, BIOS 14.0 10/17/2021
Workqueue: usb_hub_wq hub_event
RIP: 0010:recursively_mark_NOTATTACHED+0x37/0x90
Code: 48 85 ff 74 71 4c 8b a7 18 04 00 00 4d 85 e4 74 13 85 c0 74 3a 49 8b 94 24
 98 00 00 00 4c 8b a2 c8 00 00 00 85 c0 7e 27 31 db <49> 8b 84 24 50 02 00 00 48
 8b 04 d8 48 8b 38 48 85 ff 74 05 e8 b0
RSP: 0018:ffffc9000096bd28 EFLAGS: 00010046
RAX: 0000000000000001 RBX: 0000000000000000 RCX: ffff888106224c00
RDX: ffff8881037bc000 RSI: 0000000000000007 RDI: ffff88810a502000
RBP: ffff88810a502000 R08: 000000000000005a R09: 0000000000000000
R10: ffff88810a502000 R11: ffff88810a502000 R12: 0000000000000000
R13: ffff88810a502000 R14: ffff8881062241f0 R15: 0000000000000001
FS:  0000000000000000(0000) GS:ffff88842dc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000250 CR3: 0000000003636001 CR4: 00000000003706f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 ? __die+0x1e/0x60
 ? page_fault_oops+0x157/0x450
 ? set_track_prepare+0x3b/0x60
 ? usb_control_msg+0xfd/0x150
 ? check_bytes_and_report.isra.0+0x48/0x120
 ? exc_page_fault+0x66/0x140
 ? asm_exc_page_fault+0x26/0x30
 ? recursively_mark_NOTATTACHED+0x37/0x90
 ? usb_control_msg+0xfd/0x150
 usb_disconnect+0x37/0x2c0
 hub_event+0xc8f/0x1870
 usb_disconnect+0x37/0x2c0
 hub_event+0xc8f/0x1870
 ? trace_event_raw_event_sched_switch+0x51/0x150
 process_one_work+0x13f/0x330
 worker_thread+0x25a/0x370
 ? _raw_spin_unlock_irqrestore+0xd/0x20
 ? __pfx_worker_thread+0x10/0x10
 kthread+0xdc/0x110
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x2f/0x50
 ? __pfx_kthread+0x10/0x10
 ret_from_fork_asm+0x1a/0x30
 </TASK>

Robert Morris
[email protected]

Great, can you submit patches to fix these issues now that you have a reliable test program to verify the problem?

thanks,

greg k-h

I think the problem is (at least sometimes) not that hub->ports is legitimately NULL and there's a missing check, but that usb_hub_to_struct_hub() returns an object of the wrong type so that hub->ports is junk, and only accidentally NULL in the demo I previously submitted.

I've attached a new demo which crashes because hub->ports is 0xcccccccccccccccc (on a kernel with red zones). The demo presents a USB device whose DeviceClass is a hub (9), with two interfaces, but the Vendor and Product indicate an FTDI serial adaptor.

First, usb_serial_probe() sets the interface zero dev->driver_data to a struct usb_serial.

Later, when the hub code is trying to set up interface one, it calls usb_hub_to_struct_hub(hdev):

struct usb_hub *usb_hub_to_struct_hub(struct usb_device *hdev)

{
        if (!hdev || !hdev->actconfig || !hdev->maxchild)
                return NULL;
        return usb_get_intfdata(hdev->actconfig->interface[0]);
}

interface[0], however, has been set up by the serial port code, and its dev->driver_data is a struct usb_serial, not a struct usb_hub. struct usb_serial is shorter than usb_hub, and as a result the end of the usb_hub is beyond the end of the allocated memory, which causes hub->ports to refer to bytes in the red zone.

I don't understand the code well enough to suggest a patch.

# cc usbser1c.c
# ./a.out
...
hub 1-1:1.0: bad descriptor, ignoring hub
hub 1-1:1.0: probe with driver hub failed with error -5
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-gf44d154d6
e3d #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
 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_detect_static_quirks+0x41/0xf0
 usb_new_device+0x1c8/0x400
 hub_event+0x1047/0x1870
 process_one_work+0x13f/0x330
 worker_thread+0x25a/0x370
 ? _raw_spin_unlock_irqrestore+0xd/0x20
 ? __pfx_worker_thread+0x10/0x10
 kthread+0xdc/0x110
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x2f/0x50
 ? __pfx_kthread+0x10/0x10
 ret_from_fork_asm+0x1a/0x30

Robert Morris
[email protected]

Okay, that explains the problem. usb_hub_to_struct_hub() assumes that a hub device will never have more than one interface, because that requirement is in the USB spec. But of course, a bogus or malicious device can violate the requirement.

I think the best way to deal with this issue is to prevent the hub driver from binding to non-compliant devices. Does the patch below fix the problem for you?

Alan Stern

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

Alan,

Yes, your patch makes the NULL hub and hub->ports crashes I've seen go away!

Robert

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.

---

 drivers/usb/core/hub.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

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

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

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