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 <[email protected]> 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/[email protected]?part=4
