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

From: Johan Hovold <johan@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: andersson@kernel.org, Thinh.Nguyen@synopsys.com,
	gregkh@linuxfoundation.org, mathias.nyman@intel.com,
	konrad.dybcio@linaro.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, linux-arm-msm@vger.kernel.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 3/5] usb: dwc3: qcom: Fix null ptr access during runtime_suspend()
Date: Tue, 28 Mar 2023 11:23:32 +0200	[thread overview]
Message-ID: <ZCKyFEc087xoypdo@hovoldconsulting.com> (raw)
In-Reply-To: <20230325165217.31069-4-manivannan.sadhasivam@linaro.org>

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.

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

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  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...

Johan