Re: [PATCH v19 07/12] ALSA: virtio: validate indirect capture appl_ptr accounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ALSA: virtio: sub-byte frame truncation in
  virtsnd_pcm_capture_ack_validate()
--

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

ALSA: virtio: validate indirect capture appl_ptr accounting

This commit retains references to PCM DMA pages (or uses a driver-owned
buffer on NOMMU), validates full-period capture responses, seeds the RX
ring once per prepare, and validates capture appl_ptr accounting before
invoking the indirect transfer helper.

> diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
> index 343700ec50476..6a8752997d092 100644
> --- a/sound/virtio/virtio_pcm_ops.c
> +++ b/sound/virtio/virtio_pcm_ops.c

[ ... ]

> @@ -599,6 +619,69 @@ struct virtsnd_pcm_transfer {
>  	int error;
>  };
>  
> +/* Caller holds @vss->lock and the PCM stream lock. */
> +static int virtsnd_pcm_capture_ack_validate(struct virtio_pcm_substream *vss,
> +					    struct snd_pcm_runtime *runtime,
> +					    bool *skip_transfer)
> +{

[ ... ]

> +	/* The RX ring reserves its full size; only the excess is unread data. */
> +	unread_bytes = (s64)rec->hw_ready + (s64)rec->sw_ready -
> +		       (s64)rec->sw_buffer_size;
> +	if (unread_bytes < 0 || unread_bytes > (s64)hw_queue_size)
> +		return -EPIPE;
> +
> +	appl_bytes = frames_to_bytes(runtime, (snd_pcm_uframes_t)appl_diff);
> +	if ((u64)appl_bytes > (u64)unread_bytes)
> +		return -EPIPE;
> +
> +	return 0;
> +}

[Severity: Medium]
Can frames_to_bytes() truncate sub-byte frame deltas here and let an
unaligned appl_ptr advance bypass validation?

virtsnd_pcm_hw_params() permits sub-byte frame formats such as 1-channel
SNDRV_PCM_FORMAT_IMA_ADPCM (4-bit frames, runtime->min_align == 2) when
the period size is a whole number of bytes.  If userspace advances
appl_ptr by an odd number of frames via SNDRV_PCM_IOCTL_SYNC_PTR (for
example, 1 frame when unread_bytes is 0), frames_to_bytes() truncates
appl_bytes down, passing the unread_bytes check.
snd_pcm_indirect_capture_transfer() then sets rec->appl_ptr to the
unaligned appl_ptr while subtracting the truncated byte count from
rec->sw_ready, pushing appl_ptr past hw_ptr and permanently leaking
rec->sw_ready accounting so RX descriptors stop being requeued.

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