[PATCH v2] usb: gadget: f_uac1_legacy: cancel playback_work and drain buffers on free

[PATCH v2] usb: gadget: f_uac1_legacy: cancel playback_work and drain buffers on free

[Tristan Madani]

From: Tristan Madani <tristan@talencesecurity.com>

f_audio_free() calls kfree(audio) without cancelling playback_work or
freeing the queued audio buffers.  If a playback work item is pending
or running when the function is freed, the work handler references
the already-freed audio structure via container_of.  Additionally, any
f_audio_buf entries remaining on play_queue and the partially filled
copy_buf are not freed.

Cancel the pending work, drain all queued buffers from play_queue, and
free copy_buf before cleaning up the ALSA card and freeing the audio
structure.

Fixes: c6994e6f067c ("USB: gadget: add USB Audio Gadget driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
Changes in v2:
- Drain queued f_audio_buf entries from play_queue and free copy_buf
  before kfree, addressing sashiko-bot [Medium] buffer leak concern

 drivers/usb/gadget/function/f_uac1_legacy.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
index 7c8d60c7e87d1..9fab3f9c8b9d6 100644
--- a/drivers/usb/gadget/function/f_uac1_legacy.c
+++ b/drivers/usb/gadget/function/f_uac1_legacy.c
@@ -942,7 +942,15 @@ 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);
+		f_audio_buffer_free(buf);
+	}
+	if (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 v2] usb: gadget: f_uac1_legacy: cancel playback_work and drain buffers on free

[sashiko-bot]

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

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

usb: gadget: f_uac1_legacy: cancel playback_work and drain buffers on free

This commit cancels pending playback_work, drains queued audio buffers from
play_queue, and frees copy_buf in f_audio_free() before freeing the f_audio
structure.

> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index 7c8d60c7e87d1..9fab3f9c8b9d6 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c
> @@ -942,7 +942,15 @@ 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);
> +		f_audio_buffer_free(buf);
> +	}
> +	if (audio->copy_buf)
> +		f_audio_buffer_free(audio->copy_buf);

[Severity: Critical]
Can this cause a use-after-free and double-free of audio->copy_buf?

When the audio streaming interface switches to alternate setting 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);
			}
		}
    ...
}

A similar situation occurs in f_audio_out_ep_complete() if
f_audio_buffer_alloc() fails after queuing the full copy_buf onto
audio->play_queue, returning -ENOMEM before audio->copy_buf is updated:

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_out_ep_complete() {
    ...
	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_free() later runs, that buffer has either already been freed by
f_audio_playback_work() or is freed by the play_queue drain loop above, so
calling f_audio_buffer_free(audio->copy_buf) afterward frees it a second
time.

[Severity: High]
Could f_audio_free() also dereference an error pointer in audio->copy_buf?

f_audio_buffer_alloc() returns ERR_PTR(-ENOMEM) on allocation failure rather
than NULL. In f_audio_set_alt() when alt == 1, the return value is stored
directly in audio->copy_buf:

drivers/usb/gadget/function/f_uac1_legacy.c:f_audio_set_alt() {
    ...
			audio->copy_buf = f_audio_buffer_alloc(audio_buf_size);
			if (IS_ERR(audio->copy_buf))
				return -ENOMEM;
    ...
}

If that allocation fails, audio->copy_buf holds ERR_PTR(-ENOMEM), which
passes the if (audio->copy_buf) check in f_audio_free() and faults when
f_audio_buffer_free() dereferences audio_buf->buf.

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