| Message ID | 20230607100540.31045-2-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | USB: dwc3: qcom: fix NULL-deref on suspend | expand |
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 >
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
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; }
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(-)