Re: [PATCH] ALSA: usb-audio: Kill MIDI 2.0 URBs before freeing endpoints

[Takashi Iwai <tiwai@suse.de>]

On Thu, 18 Jun 2026 12:02:27 +0200,
Cen Zhang wrote:
> 
> MIDI 2.0 input URBs are started during snd_usb_midi_v2_create().
> A later setup failure can still jump to snd_usb_midi_v2_free(), which
> currently frees each endpoint and its coherent URB buffers without first
> stopping the submitted URBs. A completion can then dereference the
> embedded URB context and endpoint state after they have been freed, or
> try to resubmit from the stale endpoint.
> 
> The buggy scenario involves two paths, with each column showing the
> order within that path:
> 
> probe error path:                         USB completion path:
> 1. start_input_streams() submits          1. The HCD still owns a
>    input URBs.                               submitted input URB.
> 2. A later setup helper returns           2. input_urb_complete() runs
>    an error.                                 with urb->context in ep.
> 3. snd_usb_midi_v2_free() frees           3. The completion reads ep
>    endpoint storage and URB buffers.         state and can requeue URBs.
> 
> Make the endpoint destructor follow the same teardown ordering used for
> disconnect: publish ep->disconnected, kill the URBs synchronously, and
> drain the endpoint before freeing URB buffers and endpoint storage.
> 
> Validation reproduced this kernel report:
> BUG: KASAN: slab-use-after-free in input_urb_complete+0x37/0x1b0
> Workqueue: usb_hub_wq hub_event
> RIP: 0010:_raw_spin_unlock_irq+0x2e/0x50
> Read of size 8
> Call trace:
>   dump_stack_lvl+0x77/0xb0
>   print_report+0xce/0x5f0
>   input_urb_complete+0x37/0x1b0 (sound/usb/midi2.c:186)
>   srso_alias_return_thunk+0x5/0xfbef5
>   __virt_addr_valid+0x19f/0x330
>   kasan_report+0xe0/0x110
>   __usb_hcd_giveback_urb+0x112/0x1d0
>   dummy_timer+0xaaa/0x19a0
>   lock_is_held_type+0x9a/0x110
>   __lock_acquire+0x467/0x28b0
>   mark_held_locks+0x40/0x70
>   _raw_spin_unlock_irqrestore+0x44/0x60
>   lockdep_hardirqs_on_prepare+0xbb/0x1a0
>   __hrtimer_run_queues+0x101/0x520
>   hrtimer_run_softirq+0xd0/0x130
>   handle_softirqs+0x15b/0x670
>   __irq_exit_rcu+0xd0/0x170
>   irq_exit_rcu+0xe/0x20
>   sysvec_apic_timer_interrupt+0x6c/0x80
>   asm_sysvec_apic_timer_interrupt+0x1a/0x20
> 
> Fixes: d9c99876868c ("ALSA: usb-audio: Create UMP blocks from USB MIDI GTBs")
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
>  sound/usb/midi2.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/usb/midi2.c b/sound/usb/midi2.c
> index 04aeb9052f13..94c1a52853e0 100644
> --- a/sound/usb/midi2.c
> +++ b/sound/usb/midi2.c
> @@ -470,6 +470,9 @@ static int create_midi2_endpoint(struct snd_usb_midi2_interface *umidi,
>  static void free_midi2_endpoint(struct snd_usb_midi2_endpoint *ep)
>  {
>  	list_del(&ep->list);
> +	ep->disconnected = 1;
> +	kill_midi_urbs(ep, false);
> +	drain_urb_queue(ep);
>  	free_midi_urbs(ep);
>  	kfree(ep);
>  }

Thanks for the patch.  This can be good as a quick paper-over, so that
it can be easily backported.  But the proper fix would be rather to
call the core part of __usb_audio_disconnect() at the error path
(something like below -- based on the latest tree).

That said, I can take your patch as a quick fix, but could you correct
the patch for avoiding the doubly disconnection procedure?  The code
should have a check of ep->disconnected, such as:

static void free_midi2_endpoint(struct snd_usb_midi2_endpoint *ep)
{
	list_del(&ep->list);
	if (!ep->disconnected) {
		ep->disconnected = 1;
		kill_midi_urbs(ep, false);
		drain_urb_queue(ep);
	}
 	free_midi_urbs(ep);
 	kfree(ep);
}


Takashi

-- 8< --
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -905,6 +905,41 @@ static int try_to_register_card(struct snd_usb_audio *chip, int ifnum)
 	return 0;
 }
 
+/* disconnect / release various resources assigned to the chip */
+static void usb_audio_disconnect_components(struct snd_usb_audio *chip)
+{
+	struct list_head *p;
+	struct snd_usb_stream *as;
+	struct snd_usb_endpoint *ep;
+	struct usb_mixer_interface *mixer;
+
+	/* release the pcm resources */
+	list_for_each_entry(as, &chip->pcm_list, list)
+		snd_usb_stream_disconnect(as);
+
+	/* release the endpoint resources */
+	list_for_each_entry(ep, &chip->ep_list, list)
+		snd_usb_endpoint_release(ep);
+
+	/* release the midi resources */
+	list_for_each(p, &chip->midi_list)
+		snd_usbmidi_disconnect(p);
+
+	snd_usb_midi_v2_disconnect_all(chip);
+
+	/*
+	 * Nice to check quirk && quirk->shares_media_device and
+	 * then call the snd_media_device_delete(). Don't have
+	 * access to the quirk here. snd_media_device_delete()
+	 * accesses mixer_list
+	 */
+	snd_media_device_delete(chip);
+
+	/* release mixer resources */
+	list_for_each_entry(mixer, &chip->mixer_list, list)
+		snd_usb_mixer_disconnect(mixer);
+}
+
 /*
  * probe the active usb device
  *
@@ -1077,8 +1112,10 @@ static int usb_audio_probe(struct usb_interface *intf,
 		 * decrement before memory is possibly returned.
 		 */
 		atomic_dec(&chip->active);
-		if (!chip->num_interfaces)
+		if (!chip->num_interfaces) {
+			usb_audio_disconnect_components(chip);
 			snd_card_free(chip->card);
+		}
 	}
 	return err;
 }
@@ -1091,48 +1128,19 @@ static bool __usb_audio_disconnect(struct usb_interface *intf,
 				   struct snd_usb_audio *chip,
 				   struct snd_card *card)
 {
-	struct list_head *p;
-
 	guard(mutex)(&register_mutex);
 
 	if (platform_ops && platform_ops->disconnect_cb)
 		platform_ops->disconnect_cb(chip);
 
 	if (atomic_inc_return(&chip->shutdown) == 1) {
-		struct snd_usb_stream *as;
-		struct snd_usb_endpoint *ep;
-		struct usb_mixer_interface *mixer;
-
 		/* wait until all pending tasks done;
 		 * they are protected by snd_usb_lock_shutdown()
 		 */
 		snd_refcount_sync(&chip->usage_count);
 		snd_card_disconnect(card);
-		/* release the pcm resources */
-		list_for_each_entry(as, &chip->pcm_list, list) {
-			snd_usb_stream_disconnect(as);
-		}
-		/* release the endpoint resources */
-		list_for_each_entry(ep, &chip->ep_list, list) {
-			snd_usb_endpoint_release(ep);
-		}
-		/* release the midi resources */
-		list_for_each(p, &chip->midi_list) {
-			snd_usbmidi_disconnect(p);
-		}
-		snd_usb_midi_v2_disconnect_all(chip);
-		/*
-		 * Nice to check quirk && quirk->shares_media_device and
-		 * then call the snd_media_device_delete(). Don't have
-		 * access to the quirk here. snd_media_device_delete()
-		 * accesses mixer_list
-		 */
-		snd_media_device_delete(chip);
 
-		/* release mixer resources */
-		list_for_each_entry(mixer, &chip->mixer_list, list) {
-			snd_usb_mixer_disconnect(mixer);
-		}
+		usb_audio_disconnect_components(chip);
 	}
 
 	if (chip->quirk_flags & QUIRK_FLAG_DISABLE_AUTOSUSPEND)