| Message ID | 1615502070-19705-1-git-send-email-wcheng@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: dwc3: gadget: Prevent EP queuing while stopping transfers | expand |
Wesley Cheng wrote: > In the situations where the DWC3 gadget stops active transfers, once > calling the dwc3_gadget_giveback(), there is a chance where a function > driver can queue a new USB request in between the time where the dwc3 > lock has been released and re-aquired. This occurs after we've already > issued an ENDXFER command. When the stop active transfers continues > to remove USB requests from all dep lists, the newly added request will > also be removed, while controller still has an active TRB for it. > This can lead to the controller accessing an unmapped memory address. > > Fix this by ensuring parameters to prevent EP queuing are set before > calling the stop active transfers API. > > Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the controller") > Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> > --- > Changes since V1: > - Added Fixes tag to point to the commit this is addressing > > drivers/usb/dwc3/gadget.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 4780983..4d98fbf 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -783,8 +783,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) > > trace_dwc3_gadget_ep_disable(dep); > > - dwc3_remove_requests(dwc, dep); > - > /* make sure HW endpoint isn't stalled */ > if (dep->flags & DWC3_EP_STALL) > __dwc3_gadget_ep_set_halt(dep, 0, false); > @@ -803,6 +801,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) > dep->endpoint.desc = NULL; > } > > + dwc3_remove_requests(dwc, dep); > + > return 0; > } > > @@ -1617,7 +1617,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > { > struct dwc3 *dwc = dep->dwc; > > - if (!dep->endpoint.desc || !dwc->pullups_connected) { > + if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) { > dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n", > dep->name); > return -ESHUTDOWN; > @@ -2247,6 +2247,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) > if (!is_on) { > u32 count; > > + dwc->connected = false; You want to set this before __dwc3_gadget_stop() right? Then you may want to remove this same setting few lines below this. Otherwise, this looks good. Thanks, Thinh > /* > * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a > * Section 4.1.8 Table 4-7, it states that for a device-initiated > @@ -3329,8 +3330,6 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) > { > u32 reg; > > - dwc->connected = true; > - > /* > * WORKAROUND: DWC3 revisions <1.88a have an issue which > * would cause a missing Disconnect Event if there's a > @@ -3370,6 +3369,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) > * transfers." > */ > dwc3_stop_active_transfers(dwc); > + dwc->connected = true; > > reg = dwc3_readl(dwc->regs, DWC3_DCTL); > reg &= ~DWC3_DCTL_TSTCTRL_MASK; >
On 3/11/2021 3:18 PM, Thinh Nguyen wrote: > Wesley Cheng wrote: >> In the situations where the DWC3 gadget stops active transfers, once >> calling the dwc3_gadget_giveback(), there is a chance where a function >> driver can queue a new USB request in between the time where the dwc3 >> lock has been released and re-aquired. This occurs after we've already >> issued an ENDXFER command. When the stop active transfers continues >> to remove USB requests from all dep lists, the newly added request will >> also be removed, while controller still has an active TRB for it. >> This can lead to the controller accessing an unmapped memory address. >> >> Fix this by ensuring parameters to prevent EP queuing are set before >> calling the stop active transfers API. >> >> Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the controller") >> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >> --- >> Changes since V1: >> - Added Fixes tag to point to the commit this is addressing >> >> drivers/usb/dwc3/gadget.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 4780983..4d98fbf 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -783,8 +783,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) >> >> trace_dwc3_gadget_ep_disable(dep); >> >> - dwc3_remove_requests(dwc, dep); >> - >> /* make sure HW endpoint isn't stalled */ >> if (dep->flags & DWC3_EP_STALL) >> __dwc3_gadget_ep_set_halt(dep, 0, false); >> @@ -803,6 +801,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) >> dep->endpoint.desc = NULL; >> } >> >> + dwc3_remove_requests(dwc, dep); >> + >> return 0; >> } >> >> @@ -1617,7 +1617,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) >> { >> struct dwc3 *dwc = dep->dwc; >> >> - if (!dep->endpoint.desc || !dwc->pullups_connected) { >> + if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) { >> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n", >> dep->name); >> return -ESHUTDOWN; >> @@ -2247,6 +2247,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) >> if (!is_on) { >> u32 count; >> >> + dwc->connected = false; > > > You want to set this before __dwc3_gadget_stop() right? Then you may > want to remove this same setting few lines below this. Otherwise, this > looks good. > > Thanks, > Thinh > Hi Thinh, Ah, good catch, must've missed that while porting this over from my local branch. Will remove that connected false setting a few lines down. Thanks Wesley Cheng > >> /* >> * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a >> * Section 4.1.8 Table 4-7, it states that for a device-initiated >> @@ -3329,8 +3330,6 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) >> { >> u32 reg; >> >> - dwc->connected = true; >> - >> /* >> * WORKAROUND: DWC3 revisions <1.88a have an issue which >> * would cause a missing Disconnect Event if there's a >> @@ -3370,6 +3369,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) >> * transfers." >> */ >> dwc3_stop_active_transfers(dwc); >> + dwc->connected = true; >> >> reg = dwc3_readl(dwc->regs, DWC3_DCTL); >> reg &= ~DWC3_DCTL_TSTCTRL_MASK; >> >
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 4780983..4d98fbf 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -783,8 +783,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) trace_dwc3_gadget_ep_disable(dep); - dwc3_remove_requests(dwc, dep); - /* make sure HW endpoint isn't stalled */ if (dep->flags & DWC3_EP_STALL) __dwc3_gadget_ep_set_halt(dep, 0, false); @@ -803,6 +801,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) dep->endpoint.desc = NULL; } + dwc3_remove_requests(dwc, dep); + return 0; } @@ -1617,7 +1617,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) { struct dwc3 *dwc = dep->dwc; - if (!dep->endpoint.desc || !dwc->pullups_connected) { + if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) { dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n", dep->name); return -ESHUTDOWN; @@ -2247,6 +2247,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) if (!is_on) { u32 count; + dwc->connected = false; /* * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a * Section 4.1.8 Table 4-7, it states that for a device-initiated @@ -3329,8 +3330,6 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) { u32 reg; - dwc->connected = true; - /* * WORKAROUND: DWC3 revisions <1.88a have an issue which * would cause a missing Disconnect Event if there's a @@ -3370,6 +3369,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) * transfers." */ dwc3_stop_active_transfers(dwc); + dwc->connected = true; reg = dwc3_readl(dwc->regs, DWC3_DCTL); reg &= ~DWC3_DCTL_TSTCTRL_MASK;
In the situations where the DWC3 gadget stops active transfers, once calling the dwc3_gadget_giveback(), there is a chance where a function driver can queue a new USB request in between the time where the dwc3 lock has been released and re-aquired. This occurs after we've already issued an ENDXFER command. When the stop active transfers continues to remove USB requests from all dep lists, the newly added request will also be removed, while controller still has an active TRB for it. This can lead to the controller accessing an unmapped memory address. Fix this by ensuring parameters to prevent EP queuing are set before calling the stop active transfers API. Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the controller") Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> --- Changes since V1: - Added Fixes tag to point to the commit this is addressing drivers/usb/dwc3/gadget.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)