Re: [PATCH] usb: dwc3: qcom: fix NULL pointer dereference on dwc3_qcom_read_usb2_speed

[Prashanth K <quic_prashk@quicinc.com>]

On 14-08-24 05:47 am, Thinh Nguyen wrote:
> On Tue, Aug 13, 2024, Faisal Hassan wrote:
>> Null pointer dereference occurs when accessing 'hcd' to detect speed
>> from dwc3_qcom_suspend after the xhci-hcd is unbound.
>> 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")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Faisal Hassan <quic_faisalh@quicinc.com>
>> ---
>>   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);
> 
> What if dwc is not available?

Thats unlikely, dwc3_qcom_suspend() -> dwc3_qcom_is_host() checks for 
dwc, calls dwc3_qcom_read_usb2_speed() only if dwc is valid. But adding 
an extra check shouldn't cause harm.
> 
>> -	struct usb_device *udev;
>> +	struct usb_device __maybe_unused *udev;
> 
> This is odd.... Is there a scenario where you don't want to set
> CONFIG_USB if dwc3_qcom is in use?
> 
AFAIK this function is used to get the speeds of each ports, so that 
wakeup interrupts (dp/dm/ss irqs) can be configured accordingly before 
going to suspend, which is done during host mode only. So there 
shouldn't be any scenarios where CONFIG_USB isnt set when this is called.
>>   	struct usb_hcd __maybe_unused *hcd;
>>   
>>   	/*
>>   	 * FIXME: Fix this layering violation.
>>   	 */
>>   	hcd = platform_get_drvdata(dwc->xhci);
>> +	if (!hcd)
>> +		return USB_SPEED_UNKNOWN;
>>   
>>   #ifdef CONFIG_USB
> 
> Perhaps this #ifdef shouldn't only be checking this. But that's for
> another patch >>   	udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
>> -- 
>> 2.17.1
>>
> 
> BR,
> Thinh
Thanks,
Prashanth K