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

[rtm@csail.mit.edu]

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@rowland.harvard.edu>
> To: rtm@csail.mit.edu
> Cc: Greg KH <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org
> Subject: Re: USB hub code can dereference NULL hub and hub->ports
> 
> On Wed, Jan 22, 2025 at 06:37:45AM -0500, rtm@csail.mit.edu 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:
>  	 *