[PATCH v3] usb: gadget: f_uac1_legacy: cancel playback work and drain buffers on free

[PATCH v3] usb: gadget: f_uac1_legacy: cancel playback work and drain buffers on free

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

f_audio_free() tears down the f_audio structure without cancelling
playback_work or draining the play_queue.  The pending work can fire
after the structure is freed, and queued audio buffers leak.

Cancel the work, walk the play_queue to free every queued buffer, and
free copy_buf when it was not already queued.  When f_audio_set_alt()
is called with alt 0 it queues copy_buf to play_queue without clearing
audio->copy_buf, so the drain detects this and clears the pointer to
prevent a double free.  Use IS_ERR_OR_NULL for the final copy_buf check
because f_audio_buffer_alloc() returns ERR_PTR on failure and
f_audio_set_alt() stores the result directly in audio->copy_buf.

Fixes: c6994e6f067c ("USB: gadget: add USB Audio Gadget driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---

Changes in v3:
- Fix double-free of copy_buf: when f_audio_set_alt(alt=0) queues
  copy_buf to play_queue without clearing audio->copy_buf, the drain
  now detects this and clears the pointer before freeing
- Change copy_buf NULL check to IS_ERR_OR_NULL because
  f_audio_buffer_alloc() returns ERR_PTR(-ENOMEM) on failure

Changes in v2:
- Drain play_queue and free copy_buf in addition to cancelling work

 drivers/usb/gadget/function/f_uac1_legacy.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
index 7c8d60c7e87d1..88c816b4ba119 100644
--- a/drivers/usb/gadget/function/f_uac1_legacy.c
+++ b/drivers/usb/gadget/function/f_uac1_legacy.c
@@ -9,6 +9,7 @@
 #include <linux/slab.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/err.h>
 #include <linux/device.h>
 #include <linux/atomic.h>
 
@@ -942,7 +943,17 @@ static void f_audio_free(struct usb_function *f)
 {
 	struct f_audio *audio = func_to_audio(f);
 	struct f_uac1_legacy_opts *opts;
-
+	struct f_audio_buf *buf, *tmp;
+
+	cancel_work_sync(&audio->playback_work);
+	list_for_each_entry_safe(buf, tmp, &audio->play_queue, list) {
+		list_del(&buf->list);
+		if (buf == audio->copy_buf)
+			audio->copy_buf = NULL;
+		f_audio_buffer_free(buf);
+	}
+	if (!IS_ERR_OR_NULL(audio->copy_buf))
+		f_audio_buffer_free(audio->copy_buf);
 	gaudio_cleanup(&audio->card);
 	opts = container_of(f->fi, struct f_uac1_legacy_opts, func_inst);
 	kfree(audio);
-- 
2.47.3

Re: [PATCH v3] usb: gadget: f_uac1_legacy: cancel playback work and drain buffers on free

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] usb: gadget: f_uac1_legacy: double-free of copy_buf in f_audio_free()
--

commit fcd9ff7ea98907dc1b23d26f7dc74bbae03e0203
Author: Tristan Madani <tristan@talencesecurity.com>

usb: gadget: f_uac1_legacy: cancel playback work and drain buffers on free

This commit updates f_audio_free() to cancel pending playback work, drain and
free any buffers remaining on play_queue, and free audio->copy_buf during
gadget teardown.

> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index 7c8d60c7e87d1..88c816b4ba119 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c
[ ... ]
> @@ -942,7 +943,17 @@ static void f_audio_free(struct usb_function *f)
>  {
>  	struct f_audio *audio = func_to_audio(f);
>  	struct f_uac1_legacy_opts *opts;
> -
> +	struct f_audio_buf *buf, *tmp;
> +
> +	cancel_work_sync(&audio->playback_work);
> +	list_for_each_entry_safe(buf, tmp, &audio->play_queue, list) {
> +		list_del(&buf->list);
> +		if (buf == audio->copy_buf)
> +			audio->copy_buf = NULL;
> +		f_audio_buffer_free(buf);
> +	}
> +	if (!IS_ERR_OR_NULL(audio->copy_buf))
> +		f_audio_buffer_free(audio->copy_buf);

[Severity: Critical]
Can this still double-free audio->copy_buf if f_audio_playback_work()
already dequeued and freed it before f_audio_free() drains play_queue?

When f_audio_set_alt() is called with alt == 0 on the AudioStreaming
interface, it queues audio->copy_buf onto audio->play_queue and schedules
playback_work without clearing audio->copy_buf:

	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);
	}

A similar situation occurs in f_audio_out_ep_complete() if audio->copy_buf
is queued onto audio->play_queue and the subsequent f_audio_buffer_alloc()
fails and returns -ENOMEM before updating audio->copy_buf:

	if (audio_buf_size - copy_buf->actual < req->actual) {
		spin_lock_irq(&audio->lock);
		list_add_tail(&copy_buf->list, &audio->play_queue);
		spin_unlock_irq(&audio->lock);
		schedule_work(&audio->playback_work);
		copy_buf = f_audio_buffer_alloc(audio_buf_size);
		if (IS_ERR(copy_buf))
			return -ENOMEM;
	}

When f_audio_playback_work() runs (either before f_audio_free() is called,
or while cancel_work_sync(&audio->playback_work) waits for it to finish),
it removes the buffer from audio->play_queue and frees it without clearing
audio->copy_buf:

	play_buf = list_first_entry(&audio->play_queue,
			struct f_audio_buf, list);
	list_del(&play_buf->list);
	spin_unlock_irq(&audio->lock);

	u_audio_playback(&audio->card, play_buf->buf, play_buf->actual);
	f_audio_buffer_free(play_buf);

Because the buffer is no longer in audio->play_queue when f_audio_free()
walks the list, audio->copy_buf is never set to NULL, and the subsequent
f_audio_buffer_free(audio->copy_buf) call reads and frees the already-freed
buffer a second time.

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