Re: [PATCH 3/5] usb: dwc3: qcom: Fix null ptr access during runtime_suspend()

[Manivannan Sadhasivam]

On Tue, Mar 28, 2023 at 11:23:32AM +0200, Johan Hovold wrote:
> On Sat, Mar 25, 2023 at 10:22:15PM +0530, Manivannan Sadhasivam wrote:
> > When runtime PM is enabled during probe, the PM core suspends this driver
> > before probing the dwc3 driver. Due to this, the dwc3_qcom_is_host()
> > function dereferences the driver data of the dwc platform device which
> > will only be set if the dwc driver has been probed. This causes null
> > pointer dereference during boot time.
> 
> So this does not really appear to be an issue before your later patch
> which enables runtime PM at probe.
> 

right.

> But the layering violations we have in this driver are indeed fragile
> and should be fixed properly at some point.
> 
> > So let's add a check for dwc drvdata in the callers of dwc3_qcom_is_host()
> > such as dwc3_qcom_suspend() and dwc3_qcom_resume() functions. There is no
> > need to add the same check in another caller dwc3_qcom_resume_irq() as the
> > wakeup IRQs will only be enabled at the end of dwc3_qcom_suspend().
> > 
> > Note that the check should not be added to dwc3_qcom_is_host() function
> > itself, as there is no provision to pass the context to callers.
> > 
> > Fixes: a872ab303d5d ("usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup")
> 
> This is not the right fixes tag in any case as this layering violation
> was first added by:
> 
> 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> 
> which started accessing the dwc3 platform data and xhci host data from
> the glue driver (and broke gadget mode).
> 

ah, I missed it, thanks for spotting.

> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 959fc925ca7c..bbf67f705d0d 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -411,10 +411,11 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> >  
> >  static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> >  {
> > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >  	u32 val;
> >  	int i, ret;
> >  
> > -	if (qcom->is_suspended)
> > +	if (qcom->is_suspended || !dwc)
> >  		return 0;
> 
> I think we should try to keep the layering violations confined to the
> helper functions. So how about amending dwc3_qcom_is_host() and check
> for NULL before dereferencing the xhci pointer?
> 
> If the dwc3 driver hasn't probed yet, we're clearly not in host mode
> either...
> 

Well, that's what I initially did but then I reverted to this approach as
returning true/false from dwc3_qcom_is_host() based on the pointer availability
doesn't sound right.

For example, if we return true then it implies that the driver is in host mode
which is logically wrong (before dwc3 probe) even though there is no impact.

- Mani

> Johan

-- 
மணிவண்ணன் சதாசிவம்