| Message ID | 20230325165217.31069-4-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: qcom: Allow runtime PM | expand |
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
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@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... > 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
On Tue, Mar 28, 2023 at 03:17:18PM +0530, Manivannan Sadhasivam wrote: > 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: > > > 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. No, you should return false of course as we are *not* in host mode as I mentioned above. Johan
On Tue, Mar 28, 2023 at 11:51:53AM +0200, Johan Hovold wrote: > On Tue, Mar 28, 2023 at 03:17:18PM +0530, Manivannan Sadhasivam wrote: > > 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: > > > > > 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. > > No, you should return false of course as we are *not* in host mode as I > mentioned above. > Yes, but I interpreted it as "we are in device mode" in that case. But looking at it again, I think it just conveys that the controller is not in host mode only. - Mani > Johan
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; val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); @@ -444,10 +445,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) { + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); int ret; int i; - if (!qcom->is_suspended) + if (!qcom->is_suspended || !dwc) return 0; if (dwc3_qcom_is_host(qcom) && wakeup)
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 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") Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/usb/dwc3/dwc3-qcom.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)