Re: [PATCH v15 07/12] 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:
- [Medium] ALSA: virtio: unvalidated slice order in virtsnd_pcm_msg_complete()
--

commit b2a39e63df7fac644d0bf6ab9ec43939a12f17f0
Author: Joonho Lee <mynameisjune111@gmail.com>

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