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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Aug 2021 23:59:43 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Jerome Brunet <jbrunet@...libre.com>,
        Felipe Balbi <balbi@...nel.org>,
        Ruslan Bilovol <ruslan.bilovol@...il.com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jack Pham <jackp@...eaurora.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Subject: Re: [PATCH] usb: gadget: u_audio: fix race condition on endpoint stop

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@...opsys.com>
> Signed-off-by: Jerome Brunet <jbrunet@...libre.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__);
>  }
>