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

[PATCH v3] 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 nothing
disables the endpoint or drains the work on teardown, so:

 - 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.

Disable the OUT endpoint in f_audio_disable(): the UDC then gives back the
outstanding requests, so f_audio_complete() stops touching @audio and no
new playback_work is queued. f_audio_disable() runs in atomic context
(reset_config() holds cdev->lock), and usb_ep_disable() is safe there.

Drain the work from f_audio_free() instead, where sleeping is allowed:
cancel_work_sync(&audio->playback_work) before gaudio_cleanup() tears down
audio->card (which f_audio_playback_work() -> u_audio_playback()
dereferences) and before kfree(audio). The endpoint is already disabled by
the time f_audio_disable() has run, so the work cannot be re-queued and
cancel_work_sync() is sufficient.

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: c6994e6f067c ("USB: gadget: add USB Audio Gadget driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
v3: Fix the v2 regressions found in review:
    - Do NOT cancel_work_sync() in f_audio_disable(): that callback runs in
      atomic context (reset_config() under cdev->lock), so the sleeping
      cancel would be a sleep-in-atomic. Only usb_ep_disable() (atomic-safe)
      stays in f_audio_disable(); the work is drained from f_audio_free().
    - Drop the play_queue/copy_buf drain added in v2: audio->copy_buf can
      alias a buffer already on play_queue (f_audio_set_alt altset 0 queues
      it without clearing the pointer) and can hold ERR_PTR(-ENOMEM), so
      freeing it there double-freed / dereferenced an error pointer. That
      buffer-leak + copy_buf aliasing is a separate pre-existing issue and
      is left for a dedicated fix.
    - Correct the Fixes: tag to the original driver commit c6994e6f067c.

v2: https://lore.kernel.org/linux-usb/20261108122645.421721-1-hj351016@gmail.com/
 drivers/usb/gadget/function/f_uac1_legacy.c | 21 ++++++++++++++++++++-
 1 file changed, 20 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..c07f323 100644
--- a/drivers/usb/gadget/function/f_uac1_legacy.c
+++ b/drivers/usb/gadget/function/f_uac1_legacy.c
@@ -697,7 +697,17 @@ 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);
+
+	/*
+	 * Disable the OUT endpoint: the UDC gives back any outstanding
+	 * usb_requests, so f_audio_complete() stops dereferencing @audio and
+	 * no further playback_work is queued. This callback runs in atomic
+	 * context (reset_config() holds cdev->lock), so it must not sleep;
+	 * the work is drained from f_audio_free() instead.
+	 */
+	if (audio->out_ep)
+		usb_ep_disable(audio->out_ep);
 }
 
 /*-------------------------------------------------------------------------*/
@@ -943,6 +953,15 @@ static void f_audio_free(struct usb_function *f)
 	struct f_audio *audio = func_to_audio(f);
 	struct f_uac1_legacy_opts *opts;
 
+	/*
+	 * The OUT endpoint was disabled in f_audio_disable(), so no new
+	 * playback_work can be queued. Drain any still-pending work here, in
+	 * sleepable context and before gaudio_cleanup() tears down the card
+	 * that f_audio_playback_work() -> u_audio_playback() dereferences,
+	 * otherwise the work runs against freed memory.
+	 */
+	cancel_work_sync(&audio->playback_work);
+
 	gaudio_cleanup(&audio->card);
 	opts = container_of(f->fi, struct f_uac1_legacy_opts, func_inst);
 	kfree(audio);
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: gadget: f_uac1_legacy: playback_work not drained in
  f_audio_unbind()
--

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

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

This commit disables the OUT endpoint in f_audio_disable() and cancels
audio->playback_work in f_audio_free() before tearing down the audio card
and freeing f_audio.

> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index 7c8d60c7e87d1..c07f323bcf9b0 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c

[ ... ]

> @@ -943,6 +953,15 @@ static void f_audio_free(struct usb_function *f)
>  	struct f_audio *audio = func_to_audio(f);
>  	struct f_uac1_legacy_opts *opts;
>  
> +	/*
> +	 * The OUT endpoint was disabled in f_audio_disable(), so no new
> +	 * playback_work can be queued. Drain any still-pending work here, in
> +	 * sleepable context and before gaudio_cleanup() tears down the card
> +	 * that f_audio_playback_work() -> u_audio_playback() dereferences,
> +	 * otherwise the work runs against freed memory.
> +	 */
> +	cancel_work_sync(&audio->playback_work);
> +
>  	gaudio_cleanup(&audio->card);
>  	opts = container_of(f->fi, struct f_uac1_legacy_opts, func_inst);
>  	kfree(audio);

[Severity: Medium]
Should audio->playback_work also be drained in f_audio_unbind()?

When a ConfigFS gadget using uac1_legacy is unbound from its UDC or the UDC
is unregistered via usb_del_gadget_udc(), purge_configs_funcs() calls
f_audio_unbind() without calling f_audio_free(), which only runs when the
function symlink is removed.  A pending f_audio_playback_work() ->
u_audio_playback() can therefore keep running after gadget_unbind_driver()
returns, and if it hits an ALSA error and logs via ERROR(card, ...), it
dereferences audio->card.gadget->dev on a detached or freed usb_gadget
(use-after-free).

CPU0                                  kworker
usb_del_gadget_udc()
  gadget_unbind_driver()
    f_audio_disable()
    configfs_composite_unbind()
      purge_configs_funcs()
        f_audio_unbind()
  device_unregister(&gadget->dev)
                                      f_audio_playback_work()
                                        u_audio_playback()
                                          ERROR(card, ...)
                                            &card->gadget->dev
                                            <-- use-after-free

Calling cancel_work_sync(&audio->playback_work) in f_audio_unbind() as well
(which also runs in sleepable process context) would ensure the work is
drained across UDC unbind.

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