usb: gadget: audio: Free requests only after callback
PATCH 1/1 usb: gadget: audio: Free requests only after callback
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.
--- drivers/usb/gadget/function/u_audio.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/usb/gadget/function/u_audio.c +6 -2
@@ -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 ;/*
@@ -337,7 +342,6 @@ 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); prm->ureq[i].req =NULL ; } }
Signature · 1 line
2.24.0
Quoted 38 lines · 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 theusb_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 <[email protected]>Signed-off-by Jack Pham <[email protected]>--- drivers/usb/gadget/function/u_audio.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/usb/gadget/function/u_audio.c +6
-1@@ -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 ;/*
drivers/usb/gadget/function/u_audio.c static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) · line 340
Quoted 4 lines
drivers/usb/gadget/function/u_audio.c +0
-1@@ -337,7 +342,6 @@ 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);
Then, for normal base, eg, there is no pending request before calling free_ep, then, where these uac request will be freed?
Signature · 2 lines
Thanks, Peter Chen
Hi Peter,
Quoted 37 lines · Peter Chen
Quoted 36 lines · 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 theusb_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 <[email protected]>Signed-off-by Jack Pham <[email protected]>--- drivers/usb/gadget/function/u_audio.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/usb/gadget/function/u_audio.c +6
-1@@ -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 ;
Quoted 9 lines
drivers/usb/gadget/function/u_audio.c static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep) · line 340
Quoted 6 lines
drivers/usb/gadget/function/u_audio.c +0
-1/* @@ -337,7 +342,6 @@ 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);Then, for normal base, eg, there is no pending request before calling
free_ep, then, where these uac request will be freed?
We can see in this driver that prm->ureq[i].req != NULL will only be true from either u_audio_start_capture() or u_audio_start_playback() where after allocating the request it is immediately queued. So generally the lifetime of the request is tied to its queued/pending status.
But you bring up a good point, it's possible usb_ep_queue() could have failed so we have to take care of the potential memory leak since the completion wouldn't get called.
We should do free_request() either when ep_queue() fails or when ep_dequeue() returns error. Do you have any preference? I'm leaning toward catching it here in free_ep() as it's only one call site to take care of instead of two ;-)
Thanks,
Jack
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project