Re: [PATCH] usb: gadget: audio: Free requests only after callback

[Jack Pham <jackp@codeaurora.org>]

Hi Peter,

On Wed, Oct 28, 2020 at 07:15:31AM +0000, Peter Chen wrote:
> On 20-10-27 16:31:38, 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>
> > ---
> >  drivers/usb/gadget/function/u_audio.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> > index e6d32c536781..a3b557fad1fd 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;
> >  
> >  	/*
> > @@ -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
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project