Alan,
Yes, your patch makes the NULL hub and hub->ports crashes
I've seen go away!
Robert
> Date: Wed, 22 Jan 2025 10:55:24 -0500
> From: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> To: rtm@xxxxxxxxxxxxx
> Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>, linux-usb@xxxxxxxxxxxxxxx
> Subject: Re: USB hub code can dereference NULL hub and hub->ports
>
> 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:
> *
