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. How can that be possible in a real system? > > 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 Note, 4.9 is VERY old :) > 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 And how can that happen in a real system? > 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 > > I feel like this should be caught in hub_port_connect (which would set > 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. Patches are great to send, that's the best way to discuss issues that you find. thanks, greg k-h
