* Re: [PATCH RESEND] usb: gadget: f_midi: fix UAF race during IN request handling
2026-10-19 13:57 [PATCH RESEND] usb: gadget: f_midi: fix UAF race during IN request handling Sonali Pradhan
@ 2026-10-19 14:33 ` sashiko-bot
2026-10-20 9:06 ` Takashi Iwai
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-10-19 14:33 UTC (permalink / raw)
To: Sonali Pradhan; +Cc: linux-usb
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] usb: gadget: f_midi: UAF on req in f_midi_complete() before transmit_lock
- [Critical] usb: gadget: f_midi: race between usb_ep_disable() and f_midi_transmit()
- [High] usb: gadget: f_midi: recursive transmit_lock deadlock in f_midi_complete()
- [High] usb: gadget: f_midi: race on in_req_fifo between f_midi_set_alt() and f_midi_transmit()
--
commit f0f632d32e9280f253a7d4422bf053e57b768343
Author: Sonali Pradhan <sonalipradhan@google.com>
usb: gadget: f_midi: fix UAF race during IN request handling
This patch acquires midi->transmit_lock in f_midi_disable() when freeing
pre-allocated IN requests and adds checks for ep->enabled under
midi->transmit_lock in f_midi_transmit() and f_midi_complete().
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 63fb6ee70a3d6..c26d04eb92090 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -280,9 +280,18 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
> /* We received stuff. req is queued again, below */
> f_midi_handle_out_data(ep, req);
> } else if (ep == midi->in_ep) {
> + unsigned long flags;
> +
> /* Our transmit completed. See if there's more to go.
> * f_midi_transmit eats req, don't queue it again. */
> + spin_lock_irqsave(&midi->transmit_lock, flags);
> + if (!ep->enabled) {
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> + return;
> + }
> req->length = 0;
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
[Severity: High]
Can acquiring midi->transmit_lock here in f_midi_complete() cause a recursive
spinlock deadlock?
In f_midi_transmit(), midi->transmit_lock is held while calling
f_midi_do_transmit(), which calls usb_ep_queue(ep, req, GFP_ATOMIC):
f_midi_transmit()
spin_lock_irqsave(&midi->transmit_lock, flags);
f_midi_do_transmit()
usb_ep_queue(ep, req, GFP_ATOMIC)
Several UDC drivers (such as dummy_hcd in dummy_queue(), goku_udc, omap_udc,
pxa25x_udc, at91_udc, and net2280) can complete an IN request synchronously
inside ep->ops->queue() via usb_gadget_giveback_request() ->
f_midi_complete() with req->status == 0 before usb_ep_queue() returns.
Commit 4ab37fcb4283 ("USB: gadget: f_midi: f_midi_complete to call
queue_work") previously replaced the direct f_midi_transmit() call in
f_midi_complete() with queue_work() because re-entering f_midi_transmit()
attempted to acquire midi->transmit_lock a second time on the same CPU. Does
taking midi->transmit_lock directly in f_midi_complete() re-introduce that
same deadlock?
[Severity: Critical]
Also, if f_midi_disable() runs concurrently with an in-flight
f_midi_complete(ep, req) callback and frees req via free_ep_req(), isn't req
already dereferenced before midi->transmit_lock is acquired here?
At the start of f_midi_complete():
static void
f_midi_complete(struct usb_ep *ep, struct usb_request *req)
{
struct f_midi *midi = ep->driver_data;
struct usb_composite_dev *cdev = midi->func.config->cdev;
int status = req->status;
usb_ep_disable() does not wait for completion callbacks that have already been
dequeued by the UDC and are currently executing to return. If f_midi_disable()
frees the struct usb_request *req via free_ep_req() while f_midi_complete() is
entering, reading req->status before taking midi->transmit_lock will access
freed memory.
Similarly, if status is non-zero on midi->in_ep, f_midi_complete() never takes
midi->transmit_lock or checks ep->enabled, reading req->actual and req->length
in VDBG()/DBG() and falling through to usb_ep_queue(ep, req, GFP_ATOMIC) on
non-shutdown errors. And if f_midi_disable() is immediately followed by
f_midi_set_alt() (such as during set_config()), ep->enabled is set back to
true while req has already been freed.
> +
> queue_work(system_highpri_wq, &midi->work);
> return;
> }
> @@ -420,6 +429,7 @@ static void f_midi_disable(struct usb_function *f)
> struct f_midi *midi = func_to_midi(f);
> struct usb_composite_dev *cdev = f->config->cdev;
> struct usb_request *req = NULL;
> + unsigned long flags;
>
> DBG(cdev, "disable\n");
>
> @@ -431,8 +441,10 @@ static void f_midi_disable(struct usb_function *f)
> usb_ep_disable(midi->out_ep);
>
> /* release IN requests */
> + spin_lock_irqsave(&midi->transmit_lock, flags);
> while (kfifo_get(&midi->in_req_fifo, &req))
> free_ep_req(midi->in_ep, req);
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
>
> f_midi_drop_out_substreams(midi);
> }
> @@ -678,10 +690,11 @@ static void f_midi_transmit(struct f_midi *midi)
> unsigned long flags;
>
> /* We only care about USB requests if IN endpoint is enabled */
> - if (!ep || !ep->enabled)
> - goto drop_out;
> -
> spin_lock_irqsave(&midi->transmit_lock, flags);
> + if (!ep || !ep->enabled) {
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> + goto drop_out;
> + }
>
> do {
> ret = f_midi_do_transmit(midi, ep);
[Severity: Critical]
Does checking ep->enabled under midi->transmit_lock prevent f_midi_transmit()
from racing with usb_ep_disable(midi->in_ep) in f_midi_disable()?
In f_midi_disable(), usb_ep_disable(midi->in_ep) is called outside
midi->transmit_lock, and in drivers/usb/gadget/udc/core.c:usb_ep_disable():
ret = ep->ops->disable(ep);
if (ret)
goto out;
ep->enabled = false;
ep->enabled remains true while ep->ops->disable(ep) flushes pending requests
and is only set to false afterwards without holding midi->transmit_lock.
If f_midi_transmit() acquires midi->transmit_lock on another CPU while
usb_ep_disable(midi->in_ep) is in progress, it can still see ep->enabled ==
true and queue a request via f_midi_do_transmit() -> usb_ep_queue() after
ep->ops->disable() has finished flushing the endpoint, placing req back into
midi->in_req_fifo. Once f_midi_transmit() drops midi->transmit_lock,
f_midi_disable() will pop req from midi->in_req_fifo and call free_ep_req()
while req is still queued in the UDC.
Additionally, if ep->enabled becomes false while f_midi_transmit() is already
iterating inside the do-while loop, usb_ep_queue() in f_midi_do_transmit()
returns -ESHUTDOWN, but f_midi_do_transmit() resets req->length = 0 and
returns active (1) instead of a negative error, causing the loop to spin
draining the substream and logging an error on every iteration.
[Severity: High]
How is midi->in_req_fifo synchronized between f_midi_set_alt() and
f_midi_transmit()?
In f_midi_set_alt(), f_midi_start_ep(midi, f, midi->in_ep) enables midi->in_ep
(setting ep->enabled = true) before midi->in_req_fifo is populated, and the
subsequent allocation loop calls kfifo_avail() and kfifo_put() without holding
midi->transmit_lock:
err = f_midi_start_ep(midi, f, midi->in_ep);
if (err)
return err;
...
while (kfifo_avail(&midi->in_req_fifo)) {
struct usb_request *req =
midi_alloc_ep_req(midi->in_ep, midi->buflen);
...
kfifo_put(&midi->in_req_fifo, req);
}
Because the ALSA rawmidi card is registered in f_midi_bind(), userspace can
trigger f_midi_in_trigger() -> f_midi_transmit() concurrently with
f_midi_set_alt(). Once f_midi_start_ep() sets midi->in_ep->enabled = true,
f_midi_transmit() will pass the !ep->enabled check under midi->transmit_lock
and call f_midi_do_transmit(), which calls kfifo_peek(), kfifo_skip(), and
kfifo_put(&midi->in_req_fifo, req) concurrently with the unlocked kfifo_put()
in f_midi_set_alt().
Can those concurrent kfifo_put() calls corrupt midi->in_req_fifo (or cause
kfifo_peek() to fail on an empty FIFO before the first request is added,
dropping active substreams)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135701.2767078-1-sonalipradhan@google.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH RESEND] usb: gadget: f_midi: fix UAF race during IN request handling
2026-10-19 13:57 [PATCH RESEND] usb: gadget: f_midi: fix UAF race during IN request handling Sonali Pradhan
2026-10-19 14:33 ` sashiko-bot
@ 2026-10-20 9:06 ` Takashi Iwai
1 sibling, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2026-10-20 9:06 UTC (permalink / raw)
To: Sonali Pradhan
Cc: Greg Kroah-Hartman, Neill Kapron, Takashi Iwai,
Christophe JAILLET, Fan Wu, Victor Krawiec, Felipe Balbi,
Felipe F. Tonello, linux-usb, linux-kernel, stable
On Mon, 19 Oct 2026 15:57:01 +0200,
Sonali Pradhan wrote:
>
> f_midi_disable() frees pre-allocated IN requests from in_req_fifo
> without holding midi->transmit_lock. Concurrent f_midi_transmit() or
> f_midi_complete() calls can then access these request buffers after they
> are freed, leading to a use-after-free.
>
> Fix this by acquiring midi->transmit_lock in f_midi_disable() while
> releasing IN requests, and checking ep->enabled under midi->transmit_lock
> in f_midi_transmit() and f_midi_complete() before modifying requests.
>
> Fixes: e1e3d7ec5da3 ("usb: gadget: f_midi: pre-allocate IN requests")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sonali Pradhan <sonalipradhan@google.com>
> ---
> Resending and rebasing on latest linux-next as I haven't received
> feedback on my initial submission:
> https://lore.kernel.org/all/20260819102709.212646-1-sonalipradhan@google.com/
>
> drivers/usb/gadget/function/f_midi.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 63fb6ee70a3d..c26d04eb9209 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -280,9 +280,18 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
> /* We received stuff. req is queued again, below */
> f_midi_handle_out_data(ep, req);
> } else if (ep == midi->in_ep) {
> + unsigned long flags;
> +
> /* Our transmit completed. See if there's more to go.
> * f_midi_transmit eats req, don't queue it again. */
> + spin_lock_irqsave(&midi->transmit_lock, flags);
> + if (!ep->enabled) {
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> + return;
> + }
> req->length = 0;
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> +
Does this transmit_lock help here? As far as I see, the ep
enable/disable itself is done outside the lock.
> @@ -678,10 +690,11 @@ static void f_midi_transmit(struct f_midi *midi)
> unsigned long flags;
>
> /* We only care about USB requests if IN endpoint is enabled */
> - if (!ep || !ep->enabled)
> - goto drop_out;
> -
> spin_lock_irqsave(&midi->transmit_lock, flags);
> + if (!ep || !ep->enabled) {
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> + goto drop_out;
> + }
Ditto.
thanks,
Takashi
^ permalink raw reply [flat|nested] 3+ messages in thread