Re: [PATCH v3] usb: gadget: f_uac1_legacy: disable OUT ep and drain work on teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: gadget: f_uac1_legacy: playback_work not drained in
  f_audio_unbind()
--

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

usb: gadget: f_uac1_legacy: disable OUT ep and drain work on teardown

This commit disables the OUT endpoint in f_audio_disable() and cancels
audio->playback_work in f_audio_free() before tearing down the audio card
and freeing f_audio.

> diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
> index 7c8d60c7e87d1..c07f323bcf9b0 100644
> --- a/drivers/usb/gadget/function/f_uac1_legacy.c
> +++ b/drivers/usb/gadget/function/f_uac1_legacy.c

[ ... ]

> @@ -943,6 +953,15 @@ static void f_audio_free(struct usb_function *f)
>  	struct f_audio *audio = func_to_audio(f);
>  	struct f_uac1_legacy_opts *opts;
>  
> +	/*
> +	 * The OUT endpoint was disabled in f_audio_disable(), so no new
> +	 * playback_work can be queued. Drain any still-pending work here, in
> +	 * sleepable context and before gaudio_cleanup() tears down the card
> +	 * that f_audio_playback_work() -> u_audio_playback() dereferences,
> +	 * otherwise the work runs against freed memory.
> +	 */
> +	cancel_work_sync(&audio->playback_work);
> +
>  	gaudio_cleanup(&audio->card);
>  	opts = container_of(f->fi, struct f_uac1_legacy_opts, func_inst);
>  	kfree(audio);

[Severity: Medium]
Should audio->playback_work also be drained in f_audio_unbind()?

When a ConfigFS gadget using uac1_legacy is unbound from its UDC or the UDC
is unregistered via usb_del_gadget_udc(), purge_configs_funcs() calls
f_audio_unbind() without calling f_audio_free(), which only runs when the
function symlink is removed.  A pending f_audio_playback_work() ->
u_audio_playback() can therefore keep running after gadget_unbind_driver()
returns, and if it hits an ALSA error and logs via ERROR(card, ...), it
dereferences audio->card.gadget->dev on a detached or freed usb_gadget
(use-after-free).

CPU0                                  kworker
usb_del_gadget_udc()
  gadget_unbind_driver()
    f_audio_disable()
    configfs_composite_unbind()
      purge_configs_funcs()
        f_audio_unbind()
  device_unregister(&gadget->dev)
                                      f_audio_playback_work()
                                        u_audio_playback()
                                          ERROR(card, ...)
                                            &card->gadget->dev
                                            <-- use-after-free

Calling cancel_work_sync(&audio->playback_work) in f_audio_unbind() as well
(which also runs in sleepable process context) would ensure the work is
drained across UDC unbind.

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