Re: [PATCH v2] 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 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