[PATCH] usb: gadget: u_audio: fix race condition on endpoint stop

[PATCH] usb: gadget: u_audio: fix race condition on endpoint stop

[Jerome Brunet]

If the endpoint completion callback is call right after the ep_enabled flag
is cleared and before usb_ep_dequeue() is call, we could do a double free
on the request and the associated buffer.

Fix this by clearing ep_enabled after all the endpoint requests have been
dequeued.

Fixes: 7de8681be2cd ("usb: gadget: u_audio: Free requests only after callback")
Reported-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/usb/gadget/function/u_audio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 63d9340f008e..9e5c950612d0 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -394,8 +394,6 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
 	if (!prm->ep_enabled)
 		return;
 
-	prm->ep_enabled = false;
-
 	audio_dev = uac->audio_dev;
 	params = &audio_dev->params;
 
@@ -413,6 +411,8 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
 		}
 	}
 
+	prm->ep_enabled = false;
+
 	if (usb_ep_disable(ep))
 		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
 }
@@ -424,8 +424,6 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
 	if (!prm->fb_ep_enabled)
 		return;
 
-	prm->fb_ep_enabled = false;
-
 	if (prm->req_fback) {
 		if (usb_ep_dequeue(ep, prm->req_fback)) {
 			kfree(prm->req_fback->buf);
@@ -434,6 +432,8 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
 		prm->req_fback = NULL;
 	}
 
+	prm->fb_ep_enabled = false;
+
 	if (usb_ep_disable(ep))
 		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
 }
-- 
2.33.0

Re: [PATCH] usb: gadget: u_audio: fix race condition on endpoint stop

[Thinh Nguyen]

Jerome Brunet wrote:
> If the endpoint completion callback is call right after the ep_enabled flag
> is cleared and before usb_ep_dequeue() is call, we could do a double free
> on the request and the associated buffer.
> 
> Fix this by clearing ep_enabled after all the endpoint requests have been
> dequeued.
> 
> Fixes: 7de8681be2cd ("usb: gadget: u_audio: Free requests only after callback")
> Reported-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/usb/gadget/function/u_audio.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> index 63d9340f008e..9e5c950612d0 100644
> --- a/drivers/usb/gadget/function/u_audio.c
> +++ b/drivers/usb/gadget/function/u_audio.c
> @@ -394,8 +394,6 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
>  	if (!prm->ep_enabled)
>  		return;
>  
> -	prm->ep_enabled = false;
> -
>  	audio_dev = uac->audio_dev;
>  	params = &audio_dev->params;
>  
> @@ -413,6 +411,8 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
>  		}
>  	}
>  
> +	prm->ep_enabled = false;
> +

Hm... this looks a little odd. If the cancelled request completes before
prm->ep_enabled = false, then the audio driver will re-queue the
request. It will eventually be freed later when the endpoint is disabled
and when the controller driver completes and gives back any outstanding
request. But is this what you intended? If it is, why even usb_ep_dequeue()?

Also, another concern I have is I don't see any lock or memory barrier
when writing/reading prm->ep_enabled. Are we always reading
prm->ep_enabled in the right order as we expected?

Would it be simpler to free the request base on the request completion
status as suggested?

BR,
Thinh

>  	if (usb_ep_disable(ep))
>  		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
>  }
> @@ -424,8 +424,6 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
>  	if (!prm->fb_ep_enabled)
>  		return;
>  
> -	prm->fb_ep_enabled = false;
> -
>  	if (prm->req_fback) {
>  		if (usb_ep_dequeue(ep, prm->req_fback)) {
>  			kfree(prm->req_fback->buf);
> @@ -434,6 +432,8 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
>  		prm->req_fback = NULL;
>  	}
>  
> +	prm->fb_ep_enabled = false;
> +
>  	if (usb_ep_disable(ep))
>  		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
>  }
> 

Re: [PATCH] usb: gadget: u_audio: fix race condition on endpoint stop

[Jerome Brunet]

On Fri 27 Aug 2021 at 23:59, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:

> Jerome Brunet wrote:
>> If the endpoint completion callback is call right after the ep_enabled flag
>> is cleared and before usb_ep_dequeue() is call, we could do a double free
>> on the request and the associated buffer.
>> 
>> Fix this by clearing ep_enabled after all the endpoint requests have been
>> dequeued.
>> 
>> Fixes: 7de8681be2cd ("usb: gadget: u_audio: Free requests only after callback")
>> Reported-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/usb/gadget/function/u_audio.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
>> index 63d9340f008e..9e5c950612d0 100644
>> --- a/drivers/usb/gadget/function/u_audio.c
>> +++ b/drivers/usb/gadget/function/u_audio.c
>> @@ -394,8 +394,6 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
>>  	if (!prm->ep_enabled)
>>  		return;
>>  
>> -	prm->ep_enabled = false;
>> -
>>  	audio_dev = uac->audio_dev;
>>  	params = &audio_dev->params;
>>  
>> @@ -413,6 +411,8 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
>>  		}
>>  	}
>>  
>> +	prm->ep_enabled = false;
>> +
>
> Hm... this looks a little odd. If the cancelled request completes before
> prm->ep_enabled = false, then the audio driver will re-queue the
> request. It will eventually be freed later when the endpoint is disabled
> and when the controller driver completes and gives back any outstanding
> request. But is this what you intended? If it is, why even usb_ep_dequeue()?
>

Yes, it is what I intended. It's a quick way to address the problem you
have reported.

> Also, another concern I have is I don't see any lock or memory barrier
> when writing/reading prm->ep_enabled. Are we always reading
> prm->ep_enabled in the right order as we expected?
>
> Would it be simpler to free the request base on the request completion
> status as suggested?

I can see that it would be better. It would use the framework the way it
was intended which is certainly better. I just can't do it right now.

>
> BR,
> Thinh
>
>>  	if (usb_ep_disable(ep))
>>  		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
>>  }
>> @@ -424,8 +424,6 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
>>  	if (!prm->fb_ep_enabled)
>>  		return;
>>  
>> -	prm->fb_ep_enabled = false;
>> -
>>  	if (prm->req_fback) {
>>  		if (usb_ep_dequeue(ep, prm->req_fback)) {
>>  			kfree(prm->req_fback->buf);
>> @@ -434,6 +432,8 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
>>  		prm->req_fback = NULL;
>>  	}
>>  
>> +	prm->fb_ep_enabled = false;
>> +
>>  	if (usb_ep_disable(ep))
>>  		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
>>  }
>>