[PATCH v1 0/5] ALSA: virtio: Fix request lifetime and error recovery

[PATCH v1 0/5] ALSA: virtio: Fix request lifetime and error recovery

[Joonho Lee]

A failed PCM start or a timed-out request can leave virtio-snd with
messages still owned by the device. Retrying a stream can then reuse
pending messages, and freeing the ALSA buffer or a control payload can
leave a descriptor pointing at released memory.

This series fixes the enqueue and start error paths, then keeps control
requests and PCM backing pages alive until completion or device reset.
It is based on Takashi Iwai's sound.git for-next branch at the base
commit below. Please apply the five patches in order.

Patches 1 and 2 fix partial enqueue notification and failed-start state.
Patch 3 serializes control notification with teardown and retains queue
references through reset. Patch 4 retains the vmalloc-backed PCM pages
when ALSA releases the buffer mapping after a failed drain. Patch 5 puts
control payloads under the message reference count; it needs patch 3's
reset lifetime handling as well as normal completion handling.

Keeping a message object alive is not sufficient by itself: its payload
must remain alive too. Patches 3 and 5 therefore need to stay together
when evaluating the control-request lifetime fix. An unresponsive device
can retain memory until reset. Patch 4 adds a page-pointer array and page
references per PCM buffer; patch 5 adds owned payload storage and copies.

Validation:
- ARM64 GCC 14.2.0 W=1 builds passed for the final series and the
  intermediate tree through patch 4, with no build warnings in those
  runs. The tree through patch 3 also passed a W=1 driver rebuild.
  Patches 1 and 2 were not separately kernel-built or booted.
- Source-extracted ASan/UBSan tests passed 340 assertions covering partial
  enqueue, failed start, allocation failure, timeout/interruption, late
  access, cleanup and a subsequent operation. These use kernel shims;
  they do not prove real kernel concurrency.
- An ARM64 KASAN guest with a fault-injecting QEMU virtio-sound backend
  reproduced PCM-page and PCM_INFO-payload use-after-free on the tree
  through patch 3. Two runs of each case with the complete series had no
  KASAN or other kernel diagnostics and passed subsequent capture and
  playback checks. A separate drain-timeout/reset/rebind run also passed.

The backend deliberately holds a capture descriptor past RELEASE, or a
PCM_INFO response past the caller's timeout, then accesses the original
scatterlist. A test-only guest observer reads the referenced storage so
KASAN can check it; external DMA alone is not instrumented by KASAN. The
reset test checks that the backend's delayed callback does not run after
reset. It does not independently inspect guest memory after reset.

These are local fault-injection results, using QEMU's none audio backend.
TLV and enum payloads have source-shim coverage only. No physical audio,
Apple VZ or Docker Desktop microphone validation is claimed for this
series. VZ-specific completion compatibility changes are excluded. The
playback ACK callback's inability to propagate enqueue errors is also
outside this series. No new userspace ABI or feature is introduced.

Reproducer source and instructions:
https://github.com/whatisyourname0/virtio-snd-reproducer/tree/8e6ef41bb9c102af4f9d9809bc9fa9bd6243fb02

The source-only package includes the host harnesses, QEMU fault-injection
patch, guest observer and build/run scripts. The 340 host assertions were
rerun on macOS arm64 and Debian Linux aarch64. The portable QEMU validators
passed PCM, PCM_INFO and reset checks using existing build artifacts.
A newly generated initramfs also passed the PCM and recovery checks.
The complete QEMU/kernel source rebuild recipe has not been run end to
end from this package; that boundary is recorded in its README.

AI assistance (Codex, including GPT-6 Luna Max) was used for code analysis,
implementation, test fixtures, validation and patch text. The work was
prompted to isolate generic virtio-snd lifetime and error fixes from
VZ-specific behavior, reproduce the failures, and check recovery. Each
patch carries Assisted-by: LLM.

Joonho Lee (5):
  ALSA: virtio: Notify queued PCM messages after send errors
  ALSA: virtio: Recover PCM state after a failed start
  ALSA: virtio: Retain control requests through device reset
  ALSA: virtio: Retain PCM DMA pages until messages are freed
  ALSA: virtio: Keep control payloads in refcounted messages

 sound/virtio/virtio_card.c    |   6 +-
 sound/virtio/virtio_card.h    |   2 +
 sound/virtio/virtio_ctl_msg.c | 170 ++++++++++++++++++++++++++++------
 sound/virtio/virtio_ctl_msg.h |  20 +++-
 sound/virtio/virtio_kctl.c    | 101 ++++++++++----------
 sound/virtio/virtio_pcm.h     |   5 +
 sound/virtio/virtio_pcm_msg.c | 124 +++++++++++++++++++++++--
 sound/virtio/virtio_pcm_ops.c |  65 +++++++++++--
 8 files changed, 398 insertions(+), 95 deletions(-)


base-commit: 8d70e364e866bdfca81eee24d0688fac9472cfb9
-- 
2.54.0 (Apple Git-157)

[PATCH v1 1/5] ALSA: virtio: Notify queued PCM messages after send errors

[Joonho Lee]

virtsnd_pcm_msg_send() advances a message length before adding it to
the virtqueue. If a later add fails, the error path returns without
restoring that length or notifying the device about earlier messages
added by the same call. The device can therefore leave successfully
queued messages unprocessed, while the failed message appears
already submitted.

Restore the failed message's length increment and notify the device
about any earlier messages added by this call before returning the
error. Keep the polling feature path unchanged.

Fixes: fe981e67568c ("ALSA: virtio: use ack callback")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Validation: source-extracted ASan/UBSan partial-enqueue and polling
checks; included in the W=1 builds through patches 3 and 5. This commit
was not separately kernel-built.

 sound/virtio/virtio_pcm_msg.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index 72103f606716..5371f4985fd0 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -260,6 +260,18 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
 				dev_err(&vdev->dev,
 					"SID %u: failed to send I/O message\n",
 					vss->sid);
+				msg->length -= n;
+
+				/*
+				 * Do not leave earlier messages queued without notifying
+				 * the device when a later message could not be added.
+				 */
+				if (msg_count != vss->msg_count &&
+				    !(vss->features &
+				      (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) &&
+				    virtqueue_kick_prepare(vqueue))
+					virtqueue_notify(vqueue);
+
 				return rc;
 			}
 
-- 
2.54.0 (Apple Git-157)

[PATCH v1 2/5] ALSA: virtio: Recover PCM state after a failed start

[Joonho Lee]

Capture trigger queues I/O messages before sending PCM_START. If
allocation of the START control message fails, trigger returns an
error with capture I/O messages potentially pending, but without
marking the stream stopped or arming sync_stop. If sending START
fails, trigger also leaves transfers enabled. ALSA does not run the
action undo callback after this trigger failure, so the normal STOP
path cannot be relied on to drain those requests. Retrying START can
then submit the same capture messages again.

Disable transfers and mark the stream stopped after a failed start,
and arm runtime->stop_operating so ALSA invokes sync_stop. Make
sync_stop send RELEASE when the stream is stopped or has pending
I/O, and keep the stream stopped when RELEASE or queue draining
fails. Reject prepare, hw_params, and START retries until a later
sync_stop resolves the outstanding state.

Always cancel elapsed-period work in sync_stop, even when no RELEASE
is needed.

Fixes: da76e9f3e43a ("ALSA: virtio: PCM substream operators")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Dependency: patch 1 notifies capture messages successfully queued before
a later enqueue fails, so this error path can drain them via sync_stop.

Validation: source-extracted ASan/UBSan START allocation/send failures,
retry gating and sync_stop recovery; included in the W=1 builds through
patches 3 and 5. This commit was not separately kernel-built.

 sound/virtio/virtio_pcm_ops.c | 63 +++++++++++++++++++++++++++++------
 1 file changed, 53 insertions(+), 10 deletions(-)

diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
index 1091100a4c98..f2362f0dd80a 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -96,6 +96,15 @@ static const struct virtsnd_a2v_rate g_a2v_rate_map[] = {
 
 static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream);
 
+static void virtsnd_pcm_start_failed(struct virtio_pcm_substream *vss)
+{
+	scoped_guard(spinlock_irqsave, &vss->lock) {
+		vss->xfer_enabled = false;
+		vss->stopped = true;
+	}
+	vss->substream->runtime->stop_operating = true;
+}
+
 /**
  * virtsnd_pcm_open() - Open the PCM substream.
  * @substream: Kernel ALSA substream.
@@ -115,7 +124,7 @@ static int virtsnd_pcm_open(struct snd_pcm_substream *substream)
 	snd_pcm_hw_constraint_integer(substream->runtime,
 				      SNDRV_PCM_HW_PARAM_PERIODS);
 
-	vss->stopped = !!virtsnd_pcm_msg_pending_num(vss);
+	vss->stopped |= !!virtsnd_pcm_msg_pending_num(vss);
 	vss->suspended = false;
 
 	/*
@@ -224,6 +233,12 @@ static int virtsnd_pcm_hw_params(struct snd_pcm_substream *substream,
 			vss->sid);
 		return -EBADFD;
 	}
+	if (vss->stopped) {
+		dev_err(&vdev->dev, "SID %u: stream release failed\n",
+			vss->sid);
+		substream->runtime->stop_operating = true;
+		return -EBADFD;
+	}
 
 	rc = virtsnd_pcm_dev_set_params(vss, params_buffer_bytes(hw_params),
 					params_period_bytes(hw_params),
@@ -274,6 +289,13 @@ static int virtsnd_pcm_prepare(struct snd_pcm_substream *substream)
 	struct virtio_device *vdev = vss->snd->vdev;
 	struct virtio_snd_msg *msg;
 
+	if (vss->stopped) {
+		dev_err(&vdev->dev, "SID %u: stream release failed\n",
+			vss->sid);
+		substream->runtime->stop_operating = true;
+		return -EBADFD;
+	}
+
 	if (!vss->suspended) {
 		if (virtsnd_pcm_msg_pending_num(vss)) {
 			dev_err(&vdev->dev, "SID %u: invalid I/O queue state\n",
@@ -338,23 +360,32 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
 
 		scoped_guard(spinlock_irqsave, &queue->lock) {
 			guard(spinlock)(&vss->lock);
+			if (vss->stopped) {
+				substream->runtime->stop_operating = true;
+				return -EBADFD;
+			}
 			if (vss->direction == SNDRV_PCM_STREAM_CAPTURE)
 				rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes);
-			if (rc)
+			if (rc) {
+				vss->stopped = true;
+				substream->runtime->stop_operating = true;
 				return rc;
+			}
 			vss->xfer_enabled = true;
 		}
 
 		msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_START,
 						GFP_KERNEL);
 		if (!msg) {
-			guard(spinlock_irqsave)(&vss->lock);
-			vss->xfer_enabled = false;
-
+			virtsnd_pcm_start_failed(vss);
 			return -ENOMEM;
 		}
 
-		return virtsnd_ctl_msg_send_sync(snd, msg);
+		rc = virtsnd_ctl_msg_send_sync(snd, msg);
+		if (rc)
+			virtsnd_pcm_start_failed(vss);
+
+		return rc;
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		vss->suspended = true;
 		fallthrough;
@@ -393,21 +424,32 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
 	struct virtio_snd *snd = vss->snd;
 	struct virtio_snd_msg *msg;
 	unsigned int js = msecs_to_jiffies(virtsnd_msg_timeout_ms);
+	bool need_release;
 	int rc;
 
+	scoped_guard(spinlock_irqsave, &vss->lock) {
+		vss->xfer_enabled = false;
+		need_release = vss->stopped || vss->msg_count;
+	}
 	cancel_work_sync(&vss->elapsed_period);
-
-	if (!vss->stopped)
+	if (!need_release)
 		return 0;
 
+	scoped_guard(spinlock_irqsave, &vss->lock)
+		vss->stopped = true;
+
 	msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_RELEASE,
 					GFP_KERNEL);
-	if (!msg)
+	if (!msg) {
+		substream->runtime->stop_operating = true;
 		return -ENOMEM;
+	}
 
 	rc = virtsnd_ctl_msg_send_sync(snd, msg);
-	if (rc)
+	if (rc) {
+		substream->runtime->stop_operating = true;
 		return rc;
+	}
 
 	/*
 	 * The spec states that upon receipt of the RELEASE command "the device
@@ -421,6 +463,7 @@ static int virtsnd_pcm_sync_stop(struct snd_pcm_substream *substream)
 	if (rc <= 0) {
 		dev_warn(&snd->vdev->dev, "SID %u: failed to flush I/O queue\n",
 			 vss->sid);
+		substream->runtime->stop_operating = true;
 
 		return !rc ? -ETIMEDOUT : rc;
 	}
-- 
2.54.0 (Apple Git-157)

[PATCH v1 3/5] ALSA: virtio: Retain control requests through device reset

[Joonho Lee]

virtsnd_ctl_msg_cancel_all() currently completes and releases
pending messages before the device is reset. A sender can then free
a message while the virtqueue still holds its token, allowing a late
completion to access freed memory. Sending also notifies the device
after releasing the queue lock, so teardown can reset and delete the
queue between enqueue and notify.

Gate new requests and mark pending messages canceled under the queue
lock. Notify the device while still holding that lock, and retain
each queue reference until normal completion or post-reset cleanup.
Normal completion wakes a canceled sender after the device returns
ownership; cleanup wakes remaining senders only after reset and
virtqueue deletion.

Keep the control queue available through snd_card_free() so close-
time STOP and RELEASE commands can still be submitted.

Fixes: 9d45e514da88 ("ALSA: virtio: handling control messages")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Dependency: apply with patch 5 for control payload lifetime protection.
This patch retains the message and its headers; caller-owned payloads
are addressed by patch 5.

Validation: ARM64 W=1 build and source-extracted ASan/UBSan teardown,
late-completion and cancellation tests. The final QEMU/KASAN reset test
covers the complete series, not this commit in isolation.

 sound/virtio/virtio_card.c    |  6 +++-
 sound/virtio/virtio_card.h    |  2 ++
 sound/virtio/virtio_ctl_msg.c | 61 +++++++++++++++++++++++++++++++----
 sound/virtio/virtio_ctl_msg.h |  4 +++
 4 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
index 6f35276416fe..ed884327bb0e 100644
--- a/sound/virtio/virtio_card.c
+++ b/sound/virtio/virtio_card.c
@@ -349,13 +349,15 @@ static void virtsnd_remove(struct virtio_device *vdev)
 	unsigned int i;
 
 	virtsnd_disable_event_vq(snd);
-	virtsnd_ctl_msg_cancel_all(snd);
 
 	if (snd->card)
 		snd_card_free(snd->card);
 
+	virtsnd_ctl_msg_cancel_all(snd);
+
 	virtio_reset_device(vdev);
 	vdev->config->del_vqs(vdev);
+	virtsnd_ctl_msg_cleanup(snd);
 
 	for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) {
 		struct virtio_pcm_substream *vss = &snd->substreams[i];
@@ -385,6 +387,7 @@ static int virtsnd_freeze(struct virtio_device *vdev)
 
 	virtio_reset_device(vdev);
 	vdev->config->del_vqs(vdev);
+	virtsnd_ctl_msg_cleanup(snd);
 
 	for (i = 0; i < snd->nsubstreams; ++i)
 		cancel_work_sync(&snd->substreams[i].elapsed_period);
@@ -412,6 +415,7 @@ static int virtsnd_restore(struct virtio_device *vdev)
 		return rc;
 
 	virtio_device_ready(vdev);
+	virtsnd_ctl_msg_enable(snd);
 
 	virtsnd_enable_event_vq(snd);
 
diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h
index 3ceee4e416fc..bd065bd00d25 100644
--- a/sound/virtio/virtio_card.h
+++ b/sound/virtio/virtio_card.h
@@ -47,6 +47,7 @@ struct virtio_kctl {
  * @queues: Virtqueue wrappers.
  * @card: ALSA sound card.
  * @ctl_msgs: Pending control request list.
+ * @ctl_msgs_blocked: Control requests are blocked during teardown or suspend.
  * @event_msgs: Device events.
  * @pcm_list: VirtIO PCM device list.
  * @jacks: VirtIO jacks.
@@ -64,6 +65,7 @@ struct virtio_snd {
 	struct virtio_snd_queue queues[VIRTIO_SND_VQ_MAX];
 	struct snd_card *card;
 	struct list_head ctl_msgs;
+	bool ctl_msgs_blocked;
 	struct virtio_snd_event *event_msgs;
 	struct list_head pcm_list;
 	struct virtio_jack *jacks;
diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
index 6433c870f271..3ad1e1e1b205 100644
--- a/sound/virtio/virtio_ctl_msg.c
+++ b/sound/virtio/virtio_ctl_msg.c
@@ -15,6 +15,7 @@
  * @list: Pending message list entry.
  * @notify: Request completed notification.
  * @ref_count: Reference count used to manage a message lifetime.
+ * @cancelled: Request canceled during device teardown.
  */
 struct virtio_snd_msg {
 	struct scatterlist sg_request;
@@ -22,6 +23,7 @@ struct virtio_snd_msg {
 	struct list_head list;
 	struct completion notify;
 	refcount_t ref_count;
+	bool cancelled;
 };
 
 /**
@@ -100,7 +102,7 @@ struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
 
 	INIT_LIST_HEAD(&msg->list);
 	init_completion(&msg->notify);
-	/* This reference is dropped in virtsnd_ctl_msg_complete(). */
+	/* The caller owns this reference; send adds a queue reference. */
 	refcount_set(&msg->ref_count, 1);
 
 	return msg;
@@ -134,6 +136,7 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
 	int rc;
 
 	virtsnd_ctl_msg_ref(msg);
+	/* This reference stays with the queue until completion or teardown. */
 
 	/* Set the default status in case the message was canceled. */
 	response->code = cpu_to_le32(VIRTIO_SND_S_IO_ERR);
@@ -147,12 +150,17 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
 		psgs[nouts + nins++] = in_sgs;
 
 	scoped_guard(spinlock_irqsave, &queue->lock) {
-		rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins, msg,
-				       GFP_ATOMIC);
+		if (snd->ctl_msgs_blocked)
+			rc = -ENODEV;
+		else
+			rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins,
+					       msg, GFP_ATOMIC);
 		if (!rc) {
 			notify = virtqueue_kick_prepare(queue->vqueue);
 
 			list_add_tail(&msg->list, &snd->ctl_msgs);
+			if (notify)
+				virtqueue_notify(queue->vqueue);
 		}
 	}
 
@@ -169,9 +177,6 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
 		goto on_exit;
 	}
 
-	if (notify)
-		virtqueue_notify(queue->vqueue);
-
 	if (nowait)
 		goto on_exit;
 
@@ -187,6 +192,11 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
 		goto on_exit;
 	}
 
+	if (READ_ONCE(msg->cancelled)) {
+		rc = -ENODEV;
+		goto on_exit;
+	}
+
 	switch (le32_to_cpu(response->code)) {
 	case VIRTIO_SND_S_OK:
 		rc = 0;
@@ -227,9 +237,32 @@ void virtsnd_ctl_msg_complete(struct virtio_snd_msg *msg)
  * virtsnd_ctl_msg_cancel_all() - Cancel all pending control messages.
  * @snd: VirtIO sound device.
  *
- * Context: Any context.
+ * Context: Any context. Blocks new senders and marks pending requests canceled.
+ *          Existing senders are woken when their request completes or after
+ *          the device has been reset.
  */
 void virtsnd_ctl_msg_cancel_all(struct virtio_snd *snd)
+{
+	struct virtio_snd_queue *queue = virtsnd_control_queue(snd);
+	struct virtio_snd_msg *msg;
+
+	guard(spinlock_irqsave)(&queue->lock);
+	snd->ctl_msgs_blocked = true;
+	list_for_each_entry(msg, &snd->ctl_msgs, list) {
+		WRITE_ONCE(msg->cancelled, true);
+	}
+}
+
+/**
+ * virtsnd_ctl_msg_cleanup() - Release messages left after queue teardown.
+ * @snd: VirtIO sound device.
+ *
+ * This must be called after virtio_reset_device() and del_vqs(), when no queue
+ * callback can still access a message.
+ *
+ * Context: Any context.
+ */
+void virtsnd_ctl_msg_cleanup(struct virtio_snd *snd)
 {
 	struct virtio_snd_queue *queue = virtsnd_control_queue(snd);
 
@@ -243,6 +276,20 @@ void virtsnd_ctl_msg_cancel_all(struct virtio_snd *snd)
 	}
 }
 
+/**
+ * virtsnd_ctl_msg_enable() - Allow control requests after queue restoration.
+ * @snd: VirtIO sound device.
+ *
+ * Context: Any context.
+ */
+void virtsnd_ctl_msg_enable(struct virtio_snd *snd)
+{
+	struct virtio_snd_queue *queue = virtsnd_control_queue(snd);
+
+	guard(spinlock_irqsave)(&queue->lock);
+	snd->ctl_msgs_blocked = false;
+}
+
 /**
  * virtsnd_ctl_query_info() - Query the item configuration from the device.
  * @snd: VirtIO sound device.
diff --git a/sound/virtio/virtio_ctl_msg.h b/sound/virtio/virtio_ctl_msg.h
index 7f4db044f28e..d917435409d0 100644
--- a/sound/virtio/virtio_ctl_msg.h
+++ b/sound/virtio/virtio_ctl_msg.h
@@ -68,6 +68,10 @@ static inline int virtsnd_ctl_msg_send_async(struct virtio_snd *snd,
 
 void virtsnd_ctl_msg_cancel_all(struct virtio_snd *snd);
 
+void virtsnd_ctl_msg_cleanup(struct virtio_snd *snd);
+
+void virtsnd_ctl_msg_enable(struct virtio_snd *snd);
+
 void virtsnd_ctl_msg_complete(struct virtio_snd_msg *msg);
 
 int virtsnd_ctl_query_info(struct virtio_snd *snd, int command, int start_id,
-- 
2.54.0 (Apple Git-157)

[PATCH v1 4/5] ALSA: virtio: Retain PCM DMA pages until messages are freed

[Joonho Lee]

PCM I/O scatterlists point directly into the ALSA runtime's vmalloc
buffer. If RELEASE or queue draining times out, ALSA still frees that
managed buffer during hw_free/close while the device may retain the
queued descriptors. Keeping only the virtio_pcm_msg objects alive does
not keep their backing pages allocated.

Take a reference to each page backing the PCM buffer before building the
I/O scatterlists, and release those references only when the messages
are freed after device ownership ends. This keeps late device DMA away
from pages that the allocator could otherwise reuse after ALSA releases
the vmalloc mapping.

Fixes: f40a28679e0b ("ALSA: virtio: handling control and I/O messages for the PCM device")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Validation: separate ARM64 W=1 module build at this commit. With the
full series, delayed capture completion passed two KASAN/QEMU runs;
the tree through patch 3 reproduced freed-page reads. Playback late
access and allocation-failure paths also have ASan/UBSan shim tests.

 sound/virtio/virtio_pcm.h     |   5 ++
 sound/virtio/virtio_pcm_msg.c | 110 ++++++++++++++++++++++++++++++++--
 sound/virtio/virtio_pcm_ops.c |   2 +
 3 files changed, 112 insertions(+), 5 deletions(-)

diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h
index 8837dfc12828..56ac2769b05b 100644
--- a/sound/virtio/virtio_pcm.h
+++ b/sound/virtio/virtio_pcm.h
@@ -13,6 +13,7 @@
 
 struct virtio_pcm;
 struct virtio_pcm_msg;
+struct page;
 
 /**
  * struct virtio_pcm_substream - VirtIO PCM substream.
@@ -38,6 +39,8 @@ struct virtio_pcm_msg;
  * @msgs: Allocated I/O messages.
  * @nmsgs: Number of allocated I/O messages.
  * @msg_count: Number of pending I/O messages in the virtqueue.
+ * @dma_pages: Pages backing DMA messages, retained until messages are freed.
+ * @ndma_pages: Number of retained pages.
  * @msg_empty: Notify when msg_count is zero.
  */
 struct virtio_pcm_substream {
@@ -60,6 +63,8 @@ struct virtio_pcm_substream {
 	struct virtio_pcm_msg **msgs;
 	unsigned int nmsgs;
 	unsigned int msg_count;
+	struct page **dma_pages;
+	unsigned int ndma_pages;
 	wait_queue_head_t msg_empty;
 };
 
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index 5371f4985fd0..46bf4df65974 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -3,6 +3,9 @@
  * virtio-snd: Virtio sound device
  * Copyright (C) 2021 OpenSynergy GmbH
  */
+#include <linux/limits.h>
+#include <linux/mm.h>
+#include <linux/overflow.h>
 #include <sound/pcm_params.h>
 
 #include "virtio_card.h"
@@ -75,6 +78,73 @@ static int virtsnd_pcm_sg_num(u8 *data, unsigned int length)
 	return num;
 }
 
+/**
+ * virtsnd_pcm_dma_pages_put() - Release retained PCM buffer pages.
+ * @vss: VirtIO PCM substream.
+ *
+ * Context: Process context.
+ */
+static void virtsnd_pcm_dma_pages_put(struct virtio_pcm_substream *vss)
+{
+	while (vss->ndma_pages)
+		put_page(vss->dma_pages[--vss->ndma_pages]);
+	kfree(vss->dma_pages);
+	vss->dma_pages = NULL;
+}
+
+/**
+ * virtsnd_pcm_dma_pages_get() - Retain the pages backing the PCM buffer.
+ * @vss: VirtIO PCM substream.
+ * @data: PCM buffer address.
+ * @length: PCM buffer size.
+ *
+ * Keep the backing pages alive while virtqueue descriptors can still refer to
+ * them, even if ALSA releases the vmalloc mapping after a failed drain.
+ *
+ * Context: Process context.
+ * Return: 0 on success, -errno on failure.
+ */
+static int virtsnd_pcm_dma_pages_get(struct virtio_pcm_substream *vss,
+				     u8 *data, size_t length)
+{
+	size_t i;
+	size_t npages;
+	size_t span;
+
+	if (!data || !length || !is_vmalloc_addr(data))
+		return -EINVAL;
+	if (vss->dma_pages || vss->ndma_pages)
+		return -EBUSY;
+
+	if (check_add_overflow(length, (size_t)offset_in_page(data), &span))
+		return -EOVERFLOW;
+
+	npages = span / PAGE_SIZE + !!(span % PAGE_SIZE);
+	if (npages > UINT_MAX)
+		return -EOVERFLOW;
+
+	vss->dma_pages = kcalloc(npages, sizeof(*vss->dma_pages), GFP_KERNEL);
+	if (!vss->dma_pages)
+		return -ENOMEM;
+
+	for (i = 0; i < npages; i++) {
+		struct page *page = vmalloc_to_page(data + i * PAGE_SIZE);
+
+		if (!page)
+			goto err_put_pages;
+
+		get_page(page);
+		vss->dma_pages[vss->ndma_pages++] = page;
+	}
+
+	return 0;
+
+err_put_pages:
+	virtsnd_pcm_dma_pages_put(vss);
+
+	return -EINVAL;
+}
+
 /**
  * virtsnd_pcm_sg_from() - Build sg-list from vmalloc'ed buffer.
  * @sgs: Preallocated sg-list to populate.
@@ -133,22 +203,42 @@ int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
 			  unsigned int periods, unsigned int period_bytes)
 {
 	struct snd_pcm_runtime *runtime = vss->substream->runtime;
+	size_t buffer_bytes;
 	unsigned int i;
+	int rc;
+
+	if (vss->msgs || vss->nmsgs || vss->dma_pages || vss->ndma_pages)
+		return -EBUSY;
+
+	if (check_mul_overflow((size_t)periods, (size_t)period_bytes,
+			       &buffer_bytes))
+		return -EOVERFLOW;
+
+	if (!runtime->dma_area || buffer_bytes > runtime->dma_bytes)
+		return -EINVAL;
+
+	rc = virtsnd_pcm_dma_pages_get(vss, runtime->dma_area, buffer_bytes);
+	if (rc)
+		return rc;
 
 	vss->msgs = kzalloc_objs(*vss->msgs, periods);
-	if (!vss->msgs)
-		return -ENOMEM;
+	if (!vss->msgs) {
+		rc = -ENOMEM;
+		goto err_free_pages;
+	}
 
 	vss->nmsgs = periods;
 
 	for (i = 0; i < periods; ++i) {
-		u8 *data = runtime->dma_area + period_bytes * i;
+		u8 *data = runtime->dma_area + (size_t)period_bytes * i;
 		int sg_num = virtsnd_pcm_sg_num(data, period_bytes);
 		struct virtio_pcm_msg *msg;
 
 		msg = kzalloc_flex(*msg, sgs, sg_num + 2);
-		if (!msg)
-			return -ENOMEM;
+		if (!msg) {
+			rc = -ENOMEM;
+			goto err_free_msgs;
+		}
 
 		msg->substream = vss;
 		sg_init_one(&msg->sgs[PCM_MSG_SG_XFER], &msg->xfer,
@@ -162,6 +252,15 @@ int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
 	}
 
 	return 0;
+
+err_free_msgs:
+	virtsnd_pcm_msg_free(vss);
+	return rc;
+
+err_free_pages:
+	virtsnd_pcm_dma_pages_put(vss);
+
+	return rc;
 }
 
 /**
@@ -177,6 +276,7 @@ void virtsnd_pcm_msg_free(struct virtio_pcm_substream *vss)
 	for (i = 0; vss->msgs && i < vss->nmsgs; ++i)
 		kfree(vss->msgs[i]);
 	kfree(vss->msgs);
+	virtsnd_pcm_dma_pages_put(vss);
 
 	vss->msgs = NULL;
 	vss->nmsgs = 0;
diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
index f2362f0dd80a..82f2d2f9dd64 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -25,6 +25,8 @@
  *   signal or due to a timeout. In this case, the device can still access
  *   messages even after calling ops->hw_free(). It can also issue an interrupt,
  *   and the interrupt handler will also try to access message structures.
+ *   The messages retain references to the PCM buffer pages until they are
+ *   freed, so late DMA does not target pages released by ALSA.
  *
  *   Therefore, freeing of already allocated messages occurs:
  *
-- 
2.54.0 (Apple Git-157)

[PATCH v1 5/5] ALSA: virtio: Keep control payloads in refcounted messages

[Joonho Lee]

Control requests can include scatterlists that point to caller-owned
payloads. A synchronous send can time out while the device still owns
those descriptors, so the caller can free a TLV or other payload before
a late device access.

Allocate request and response payloads with the refcounted control
message, retain separate header and payload scatterlist entries, and
copy caller data into or out of the owned payload while the caller still
holds a message reference. Check payload sizes and allocation arithmetic
before building the scatterlists.

Fixes: 9d45e514da88 ("ALSA: virtio: handling control messages")
Assisted-by: LLM
Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>
---
Dependency: patch 3 retains the queue reference until completion or
post-reset cleanup, which this payload ownership change relies on.

Validation: ARM64 W=1 build; two delayed PCM_INFO KASAN/QEMU runs
with subsequent capture/playback recovery; ASan/UBSan source-shim
tests for query, enum and TLV paths. No real TLV backend was tested.

 sound/virtio/virtio_ctl_msg.c | 109 ++++++++++++++++++++++++++++------
 sound/virtio/virtio_ctl_msg.h |  16 +++--
 sound/virtio/virtio_kctl.c    | 101 ++++++++++++++++---------------
 sound/virtio/virtio_pcm_msg.c |   2 +-
 4 files changed, 156 insertions(+), 72 deletions(-)

diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
index 3ad1e1e1b205..62d4c529922c 100644
--- a/sound/virtio/virtio_ctl_msg.c
+++ b/sound/virtio/virtio_ctl_msg.c
@@ -3,7 +3,9 @@
  * virtio-snd: Virtio sound device
  * Copyright (C) 2021 OpenSynergy GmbH
  */
+#include <linux/limits.h>
 #include <linux/moduleparam.h>
+#include <linux/overflow.h>
 #include <linux/virtio_config.h>
 
 #include "virtio_card.h"
@@ -11,7 +13,11 @@
 /**
  * struct virtio_snd_msg - Control message.
  * @sg_request: Scattergather list containing a device request (header).
+ * @sg_request_data: Scattergather list containing request data.
  * @sg_response: Scattergather list containing a device response (status).
+ * @sg_response_data: Scattergather list containing response data.
+ * @request_data_size: Size of the request data buffer.
+ * @response_data_size: Size of the response data buffer.
  * @list: Pending message list entry.
  * @notify: Request completed notification.
  * @ref_count: Reference count used to manage a message lifetime.
@@ -19,7 +25,11 @@
  */
 struct virtio_snd_msg {
 	struct scatterlist sg_request;
+	struct scatterlist sg_request_data;
 	struct scatterlist sg_response;
+	struct scatterlist sg_response_data;
+	size_t request_data_size;
+	size_t response_data_size;
 	struct list_head list;
 	struct completion notify;
 	refcount_t ref_count;
@@ -73,10 +83,34 @@ void *virtsnd_ctl_msg_response(struct virtio_snd_msg *msg)
 	return sg_virt(&msg->sg_response);
 }
 
+/**
+ * virtsnd_ctl_msg_request_data() - Get the request data buffer.
+ * @msg: Control message.
+ *
+ * Context: Any context.
+ */
+void *virtsnd_ctl_msg_request_data(struct virtio_snd_msg *msg)
+{
+	return msg->request_data_size ? sg_virt(&msg->sg_request_data) : NULL;
+}
+
+/**
+ * virtsnd_ctl_msg_response_data() - Get the response data buffer.
+ * @msg: Control message.
+ *
+ * Context: Any context.
+ */
+void *virtsnd_ctl_msg_response_data(struct virtio_snd_msg *msg)
+{
+	return msg->response_data_size ? sg_virt(&msg->sg_response_data) : NULL;
+}
+
 /**
  * virtsnd_ctl_msg_alloc() - Allocate and initialize a control message.
  * @request_size: Size of request header.
  * @response_size: Size of response header.
+ * @request_data_size: Size of request data buffer.
+ * @response_data_size: Size of response data buffer.
  * @gfp: Kernel flags for memory allocation.
  *
  * The message will be automatically freed when the ref_count value is 0.
@@ -85,20 +119,43 @@ void *virtsnd_ctl_msg_response(struct virtio_snd_msg *msg)
  * Return: Allocated message on success, NULL on failure.
  */
 struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
-					     size_t response_size, gfp_t gfp)
+					     size_t response_size,
+					     size_t request_data_size,
+					     size_t response_data_size,
+					     gfp_t gfp)
 {
 	struct virtio_snd_msg *msg;
-
-	if (!request_size || !response_size)
+	size_t total_size = sizeof(*msg);
+	u8 *data;
+
+	if (!request_size || !response_size || request_size > UINT_MAX ||
+	    response_size > UINT_MAX || request_data_size > UINT_MAX ||
+	    response_data_size > UINT_MAX ||
+	    check_add_overflow(total_size, request_size, &total_size) ||
+	    check_add_overflow(total_size, response_size, &total_size) ||
+	    check_add_overflow(total_size, request_data_size, &total_size) ||
+	    check_add_overflow(total_size, response_data_size, &total_size))
 		return NULL;
 
-	msg = kzalloc(sizeof(*msg) + request_size + response_size, gfp);
+	msg = kzalloc(total_size, gfp);
 	if (!msg)
 		return NULL;
 
-	sg_init_one(&msg->sg_request, (u8 *)msg + sizeof(*msg), request_size);
-	sg_init_one(&msg->sg_response, (u8 *)msg + sizeof(*msg) + request_size,
-		    response_size);
+	data = (u8 *)msg + sizeof(*msg);
+	sg_init_one(&msg->sg_request, data, (unsigned int)request_size);
+	data += request_size;
+	sg_init_one(&msg->sg_response, data, (unsigned int)response_size);
+	data += response_size;
+	msg->request_data_size = request_data_size;
+	msg->response_data_size = response_data_size;
+	if (request_data_size) {
+		sg_init_one(&msg->sg_request_data, data,
+			    (unsigned int)request_data_size);
+		data += request_data_size;
+	}
+	if (response_data_size)
+		sg_init_one(&msg->sg_response_data, data,
+			    (unsigned int)response_data_size);
 
 	INIT_LIST_HEAD(&msg->list);
 	init_completion(&msg->notify);
@@ -112,8 +169,6 @@ struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
  * virtsnd_ctl_msg_send() - Send a control message.
  * @snd: VirtIO sound device.
  * @msg: Control message.
- * @out_sgs: Additional sg-list to attach to the request header (may be NULL).
- * @in_sgs: Additional sg-list to attach to the response header (may be NULL).
  * @nowait: Flag indicating whether to wait for completion.
  *
  * Context: Any context. Takes and releases the control queue spinlock.
@@ -121,8 +176,7 @@ struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
  * Return: 0 on success, -errno on failure.
  */
 int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
-			 struct scatterlist *out_sgs,
-			 struct scatterlist *in_sgs, bool nowait)
+			 bool nowait)
 {
 	struct virtio_device *vdev = snd->vdev;
 	struct virtio_snd_queue *queue = virtsnd_control_queue(snd);
@@ -142,12 +196,12 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
 	response->code = cpu_to_le32(VIRTIO_SND_S_IO_ERR);
 
 	psgs[nouts++] = &msg->sg_request;
-	if (out_sgs)
-		psgs[nouts++] = out_sgs;
+	if (msg->request_data_size)
+		psgs[nouts++] = &msg->sg_request_data;
 
 	psgs[nouts + nins++] = &msg->sg_response;
-	if (in_sgs)
-		psgs[nouts + nins++] = in_sgs;
+	if (msg->response_data_size)
+		psgs[nouts + nins++] = &msg->sg_response_data;
 
 	scoped_guard(spinlock_irqsave, &queue->lock) {
 		if (snd->ctl_msgs_blocked)
@@ -307,10 +361,23 @@ int virtsnd_ctl_query_info(struct virtio_snd *snd, int command, int start_id,
 {
 	struct virtio_snd_msg *msg;
 	struct virtio_snd_query_info *query;
-	struct scatterlist sg;
+	size_t info_size;
+	int rc;
+
+	if (count < 0)
+		return -EINVAL;
+
+	if (size > UINT_MAX ||
+	    check_mul_overflow((size_t)count, size, &info_size) ||
+	    info_size > UINT_MAX)
+		return -EOVERFLOW;
+
+	if (info_size && !info)
+		return -EINVAL;
 
 	msg = virtsnd_ctl_msg_alloc(sizeof(*query),
-				    sizeof(struct virtio_snd_hdr), GFP_KERNEL);
+				    sizeof(struct virtio_snd_hdr), 0, info_size,
+				    GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
 
@@ -320,9 +387,13 @@ int virtsnd_ctl_query_info(struct virtio_snd *snd, int command, int start_id,
 	query->count = cpu_to_le32(count);
 	query->size = cpu_to_le32(size);
 
-	sg_init_one(&sg, info, count * size);
+	virtsnd_ctl_msg_ref(msg);
+	rc = virtsnd_ctl_msg_send(snd, msg, false);
+	if (!rc && info_size)
+		memcpy(info, virtsnd_ctl_msg_response_data(msg), info_size);
+	virtsnd_ctl_msg_unref(msg);
 
-	return virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
+	return rc;
 }
 
 /**
diff --git a/sound/virtio/virtio_ctl_msg.h b/sound/virtio/virtio_ctl_msg.h
index d917435409d0..834ddeb82b90 100644
--- a/sound/virtio/virtio_ctl_msg.h
+++ b/sound/virtio/virtio_ctl_msg.h
@@ -21,11 +21,17 @@ void *virtsnd_ctl_msg_request(struct virtio_snd_msg *msg);
 void *virtsnd_ctl_msg_response(struct virtio_snd_msg *msg);
 
 struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
-					     size_t response_size, gfp_t gfp);
+					     size_t response_size,
+					     size_t request_data_size,
+					     size_t response_data_size,
+					     gfp_t gfp);
+
+void *virtsnd_ctl_msg_request_data(struct virtio_snd_msg *msg);
+
+void *virtsnd_ctl_msg_response_data(struct virtio_snd_msg *msg);
 
 int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
-			 struct scatterlist *out_sgs,
-			 struct scatterlist *in_sgs, bool nowait);
+			 bool nowait);
 
 /**
  * virtsnd_ctl_msg_send_sync() - Simplified sending of synchronous message.
@@ -49,7 +55,7 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
 static inline int virtsnd_ctl_msg_send_sync(struct virtio_snd *snd,
 					    struct virtio_snd_msg *msg)
 {
-	return virtsnd_ctl_msg_send(snd, msg, NULL, NULL, false);
+	return virtsnd_ctl_msg_send(snd, msg, false);
 }
 
 /**
@@ -63,7 +69,7 @@ static inline int virtsnd_ctl_msg_send_sync(struct virtio_snd *snd,
 static inline int virtsnd_ctl_msg_send_async(struct virtio_snd *snd,
 					     struct virtio_snd_msg *msg)
 {
-	return virtsnd_ctl_msg_send(snd, msg, NULL, NULL, true);
+	return virtsnd_ctl_msg_send(snd, msg, true);
 }
 
 void virtsnd_ctl_msg_cancel_all(struct virtio_snd *snd);
diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c
index 45f7b6a5b308..44eb59d7d093 100644
--- a/sound/virtio/virtio_kctl.c
+++ b/sound/virtio/virtio_kctl.c
@@ -3,8 +3,10 @@
  * virtio-snd: Virtio sound device
  * Copyright (C) 2022 OpenSynergy GmbH
  */
-#include <sound/control.h>
+#include <linux/limits.h>
+#include <linux/overflow.h>
 #include <linux/virtio_config.h>
+#include <sound/control.h>
 
 #include "virtio_card.h"
 
@@ -161,7 +163,8 @@ static int virtsnd_kctl_get(struct snd_kcontrol *kcontrol,
 	unsigned int i;
 	int rc;
 
-	msg = virtsnd_ctl_msg_alloc(request_size, response_size, GFP_KERNEL);
+	msg = virtsnd_ctl_msg_alloc(request_size, response_size, 0, 0,
+				    GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
 
@@ -233,7 +236,8 @@ static int virtsnd_kctl_put(struct snd_kcontrol *kcontrol,
 	size_t response_size = sizeof(struct virtio_snd_hdr);
 	unsigned int i;
 
-	msg = virtsnd_ctl_msg_alloc(request_size, response_size, GFP_KERNEL);
+	msg = virtsnd_ctl_msg_alloc(request_size, response_size, 0, 0,
+				    GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
 
@@ -288,23 +292,28 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
 	struct virtio_snd *snd = snd_kcontrol_chip(kcontrol);
 	struct virtio_snd_msg *msg;
 	struct virtio_snd_ctl_hdr *hdr;
-	unsigned int *tlv;
-	struct scatterlist sg;
+	size_t request_data_size = 0;
+	size_t response_data_size = 0;
 	int rc;
 
+	switch (op_flag) {
+	case SNDRV_CTL_TLV_OP_READ:
+		response_data_size = size;
+		break;
+	case SNDRV_CTL_TLV_OP_WRITE:
+	case SNDRV_CTL_TLV_OP_CMD:
+		request_data_size = size;
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	msg = virtsnd_ctl_msg_alloc(sizeof(*hdr), sizeof(struct virtio_snd_hdr),
+				    request_data_size, response_data_size,
 				    GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
 
-	tlv = kzalloc(size, GFP_KERNEL);
-	if (!tlv) {
-		rc = -ENOMEM;
-		goto on_msg_unref;
-	}
-
-	sg_init_one(&sg, tlv, size);
-
 	hdr = virtsnd_ctl_msg_request(msg);
 	hdr->control_id = cpu_to_le32(kcontrol->private_value);
 
@@ -312,13 +321,15 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
 	case SNDRV_CTL_TLV_OP_READ:
 		hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_TLV_READ);
 
-		rc = virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
-		if (!rc) {
-			if (copy_to_user(utlv, tlv, size))
-				rc = -EFAULT;
-		}
+		virtsnd_ctl_msg_ref(msg);
+		rc = virtsnd_ctl_msg_send(snd, msg, false);
+		if (!rc && size &&
+		    copy_to_user(utlv, virtsnd_ctl_msg_response_data(msg),
+				 size))
+			rc = -EFAULT;
+		virtsnd_ctl_msg_unref(msg);
 
-		break;
+		return rc;
 	case SNDRV_CTL_TLV_OP_WRITE:
 	case SNDRV_CTL_TLV_OP_CMD:
 		if (op_flag == SNDRV_CTL_TLV_OP_WRITE)
@@ -327,28 +338,16 @@ static int virtsnd_kctl_tlv_op(struct snd_kcontrol *kcontrol, int op_flag,
 			hdr->hdr.code =
 				cpu_to_le32(VIRTIO_SND_R_CTL_TLV_COMMAND);
 
-		if (copy_from_user(tlv, utlv, size)) {
-			rc = -EFAULT;
-			goto on_msg_unref;
-		} else {
-			rc = virtsnd_ctl_msg_send(snd, msg, &sg, NULL, false);
+		if (size && copy_from_user(virtsnd_ctl_msg_request_data(msg), utlv,
+					   size)) {
+			virtsnd_ctl_msg_unref(msg);
+			return -EFAULT;
 		}
 
-		break;
-	default:
-		rc = -EINVAL;
-		/* We never get here - we listed all values for op_flag */
-		WARN_ON(1);
-		goto on_msg_unref;
+		return virtsnd_ctl_msg_send(snd, msg, false);
 	}
-	kfree(tlv);
-	return rc;
-
-on_msg_unref:
-	virtsnd_ctl_msg_unref(msg);
-	kfree(tlv);
 
-	return rc;
+	return -EINVAL;
 }
 
 /**
@@ -369,27 +368,35 @@ static int virtsnd_kctl_get_enum_items(struct virtio_snd *snd, unsigned int cid)
 	struct virtio_snd_msg *msg;
 	struct virtio_snd_ctl_hdr *hdr;
 	unsigned int n = le32_to_cpu(kinfo->value.enumerated.items);
-	struct scatterlist sg;
+	size_t items_size;
+	int rc;
 
-	msg = virtsnd_ctl_msg_alloc(sizeof(*hdr),
-				    sizeof(struct virtio_snd_hdr), GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
+	if (check_mul_overflow((size_t)n, sizeof(*kctl->items), &items_size) ||
+	    items_size > UINT_MAX)
+		return -EOVERFLOW;
 
 	kctl->items = devm_kcalloc(&vdev->dev, n, sizeof(*kctl->items),
 				   GFP_KERNEL);
-	if (!kctl->items) {
-		virtsnd_ctl_msg_unref(msg);
+	if (!kctl->items)
 		return -ENOMEM;
-	}
 
-	sg_init_one(&sg, kctl->items, n * sizeof(*kctl->items));
+	msg = virtsnd_ctl_msg_alloc(sizeof(*hdr),
+				    sizeof(struct virtio_snd_hdr), 0, items_size,
+				    GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
 
 	hdr = virtsnd_ctl_msg_request(msg);
 	hdr->hdr.code = cpu_to_le32(VIRTIO_SND_R_CTL_ENUM_ITEMS);
 	hdr->control_id = cpu_to_le32(cid);
 
-	return virtsnd_ctl_msg_send(snd, msg, NULL, &sg, false);
+	virtsnd_ctl_msg_ref(msg);
+	rc = virtsnd_ctl_msg_send(snd, msg, false);
+	if (!rc && items_size)
+		memcpy(kctl->items, virtsnd_ctl_msg_response_data(msg), items_size);
+	virtsnd_ctl_msg_unref(msg);
+
+	return rc;
 }
 
 /**
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index 46bf4df65974..7fb945d0a36a 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -534,7 +534,7 @@ virtsnd_pcm_ctl_msg_alloc(struct virtio_pcm_substream *vss,
 		break;
 	}
 
-	msg = virtsnd_ctl_msg_alloc(request_size, response_size, gfp);
+	msg = virtsnd_ctl_msg_alloc(request_size, response_size, 0, 0, gfp);
 	if (msg) {
 		struct virtio_snd_pcm_hdr *hdr = virtsnd_ctl_msg_request(msg);
 
-- 
2.54.0 (Apple Git-157)

Re: [PATCH v1 4/5] ALSA: virtio: Retain PCM DMA pages until messages are freed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] alsa/virtio: !is_vmalloc_addr check breaks NOMMU support
--

commit 011a5cf5cd03694cab8083a5256df199dd8e906f
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: Retain PCM DMA pages until messages are freed

This patch takes a reference to each page backing the PCM buffer before
building the I/O scatterlists, releasing them only when the messages are
freed. This prevents late device DMA from accessing pages that the
allocator could reuse after ALSA releases the vmalloc mapping.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 5371f4985fd06..46bf4df65974f 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
[ ... ]
> +static int virtsnd_pcm_dma_pages_get(struct virtio_pcm_substream *vss,
> +				     u8 *data, size_t length)
> +{
> +	size_t i;
> +	size_t npages;
> +	size_t span;
> +
> +	if (!data || !length || !is_vmalloc_addr(data))
> +		return -EINVAL;

[Severity: High]
Does this check break NOMMU architecture support?

The VirtIO sound driver uses SNDRV_DMA_TYPE_VMALLOC for its PCM buffers.
On NOMMU architectures, vmalloc() typically falls back to kmalloc(), and
is_vmalloc_addr() unconditionally returns false.

Because of this, the new is_vmalloc_addr() check in
virtsnd_pcm_dma_pages_get() will unconditionally fail with -EINVAL on NOMMU
systems, preventing the allocation of PCM messages and breaking audio
playback and capture.

Prior to this patch, vmalloc_to_page() was called directly without this
check, which safely translates the kmalloc address via virt_to_page() on
NOMMU. Can this validation be adjusted to support NOMMU configurations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791867377.git.mynameisjune111@gmail.com?part=4

Re: [PATCH v1 5/5] ALSA: virtio: Keep control payloads in refcounted messages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] virtio/snd: DMA cacheline sharing for control message payloads
--

commit 332dc283a2bc483bd13699b7e6a008d8c41b3af6
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: Keep control payloads in refcounted messages
    
Control requests can include scatterlists that point to caller-owned
payloads. A synchronous send can time out while the device still owns
those descriptors, so the caller can free a TLV or other payload before
a late device access.

> diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
> index 3ad1e1e1b2054..62d4c529922ca 100644
> --- a/sound/virtio/virtio_ctl_msg.c
> +++ b/sound/virtio/virtio_ctl_msg.c

[ ... ]

> @@ -85,20 +119,43 @@ void *virtsnd_ctl_msg_response(struct virtio_snd_msg *msg)
>   * Return: Allocated message on success, NULL on failure.
>   */
>  struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size,
> -					     size_t response_size, gfp_t gfp)
> +					     size_t response_size,
> +					     size_t request_data_size,
> +					     size_t response_data_size,
> +					     gfp_t gfp)
>  {
>  	struct virtio_snd_msg *msg;
> -
> -	if (!request_size || !response_size)
> +	size_t total_size = sizeof(*msg);
> +	u8 *data;
> +
> +	if (!request_size || !response_size || request_size > UINT_MAX ||
> +	    response_size > UINT_MAX || request_data_size > UINT_MAX ||
> +	    response_data_size > UINT_MAX ||
> +	    check_add_overflow(total_size, request_size, &total_size) ||
> +	    check_add_overflow(total_size, response_size, &total_size) ||
> +	    check_add_overflow(total_size, request_data_size, &total_size) ||
> +	    check_add_overflow(total_size, response_data_size, &total_size))
>  		return NULL;
>  
> -	msg = kzalloc(sizeof(*msg) + request_size + response_size, gfp);
> +	msg = kzalloc(total_size, gfp);
>  	if (!msg)
>  		return NULL;
>  
> -	sg_init_one(&msg->sg_request, (u8 *)msg + sizeof(*msg), request_size);
> -	sg_init_one(&msg->sg_response, (u8 *)msg + sizeof(*msg) + request_size,
> -		    response_size);
> +	data = (u8 *)msg + sizeof(*msg);
> +	sg_init_one(&msg->sg_request, data, (unsigned int)request_size);
> +	data += request_size;
> +	sg_init_one(&msg->sg_response, data, (unsigned int)response_size);
> +	data += response_size;
> +	msg->request_data_size = request_data_size;
> +	msg->response_data_size = response_data_size;
> +	if (request_data_size) {
> +		sg_init_one(&msg->sg_request_data, data,
> +			    (unsigned int)request_data_size);
> +		data += request_data_size;
> +	}
> +	if (response_data_size)
> +		sg_init_one(&msg->sg_response_data, data,
> +			    (unsigned int)response_data_size);

[Severity: High]
Does packing these DMA buffers tightly together after the virtio_snd_msg
struct cause cacheline sharing issues?

Because sizeof(*msg) and the header sizes are not aligned to
ARCH_DMA_MINALIGN, these buffers might share CPU cachelines with each other
and with CPU-accessed fields of virtio_snd_msg (like ref_count and
cancelled).

On non-cache-coherent architectures, mapping a buffer as DMA_FROM_DEVICE
(such as sg_response and sg_response_data) causes the DMA API to invalidate
the shared cacheline upon unmapping so the CPU sees the device's writes.

If the CPU concurrently modifies msg->cancelled or msg->ref_count in that
same cacheline (e.g., upon a timeout), could those writes be silently
discarded by the cache invalidation?

Previously, payloads were allocated separately via kzalloc(size), which
guaranteed safe ARCH_DMA_MINALIGN boundaries. Embedding them inline appears
to introduce a DMA cacheline sharing hazard.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791867377.git.mynameisjune111@gmail.com?part=5

Re: [PATCH v1 1/5] ALSA: virtio: Notify queued PCM messages after send errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] alsa: virtio: state desync on msg send failure
--

commit e0fc1ff7e6c503fbe54480dc784a6aac80eec85b
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: Notify queued PCM messages after send errors

virtsnd_pcm_msg_send() advances a message length before adding it to
the virtqueue. If a later add fails, the error path returns without
restoring that length or notifying the device about earlier messages
added by the same call. The device can therefore leave successfully
queued messages unprocessed, while the failed message appears
already submitted.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 72103f6067169..5371f4985fd06 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c
> @@ -260,6 +260,18 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
>  				dev_err(&vdev->dev,
>  					"SID %u: failed to send I/O message\n",
>  					vss->sid);
> +				msg->length -= n;

[Severity: High]
Does this length rollback desynchronize the virtio message state from the ALSA
indirect layer?

When virtsnd_pcm_msg_send() fails and returns an error, its caller,
virtsnd_pcm_trans_copy(), ignores the return value:

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trans_copy() {
	struct virtio_pcm_substream *vss = snd_pcm_substream_chip(substream);

	virtsnd_pcm_msg_send(vss, rec->sw_data, bytes);
}

Because virtsnd_pcm_trans_copy() returns void, the ALSA indirect layer 
(snd_pcm_indirect_playback_transfer()) assumes success and unconditionally 
advances its software offset pointers. 

Since the ALSA pointer advanced but the virtio message length rolled back, the
two states become out of sync. On the next wrap-around, will the skipped
message artificially reach period_bytes with old/new mixed data and be
submitted to the virtqueue out of order?

Should the driver set substream->runtime->stop_operating = true inside
virtsnd_pcm_msg_send() to properly abort the stream when an I/O message
fails?

> +
> +				/*
> +				 * Do not leave earlier messages queued without notifying
> +				 * the device when a later message could not be added.
> +				 */
> +				if (msg_count != vss->msg_count &&
> +				    !(vss->features &
> +				      (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) &&
> +				    virtqueue_kick_prepare(vqueue))
> +					virtqueue_notify(vqueue);
> +
>  				return rc;
>  			}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791867377.git.mynameisjune111@gmail.com?part=1

Re: [PATCH v1 2/5] ALSA: virtio: Recover PCM state after a failed start

[sashiko-bot]

> Capture trigger queues I/O messages before sending PCM_START. If
> allocation of the START control message fails, trigger returns an
> error with capture I/O messages potentially pending, but without
> marking the stream stopped or arming sync_stop. If sending START
> fails, trigger also leaves transfers enabled. ALSA does not run the
> action undo callback after this trigger failure, so the normal STOP
> path cannot be relied on to drain those requests. Retrying START can
> then submit the same capture messages again.
> 
> Disable transfers and mark the stream stopped after a failed start,
> and arm runtime->stop_operating so ALSA invokes sync_stop. Make
> sync_stop send RELEASE when the stream is stopped or has pending
> I/O, and keep the stream stopped when RELEASE or queue draining
> fails. Reject prepare, hw_params, and START retries until a later
> sync_stop resolves the outstanding state.
> 
> Always cancel elapsed-period work in sync_stop, even when no RELEASE
> is needed.
> 
> Fixes: da76e9f3e43a ("ALSA: virtio: PCM substream operators")
> Assisted-by: LLM
> Signed-off-by: Joonho Lee <mynameisjune111@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791867377.git.mynameisjune111@gmail.com?part=2

Re: [PATCH v1 3/5] ALSA: virtio: Retain control requests through device reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] virtio_sound: DMA mapping leak from uncalled virtqueue_detach_unused_buf()
- [Low] cleanup: mixing goto and scoped_guard in virtsnd_ctl_msg_send()
--

commit 58ca5d3aaf697e12525c1d956e26244a616c7d76
Author: Joonho Lee <mynameisjune111@gmail.com>
ALSA: virtio: Retain control requests through device reset

This commit updates the virtio sound driver to handle control message teardown
by keeping references to messages through device reset. It marks messages
canceled under the queue lock and defers freeing them until the queue has been
deleted.

> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
> index 6f35276416fed..ed884327bb0ef 100644
> --- a/sound/virtio/virtio_card.c
> +++ b/sound/virtio/virtio_card.c
> @@ -349,13 +349,15 @@ static void virtsnd_remove(struct virtio_device *vdev)
>  	unsigned int i;
>  
>  	virtsnd_disable_event_vq(snd);
> -	virtsnd_ctl_msg_cancel_all(snd);
>  
>  	if (snd->card)
>  		snd_card_free(snd->card);
>  
> +	virtsnd_ctl_msg_cancel_all(snd);
> +
>  	virtio_reset_device(vdev);
>  	vdev->config->del_vqs(vdev);
> +	virtsnd_ctl_msg_cleanup(snd);

[Severity: Medium]
Does deleting the virtqueues here after reset without first detaching the
unused buffers leak pending DMA mappings? Because virtqueue_add_sgs() creates
DMA mappings for these buffers, if they aren't drained with
virtqueue_detach_unused_buf() before del_vqs(), those mappings might not be
unmapped.

[ ... ]

> @@ -385,6 +387,7 @@ static int virtsnd_freeze(struct virtio_device *vdev)
>  
>  	virtio_reset_device(vdev);
>  	vdev->config->del_vqs(vdev);
> +	virtsnd_ctl_msg_cleanup(snd);

[Severity: Medium]
Similarly, does suspending the device here destroy the virtqueues without
detaching and unmapping pending DMA buffers?


> diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c
> index 6433c870f271a..3ad1e1e1b2054 100644
> --- a/sound/virtio/virtio_ctl_msg.c
> +++ b/sound/virtio/virtio_ctl_msg.c
> @@ -147,12 +150,17 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
>  		psgs[nouts + nins++] = in_sgs;
>  
>  	scoped_guard(spinlock_irqsave, &queue->lock) {
> -		rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins, msg,
> -				       GFP_ATOMIC);
> +		if (snd->ctl_msgs_blocked)
> +			rc = -ENODEV;
> +		else
> +			rc = virtqueue_add_sgs(queue->vqueue, psgs, nouts, nins,
> +					       msg, GFP_ATOMIC);
>  		if (!rc) {
>  			notify = virtqueue_kick_prepare(queue->vqueue);
>  
>  			list_add_tail(&msg->list, &snd->ctl_msgs);
> +			if (notify)
> +				virtqueue_notify(queue->vqueue);
>  		}
>  	}

[ ... ]

> @@ -187,6 +192,11 @@ int virtsnd_ctl_msg_send(struct virtio_snd *snd, struct virtio_snd_msg *msg,
>  		goto on_exit;
>  	}
>  
> +	if (READ_ONCE(msg->cancelled)) {
> +		rc = -ENODEV;
> +		goto on_exit;
> +	}
> +
>  	switch (le32_to_cpu(response->code)) {
>  	case VIRTIO_SND_S_OK:
>  		rc = 0;

[Severity: Low]
This isn't a bug, but does mixing scope-based cleanup like scoped_guard with
traditional goto-based error handling (goto on_exit) violate the cleanup
guidelines?

The guidelines state that for a given routine, either all resources should be
converted to scope-based cleanup or none of them should be, to prevent brittle
ownership semantics and future bugs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791867377.git.mynameisjune111@gmail.com?part=3