Re: [PATCH] usb: gadget: f_uac1_legacy: cancel playback_work before freeing f_audio

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] usb: gadget: f_uac1_legacy: cancel_work_sync() after
  gaudio_cleanup()
- [Medium] usb: gadget: f_uac1_legacy: play_queue buffer leak in
  f_audio_free()
- [Low] usb: gadget: f_uac1_legacy: Fixes tag cites file rename commit
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] usb: gadget: f_uac1_legacy: use-after-free in
  f_audio_complete() due to undisabled out_ep [1]
--

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

> usb: gadget: f_uac1_legacy: cancel playback_work before freeing f_audio
>
> The OUT endpoint completion (f_audio_out_ep_complete) and the set_alt path
> schedule audio->playback_work, whose handler (f_audio_playback_work)
> dereferences the f_audio (audio->lock, audio->play_queue, audio->card).
> On teardown f_audio_free() frees the f_audio with kfree(audio) but never
> cancels playback_work -- f_audio_disable() is empty and nothing else
> drains it. A playback_work still pending when the function is freed runs
> against freed memory, a use-after-free.
>
> Cancel playback_work in f_audio_free() before kfree(audio). By then the
> function is unbound and the OUT endpoint is disabled, so the work cannot
> be re-queued and cancel_work_sync() is sufficient.
>
> This was reproduced with KASAN as a slab-use-after-free in
> u_audio_playback(), reached from f_audio_playback_work(), under dummy_hcd:
> with a UAC1 playback stream active, unlinking the configfs function frees
> the f_audio in f_audio_free() while the work is still in flight. A delay
> was added to the gadget streaming path to make the schedule-vs-free race
> land deterministically; the missing cancel is present regardless. With
> this patch applied the splat no longer occurs.
>
> Fixes: d355339eecd9 ("usb: gadget: function: make current f_uac1 implementation legacy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>

[Severity: Low]
Commit d355339eecd9 ("usb: gadget: function: make current f_uac1
implementation legacy") only renamed f_uac1.c to f_uac1_legacy.c.  Should
the Fixes: tag point to f3a3406b3f56 ("usb: gadget: f_uac1: convert to new
function interface with backward compatibility"), which introduced
f_audio_free() without cancelling playback_work?

> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index 7c8d60c7e87d1..2dca5cda8cfd7 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c
> @@ -945,6 +945,12 @@ static void f_audio_free(struct usb_function *f)
>  
>  	gaudio_cleanup(&audio->card);
>  	opts = container_of(f->fi, struct f_uac1_legacy_opts, func_inst);
> +	/*
> +	 * The function is already unbound, so the OUT endpoint can no longer
> +	 * queue playback_work. Drain any work still pending before the
> +	 * containing f_audio is freed, otherwise it runs against freed memory.
> +	 */
> +	cancel_work_sync(&audio->playback_work);
>  	kfree(audio);

[Severity: Medium]
Should cancel_work_sync(&audio->playback_work) be called before
gaudio_cleanup(&audio->card)?

f_audio_playback_work() calls u_audio_playback(&audio->card, ...), which
reads substream->runtime from card->playback.substream and loops back to
try_again when snd_pcm_kernel_write() fails.  Calling gaudio_cleanup()
first closes card->playback.filp while playback_work may still be running,
which can free substream->runtime underneath u_audio_playback() or leave it
retrying in try_again while cancel_work_sync() waits.

[Severity: Medium]
Does cancelling audio->playback_work here leak buffers left on
audio->play_queue (as well as audio->copy_buf)?

f_audio_out_ep_complete() and f_audio_set_alt() append each allocated
struct f_audio_buf to audio->play_queue and schedule audio->playback_work,
and f_audio_playback_work() is the only place that dequeues and frees them
with f_audio_buffer_free().  When cancel_work_sync() cancels a pending work
item, or when more than one buffer is queued, the remaining buffers on
audio->play_queue are never freed before kfree(audio).

[Severity: Critical]
This problem wasn't introduced by this patch, but nothing disables
audio->out_ep before f_audio_free() runs [1], so can
f_audio_complete() still race with teardown and use audio after free?

When the host selects alternate setting 1, f_audio_set_alt() enables
audio->out_ep and queues usb_request objects with req->context = audio and
req->complete = f_audio_complete, while f_audio_disable() is a no-op and
neither f_audio_set_alt(), f_audio_unbind(), nor f_audio_free() calls
usb_ep_disable(audio->out_ep):

CPU0                                  CPU1
f_audio_free()
  cancel_work_sync(&audio->playback_work)
  kfree(audio)
                                      f_audio_complete()
                                        audio = req->context
                                        out_ep = audio->out_ep <-- use-after-free
                                        f_audio_out_ep_complete()
                                          schedule_work(&audio->playback_work)

During gadget_unbind_driver(), udc->driver->unbind() runs before
usb_gadget_udc_stop_locked() gives back pending requests on the enabled
endpoint with -ESHUTDOWN, so f_audio_complete() can also be invoked after
f_audio is freed.

[1] https://sashiko.dev/#/bug/linux-7598f007-24ed-4b73-bc9a-a11e4ec9838e

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