Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ALSA: virtio: sw_ready desynchronization in virtsnd_pcm_cp_ack() --
commit b314c942dc53f8c652b76808e69e1ee4418c9e8e Author: Joonho Lee <[email protected]> ALSA: virtio: retain PCM DMA pages and validate capture ring ownership This commit retains references to ALSA PCM DMA pages (or uses driver-owned buffers on NOMMU systems) until queued I/O messages are freed, and adds validation for period boundaries and fixed-slot capture completions. It also seeds the ordinary capture ring once per prepare and reserves the RX ring via sw_ready until appl_ptr consumes the captured data. > diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c > index 343700ec50476..bf9dafe149dac 100644 > --- a/sound/virtio/virtio_pcm_ops.c > +++ b/sound/virtio/virtio_pcm_ops.c [ ... ] > @@ -433,8 +446,15 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream > *substream, int command) > substream->runtime->stop_operating = true; > return -EBADFD; > } > - if (vss->direction == SNDRV_PCM_STREAM_CAPTURE) > + if (vss->direction == SNDRV_PCM_STREAM_CAPTURE && > + !vss->capture_rx_seeded) { > rc = virtsnd_pcm_msg_send(vss, 0, > vss->buffer_bytes); > + if (!rc) { > + /* The full RX ring is reserved until > userspace reads it. */ > + vss->pcm_indirect.sw_ready = > vss->buffer_bytes; > + vss->capture_rx_seeded = true; > + } > + } > if (rc) { > vss->stopped = true; > substream->runtime->stop_operating = true; [Severity: High] Can userspace desynchronize or underflow vss->pcm_indirect.sw_ready via SNDRV_PCM_IOCTL_SYNC_PTR in virtsnd_pcm_cp_ack()? In sound/virtio/virtio_pcm_ops.c, virtsnd_pcm_ack_state() allows ACK in SNDRV_PCM_STATE_PREPARED, and virtsnd_pcm_cp_ack() does not check vss->capture_rx_seeded before calling snd_pcm_indirect_capture_transfer(): sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_cp_ack() { ... scoped_guard(spinlock_irqsave, &queue->lock) { guard(spinlock)(&vss->lock); if (vss->preparing || !vss->prepared) return -EBADFD; /* Keep only the post-STOP capture tail available during drain. */ if (vss->stopped && (vss->xfer_xrun || substream->runtime->state != SNDRV_PCM_STATE_DRAINING)) return -EBADFD; if (vss->xfer_xrun) { rc = -EPIPE; } else { transfer.indirect = vss->pcm_indirect; rc = snd_pcm_indirect_capture_transfer(substream, &transfer.indirect, virtsnd_pcm_trans_copy); ... } If userspace invokes SNDRV_PCM_IOCTL_SYNC_PTR (without SNDRV_PCM_SYNC_PTR_APPL) while the capture stream is in SNDRV_PCM_STATE_PREPARED before SNDRV_PCM_TRIGGER_START, pcm_lib_apply_appl_ptr() invokes virtsnd_pcm_cp_ack() -> snd_pcm_indirect_capture_transfer(), which updates vss->pcm_indirect.appl_ptr and decrements vss->pcm_indirect.sw_ready below zero while sw_data and hw_data remain 0. When virtsnd_pcm_trigger() later handles SNDRV_PCM_TRIGGER_START, it sets vss->pcm_indirect.sw_ready = vss->buffer_bytes while vss->pcm_indirect.appl_ptr is already non-zero, leaving sw_ready and sw_data out of sync with appl_ptr. Similarly, after SNDRV_PCM_TRIGGER_START in SNDRV_PCM_STATE_RUNNING, pcm_lib_apply_appl_ptr() only bounds each appl_ptr advance to runtime->buffer_size rather than the unread captured frames. If userspace uses SNDRV_PCM_IOCTL_SYNC_PTR to advance appl_ptr ahead of captured bytes (for example when hw_ready is 0 before completions arrive), snd_pcm_indirect_capture_transfer() subtracts frames_to_bytes(runtime, diff) from rec->sw_ready without transferring anything. Wouldn't subsequent period completions in virtsnd_pcm_cp_pointer() then see rec->sw_ready < rec->sw_buffer_size and prematurely re-queue capture descriptors before userspace reads them, or allow repeated SYNC_PTR calls to underflow rec->sw_ready so (int)(rec->sw_buffer_size - rec->sw_ready) wraps negative and calls virtsnd_pcm_msg_send() on already-queued descriptors? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
