Re: [PATCH v5 05/11] 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 control value access in virtsnd_kctl_get()
--

commit 15e13c28bcb1079484dfa947a5ae6109ebb5b3c9
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: virtio: Keep control payloads alive through DMA completion

This commit moves control request and response payloads into a
reference-counted DMA buffer owned by struct virtio_snd_msg so caller
buffers are not exposed to DMA after a timeout or interruption. It also
aligns each scatterlist region to the greater of the DMA cacheline size
and the alignment of struct virtio_snd_ctl_value.

> 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)
>  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);
>  	if (!msg)
>  		return -ENOMEM;

[Severity: Medium]
Does packing both the response header and struct virtio_snd_ctl_value into
sg_response leave kvalue misaligned in virtsnd_kctl_get()?

In virtsnd_ctl_msg_alloc(), each scatterlist region is aligned to at least
__alignof__(struct virtio_snd_ctl_value), which is 8 bytes because of the
__le64 integer64[64] array.

In virtsnd_kctl_put(), the request header is struct virtio_snd_ctl_hdr (8
bytes), so placing kvalue immediately after it preserves 8-byte alignment.

However, in virtsnd_kctl_get(), response_data_size is passed as 0 and
response_size is sizeof(struct virtio_snd_hdr) + sizeof(*kvalue), with
kvalue computed right after the 4-byte struct virtio_snd_hdr:

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;
	...
}

Since virtsnd_ctl_msg_response(msg) is aligned to a multiple of 8 bytes and
sizeof(struct virtio_snd_hdr) is 4 bytes, kvalue is guaranteed to land at an
8n + 4 address, causing unaligned 64-bit loads when reading a
VIRTIO_SND_CTL_TYPE_INTEGER64 control.

Should the control value payload be placed in response_data (or the
request/response payloads placed in sg_request_data and sg_response_data) so
that kvalue starts on an aligned boundary?

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