Re: [PATCH v10 04/14] ALSA: virtio: Keep PCM DMA buffers alive through device ownership
flat view
From: sashiko-bot@kernel.org
Date: 2026-10-30 05:56:46
Also in:
linux-sound
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: virtio: pause release inflates msg->length in virtsnd_pcm_msg_send()
--
commit ec627d670b7dce9aa5415f6f76196d33998b7e23
Author: Joonho Lee [off-list ref]
ALSA: virtio: Keep PCM DMA buffers alive through device ownership
This commit retains references to ALSA's vmalloc buffer pages while I/O
messages are queued (or uses a driver-owned buffer on NOMMU systems), validates
buffer and period boundaries, and clamps device-reported capture completion
lengths to msg->length.
quoted hunk ↗ jump to hunk
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c index 762480363b861..bc2002312a282 100644 --- a/sound/virtio/virtio_pcm_msg.c +++ b/sound/virtio/virtio_pcm_msg.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -363,15 +546,48 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg, * the hw_ptr value is unconditionally advanced. */ guard(spinlock)(&vss->lock); +#ifndef CONFIG_MMU + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && + vss->xfer_enabled && written_bytes > sizeof(msg->status)) { + struct snd_pcm_runtime *runtime = + vss->substream ? vss->substream->runtime : NULL; + size_t reported_bytes = written_bytes - sizeof(msg->status); + size_t data_bytes = min_t(size_t, reported_bytes, msg->length); + + if (!runtime || !runtime->dma_area || !vss->owned_dma_area || + msg->data_offset > runtime->dma_bytes || + data_bytes > runtime->dma_bytes - msg->data_offset || + msg->data_offset > vss->owned_dma_bytes || + data_bytes > vss->owned_dma_bytes - msg->data_offset) + dev_err_ratelimited(&vss->snd->vdev->dev, + "SID %u: invalid capture buffer for I/O message\n", + vss->sid); + else + memcpy(runtime->dma_area + msg->data_offset, + vss->owned_dma_area + msg->data_offset, data_bytes); + } +#endif + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && + written_bytes > sizeof(msg->status) && + written_bytes - sizeof(msg->status) > msg->length) + dev_err_ratelimited(&vss->snd->vdev->dev, + "SID %u: capture length exceeds I/O message capacity\n", + vss->sid); /* * If the capture substream returned an incorrect status, then just - * increase the hw_ptr by the message size. + * increase the hw_ptr by the message size. Clamp valid capture lengths to + * the actual SG payload capacity before accounting them. */ if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK || - written_bytes <= sizeof(msg->status)) + written_bytes <= sizeof(msg->status)) { vss->hw_ptr += msg->length; - else - vss->hw_ptr += written_bytes - sizeof(msg->status); + } else { + size_t data_bytes = min_t(size_t, + written_bytes - sizeof(msg->status), + msg->length); + + vss->hw_ptr += data_bytes; + }
[Severity: High] Can SNDRV_PCM_TRIGGER_PAUSE_RELEASE on a capture substream inflate msg->length beyond period_bytes in virtsnd_pcm_msg_send(), bypassing the new clamp in virtsnd_pcm_msg_complete() and advancing vss->hw_ptr out of bounds? In sound/virtio/virtio_pcm_ops.c, virtsnd_pcm_trigger() handles both SNDRV_PCM_TRIGGER_START and SNDRV_PCM_TRIGGER_PAUSE_RELEASE by submitting the entire buffer on capture substreams: sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: ... if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes); } When a capture stream is paused with SNDRV_PCM_TRIGGER_PAUSE_PUSH, the driver sends VIRTIO_SND_R_PCM_STOP rather than VIRTIO_SND_R_PCM_RELEASE, so uncompleted RX messages remain queued in the virtqueue with msg->length == period_bytes. When SNDRV_PCM_TRIGGER_PAUSE_RELEASE subsequently calls virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes), the loop adds n to msg->length unconditionally for every period: sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_send() { n = period_bytes - (offset % period_bytes); if (n > bytes) n = bytes; msg->length += n; if (msg->length == period_bytes) { ... } For messages already queued in the virtqueue, msg->length becomes 2 * period_bytes (or (K + 1) * period_bytes after K pause/release cycles), and for any partially acknowledged message (0 < msg->length < period_bytes), msg->length is incremented past period_bytes without ever matching msg->length == period_bytes to be re-queued. In virtsnd_pcm_msg_complete(), clamping data_bytes to msg->length then permits oversized device completions up to the inflated msg->length rather than the single-period SG payload capacity (copying across adjacent periods on NOMMU and advancing vss->hw_ptr by multiple periods). Likewise, if the stream is released and pending RX descriptors complete with written_bytes <= sizeof(msg->status), vss->hw_ptr += msg->length advances vss->hw_ptr by a multiple of period_bytes, where the single subtraction of vss->buffer_bytes can leave vss->hw_ptr >= vss->buffer_bytes. -- Sashiko AI review · https://sashiko.dev/#/patchset/20261030052629.27742-1-mynameisjune111@gmail.com?part=4