Re: [PATCH v8 05/14] ALSA: virtio: Keep control payloads alive through DMA completion

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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