[PATCH 3/5] ALSA: usx2y: us144mkii: only touch the buffer of a running PCM stream

[Alexandre Becholey <alex@becholey.ch>]

The playback and capture URBs of the US-144MKII always run together:
tascam_pcm_trigger() sets both playback_active and capture_active and
submits all URBs when either substream is started. The capture work
handler and the playback URB completion handler, however, use the
buffer of their own substream as soon as the corresponding *_active
flag is set and the substream is open, without checking that this
substream is actually running.

So if only one direction is started, the handler of the other direction
works on a substream that may have no buffer at all:

 - capture PCM opened without HW_PARAMS, playback started:
   tascam_capture_work_handler() writes the captured frames through the
   NULL runtime->dma_area, and divides by the zero buffer_size.

 - playback PCM prepared but not started (or opened without HW_PARAMS),
   capture started, then HW_FREE on the playback PCM, which ALSA allows
   in the PREPARED state: playback_urb_complete() copies from the NULL
   runtime->dma_area.

With KASAN, on torvalds/master 9d66f26fefac:

  BUG: KASAN: null-ptr-deref in tascam_capture_work_handler
  Write of size 3 at addr 0000000000000000 by task kworker/0:2/62

  BUG: KASAN: null-ptr-deref in playback_urb_complete
  Read of size 480 at addr 0000000000000660 by task ksoftirqd/0/15

Both only need an attached device and this order of PCM ioctls from a
user with access to the PCM devices.

Only copy to or from the substream buffer while that substream is
running. The capture data is then dropped, and the playback URBs carry
silence, as the device keeps streaming in both directions.

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

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 under QEMU arm64 with KASAN; the
capture case on dummy_hcd (raw-gadget), the playback case on an
emulated xHCI controller, because the playback URBs are isochronous.
The reproducers are available to maintainers on request.

Decoded, torvalds/master 9d66f26fefac:
  tascam_capture_work_handler (sound/usb/usx2y/us144mkii_capture.c:246)
  playback_urb_complete (sound/usb/usx2y/us144mkii_playback.c:230)

This patch depends on patch 2/5: the new snd_pcm_running() check reads
substream->runtime, so a capture work instance must not outlive the
close of its substream. With this patch alone, the close/reopen
reproducer of 2/5 crashed in the check (KASAN null-ptr-deref at
include/sound/pcm.h:711 from us144mkii_capture.c:231, v7.3.1).

Testing, KASAN: without the series both cases crashed on the first
attempt (v7.3.1 and 9d66f26fefac, 1/1 each). With patches 1/5..3/5
applied: no report in v7.3.1 15 runs (capture 10, playback 5 with two
programs) and 9d66f26fefac 5 runs (capture 3, playback 2). Normal
streaming in both directions was checked on v7.3.1 with patches
1/5..3/5 applied (2 runs): the playback and capture positions keep
advancing. The playback test of patch 5/5 (normal streaming, 187 of
187 STARTs) also ran with all five patches.

Thanks,
Alexandre Becholey

 sound/usb/usx2y/us144mkii_capture.c  |  8 +++++++-
 sound/usb/usx2y/us144mkii_playback.c | 23 ++++++++++++++++++-----
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/sound/usb/usx2y/us144mkii_capture.c b/sound/usb/usx2y/us144mkii_capture.c
index 973bed60bfcd..9c142a85e8b2 100644
--- a/sound/usb/usx2y/us144mkii_capture.c
+++ b/sound/usb/usx2y/us144mkii_capture.c
@@ -231,7 +231,13 @@ void tascam_capture_work_handler(struct work_struct *work)
 						  routed_block);
 
 		scoped_guard(spinlock_irqsave, &tascam->lock) {
-			if (atomic_read(&tascam->capture_active)) {
+			/*
+			 * capture_active is also set when only the playback
+			 * stream was started; the capture buffer is only valid
+			 * while the capture stream itself is running.
+			 */
+			if (atomic_read(&tascam->capture_active) &&
+			    snd_pcm_running(substream)) {
 				int f;
 
 				for (f = 0; f < FRAMES_PER_DECODE_BLOCK; ++f) {
diff --git a/sound/usb/usx2y/us144mkii_playback.c b/sound/usb/usx2y/us144mkii_playback.c
index 0df95dbf585d..e6266488cf32 100644
--- a/sound/usb/usx2y/us144mkii_playback.c
+++ b/sound/usb/usx2y/us144mkii_playback.c
@@ -166,6 +166,7 @@ void playback_urb_complete(struct urb *urb)
 	size_t total_bytes_for_urb = 0;
 	snd_pcm_uframes_t offset_frames;
 	snd_pcm_uframes_t frames_to_copy;
+	bool running;
 	int ret, i;
 
 	if (urb->status) {
@@ -207,13 +208,25 @@ void playback_urb_complete(struct urb *urb)
 		}
 		urb->transfer_buffer_length = total_bytes_for_urb;
 
-		offset_frames = tascam->driver_playback_pos;
-		frames_to_copy = bytes_to_frames(runtime, total_bytes_for_urb);
-		tascam->driver_playback_pos =
-			(offset_frames + frames_to_copy) % runtime->buffer_size;
+		/*
+		 * The URBs also run when only the capture stream was started;
+		 * the playback buffer is only valid while the playback stream
+		 * itself is running.  Send silence otherwise.
+		 */
+		running = snd_pcm_running(substream);
+		if (running) {
+			offset_frames = tascam->driver_playback_pos;
+			frames_to_copy = bytes_to_frames(runtime,
+							 total_bytes_for_urb);
+			tascam->driver_playback_pos =
+				(offset_frames + frames_to_copy) %
+				runtime->buffer_size;
+		}
 	}
 
-	if (total_bytes_for_urb > 0) {
+	if (!running) {
+		memset(urb->transfer_buffer, 0, total_bytes_for_urb);
+	} else if (total_bytes_for_urb > 0) {
 		u8 *dst_buf = urb->transfer_buffer;
 
 		/* Handle ring buffer wrap-around */