Re: [PATCH v18 07/12] ALSA: virtio: retain PCM DMA pages and validate capture ring ownership

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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/20261106000644.20460-1-mynameisjune111@gmail.com?part=7