[PATCH] usb: gadget: f_uac1_legacy: disable OUT ep and drain work on teardown

[PATCH] usb: gadget: f_uac1_legacy: disable OUT ep and drain work on teardown

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

f_uac1_legacy enables the OUT endpoint in f_audio_set_alt() and queues
usb_requests whose completion handler f_audio_complete() dereferences the
f_audio (req->context) and audio->out_ep, and which schedules
audio->playback_work. However f_audio_disable() is empty and neither
f_audio_unbind() nor f_audio_free() disables the endpoint or drains the
work, so on teardown:

 - f_audio_complete() can run after kfree(audio) (the UDC gives back
   pending requests on the still-enabled endpoint during unbind), using
   freed memory -- a use-after-free; and
 - a playback_work still pending runs f_audio_playback_work() against the
   freed f_audio -- a second use-after-free.

Implement f_audio_disable() to quiesce the function properly: disable the
OUT endpoint (which gives back outstanding requests, so f_audio_complete()
stops touching @audio and no new work is queued), then cancel_work_sync()
the playback work, then free the buffers left on audio->play_queue and
audio->copy_buf (f_audio_playback_work() is the only consumer, so a
cancelled work would otherwise leak them).

The endpoint is disabled before the work is cancelled, and the work is
cancelled before f_audio_free() runs gaudio_cleanup() on audio->card that
u_audio_playback() dereferences, so none of them race the teardown.

Found by static analysis and an automated reviewer; the playback_work
use-after-free was reproduced with KASAN under dummy_hcd (slab-use-after-
free in u_audio_playback() from f_audio_playback_work() after
f_audio_free(); a delay was added to the streaming path to make the
schedule-vs-free race deterministic). The out_ep/f_audio_complete
use-after-free is established by code inspection.

Fixes: f3a3406b3f56 ("usb: gadget: f_uac1: convert to new function interface with backward compatibility")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
v2: Completely reworked after review of v1 (which only cancel_work_sync'd
    playback_work in f_audio_free()). v1 did not address the more serious
    out_ep/f_audio_complete() use-after-free, cancelled after gaudio_cleanup()
    rather than before, and leaked audio->play_queue buffers. v2 implements
    f_audio_disable() to disable the endpoint, drain the work, and free the
    buffers, and corrects the Fixes: tag (v1 cited the file-rename commit).
    v1: https://lore.kernel.org/linux-usb/20261108075116.251501-1-hj351016@gmail.com/
 drivers/usb/gadget/function/f_uac1_legacy.c | 30 ++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
index 7c8d60c..0f937a7 100644
--- a/drivers/usb/gadget/function/f_uac1_legacy.c
+++ b/drivers/usb/gadget/function/f_uac1_legacy.c
@@ -697,7 +697,35 @@ static int f_audio_get_alt(struct usb_function *f, unsigned intf)
 
 static void f_audio_disable(struct usb_function *f)
 {
-	return;
+	struct f_audio *audio = func_to_audio(f);
+	struct f_audio_buf *buf;
+
+	/*
+	 * Disable the OUT endpoint first: the UDC gives back any outstanding
+	 * usb_requests, so f_audio_complete() stops dereferencing @audio and
+	 * no new playback_work is queued. Then drain the work and free the
+	 * buffers it would otherwise have consumed, before the function is
+	 * unbound and freed.
+	 */
+	if (audio->out_ep)
+		usb_ep_disable(audio->out_ep);
+
+	cancel_work_sync(&audio->playback_work);
+
+	/*
+	 * The endpoint is disabled and the work is drained, so nothing else
+	 * can touch the queue now; free the leftover buffers.
+	 */
+	while (!list_empty(&audio->play_queue)) {
+		buf = list_first_entry(&audio->play_queue,
+				       struct f_audio_buf, list);
+		list_del(&buf->list);
+		f_audio_buffer_free(buf);
+	}
+	if (audio->copy_buf) {
+		f_audio_buffer_free(audio->copy_buf);
+		audio->copy_buf = NULL;
+	}
 }
 
 /*-------------------------------------------------------------------------*/
-- 
2.43.0

Re: [PATCH] usb: gadget: f_uac1_legacy: disable OUT ep and drain work on teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] usb: gadget: f_uac1_legacy: sleep in atomic in
  f_audio_disable()
- [High] usb: gadget: f_uac1_legacy: double free and ERR_PTR deref in
  f_audio_disable()
- [Low] usb: gadget: f_uac1_legacy: inaccurate Fixes tag in commit
  message
--

commit 167ec12aa25bf4c3879c873681717d3fce869739
Author: Hyunjung Ko <hj351016@gmail.com>

> usb: gadget: f_uac1_legacy: disable OUT ep and drain work on teardown
>
> f_uac1_legacy enables the OUT endpoint in f_audio_set_alt() and queues
> usb_requests whose completion handler f_audio_complete() dereferences the
> f_audio (req->context) and audio->out_ep, and which schedules
> audio->playback_work. However f_audio_disable() is empty and neither
> f_audio_unbind() nor f_audio_free() disables the endpoint or drains the
> work, so on teardown:
>
>  - f_audio_complete() can run after kfree(audio) (the UDC gives back
>    pending requests on the still-enabled endpoint during unbind), using
>    freed memory -- a use-after-free; and
>  - a playback_work still pending runs f_audio_playback_work() against the
>    freed f_audio -- a second use-after-free.
>
> Implement f_audio_disable() to quiesce the function properly: disable the
> OUT endpoint (which gives back outstanding requests, so f_audio_complete()
> stops touching @audio and no new work is queued), then cancel_work_sync()
> the playback work, then free the buffers left on audio->play_queue and
> audio->copy_buf (f_audio_playback_work() is the only consumer, so a
> cancelled work would otherwise leak them).
>
> The endpoint is disabled before the work is cancelled, and the work is
> cancelled before f_audio_free() runs gaudio_cleanup() on audio->card that
> u_audio_playback() dereferences, so none of them race the teardown.
>
> Found by static analysis and an automated reviewer; the playback_work
> use-after-free was reproduced with KASAN under dummy_hcd (slab-use-after-
> free in u_audio_playback() from f_audio_playback_work() after
> f_audio_free(); a delay was added to the streaming path to make the
> schedule-vs-free race deterministic). The out_ep/f_audio_complete
> use-after-free is established by code inspection.
>
> Fixes: f3a3406b3f56 ("usb: gadget: f_uac1: convert to new function interface with backward compatibility")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>

[Severity: Low]
The empty f_audio_disable() stub and the teardown lifecycle without
disabling out_ep or draining playback_work date back to the initial driver
commit rather than f3a3406b3f56 ("usb: gadget: f_uac1: convert to new
function interface with backward compatibility").  Should the Fixes tag be:

  Fixes: c6994e6f067c ("USB: gadget: add USB Audio Gadget driver")

> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index 7c8d60c7e87d1..0f937a7bd33b0 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c
> @@ -697,7 +697,35 @@ static int f_audio_get_alt(struct usb_function *f, unsigned intf)
>  
>  static void f_audio_disable(struct usb_function *f)
>  {
> -	return;
> +	struct f_audio *audio = func_to_audio(f);
> +	struct f_audio_buf *buf;
> +
> +	/*
> +	 * Disable the OUT endpoint first: the UDC gives back any outstanding
> +	 * usb_requests, so f_audio_complete() stops dereferencing @audio and
> +	 * no new playback_work is queued. Then drain the work and free the
> +	 * buffers it would otherwise have consumed, before the function is
> +	 * unbound and freed.
> +	 */
> +	if (audio->out_ep)
> +		usb_ep_disable(audio->out_ep);
> +
> +	cancel_work_sync(&audio->playback_work);

[Severity: High]
Can cancel_work_sync(&audio->playback_work) be called from
f_audio_disable()?

The usb_function.disable callback is invoked by reset_config() while
holding spinlocks in atomic context, for example via
__composite_disconnect() under spin_lock_irqsave(&cdev->lock, flags) or
via composite_setup() -> set_config() -> reset_config() under
spin_lock(&cdev->lock) in UDC interrupt context.  Because cancel_work_sync()
calls might_sleep() and blocks waiting for f_audio_playback_work() to
finish, a USB disconnect, reset, or reconfiguration will sleep in atomic
context and can deadlock.

> +
> +	/*
> +	 * The endpoint is disabled and the work is drained, so nothing else
> +	 * can touch the queue now; free the leftover buffers.
> +	 */
> +	while (!list_empty(&audio->play_queue)) {
> +		buf = list_first_entry(&audio->play_queue,
> +				       struct f_audio_buf, list);
> +		list_del(&buf->list);
> +		f_audio_buffer_free(buf);
> +	}
> +	if (audio->copy_buf) {
> +		f_audio_buffer_free(audio->copy_buf);
> +		audio->copy_buf = NULL;
> +	}
>  }

[Severity: High]
Can freeing audio->copy_buf here cause a use-after-free and double free, or
dereference an error pointer?

When the host selects altsetting 0 in f_audio_set_alt(), audio->copy_buf is
queued onto audio->play_queue without clearing audio->copy_buf:

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_set_alt() {
    ...
		} else {
			struct f_audio_buf *copy_buf = audio->copy_buf;
			if (copy_buf) {
				list_add_tail(&copy_buf->list,
						&audio->play_queue);
				schedule_work(&audio->playback_work);
			}
		}
    ...
}

The same stale pointer is left behind if f_audio_buffer_alloc() fails in
f_audio_out_ep_complete() after adding copy_buf to audio->play_queue.  When
f_audio_disable() later runs, that buffer has already been freed by
f_audio_playback_work() or by the play_queue loop right above, so
f_audio_buffer_free(audio->copy_buf) reads audio_buf->buf from freed memory
and double-frees both allocations.

In addition, f_audio_buffer_alloc() returns ERR_PTR(-ENOMEM) on failure,
which f_audio_set_alt() stores directly in audio->copy_buf; checking
if (audio->copy_buf) treats ERR_PTR(-ENOMEM) as non-NULL and dereferences
it in f_audio_buffer_free().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108122645.421721-1-hj351016@gmail.com?part=1

Re: [PATCH] usb: gadget: f_uac1_legacy: disable OUT ep and drain work on teardown

[khj]

Thanks -- all three are correct.

You're right that f_audio_disable() runs in atomic context (reset_config()
under cdev->lock), so cancel_work_sync() there is a sleep-in-atomic bug. v3
keeps only usb_ep_disable() (which is safe in that context) in
f_audio_disable() to give back the outstanding requests, and moves
cancel_work_sync(&audio->playback_work) to f_audio_free(), where sleeping
is allowed, before gaudio_cleanup().

I've also dropped the play_queue/copy_buf drain: as you note, audio->copy_buf
can alias a buffer already on play_queue and can hold ERR_PTR(-ENOMEM), so
freeing it there was a double-free / error-pointer deref. That buffer leak
plus the copy_buf aliasing is a separate pre-existing issue I'll address on
its own rather than fold into this fix.

Fixes: tag corrected to c6994e6f067c.

2026년 11월 8일 (일) 오후 9:41, <sashiko-bot@kernel.org>님이 작성:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [High] usb: gadget: f_uac1_legacy: sleep in atomic in
>   f_audio_disable()
> - [High] usb: gadget: f_uac1_legacy: double free and ERR_PTR deref in
>   f_audio_disable()
> - [Low] usb: gadget: f_uac1_legacy: inaccurate Fixes tag in commit
>   message
> --
>
> commit 167ec12aa25bf4c3879c873681717d3fce869739
> Author: Hyunjung Ko <hj351016@gmail.com>
>
> > usb: gadget: f_uac1_legacy: disable OUT ep and drain work on teardown
> >
> > f_uac1_legacy enables the OUT endpoint in f_audio_set_alt() and queues
> > usb_requests whose completion handler f_audio_complete() dereferences the
> > f_audio (req->context) and audio->out_ep, and which schedules
> > audio->playback_work. However f_audio_disable() is empty and neither
> > f_audio_unbind() nor f_audio_free() disables the endpoint or drains the
> > work, so on teardown:
> >
> >  - f_audio_complete() can run after kfree(audio) (the UDC gives back
> >    pending requests on the still-enabled endpoint during unbind), using
> >    freed memory -- a use-after-free; and
> >  - a playback_work still pending runs f_audio_playback_work() against the
> >    freed f_audio -- a second use-after-free.
> >
> > Implement f_audio_disable() to quiesce the function properly: disable the
> > OUT endpoint (which gives back outstanding requests, so f_audio_complete()
> > stops touching @audio and no new work is queued), then cancel_work_sync()
> > the playback work, then free the buffers left on audio->play_queue and
> > audio->copy_buf (f_audio_playback_work() is the only consumer, so a
> > cancelled work would otherwise leak them).
> >
> > The endpoint is disabled before the work is cancelled, and the work is
> > cancelled before f_audio_free() runs gaudio_cleanup() on audio->card that
> > u_audio_playback() dereferences, so none of them race the teardown.
> >
> > Found by static analysis and an automated reviewer; the playback_work
> > use-after-free was reproduced with KASAN under dummy_hcd (slab-use-after-
> > free in u_audio_playback() from f_audio_playback_work() after
> > f_audio_free(); a delay was added to the streaming path to make the
> > schedule-vs-free race deterministic). The out_ep/f_audio_complete
> > use-after-free is established by code inspection.
> >
> > Fixes: f3a3406b3f56 ("usb: gadget: f_uac1: convert to new function interface with backward compatibility")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
>
> [Severity: Low]
> The empty f_audio_disable() stub and the teardown lifecycle without
> disabling out_ep or draining playback_work date back to the initial driver
> commit rather than f3a3406b3f56 ("usb: gadget: f_uac1: convert to new
> function interface with backward compatibility").  Should the Fixes tag be:
>
>   Fixes: c6994e6f067c ("USB: gadget: add USB Audio Gadget driver")
>
> > diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> > index 7c8d60c7e87d1..0f937a7bd33b0 100644
> > --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> > +++ b/drivers/usb/gadget/function/f_uac1_legacy.c
> > @@ -697,7 +697,35 @@ static int f_audio_get_alt(struct usb_function *f, unsigned intf)
> >
> >  static void f_audio_disable(struct usb_function *f)
> >  {
> > -     return;
> > +     struct f_audio *audio = func_to_audio(f);
> > +     struct f_audio_buf *buf;
> > +
> > +     /*
> > +      * Disable the OUT endpoint first: the UDC gives back any outstanding
> > +      * usb_requests, so f_audio_complete() stops dereferencing @audio and
> > +      * no new playback_work is queued. Then drain the work and free the
> > +      * buffers it would otherwise have consumed, before the function is
> > +      * unbound and freed.
> > +      */
> > +     if (audio->out_ep)
> > +             usb_ep_disable(audio->out_ep);
> > +
> > +     cancel_work_sync(&audio->playback_work);
>
> [Severity: High]
> Can cancel_work_sync(&audio->playback_work) be called from
> f_audio_disable()?
>
> The usb_function.disable callback is invoked by reset_config() while
> holding spinlocks in atomic context, for example via
> __composite_disconnect() under spin_lock_irqsave(&cdev->lock, flags) or
> via composite_setup() -> set_config() -> reset_config() under
> spin_lock(&cdev->lock) in UDC interrupt context.  Because cancel_work_sync()
> calls might_sleep() and blocks waiting for f_audio_playback_work() to
> finish, a USB disconnect, reset, or reconfiguration will sleep in atomic
> context and can deadlock.
>
> > +
> > +     /*
> > +      * The endpoint is disabled and the work is drained, so nothing else
> > +      * can touch the queue now; free the leftover buffers.
> > +      */
> > +     while (!list_empty(&audio->play_queue)) {
> > +             buf = list_first_entry(&audio->play_queue,
> > +                                    struct f_audio_buf, list);
> > +             list_del(&buf->list);
> > +             f_audio_buffer_free(buf);
> > +     }
> > +     if (audio->copy_buf) {
> > +             f_audio_buffer_free(audio->copy_buf);
> > +             audio->copy_buf = NULL;
> > +     }
> >  }
>
> [Severity: High]
> Can freeing audio->copy_buf here cause a use-after-free and double free, or
> dereference an error pointer?
>
> When the host selects altsetting 0 in f_audio_set_alt(), audio->copy_buf is
> queued onto audio->play_queue without clearing audio->copy_buf:
>
> drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_set_alt() {
>     ...
>                 } else {
>                         struct f_audio_buf *copy_buf = audio->copy_buf;
>                         if (copy_buf) {
>                                 list_add_tail(&copy_buf->list,
>                                                 &audio->play_queue);
>                                 schedule_work(&audio->playback_work);
>                         }
>                 }
>     ...
> }
>
> The same stale pointer is left behind if f_audio_buffer_alloc() fails in
> f_audio_out_ep_complete() after adding copy_buf to audio->play_queue.  When
> f_audio_disable() later runs, that buffer has already been freed by
> f_audio_playback_work() or by the play_queue loop right above, so
> f_audio_buffer_free(audio->copy_buf) reads audio_buf->buf from freed memory
> and double-frees both allocations.
>
> In addition, f_audio_buffer_alloc() returns ERR_PTR(-ENOMEM) on failure,
> which f_audio_set_alt() stores directly in audio->copy_buf; checking
> if (audio->copy_buf) treats ERR_PTR(-ENOMEM) as non-NULL and dereferences
> it in f_audio_buffer_free().
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261108122645.421721-1-hj351016@gmail.com?part=1