Re: [PATCH] usb: gadget: f_midi: cancel pending IN work before freeing the midi object

[Takashi Iwai]

On Thu, 09 Jul 2026 17:07:17 +0200,
Fan Wu wrote:
> 
> The f_midi driver embeds a work item (midi->work) whose handler,
> f_midi_in_work(), dereferences the enclosing struct f_midi through
> container_of().  This work is armed from two sites: f_midi_complete(),
> on a normal IN-endpoint completion, and f_midi_in_trigger(), on an ALSA
> rawmidi output-stream start.
> 
> Neither f_midi_disable() nor f_midi_unbind() cancels midi->work.
> f_midi_disable() only disables the endpoints and drains the in_req_fifo;
> it does not synchronize the work item, and the sound card is released
> asynchronously to the final free of the midi object.
> 
> The midi object is reference-counted (midi->free_ref) and is freed in
> f_midi_free() only once both the usb_function reference and the rawmidi
> private_data reference have been dropped.  In f_midi_unbind(),
> f_midi_disable() runs before the sound card is released, so while the
> USB endpoints are already disabled the rawmidi device is still usable by
> an open substream.  A concurrent userspace write on such a substream can
> reach f_midi_in_trigger() and queue midi->work again after
> f_midi_disable() has returned.  A work item armed this way may still be
> pending when the last reference drops and f_midi_free() proceeds to
> kfree(midi), letting f_midi_in_work() dereference the struct after it
> has been freed, a use-after-free.
> 
> For this reason cancelling midi->work in f_midi_disable() would not be
> sufficient: the ALSA trigger path can rearm the work after disable()
> returns.  Cancelling at the refcount-zero free site is the boundary
> after which neither arming source can survive, because by then both
> references that keep the midi object alive have been dropped: the USB
> endpoints are already disabled and the rawmidi device has been released.
> 
> Fix this by calling cancel_work_sync(&midi->work) in the refcount-zero
> block of f_midi_free(), before the embedded work_struct is freed along
> with the rest of the structure.  opts->lock is a sleeping mutex, so
> calling cancel_work_sync() under it is permitted, and the handler takes
> midi->transmit_lock rather than opts->lock, so no self-deadlock can
> occur while it waits for a running instance of the work to finish.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 8653d71ce3763 ("usb/gadget: f_midi: Replace tasklet with work")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
> ---
>  drivers/usb/gadget/function/f_midi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 4d9e4bd70..fba8cf787 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -1309,6 +1309,7 @@ static void f_midi_free(struct usb_function *f)
>  	opts = container_of(f->fi, struct f_midi_opts, func_inst);
>  	mutex_lock(&opts->lock);
>  	if (!--midi->free_ref) {
> +		cancel_work_sync(&midi->work);
>  		kfree(midi->id);
>  		kfifo_free(&midi->in_req_fifo);
>  		kfree(midi);

I guess disable_work_sync() would be even safer in this case.


thanks,

Takashi