[PATCH 1/1] ALSA: bcd2000: fix use-after-free of MIDI URBs on file close after disconnect

[Yuanzhe Liu]

Closing a rawmidi fd of the BCD2000 after its USB interface has been
unbound crashes with a slab-use-after-free in usb_kill_urb():

  BUG: KASAN: slab-use-after-free in usb_kill_urb+0x74/0x80
  Read of size 8 at addr ffff8880078e7d40 by task amidi/30287
   usb_kill_urb+0x74/0x80 drivers/usb/core/urb.c:706
   bcd2000_midi_output_close+0xd4/0x140 sound/usb/bcd2000/bcd2000.c:182
   close_substream.part.0+0x15f/0x8d0 sound/core/rawmidi.c:560
   rawmidi_release_priv+0x21d/0x290 sound/core/rawmidi.c:580
   snd_rawmidi_release+0x4e/0xa0 sound/core/rawmidi.c:610
   __fput+0x3a6/0xac0 fs/file_table.c:510

  Allocated by: usb_alloc_urb <- bcd2000_init_midi <- bcd2000_probe
  Freed by:     usb_free_urb <- bcd2000_free_usb_related_resources
                <- bcd2000_disconnect <- usb_driver_release_interface
                <- usbdev_ioctl (USBDEVFS_DISCONNECT)

Root cause: bcd2000_disconnect() calls snd_card_disconnect(), which only
blocks *new* opens, and then immediately kills and frees midi_out_urb and
midi_in_urb. Already-open rawmidi fds, however, keep working: their
->close() callback bcd2000_midi_output_close() unconditionally calls
usb_kill_urb(bcd2k->midi_out_urb) when midi_out_active is set (which it
is, since the completion handler re-submits the out URB), and the URB
completion callbacks re-submit the URBs via bcd2k->midi_{in,out}_urb.
Both dereference pointers that were freed in disconnect -- a UAF. No
physical unplug is needed; USBDEVFS_DISCONNECT on the usbfs node triggers
the same path.

Fix the lifetime mismatch in two parts:

- Wait for the users that are already there: use
  snd_card_disconnect_sync() instead of snd_card_disconnect() so that
  disconnect blocks until all open card files are released. The rawmidi
  close callbacks then run (and call usb_kill_urb()) while the URBs are
  still valid, and no userspace-triggered callback can run afterwards.
  devices_mutex held by bcd2000_disconnect() is not taken by the rawmidi
  close path, so the wait cannot deadlock.

- Mark the device shut down before freeing: add an atomic shutdown flag,
  set first in bcd2000_disconnect(), and check it in bcd2000_midi_send()
  and in both URB completion handlers so no new URB submission can be
  queued while disconnect frees the old ones.  Also NULL the URB
  pointers after usb_free_urb() so any stray access becomes an
  immediate, diagnosable NULL dereference instead of a silent UAF.

Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Yuanzhe Liu <25031212351@xxxxxxxxxxxxxxxxx>
---
 sound/usb/bcd2000/bcd2000.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/sound/usb/bcd2000/bcd2000.c b/sound/usb/bcd2000/bcd2000.c
index bebb48c..cbf6d39 100644
--- a/sound/usb/bcd2000/bcd2000.c
+++ b/sound/usb/bcd2000/bcd2000.c
@@ -55,6 +55,7 @@ struct bcd2000 {
 	struct urb *midi_in_urb;
 
 	struct usb_anchor anchor;
+	atomic_t shutdown;
 };
 
 static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
@@ -131,7 +132,7 @@ static void bcd2000_midi_send(struct bcd2000 *bcd2k)
 	BUILD_BUG_ON(sizeof(device_cmd_prefix) >= BUFSIZE);
 
 	midi_out_substream = READ_ONCE(bcd2k->midi_out_substream);
-	if (!midi_out_substream)
+	if (!midi_out_substream || atomic_read(&bcd2k->shutdown))
 		return;
 
 	/* copy command prefix bytes */
@@ -212,7 +213,7 @@ static void bcd2000_output_complete(struct urb *urb)
 		dev_warn(&urb->dev->dev,
 			PREFIX "output urb->status: %d\n", urb->status);
 
-	if (urb->status == -ESHUTDOWN)
+	if (urb->status == -ESHUTDOWN || atomic_read(&bcd2k->shutdown))
 		return;
 
 	/* check if there is more data userspace wants to send */
@@ -228,7 +229,8 @@ static void bcd2000_input_complete(struct urb *urb)
 		dev_warn(&urb->dev->dev,
 			PREFIX "input urb->status: %i\n", urb->status);
 
-	if (!bcd2k || urb->status == -ESHUTDOWN)
+	if (!bcd2k || urb->status == -ESHUTDOWN ||
+	    atomic_read(&bcd2k->shutdown))
 		return;
 
 	if (urb->actual_length > 0)
@@ -353,6 +355,8 @@ static void bcd2000_free_usb_related_resources(struct bcd2000 *bcd2k,
 
 	usb_free_urb(bcd2k->midi_out_urb);
 	usb_free_urb(bcd2k->midi_in_urb);
+	bcd2k->midi_out_urb = NULL;
+	bcd2k->midi_in_urb = NULL;
 
 	if (bcd2k->intf) {
 		usb_set_intfdata(bcd2k->intf, NULL);
@@ -427,8 +431,19 @@ static void bcd2000_disconnect(struct usb_interface *interface)
 
 	guard(mutex)(&devices_mutex);
 
-	/* make sure that userspace cannot create new requests */
-	snd_card_disconnect(bcd2k->card);
+	/*
+	 * Make sure that the URB completion handlers and the rawmidi ops
+	 * don't touch the USB device or the URBs any longer.
+	 */
+	atomic_set(&bcd2k->shutdown, 1);
+
+	/*
+	 * Make sure that userspace cannot create new requests, and wait
+	 * until all already-open files are closed so that the rawmidi
+	 * close callbacks (which may kill the out URB) run while the
+	 * URBs are still valid.
+	 */
+	snd_card_disconnect_sync(bcd2k->card);
 
 	bcd2000_free_usb_related_resources(bcd2k, interface);
 
-- 
2.45.1.windows.1