* [PATCH v2] usb: gadget: audio: Free requests only after callback
@ 2020-10-29 6:21 Jack Pham
2020-10-29 8:24 ` Peter Chen
0 siblings, 1 reply; 3+ messages in thread
From: Jack Pham @ 2020-10-29 6:21 UTC (permalink / raw)
To: Felipe Balbi, Ruslan Bilovol
Cc: Greg Kroah-Hartman, Thinh Nguyen, Andy Shevchenko, linux-usb,
Peter Chen, Jack Pham, Ferry Toth
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>
Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
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, req);
+ /* else will be freed in u_audio_iso_complete() */
prm->ureq[i].req = NULL;
}
}
--
2.24.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] usb: gadget: audio: Free requests only after callback
2020-10-29 6:21 [PATCH v2] usb: gadget: audio: Free requests only after callback Jack Pham
@ 2020-10-29 8:24 ` Peter Chen
2020-10-29 16:01 ` Jack Pham
0 siblings, 1 reply; 3+ messages in thread
From: Peter Chen @ 2020-10-29 8:24 UTC (permalink / raw)
To: Jack Pham
Cc: Felipe Balbi, Ruslan Bilovol, Greg Kroah-Hartman, Thinh Nguyen,
Andy Shevchenko, linux-usb@vger.kernel.org, Ferry Toth
On 20-10-28 23:21:44, Jack Pham 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>
> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> ---
> 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, req);
There is a build error for "req", otherwise:
Reviewed-and-tested-by: Peter Chen <peter.chen@nxp.com>
> + /* else will be freed in u_audio_iso_complete() */
> prm->ureq[i].req = NULL;
> }
> }
> --
> 2.24.0
>
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] usb: gadget: audio: Free requests only after callback
2020-10-29 8:24 ` Peter Chen
@ 2020-10-29 16:01 ` Jack Pham
0 siblings, 0 replies; 3+ messages in thread
From: Jack Pham @ 2020-10-29 16:01 UTC (permalink / raw)
To: Peter Chen
Cc: Felipe Balbi, Ruslan Bilovol, Greg Kroah-Hartman, Thinh Nguyen,
Andy Shevchenko, linux-usb@vger.kernel.org, Ferry Toth
On Thu, Oct 29, 2020 at 08:24:52AM +0000, Peter Chen wrote:
> On 20-10-28 23:21:44, Jack Pham 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>
> > Signed-off-by: Jack Pham <jackp@codeaurora.org>
> > ---
> > 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, req);
>
> There is a build error for "req", otherwise:
>
> Reviewed-and-tested-by: Peter Chen <peter.chen@nxp.com>
Sorry, careless mistake!! Thanks for testing, V3 on its way.
Jack
> > + /* else will be freed in u_audio_iso_complete() */
> > prm->ureq[i].req = NULL;
> > }
> > }
> > --
> > 2.24.0
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-29 16:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-29 6:21 [PATCH v2] usb: gadget: audio: Free requests only after callback Jack Pham
2020-10-29 8:24 ` Peter Chen
2020-10-29 16:01 ` Jack Pham
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).