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

[sashiko-bot@kernel.org]

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