Re: [PATCH v8 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: out-of-bounds vss->msgs[] access when buffer_bytes < dma_bytes
--

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

ALSA: virtio: Keep PCM DMA buffers alive through device ownership

This commit retains references to pages backing ALSA vmalloc buffers (or uses
a driver-owned DMA buffer on NOMMU systems) until queued virtio-snd I/O
messages are freed. It also adds buffer validation during message allocation
and clamps device-reported capture completion lengths to the message capacity.

> diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
> index 762480363b861..b7f2334bb221c 100644
> --- a/sound/virtio/virtio_pcm_msg.c
> +++ b/sound/virtio/virtio_pcm_msg.c

[ ... ]

> @@ -132,36 +211,99 @@ static void virtsnd_pcm_sg_from(struct scatterlist *sgs, int nsgs, u8 *data,
>  int virtsnd_pcm_msg_alloc(struct virtio_pcm_substream *vss,
>  			  unsigned int periods, unsigned int period_bytes)
>  {
> -	struct snd_pcm_runtime *runtime = vss->substream->runtime;
> +	struct snd_pcm_runtime *runtime;
> +	size_t buffer_bytes;
>  	unsigned int i;
> +	int rc;
>  
> -	vss->msgs = kzalloc_objs(*vss->msgs, periods);
> -	if (!vss->msgs)
> +	if (vss->msgs || vss->nmsgs || vss->dma_pages || vss->ndma_pages)
> +		return -EBUSY;
> +#ifndef CONFIG_MMU
> +	if (vss->owned_dma_area || vss->owned_dma_bytes)
> +		return -EBUSY;
> +#endif
> +	if (!periods || !period_bytes)
> +		return -EINVAL;
> +
> +	if (check_mul_overflow((size_t)periods, (size_t)period_bytes,
> +			       &buffer_bytes))
> +		return -EOVERFLOW;
> +
> +	if (!vss->substream || !vss->substream->runtime)
> +		return -EINVAL;
> +	runtime = vss->substream->runtime;
> +	if (!runtime->dma_area || buffer_bytes > runtime->dma_bytes)
> +		return -EINVAL;

[Severity: High]
Can this validation allow buffer_bytes < runtime->dma_bytes to pass and lead
to an out-of-bounds read on vss->msgs[] in virtsnd_pcm_msg_send()?

virtsnd_pcm_open() constrains SNDRV_PCM_HW_PARAM_PERIODS to an integer, and
ALSA core constrains SNDRV_PCM_HW_PARAM_PERIOD_SIZE and
SNDRV_PCM_HW_PARAM_BUFFER_BYTES to integers, but SNDRV_PCM_HW_PARAM_PERIOD_BYTES
is not constrained to an integer. When the device supports the 4-bit
SNDRV_PCM_FORMAT_IMA_ADPCM format (VIRTIO_SND_PCM_FMT_IMA_ADPCM), a 1-channel
stream (frame_bits == 4) configured with an odd period_size (for example, 2001
frames) and an even number of periods (for example, 2, giving buffer_size ==
4002 frames) refines SNDRV_PCM_HW_PARAM_BUFFER_BYTES to the exact integer 2001
(so runtime->dma_bytes is 2001), while SNDRV_PCM_HW_PARAM_PERIOD_BYTES refines
to the non-empty open interval (1000, 1001). Both params_period_bytes() and
snd_pcm_lib_period_bytes() truncate that period byte size to 1000.

In virtsnd_pcm_msg_alloc(), buffer_bytes is computed as periods * period_bytes
(2000), which passes buffer_bytes > runtime->dma_bytes and allocates only
vss->nmsgs = 2 messages (along with 2000 bytes of retained pages or
owned_dma_area).

Later, virtsnd_pcm_prepare() sets vss->buffer_bytes and the pcm_indirect
buffer sizes from snd_pcm_lib_buffer_bytes(substream) (2001):

sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_prepare() {
	...
		vss->buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
		vss->hw_ptr = 0;
	...
	vss->pcm_indirect.sw_buffer_size =
		vss->pcm_indirect.hw_buffer_size =
		snd_pcm_lib_buffer_bytes(substream);
}

When capture starts via virtsnd_pcm_trigger() ->
virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes), or playback transfers the
final byte of the buffer via virtsnd_pcm_pb_ack() -> virtsnd_pcm_msg_send(),
virtsnd_pcm_msg_send() computes end = (offset + bytes - 1) / period_bytes =
2000 / 1000 = 2 and indexes vss->msgs[2] past the end of the 2-element
vss->msgs[] array:

sound/virtio/virtio_pcm_msg.c:virtsnd_pcm_msg_send() {
	...
	start = offset / period_bytes;
	end = (offset + bytes - 1) / period_bytes;

	for (i = start; i <= end; i++) {
		struct virtio_pcm_msg *msg = vss->msgs[i];
	...
}

Should virtsnd_pcm_msg_alloc() check buffer_bytes != runtime->dma_bytes (or
constrain SNDRV_PCM_HW_PARAM_PERIOD_BYTES to an integer in virtsnd_pcm_open())
so that periods * period_bytes always covers the full PCM buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029054354.2931-1-mynameisjune111@gmail.com?part=4