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

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

[Faisal Hassan]

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);
-	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;
 
 #ifdef CONFIG_USB
 	udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
-- 
2.17.1

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

[Thinh Nguyen]

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?

> -	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?

>  	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

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

[Prashanth K]

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

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

[Faisal Hassan]

On 8/14/2024 11:05 AM, Prashanth K wrote:
> 
> 
> 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.

Thanks Thinh and Prashanth for reviewing the patch.
Since the caller is validating 'dwc', I think there is no need to recheck.

>>
>>> -    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.

From history I see CONFIG_USB was added to fix build issues for gadget
only configuration. So configuration without CONFIG_USB also exists.

>>>       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
Thanks,
Faisal

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

[Thinh Nguyen]

On Wed, Aug 14, 2024, Faisal Hassan wrote:
> 
> 
> On 8/14/2024 11:05 AM, Prashanth K wrote:
> > 
> > 
> > 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.
> 
> Thanks Thinh and Prashanth for reviewing the patch.
> Since the caller is validating 'dwc', I think there is no need to recheck.

Ok. This is a bit hard to follow. The caller indirectly checks from
dwc3_qcom_is_host() prior to calling this. Hopefully with the
"flattening device" update, we can clean these up.

> 
> >>
> >>> -    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.
> 
> From history I see CONFIG_USB was added to fix build issues for gadget
> only configuration. So configuration without CONFIG_USB also exists.

If it's gadget only, then it wouldn't be calling this function. The
#ifdef CONFIG_USB guard placement can be reworked. But it shouldn't
block this patch.

> 
> >>>       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
> >>>
> >>

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

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

[Greg Kroah-Hartman]

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.
> 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);
> -	struct usb_device *udev;
> +	struct usb_device __maybe_unused *udev;

This change is not relevant to this overall patch, please remove it and
submit it separately if still needed.

thanks,

greg k-h

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

[Faisal Hassan]

On 8/22/2024 3:03 PM, Greg Kroah-Hartman 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.
>> 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);
>> -	struct usb_device *udev;
>> +	struct usb_device __maybe_unused *udev;
> 
> This change is not relevant to this overall patch, please remove it and
> submit it separately if still needed.

Understood. I’ll remove the change from this patch and submit it
separately if it’s still required. Thank you for the feedback!

> 
> thanks,
> 
> greg k-h

Thanks,
Faisal

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

[Johan Hovold]

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?

> 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.

Also make sure to CC the author of any patch introducing a bug so that
they may help with review.

> 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);
> -	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.

>  
>  #ifdef CONFIG_USB
>  	udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);

Johan

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

[Faisal Hassan]

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@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);
>> -	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

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

[Greg Kroah-Hartman]

On Fri, Aug 23, 2024 at 09:26:18AM +0530, Faisal Hassan wrote:
> 
> 
> 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.

That used to be the case a decade or so ago, but shouldn't be needed
anymore if your hardware is "sane" and can properly go to sleep.  Why
not just fix the driver to correctly sleep instead of unloading
everything?  This would require you to go through the whole
initialization sequence again when waking up and that can be a long time
overall, right?

thanks,

greg k-h