[PATCH 4/5] ALSA: usx2y: us144mkii: wait for the URB completion handlers on stop and close

[Alexandre Becholey <alex@becholey.ch>]

The playback, feedback and capture URBs are started and stopped together,
so they keep running while either PCM stream is running, also when the
other substream is closed. playback_urb_complete() and
feedback_urb_complete() read tascam->playback_substream (and
capture_substream) and its runtime without holding tascam->lock, then
use runtime->rate, runtime->period_size and runtime->buffer_size under
the lock, copy from runtime->dma_area and call snd_pcm_period_elapsed()
after dropping it. Nothing waits for a completion handler that has
already loaded these pointers: the PCM .close callbacks only clear them
and the stop path only schedules stop_work.

So when a PCM is closed while the URBs are running, the managed buffer
is freed by do_hw_free() and the runtime by snd_pcm_detach_substream()
while a completion handler may still be using them. A device can make
the window wide: a feedback completion that prints "Fatal: Feedback
sync lost" does so between two uses of the playback runtime. With
KASAN, on torvalds/master 7e1392a42ea2:

  BUG: KASAN: slab-use-after-free in feedback_urb_complete+0xe64/0x1174
  Read of size 8 at addr ffff0000132ba078 by task repro/82

The object is the snd_pcm_runtime, freed by snd_pcm_detach_substream()
from snd_pcm_release().

Read the substream pointers inside an RCU read-side section in both
completion handlers and keep it until they no longer use the substream.
Call synchronize_rcu() in the .close callbacks, after the pointer was
cleared, and in a new .sync_stop callback, which the PCM core calls
after a stop before the buffer is freed or reallocated. The capture
work, which writes to the capture buffer with tascam->lock held, is
covered by the same grace period; on close it is already cancelled.

The bug was found, and the reproducer and this fix were written, by AI
agents (Claude Mythos and Claude Opus models). The analysis and the
patch were reviewed by a human.

Reported-by: Hanxiang Xu <xuhx@hust.edu.cn>
Closes: https://lore.kernel.org/all/12a3a26a.26361.1a1d4e1ad57.Coremail.xuhx@hust.edu.cn/
Fixes: a2a2210f2c2e ("ALSA: usb-audio: us144mkii: Implement audio playback and feedback")
Fixes: c1bb0c13e430 ("ALSA: usb-audio: us144mkii: Implement audio capture and decoding")
Cc: stable@vger.kernel.org
Signed-off-by: Alexandre Becholey <alex@becholey.ch>
---
Notes for reviewers (not part of the commit message):

Trigger: an emulated US-144MKII on an emulated xHCI controller (the
URBs are isochronous), QEMU arm64, KASAN. The device completes the
probe and then sends empty feedback packets. The program starts the
capture PCM, which starts all URBs, while the playback PCM is only
prepared, and closes the playback PCM when the driver logs "Feedback
sync lost" (a close of a PCM that is not running does not stop the
URBs). A variant does the same with the capture runtime (line 414).
Two more programs serve the device over USB/IP (vhci_hcd) and close
the PCMs at a high rate. The reproducers are available to maintainers
on request.

Decoded, torvalds/master 7e1392a42ea2 (the object is the kmalloc-2k
snd_pcm_runtime, allocated by snd_pcm_attach_substream() on open):

  BUG: KASAN: slab-use-after-free in feedback_urb_complete (sound/usb/usx2y/us144mkii_playback.c:402)
  Read of size 8 at addr ffff0000132ba078 by task repro/82
  Call trace:
   feedback_urb_complete (sound/usb/usx2y/us144mkii_playback.c:402)
   __usb_hcd_giveback_urb (drivers/usb/core/hcd.c:1657)
   usb_giveback_urb_bh (drivers/usb/core/hcd.c:1691)
   process_one_work (kernel/workqueue.c:3495)
   bh_worker (kernel/workqueue.c:3578 kernel/workqueue.c:3885)
  Freed by task 83:
   kfree (include/linux/kasan.h:235 mm/slub.c:2780 mm/slub.c:6630 mm/slub.c:6923)
   snd_pcm_detach_substream (sound/core/pcm.c:1014)
   snd_pcm_release_substream.part.0 (sound/core/pcm_native.c:2858)
   snd_pcm_release (sound/core/pcm_native.c:2840 sound/core/pcm_native.c:3037)
   __fput (fs/file_table.c:512)

Line 402 is the playback_rt->period_size read after the locked
section's dev_err(); line 414 (capture_rt->period_size) was hit by the
capture variant.

Testing, KASAN: without the patch the programs crashed in
feedback_urb_complete() on v7.3.1 (3/3 in this test, :402 and :414)
and on 7e1392a42ea2 (4/4 in the verification). With the whole series
applied, no us144mkii report in 27 runs: v7.3.1 15, 97d73c825122 7,
and 5 runs on v7.3.1 with a debug patch that holds the race window
open for 20 ms (181 windows held with a close pending; the same debug
kernel without this patch crashed 3/3 at the first window).

Four of the 27 runs (USB/IP programs) ended in a KASAN use-after-free
in vhci_urb_dequeue(), a race in the vhci_hcd test harness between
vhci_rx and usb_kill_anchored_urbs(), not in this driver; the same
programs with KASAN multi-shot and no panic ran the full time with no
report (3 runs).

Cost: one RCU grace period in each .close and in .sync_stop (prepare,
hw_params or hw_free after a stop). The capture work is covered by
.sync_stop because synchronize_rcu() also waits for regions with
interrupts disabled; an explicit rcu_read_lock() in
tascam_capture_work_handler() is a two-line alternative.

sound/usb/usx2y/us144mkii*.c are identical in v7.3.1, 7e1392a42ea2,
9d66f26fefac and 97d73c825122.

Thanks,
Alexandre Becholey

 sound/usb/usx2y/us144mkii_capture.c  |  3 +++
 sound/usb/usx2y/us144mkii_pcm.c      | 14 ++++++++++++++
 sound/usb/usx2y/us144mkii_pcm.h      |  8 ++++++++
 sound/usb/usx2y/us144mkii_playback.c | 23 ++++++++++++++++++-----
 4 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/sound/usb/usx2y/us144mkii_capture.c b/sound/usb/usx2y/us144mkii_capture.c
index 9c142a85e8b2..fa7b0cd94c71 100644
--- a/sound/usb/usx2y/us144mkii_capture.c
+++ b/sound/usb/usx2y/us144mkii_capture.c
@@ -40,6 +40,8 @@ static int tascam_capture_close(struct snd_pcm_substream *substream)
 		tascam->capture_substream = NULL;
 	/* The stop work may still be using this substream; wait for it. */
 	flush_work(&tascam->stop_pcm_work);
+	/* So may the URB completion handlers, see tascam_pcm_sync_stop(). */
+	synchronize_rcu();
 	/*
 	 * The capture work keeps a pointer to the runtime, which is freed
 	 * when this callback returns. The capture URBs (and so the work) keep
@@ -113,6 +115,7 @@ const struct snd_pcm_ops tascam_capture_ops = {
 	.ioctl = snd_pcm_lib_ioctl,
 	.hw_params = tascam_pcm_hw_params,
 	.hw_free = tascam_pcm_hw_free,
+	.sync_stop = tascam_pcm_sync_stop,
 	.prepare = tascam_capture_prepare,
 	.trigger = tascam_pcm_trigger,
 	.pointer = tascam_capture_pointer,
diff --git a/sound/usb/usx2y/us144mkii_pcm.c b/sound/usb/usx2y/us144mkii_pcm.c
index 03dfb1f38801..5add9c1b253a 100644
--- a/sound/usb/usx2y/us144mkii_pcm.c
+++ b/sound/usb/usx2y/us144mkii_pcm.c
@@ -259,6 +259,20 @@ int tascam_pcm_hw_free(struct snd_pcm_substream *substream)
 	return 0;
 }
 
+int tascam_pcm_sync_stop(struct snd_pcm_substream *substream)
+{
+	/*
+	 * The URBs of both streams keep running as long as one of them does.
+	 * The playback URB completion handler uses the substream inside an RCU
+	 * read-side section, and the capture work writes to the buffer with
+	 * tascam->lock held (interrupts disabled), which synchronize_rcu()
+	 * also waits for.  Wait for both before the buffer is freed or
+	 * reallocated.
+	 */
+	synchronize_rcu();
+	return 0;
+}
+
 int tascam_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	struct tascam_card *tascam = snd_pcm_substream_chip(substream);
diff --git a/sound/usb/usx2y/us144mkii_pcm.h b/sound/usb/usx2y/us144mkii_pcm.h
index a130678405e3..22c05c61fdd8 100644
--- a/sound/usb/usx2y/us144mkii_pcm.h
+++ b/sound/usb/usx2y/us144mkii_pcm.h
@@ -139,6 +139,14 @@ int tascam_pcm_hw_params(struct snd_pcm_substream *substream,
  */
 int tascam_pcm_hw_free(struct snd_pcm_substream *substream);
 
+/**
+ * tascam_pcm_sync_stop() - Waits for the URB completion handlers after a stop.
+ * @substream: The ALSA PCM substream.
+ *
+ * Return: 0.
+ */
+int tascam_pcm_sync_stop(struct snd_pcm_substream *substream);
+
 /**
  * tascam_pcm_trigger() - Triggers the start or stop of PCM streams.
  * @substream: The ALSA PCM substream.
diff --git a/sound/usb/usx2y/us144mkii_playback.c b/sound/usb/usx2y/us144mkii_playback.c
index e6266488cf32..36d5417d9975 100644
--- a/sound/usb/usx2y/us144mkii_playback.c
+++ b/sound/usb/usx2y/us144mkii_playback.c
@@ -40,6 +40,8 @@ static int tascam_playback_close(struct snd_pcm_substream *substream)
 		tascam->playback_substream = NULL;
 	/* The stop work may still be using this substream; wait for it. */
 	flush_work(&tascam->stop_pcm_work);
+	/* So may the URB completion handlers, see tascam_pcm_sync_stop(). */
+	synchronize_rcu();
 
 	return 0;
 }
@@ -153,6 +155,7 @@ const struct snd_pcm_ops tascam_playback_ops = {
 	.ioctl = snd_pcm_lib_ioctl,
 	.hw_params = tascam_pcm_hw_params,
 	.hw_free = tascam_pcm_hw_free,
+	.sync_stop = tascam_pcm_sync_stop,
 	.prepare = tascam_playback_prepare,
 	.trigger = tascam_pcm_trigger,
 	.pointer = tascam_playback_pointer,
@@ -180,9 +183,13 @@ void playback_urb_complete(struct urb *urb)
 	if (!tascam || !atomic_read(&tascam->playback_active))
 		goto out;
 
-	substream = tascam->playback_substream;
-	if (!substream || !substream->runtime)
+	/* Pairs with synchronize_rcu() in close and sync_stop. */
+	rcu_read_lock();
+	substream = READ_ONCE(tascam->playback_substream);
+	if (!substream || !substream->runtime) {
+		rcu_read_unlock();
 		goto out;
+	}
 	runtime = substream->runtime;
 
 	scoped_guard(spinlock_irqsave, &tascam->lock) {
@@ -252,6 +259,7 @@ void playback_urb_complete(struct urb *urb)
 		process_playback_routing_us144mkii(tascam, dst_buf, dst_buf,
 						   frames_to_copy);
 	}
+	rcu_read_unlock();
 
 	urb->dev = tascam->dev;
 	usb_get_urb(urb);
@@ -295,12 +303,16 @@ void feedback_urb_complete(struct urb *urb)
 	if (!tascam || !atomic_read(&tascam->playback_active))
 		goto out;
 
-	playback_ss = tascam->playback_substream;
-	if (!playback_ss || !playback_ss->runtime)
+	/* Pairs with synchronize_rcu() in close and sync_stop. */
+	rcu_read_lock();
+	playback_ss = READ_ONCE(tascam->playback_substream);
+	if (!playback_ss || !playback_ss->runtime) {
+		rcu_read_unlock();
 		goto out;
+	}
 	playback_rt = playback_ss->runtime;
 
-	capture_ss = tascam->capture_substream;
+	capture_ss = READ_ONCE(tascam->capture_substream);
 	capture_rt = capture_ss ? capture_ss->runtime : NULL;
 
 	scoped_guard(spinlock_irqsave, &tascam->lock) {
@@ -444,6 +456,7 @@ void feedback_urb_complete(struct urb *urb)
 		snd_pcm_period_elapsed(playback_ss);
 	if (capture_period_elapsed)
 		snd_pcm_period_elapsed(capture_ss);
+	rcu_read_unlock();
 
 	urb->dev = tascam->dev;
 	usb_get_urb(urb);