On Wed, Jan 22, 2025 at 06:37:45AM -0500, rtm@xxxxxxxxxxxxx wrote:
> > Great, can you submit patches to fix these issues now that you have a
> > reliable test program to verify the problem?
>
> 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.
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
===================================================================
--- 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:
*
