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 <[email protected]> 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. > 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 [ ... ] > @@ -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/[email protected]?part=4
