| Message ID | 20201029175949.6052-1-jackp@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] usb: gadget: audio: Free requests only after callback | expand |
Hi Op 29-10-2020 om 18:59 schreef Jack Pham: > As per the kernel doc for usb_ep_dequeue(), it states that "this > routine is asynchronous, that is, it may return before the completion > routine runs". And indeed since v5.0 the dwc3 gadget driver updated > its behavior to place dequeued requests on to a cancelled list to be > given back later after the endpoint is stopped. > > The free_ep() was incorrectly assuming that a request was ready to > be freed after calling dequeue which results in a use-after-free > in dwc3 when it traverses its cancelled list. Fix this by moving > the usb_ep_free_request() call to the callback itself in case the > ep is disabled. > > Fixes: eb9fecb9e69b0 ("usb: gadget: f_uac2: split out audio core") > Reported-and-tested-by: Ferry Toth<fntoth@gmail.com> > Reviewed-and-tested-by: Peter Chen<peter.chen@nxp.com> > Signed-off-by: Jack Pham<jackp@codeaurora.org> > --- > v3: Fixed incorrect 'req' parameter and added Peter's tag > > v2: call free_request() in case of ep_dequeue() failure > > drivers/usb/gadget/function/u_audio.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c > index e6d32c536781..6e69ccf02c95 100644 > --- a/drivers/usb/gadget/function/u_audio.c > +++ b/drivers/usb/gadget/function/u_audio.c > @@ -89,7 +89,12 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req) > struct snd_uac_chip *uac = prm->uac; > > /* i/f shutting down */ > - if (!prm->ep_enabled || req->status == -ESHUTDOWN) > + if (!prm->ep_enabled) { > + usb_ep_free_request(ep, req); > + return; > + } > + > + if (req->status == -ESHUTDOWN) > return; > > /* > @@ -336,8 +341,9 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) > > for (i = 0; i < params->req_number; i++) { > if (prm->ureq[i].req) { > - usb_ep_dequeue(ep, prm->ureq[i].req); > - usb_ep_free_request(ep, prm->ureq[i].req); > + if (usb_ep_dequeue(ep, prm->ureq[i].req)) > + usb_ep_free_request(ep, prm->ureq[i].req); > + /* else will be freed in u_audio_iso_complete() */ > prm->ureq[i].req = NULL; > } > } I retested this version on edison. It resolves also another issue where dwc3 (in gadget mode) was periodically resetting. Thanks!
On Thu 29 Oct 2020 at 18:59, Jack Pham <jackp@codeaurora.org> wrote: > As per the kernel doc for usb_ep_dequeue(), it states that "this > routine is asynchronous, that is, it may return before the completion > routine runs". And indeed since v5.0 the dwc3 gadget driver updated > its behavior to place dequeued requests on to a cancelled list to be > given back later after the endpoint is stopped. > > The free_ep() was incorrectly assuming that a request was ready to > be freed after calling dequeue which results in a use-after-free > in dwc3 when it traverses its cancelled list. Fix this by moving > the usb_ep_free_request() call to the callback itself in case the > ep is disabled. > > Fixes: eb9fecb9e69b0 ("usb: gadget: f_uac2: split out audio core") > Reported-and-tested-by: Ferry Toth <fntoth@gmail.com> > Reviewed-and-tested-by: Peter Chen <peter.chen@nxp.com> Tested-by: Jerome Brunet <jbrunet@baylibre.com> > Signed-off-by: Jack Pham <jackp@codeaurora.org> > --- > v3: Fixed incorrect 'req' parameter and added Peter's tag > > v2: call free_request() in case of ep_dequeue() failure > > drivers/usb/gadget/function/u_audio.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c > index e6d32c536781..6e69ccf02c95 100644 > --- a/drivers/usb/gadget/function/u_audio.c > +++ b/drivers/usb/gadget/function/u_audio.c > @@ -89,7 +89,12 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req) > struct snd_uac_chip *uac = prm->uac; > > /* i/f shutting down */ > - if (!prm->ep_enabled || req->status == -ESHUTDOWN) > + if (!prm->ep_enabled) { > + usb_ep_free_request(ep, req); > + return; > + } > + > + if (req->status == -ESHUTDOWN) > return; > > /* > @@ -336,8 +341,9 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) > > for (i = 0; i < params->req_number; i++) { > if (prm->ureq[i].req) { > - usb_ep_dequeue(ep, prm->ureq[i].req); > - usb_ep_free_request(ep, prm->ureq[i].req); > + if (usb_ep_dequeue(ep, prm->ureq[i].req)) > + usb_ep_free_request(ep, prm->ureq[i].req); > + /* else will be freed in u_audio_iso_complete() */ > prm->ureq[i].req = NULL; > } > }
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c index e6d32c536781..6e69ccf02c95 100644 --- a/drivers/usb/gadget/function/u_audio.c +++ b/drivers/usb/gadget/function/u_audio.c @@ -89,7 +89,12 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req) struct snd_uac_chip *uac = prm->uac; /* i/f shutting down */ - if (!prm->ep_enabled || req->status == -ESHUTDOWN) + if (!prm->ep_enabled) { + usb_ep_free_request(ep, req); + return; + } + + if (req->status == -ESHUTDOWN) return; /* @@ -336,8 +341,9 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) for (i = 0; i < params->req_number; i++) { if (prm->ureq[i].req) { - usb_ep_dequeue(ep, prm->ureq[i].req); - usb_ep_free_request(ep, prm->ureq[i].req); + if (usb_ep_dequeue(ep, prm->ureq[i].req)) + usb_ep_free_request(ep, prm->ureq[i].req); + /* else will be freed in u_audio_iso_complete() */ prm->ureq[i].req = NULL; } }
As per the kernel doc for usb_ep_dequeue(), it states that "this routine is asynchronous, that is, it may return before the completion routine runs". And indeed since v5.0 the dwc3 gadget driver updated its behavior to place dequeued requests on to a cancelled list to be given back later after the endpoint is stopped. The free_ep() was incorrectly assuming that a request was ready to be freed after calling dequeue which results in a use-after-free in dwc3 when it traverses its cancelled list. Fix this by moving the usb_ep_free_request() call to the callback itself in case the ep is disabled. Fixes: eb9fecb9e69b0 ("usb: gadget: f_uac2: split out audio core") Reported-and-tested-by: Ferry Toth <fntoth@gmail.com> Reviewed-and-tested-by: Peter Chen <peter.chen@nxp.com> Signed-off-by: Jack Pham <jackp@codeaurora.org> --- v3: Fixed incorrect 'req' parameter and added Peter's tag v2: call free_request() in case of ep_dequeue() failure drivers/usb/gadget/function/u_audio.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)