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@vger.kernel.org Signed-off-by: Yuanzhe Liu <25031212351@stu.xidian.edu.cn> --- 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