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
