linux-kernel - Re: [PATCH v2 1/5] usb: gadget: u_audio: Free requests only after callback

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]

Message-ID: <87y2h5ayol.fsf@kernel.org>
Date:   Thu, 07 Jan 2021 11:22:50 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Jerome Brunet <jbrunet@...libre.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Jack Pham <jackp@...eaurora.org>,
        Ruslan Bilovol <ruslan.bilovol@...il.com>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ferry Toth <fntoth@...il.com>, Peter Chen <peter.chen@....com>,
        Jerome Brunet <jbrunet@...libre.com>
Subject: Re: [PATCH v2 1/5] usb: gadget: u_audio: Free requests only after
 callback

Jerome Brunet <jbrunet@...libre.com> writes:

> From: Jack Pham <jackp@...eaurora.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@...il.com>
> Reviewed-and-tested-by: Peter Chen <peter.chen@....com>
> Signed-off-by: Jack Pham <jackp@...eaurora.org>
> Signed-off-by: Jerome Brunet <jbrunet@...libre.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@...nel.org>

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (858 bytes)