[PATCH 1/2] USB: dwc3: qcom: fix NULL-deref on suspend

[PATCH 1/2] USB: dwc3: qcom: fix NULL-deref on suspend

[Johan Hovold]

The Qualcomm dwc3 glue driver is currently accessing the driver data of
the child core device during suspend and on wakeup interrupts. This is
clearly a bad idea as the child may not have probed yet or could have
been unbound from its driver.

The first such layering violation was part of the initial version of the
driver, but this was later made worse when the hack that accesses the
driver data of the grand child xhci device to configure the wakeup
interrupts was added.

Fixing this properly is not that easily done, so add a sanity check to
make sure that the child driver data is non-NULL before dereferencing it
for now.

Note that this relies on subtleties like the fact that driver core is
making sure that the parent is not suspended while the child is probing.

Reported-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Link: https://lore.kernel.org/all/20230325165217.31069-4-manivannan.sadhasivam@linaro.org/
Fixes: d9152161b4bf ("usb: dwc3: Add Qualcomm DWC3 glue layer driver")
Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
Cc: stable@vger.kernel.org	# 3.18: a872ab303d5d: "usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup"
Cc: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
Cc: Krishna Kurapati <quic_kriskura@quicinc.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 959fc925ca7c..79b22abf9727 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -308,7 +308,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
 /* Only usable in contexts where the role can not change. */
 static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
 {
-	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	struct dwc3 *dwc;
+
+	/*
+	 * FIXME: Fix this layering violation.
+	 */
+	dwc = platform_get_drvdata(qcom->dwc3);
+
+	/* Core driver may not have probed yet. */
+	if (!dwc)
+		return false;
 
 	return dwc->xhci;
 }
-- 
2.39.3

Re: [PATCH 1/2] USB: dwc3: qcom: fix NULL-deref on suspend

[Manivannan Sadhasivam]

On Wed, Jun 07, 2023 at 12:05:39PM +0200, Johan Hovold wrote:
> The Qualcomm dwc3 glue driver is currently accessing the driver data of
> the child core device during suspend and on wakeup interrupts. This is
> clearly a bad idea as the child may not have probed yet or could have
> been unbound from its driver.
> 
> The first such layering violation was part of the initial version of the
> driver, but this was later made worse when the hack that accesses the
> driver data of the grand child xhci device to configure the wakeup
> interrupts was added.
> 
> Fixing this properly is not that easily done, so add a sanity check to
> make sure that the child driver data is non-NULL before dereferencing it
> for now.
> 
> Note that this relies on subtleties like the fact that driver core is
> making sure that the parent is not suspended while the child is probing.
> 
> Reported-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Link: https://lore.kernel.org/all/20230325165217.31069-4-manivannan.sadhasivam@linaro.org/
> Fixes: d9152161b4bf ("usb: dwc3: Add Qualcomm DWC3 glue layer driver")
> Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> Cc: stable@vger.kernel.org	# 3.18: a872ab303d5d: "usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup"
> Cc: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> Cc: Krishna Kurapati <quic_kriskura@quicinc.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 959fc925ca7c..79b22abf9727 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -308,7 +308,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
>  /* Only usable in contexts where the role can not change. */
>  static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
>  {
> -	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +	struct dwc3 *dwc;
> +
> +	/*
> +	 * FIXME: Fix this layering violation.
> +	 */
> +	dwc = platform_get_drvdata(qcom->dwc3);
> +
> +	/* Core driver may not have probed yet. */
> +	if (!dwc)
> +		return false;
>  
>  	return dwc->xhci;
>  }
> -- 
> 2.39.3
> 

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

Re: [PATCH 1/2] USB: dwc3: qcom: fix NULL-deref on suspend

[Thinh Nguyen]

On Wed, Jun 07, 2023, Johan Hovold wrote:
> The Qualcomm dwc3 glue driver is currently accessing the driver data of
> the child core device during suspend and on wakeup interrupts. This is
> clearly a bad idea as the child may not have probed yet or could have
> been unbound from its driver.
> 
> The first such layering violation was part of the initial version of the
> driver, but this was later made worse when the hack that accesses the
> driver data of the grand child xhci device to configure the wakeup
> interrupts was added.
> 
> Fixing this properly is not that easily done, so add a sanity check to
> make sure that the child driver data is non-NULL before dereferencing it
> for now.
> 
> Note that this relies on subtleties like the fact that driver core is
> making sure that the parent is not suspended while the child is probing.
> 
> Reported-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Link: https://urldefense.com/v3/__https://lore.kernel.org/all/20230325165217.31069-4-manivannan.sadhasivam@linaro.org/__;!!A4F2R9G_pg!dk6pg2XJjKSKBmzlobQwPOXDen4GHR-wW53sBe71g0X6gbC7AfOePhrb76oTLN6yHWTvKBy_y5pwSE1_HTASMhOnd5zz$ 
> Fixes: d9152161b4bf ("usb: dwc3: Add Qualcomm DWC3 glue layer driver")
> Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> Cc: stable@vger.kernel.org	# 3.18: a872ab303d5d: "usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup"
> Cc: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> Cc: Krishna Kurapati <quic_kriskura@quicinc.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 959fc925ca7c..79b22abf9727 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -308,7 +308,16 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
>  /* Only usable in contexts where the role can not change. */
>  static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
>  {
> -	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +	struct dwc3 *dwc;
> +
> +	/*
> +	 * FIXME: Fix this layering violation.
> +	 */
> +	dwc = platform_get_drvdata(qcom->dwc3);
> +
> +	/* Core driver may not have probed yet. */
> +	if (!dwc)
> +		return false;
>  
>  	return dwc->xhci;
>  }
> -- 
> 2.39.3
> 

Thanks for the catch.

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

BR,
Thinh

[PATCH 2/2] USB: dwc3: fix use-after-free on core driver unbind

[Johan Hovold]

Some dwc3 glue drivers are currently accessing the driver data of the
child core device directly, which is clearly a bad idea as the child may
not have probed yet or may have been unbound from its driver.

As a workaround until the glue drivers have been fixed, clear the driver
data pointer before allowing the glue parent device to runtime suspend
to prevent its driver from accessing data that has been freed during
unbind.

Fixes: 6dd2565989b4 ("usb: dwc3: add imx8mp dwc3 glue layer driver")
Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
Cc: stable@vger.kernel.org      # 5.12
Cc: Li Jun <jun.li@nxp.com>
Cc: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
Cc: Krishna Kurapati <quic_kriskura@quicinc.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 7b2ce013cc5b..d68958e151a7 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1929,6 +1929,11 @@ static int dwc3_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
+	/*
+	 * HACK: Clear the driver data, which is currently accessed by parent
+	 * glue drivers, before allowing the parent to suspend.
+	 */
+	platform_set_drvdata(pdev, NULL);
 	pm_runtime_set_suspended(&pdev->dev);
 
 	dwc3_free_event_buffers(dwc);
-- 
2.39.3

Re: [PATCH 2/2] USB: dwc3: fix use-after-free on core driver unbind

[Manivannan Sadhasivam]

On Wed, Jun 07, 2023 at 12:05:40PM +0200, Johan Hovold wrote:
> Some dwc3 glue drivers are currently accessing the driver data of the
> child core device directly, which is clearly a bad idea as the child may
> not have probed yet or may have been unbound from its driver.
> 
> As a workaround until the glue drivers have been fixed, clear the driver
> data pointer before allowing the glue parent device to runtime suspend
> to prevent its driver from accessing data that has been freed during
> unbind.
> 
> Fixes: 6dd2565989b4 ("usb: dwc3: add imx8mp dwc3 glue layer driver")
> Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> Cc: stable@vger.kernel.org      # 5.12
> Cc: Li Jun <jun.li@nxp.com>
> Cc: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> Cc: Krishna Kurapati <quic_kriskura@quicinc.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/usb/dwc3/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 7b2ce013cc5b..d68958e151a7 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1929,6 +1929,11 @@ static int dwc3_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_put_noidle(&pdev->dev);
> +	/*
> +	 * HACK: Clear the driver data, which is currently accessed by parent
> +	 * glue drivers, before allowing the parent to suspend.
> +	 */
> +	platform_set_drvdata(pdev, NULL);

This is required because you have seen the glue driver going to runtime suspend
once the below pm_runtime_set_suspended() is completed?

- Mani

>  	pm_runtime_set_suspended(&pdev->dev);
>  
>  	dwc3_free_event_buffers(dwc);
> -- 
> 2.39.3
> 

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

Re: [PATCH 2/2] USB: dwc3: fix use-after-free on core driver unbind

[Johan Hovold]

On Thu, Jun 08, 2023 at 06:32:46PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jun 07, 2023 at 12:05:40PM +0200, Johan Hovold wrote:
> > Some dwc3 glue drivers are currently accessing the driver data of the
> > child core device directly, which is clearly a bad idea as the child may
> > not have probed yet or may have been unbound from its driver.
> > 
> > As a workaround until the glue drivers have been fixed, clear the driver
> > data pointer before allowing the glue parent device to runtime suspend
> > to prevent its driver from accessing data that has been freed during
> > unbind.

> > @@ -1929,6 +1929,11 @@ static int dwc3_remove(struct platform_device *pdev)
> >  	pm_runtime_disable(&pdev->dev);
> >  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> >  	pm_runtime_put_noidle(&pdev->dev);
> > +	/*
> > +	 * HACK: Clear the driver data, which is currently accessed by parent
> > +	 * glue drivers, before allowing the parent to suspend.
> > +	 */
> > +	platform_set_drvdata(pdev, NULL);
> 
> This is required because you have seen the glue driver going to runtime suspend
> once the below pm_runtime_set_suspended() is completed?

This is based on analysis of the code. The parent (glue) can not suspend
while the child (core) is in the active state, but once we set the
suspended state that could happen.

> >  
> >  	dwc3_free_event_buffers(dwc);

Johan

Re: [PATCH 2/2] USB: dwc3: fix use-after-free on core driver unbind

[Manivannan Sadhasivam]

On Thu, Jun 08, 2023 at 03:09:27PM +0200, Johan Hovold wrote:
> On Thu, Jun 08, 2023 at 06:32:46PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jun 07, 2023 at 12:05:40PM +0200, Johan Hovold wrote:
> > > Some dwc3 glue drivers are currently accessing the driver data of the
> > > child core device directly, which is clearly a bad idea as the child may
> > > not have probed yet or may have been unbound from its driver.
> > > 
> > > As a workaround until the glue drivers have been fixed, clear the driver
> > > data pointer before allowing the glue parent device to runtime suspend
> > > to prevent its driver from accessing data that has been freed during
> > > unbind.
> 
> > > @@ -1929,6 +1929,11 @@ static int dwc3_remove(struct platform_device *pdev)
> > >  	pm_runtime_disable(&pdev->dev);
> > >  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> > >  	pm_runtime_put_noidle(&pdev->dev);
> > > +	/*
> > > +	 * HACK: Clear the driver data, which is currently accessed by parent
> > > +	 * glue drivers, before allowing the parent to suspend.
> > > +	 */
> > > +	platform_set_drvdata(pdev, NULL);
> > 
> > This is required because you have seen the glue driver going to runtime suspend
> > once the below pm_runtime_set_suspended() is completed?
> 
> This is based on analysis of the code. The parent (glue) can not suspend
> while the child (core) is in the active state, but once we set the
> suspended state that could happen.
> 

I could see that the driver core is setting drvdata to NULL during
device_unbind_cleanup(), so not sure if this scenario could be met otherwise it
will be redundant.

- Mani


> > >  
> > >  	dwc3_free_event_buffers(dwc);
> 
> Johan

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

Re: [PATCH 2/2] USB: dwc3: fix use-after-free on core driver unbind

[Johan Hovold]

On Thu, Jun 08, 2023 at 06:53:13PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jun 08, 2023 at 03:09:27PM +0200, Johan Hovold wrote:
> > On Thu, Jun 08, 2023 at 06:32:46PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Jun 07, 2023 at 12:05:40PM +0200, Johan Hovold wrote:
> > > > Some dwc3 glue drivers are currently accessing the driver data of the
> > > > child core device directly, which is clearly a bad idea as the child may
> > > > not have probed yet or may have been unbound from its driver.
> > > > 
> > > > As a workaround until the glue drivers have been fixed, clear the driver
> > > > data pointer before allowing the glue parent device to runtime suspend
> > > > to prevent its driver from accessing data that has been freed during
> > > > unbind.
> > 
> > > > @@ -1929,6 +1929,11 @@ static int dwc3_remove(struct platform_device *pdev)
> > > >  	pm_runtime_disable(&pdev->dev);
> > > >  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> > > >  	pm_runtime_put_noidle(&pdev->dev);
> > > > +	/*
> > > > +	 * HACK: Clear the driver data, which is currently accessed by parent
> > > > +	 * glue drivers, before allowing the parent to suspend.
> > > > +	 */
> > > > +	platform_set_drvdata(pdev, NULL);
> > > 
> > > This is required because you have seen the glue driver going to runtime suspend
> > > once the below pm_runtime_set_suspended() is completed?
> > 
> > This is based on analysis of the code. The parent (glue) can not suspend
> > while the child (core) is in the active state, but once we set the
> > suspended state that could happen.
> 
> I could see that the driver core is setting drvdata to NULL during
> device_unbind_cleanup(), so not sure if this scenario could be met otherwise it
> will be redundant.

If this was redundant I wouldn't have added it. ;)

The parent driver has no business accessing the driver data of the child
in the first place, but it must absolutely not do so after the child has
been unbound from its driver and the driver data is getting freed.

Relying on the clean up in driver core that resets this pointer does not
work as that would still leave a window where the parent could access
this stale data.

Johan

Re: [PATCH 2/2] USB: dwc3: fix use-after-free on core driver unbind

[Manivannan Sadhasivam]

On Thu, Jun 08, 2023 at 03:49:58PM +0200, Johan Hovold wrote:
> On Thu, Jun 08, 2023 at 06:53:13PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jun 08, 2023 at 03:09:27PM +0200, Johan Hovold wrote:
> > > On Thu, Jun 08, 2023 at 06:32:46PM +0530, Manivannan Sadhasivam wrote:
> > > > On Wed, Jun 07, 2023 at 12:05:40PM +0200, Johan Hovold wrote:
> > > > > Some dwc3 glue drivers are currently accessing the driver data of the
> > > > > child core device directly, which is clearly a bad idea as the child may
> > > > > not have probed yet or may have been unbound from its driver.
> > > > > 
> > > > > As a workaround until the glue drivers have been fixed, clear the driver
> > > > > data pointer before allowing the glue parent device to runtime suspend
> > > > > to prevent its driver from accessing data that has been freed during
> > > > > unbind.
> > > 
> > > > > @@ -1929,6 +1929,11 @@ static int dwc3_remove(struct platform_device *pdev)
> > > > >  	pm_runtime_disable(&pdev->dev);
> > > > >  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> > > > >  	pm_runtime_put_noidle(&pdev->dev);
> > > > > +	/*
> > > > > +	 * HACK: Clear the driver data, which is currently accessed by parent
> > > > > +	 * glue drivers, before allowing the parent to suspend.
> > > > > +	 */
> > > > > +	platform_set_drvdata(pdev, NULL);
> > > > 
> > > > This is required because you have seen the glue driver going to runtime suspend
> > > > once the below pm_runtime_set_suspended() is completed?
> > > 
> > > This is based on analysis of the code. The parent (glue) can not suspend
> > > while the child (core) is in the active state, but once we set the
> > > suspended state that could happen.
> > 
> > I could see that the driver core is setting drvdata to NULL during
> > device_unbind_cleanup(), so not sure if this scenario could be met otherwise it
> > will be redundant.
> 
> If this was redundant I wouldn't have added it. ;)
> 
> The parent driver has no business accessing the driver data of the child
> in the first place, but it must absolutely not do so after the child has
> been unbound from its driver and the driver data is getting freed.
> 
> Relying on the clean up in driver core that resets this pointer does not
> work as that would still leave a window where the parent could access
> this stale data.
> 

Well, I agree with the small window here.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Johan

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

Re: [PATCH 2/2] USB: dwc3: fix use-after-free on core driver unbind

[Thinh Nguyen]

On Wed, Jun 07, 2023, Johan Hovold wrote:
> Some dwc3 glue drivers are currently accessing the driver data of the
> child core device directly, which is clearly a bad idea as the child may
> not have probed yet or may have been unbound from its driver.
> 
> As a workaround until the glue drivers have been fixed, clear the driver
> data pointer before allowing the glue parent device to runtime suspend
> to prevent its driver from accessing data that has been freed during
> unbind.
> 
> Fixes: 6dd2565989b4 ("usb: dwc3: add imx8mp dwc3 glue layer driver")
> Fixes: 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> Cc: stable@vger.kernel.org      # 5.12
> Cc: Li Jun <jun.li@nxp.com>
> Cc: Sandeep Maheswaram <quic_c_sanm@quicinc.com>
> Cc: Krishna Kurapati <quic_kriskura@quicinc.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/usb/dwc3/core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 7b2ce013cc5b..d68958e151a7 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1929,6 +1929,11 @@ static int dwc3_remove(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_put_noidle(&pdev->dev);
> +	/*
> +	 * HACK: Clear the driver data, which is currently accessed by parent
> +	 * glue drivers, before allowing the parent to suspend.
> +	 */
> +	platform_set_drvdata(pdev, NULL);
>  	pm_runtime_set_suspended(&pdev->dev);
>  
>  	dwc3_free_event_buffers(dwc);
> -- 
> 2.39.3
> 

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

Thanks,
Thinh