On 8/22/2024 4:55 PM, Johan Hovold wrote: > On Tue, Aug 13, 2024 at 04:48:47PM +0530, Faisal Hassan wrote: >> Null pointer dereference occurs when accessing 'hcd' to detect speed >> from dwc3_qcom_suspend after the xhci-hcd is unbound. > > Why are you unbinding the xhci driver? > On our automotive platforms, when preparing for suspend, a script unbinds the xhci driver to remove all devices, ensuring the platform reaches the lowest power state. >> To avoid this issue, ensure to check for NULL in dwc3_qcom_read_usb2_speed. >> >> echo xhci-hcd.0.auto > /sys/bus/platform/drivers/xhci-hcd/unbind >> xhci_plat_remove() -> usb_put_hcd() -> hcd_release() -> kfree(hcd) >> >> Unable to handle kernel NULL pointer dereference at virtual address >> 0000000000000060 >> Call trace: >> dwc3_qcom_suspend.part.0+0x17c/0x2d0 [dwc3_qcom] >> dwc3_qcom_runtime_suspend+0x2c/0x40 [dwc3_qcom] >> pm_generic_runtime_suspend+0x30/0x44 >> __rpm_callback+0x4c/0x190 >> rpm_callback+0x6c/0x80 >> rpm_suspend+0x10c/0x620 >> pm_runtime_work+0xc8/0xe0 >> process_one_work+0x1e4/0x4f4 >> worker_thread+0x64/0x43c >> kthread+0xec/0x100 >> ret_from_fork+0x10/0x20 >> >> Fixes: c5f14abeb52b ("usb: dwc3: qcom: fix peripheral and OTG suspend") > > This is clearly not the commit that introduced this issue, please be > more careful. > My mistake, I should have verified that. I appreciate your feedback. I’ll make sure to be more careful in the future. > Also make sure to CC the author of any patch introducing a bug so that > they may help with review. > Understood. I’ll ensure to CC the author from next time. >> Cc: stable@xxxxxxxxxxxxxxx >> Signed-off-by: Faisal Hassan <quic_faisalh@xxxxxxxxxxx> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 88fb6706a18d..0c7846478655 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -319,13 +319,15 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) >> static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, int port_index) >> { >> struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); >> - struct usb_device *udev; >> + struct usb_device __maybe_unused *udev; >> struct usb_hcd __maybe_unused *hcd; >> >> /* >> * FIXME: Fix this layering violation. >> */ >> hcd = platform_get_drvdata(dwc->xhci); >> + if (!hcd) >> + return USB_SPEED_UNKNOWN; > > This is just papering over the real issue here which is the layering > violation of having drivers accessing driver data of their children. > > Nothing is preventing the driver data from being deallocated after you > check for NULL here. > > I suggest leaving this as is until Bjorn's patches that should address > this properly lands. > I agree that this part needs to be cleaned up. Currently, this is 100% reproducible on our platform, and adding this NULL check is very helpful, but I agree that the vulnerability is still not completely eliminated. Until this part is cleaned up, can this check be added to reduce the risk? >> >> #ifdef CONFIG_USB >> udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1); > > Johan Thanks, Faisal