Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: virtio: misaligned virtio_snd_ctl_value access in virtsnd_kctl_get() --
commit b0116a304a65fa5afc94f948da091c12c54f919b Author: Joonho Lee <[email protected]> ALSA: virtio: Keep control payloads alive through DMA completion This commit moves control message headers and data payloads into a reference-counted DMA buffer owned by struct virtio_snd_msg so that caller cleanup on timeout or interruption does not invalidate in-flight DMA. It also aligns scatterlist regions to the greater of the DMA cacheline and struct virtio_snd_ctl_value alignment. > diff --git a/sound/virtio/virtio_ctl_msg.c b/sound/virtio/virtio_ctl_msg.c > index 7114bea4033a5..2e0ff15e729da 100644 > --- a/sound/virtio/virtio_ctl_msg.c > +++ b/sound/virtio/virtio_ctl_msg.c [ ... ] > @@ -85,20 +161,85 @@ void *virtsnd_ctl_msg_response(struct virtio_snd_msg > *msg) > * Return: Allocated message on success, NULL on failure. > */ > struct virtio_snd_msg *virtsnd_ctl_msg_alloc(size_t request_size, > - size_t response_size, gfp_t gfp) > + size_t response_size, > + size_t request_data_size, > + size_t response_data_size, > + gfp_t gfp) > { [ ... ] > + cache_alignment = dma_get_cache_alignment(); > + if (cache_alignment <= 0) > + return NULL; > + /* DMA coherence does not remove the CPU's type alignment requirement. > */ > + alignment = max_t(size_t, cache_alignment, > + __alignof__(struct virtio_snd_ctl_value)); [ ... ] > diff --git a/sound/virtio/virtio_kctl.c b/sound/virtio/virtio_kctl.c > index 45f7b6a5b3080..4cf75dedbe9a3 100644 > --- a/sound/virtio/virtio_kctl.c > +++ b/sound/virtio/virtio_kctl.c > @@ -161,10 +163,12 @@ static int virtsnd_kctl_get(struct snd_kcontrol > *kcontrol, > unsigned int i; > int rc; > > - msg = virtsnd_ctl_msg_alloc(request_size, response_size, GFP_KERNEL); > + msg = virtsnd_ctl_msg_alloc(request_size, response_size, 0, 0, > + GFP_KERNEL); [Severity: Medium] Does packing both struct virtio_snd_hdr and struct virtio_snd_ctl_value into response_size here leave kvalue misaligned in virtsnd_kctl_get()? In sound/virtio/virtio_ctl_msg.c, virtsnd_ctl_msg_alloc() aligns the start of each scatterlist region (including msg->sg_response) to a multiple of __alignof__(struct virtio_snd_ctl_value) (8 bytes) via virtsnd_ctl_msg_sg_init(). However, in sound/virtio/virtio_kctl.c, virtsnd_kctl_get() passes response_size = sizeof(struct virtio_snd_hdr) + sizeof(*kvalue) and response_data_size = 0, and then computes kvalue by adding the 4-byte sizeof(struct virtio_snd_hdr) to the 8-byte-aligned response buffer: sound/virtio/virtio_kctl.c:virtsnd_kctl_get() { ... kvalue = (void *)((u8 *)virtsnd_ctl_msg_response(msg) + sizeof(struct virtio_snd_hdr)); switch (type) { ... case VIRTIO_SND_CTL_TYPE_INTEGER64: for (i = 0; i < count; ++i) uvalue->value.integer64.value[i] = le64_to_cpu(kvalue->value.integer64[i]); break; ... } Because virtsnd_ctl_msg_response(msg) is 8-byte aligned and sizeof(struct virtio_snd_hdr) is 4 bytes, kvalue is guaranteed to be at an address congruent to 4 mod 8. When userspace reads a control element with type VIRTIO_SND_CTL_TYPE_INTEGER64 via SNDRV_CTL_IOCTL_ELEM_READ, le64_to_cpu(kvalue->value.integer64[i]) performs an unaligned 64-bit typed load through kvalue. Note that a later commit in the series, 775188574a68 ("ALSA: virtio: add staged capture accounting and lifecycle recovery"), addresses this by passing sizeof(*kvalue) as response_data_size and retrieving kvalue via virtsnd_ctl_msg_response_data(msg). Should that change be folded into this commit so that kvalue is aligned here as intended? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
