Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio: unvalidated slice order in virtsnd_pcm_msg_complete() --
commit b2a39e63df7fac644d0bf6ab9ec43939a12f17f0 Author: Joonho Lee <[email protected]> ALSA: virtio: Keep PCM DMA buffers alive through device ownership This commit retains references to the pages backing ALSA vmalloc buffers (or uses driver-owned buffers on NOMMU systems) until queued I/O messages are freed, and adds validation for PCM buffer bounds, period sizes, and capture completion status and payload lengths. > diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c > index 59ad99e21e899..9fb966a02d923 100644 > --- a/sound/virtio/virtio_pcm_msg.c > +++ b/sound/virtio/virtio_pcm_msg.c [ ... ] > @@ -356,42 +540,80 @@ static void virtsnd_pcm_msg_complete(struct > virtio_pcm_msg *msg, > size_t written_bytes) > { > struct virtio_pcm_substream *vss = msg->substream; > + bool capture_invalid = false; > > /* > - * hw_ptr always indicates the buffer position of the first I/O message > - * in the virtqueue. Therefore, on each completion of an I/O message, > - * the hw_ptr value is unconditionally advanced. > + * Completed requests can return captured data after STOP while the PCM > + * buffer remains prepared. Once preparation ends, completions only > retire > + * queue ownership. > */ > guard(spinlock)(&vss->lock); > - /* > - * If the capture substream returned an incorrect status, then just > - * increase the hw_ptr by the message size. > - */ > - if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK || > - written_bytes <= sizeof(msg->status)) > - vss->hw_ptr += msg->length; > - else > - vss->hw_ptr += written_bytes - sizeof(msg->status); > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + (!vss->prepared || > + (vss->stopped && written_bytes <= sizeof(msg->status)))) { > + msg->length = 0; > + vss->msg_count--; > + if (!vss->msg_count) > + wake_up_all(&vss->msg_empty); > + return; > + } > > - if (vss->hw_ptr >= vss->buffer_bytes) > - vss->hw_ptr -= vss->buffer_bytes; > + /* A non-staged capture request must return its full payload. */ > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + (written_bytes < sizeof(msg->status) || > + le32_to_cpu(msg->status.status) != VIRTIO_SND_S_OK || > + written_bytes - sizeof(msg->status) != msg->length)) { > + dev_err_ratelimited(&vss->snd->vdev->dev, > + "SID %u: invalid capture status or > length\n", > + vss->sid); > + vss->xfer_xrun = true; > + capture_invalid = true; > + } > +#ifndef CONFIG_MMU > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + vss->prepared && !capture_invalid && > + written_bytes > sizeof(msg->status)) { > + struct snd_pcm_runtime *runtime = > + vss->substream ? vss->substream->runtime : NULL; > + > + if (!runtime || !runtime->dma_area || !vss->owned_dma_area || > + msg->data_offset > runtime->dma_bytes || > + msg->length > runtime->dma_bytes - msg->data_offset || > + msg->data_offset > vss->owned_dma_bytes || > + msg->length > 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); > + vss->xfer_xrun = true; > + capture_invalid = true; > + } else { > + memcpy(runtime->dma_area + msg->data_offset, > + vss->owned_dma_area + msg->data_offset, > msg->length); > + } > + } > +#endif > + /* Publish pointer progress only for a complete TX or valid RX request. > */ > + if (!capture_invalid) { > + vss->hw_ptr += msg->length; > + if (vss->hw_ptr >= vss->buffer_bytes) > + vss->hw_ptr -= vss->buffer_bytes; > + } [Severity: Medium] Can virtsnd_pcm_msg_complete() expose unwritten ring buffer slices to userspace if capture descriptors complete out of order or after an earlier descriptor is retired without advancing vss->hw_ptr? In ordinary capture mode, each descriptor vss->msgs[i] targets a fixed ring buffer slice at offset i * period_bytes, while virtsnd_pcm_msg_complete() advances vss->hw_ptr by msg->length without checking that the completing msg corresponds to the slice at vss->hw_ptr. When vss->stopped is true while vss->prepared remains true (such as after SNDRV_PCM_TRIGGER_STOP), if vss->msgs[0] (slice 0) completes empty with written_bytes <= sizeof(msg->status), virtsnd_pcm_msg_complete() retires vss->msgs[0] and returns early without advancing vss->hw_ptr or latching vss->xfer_xrun. If vss->msgs[1] (slice 1, at offset period_bytes) subsequently completes with a full period, or if the device completes vss->msgs[1] before vss->msgs[0] out of order, virtsnd_pcm_msg_complete() writes or copies the payload into slice 1 at offset period_bytes and advances vss->hw_ptr from 0 to period_bytes. Subsequent calls to virtsnd_pcm_pointer() and virtsnd_pcm_ack() see vss->xfer_xrun == false and publish slice 0, which was never filled by vss->msgs[0], as readable captured audio. Similarly, if an earlier capture descriptor failed validation and set vss->xfer_xrun = true without advancing vss->hw_ptr, a later valid descriptor completion still copies NOMMU data into its own offset and advances vss->hw_ptr because virtsnd_pcm_msg_complete() only checks the local capture_invalid flag rather than vss->xfer_xrun. Should virtsnd_pcm_msg_complete() verify that the completing message matches the expected ring slice at vss->hw_ptr and avoid advancing vss->hw_ptr (or accepting further completions) once an empty stopped descriptor has been retired or vss->xfer_xrun has been latched? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
