Re: [PATCH v10 04/14] ALSA: virtio: Keep PCM DMA buffers alive through device ownership

[sashiko-bot@kernel.org]

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

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