Re: Null ptr deref in core/hub.c

[Alan Stern <stern@rowland.harvard.edu>]

On Sat, Apr 24, 2021 at 02:08:45PM +0200, Felicitas Hetzelt wrote:
> Hello,
> 
> I triggered a few potential npds in core/hub.c. The bugs trigger
> reliably. Unfortunately I don't have a reproducer, though i tried my
> best to root-cause the bugs. I'm using my own emulated xhci host
> controller device and a slightly exotic kernel environment based on
> kernel version 5.10.0-rc6, so it might be that the bug is not
> trigger-able under normal conditions.
> 
> I was hoping you could maybe quickly determine whether this is a valid
> issue.
> 
> usb_hub_to_struct can return zero if hdev->actconfig->interface[0]->dev
> is NULL.

I'm not sure what you mean here.  dev is a member of the structure that 
hdev->actconfig->interface[0] points to; it isn't a pointer itself.  As 
such, to say that it is NULL is meaningless.

Are you saying that sometimes hdev->actconfig->interface[0] is NULL when 
usb_hub_to_struct runs?  How can that be?  usb_hub_to_struct doesn't 
even refer to that field unless hdev->maxchild is nonzero, and that 
field doesn't get set unless the hub driver is bound to the interface -- 
which doesn't happen if there is no interface.

> https://elixir.bootlin.com/linux/v4.9/source/drivers/usb/core/hub.c#L124
> 
> https://elixir.bootlin.com/linux/v4.9/source/include/linux/usb.h#L194
> 
> This is the case when usb_probe_interface fails to probe the device
> driver (called via usb_set_configuration -> device_add -> ...)
> 
> https://elixir.bootlin.com/linux/v4.9/source/drivers/usb/core/driver.c#L372

That line sets the interface's driver data, not interface[0]->dev.  
What's wrong with having the driver data be NULL?  That just means the 
device isn't a hub.  In this situation hdev->maxchild will be 0.

Also, you say you're using a modified version of the 5.10.0-rc6 kernel 
(kind of strange to be doing development on an -rc kernel instead of a 
normal release, but never mind).  So why are you posting links to the 
4.9 kernel source?  There have been a lot of changes between 4.9 and 
5.10.

> Then e.g. on a new invocation of hub_port_connect, the function tries to
> un-attach the previously attached devices (listed as port_dev->child)
> and calls recursively_mark_NOTATTACHED (via usb_set_device_state(udev,
> USB_STATE_NOTATTACHED), which in turn tries to get a pointer to the hub
> via usb_hub_to_struct_hub, which is NULL which leads to the crash.
> 
> https://elixir.bootlin.com/linux/v4.9/source/drivers/usb/core/hub.c#L4742

That's the call to usb_disconnect, which isn't really important to what 
you're saying.  You seem to be complaining about a crash in 
recursively_mark_NOTATTACHED.  Why didn't you insert a URL for that 
function instead?

> I feel like this should be caught in hub_port_connect (which would set

You're not making yourself clear.  What should be caught in 
hub_port_connect?

Are you saying that sometimes we can have udev->maxchild > 0 in 
recursively_mark_NOTATTACHED even though udev isn't bound to the hub 
driver?  If that's not it, then what _are_ you saying?

> port_dev->child = NULL, avoiding the later invocation of
> recursively_mark_NOTATTACHED), but the return value of usb_new_device is
> always valid (in fact usb_set_configuration can only return 0 once it
> gets to the calling add_device and probe).
> 
> https://elixir.bootlin.com/linux/v4.9/source/drivers/usb/core/hub.c#L4891
> 
> https://elixir.bootlin.com/linux/v4.9/source/drivers/usb/core/message.c#L1931
> 
> To fix this one could check whether the interface is actually properly
> setup instead of just checking status, or alternatively always check the
> return value of usb_hub_to_struct_hub on later invocations.
> 
> I attached the kernel log, it is a bit messy though i marked the
> relevant parts with 'XXNOTE'.

TL;DR.  You should trim logs so that only the important parts get 
posted.

> Let me know if you need any further information.

Please try to give a more explicit description of what's actually going 
wrong.  If you need to refer to lines of code in your email, just copy 
them into the message -- don't put a URL to somewhere else, forcing the 
reader to do extra work and lose the mental thread of the discussion.

Alan Stern