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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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