diff mbox series

[v2,1/5] usb: gadget: u_audio: Free requests only after callback

Message ID 20210106133652.512178-2-jbrunet@baylibre.com (mailing list archive)
State Superseded
Headers show
Series usb: gadget: audio fixes and clean ups | expand

Commit Message

Jerome Brunet Jan. 6, 2021, 1:36 p.m. UTC 
From: Jack Pham <jackp@codeaurora.org>

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>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/usb/gadget/function/u_audio.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Felipe Balbi Jan. 7, 2021, 9:22 a.m. UTC | #1 
Jerome Brunet <jbrunet@baylibre.com> writes:

> From: Jack Pham <jackp@codeaurora.org>
>
> 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>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

Looks good to me, just one comment below:

> @@ -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);

do you mind adding a comment here stating that this is coping with a
possible error during usb_ep_dequeue()?

Other than that:

Acked-by: Felipe Balbi <balbi@kernel.org>
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index e6d32c536781..71dd9f16c246 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;
 		}
 	}