Re: [PATCH v1] ALSA: virtio: reset device before deleting virtqueues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver permanently leaks DMA mappings for unused virtqueue buffers because they are not detached before virtqueue deletion.
- [High] Prematurely cancelling and freeing pending control messages before resetting the device leads to hardware Use-After-Free, list corruption, and double frees.
--

commit a186967d39bb57120371bc04edb4ff625893e99e
Author: Yuho Choi <oss.patchbox@gmail.com>

ALSA: virtio: reset device before deleting virtqueues

This commit moves virtio_reset_device() before del_vqs() in driver
remove and freeze paths. This prevents the hardware from accessing
virtqueue memory after it has been freed.

> diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c
> index 647190f4d5afc..6f35276416fed 100644
> --- a/sound/virtio/virtio_card.c
> +++ b/sound/virtio/virtio_card.c
> @@ -354,8 +354,8 @@ static void virtsnd_remove(struct virtio_device *vdev)
>  	if (snd->card)
>  		snd_card_free(snd->card);
>  
> -	vdev->config->del_vqs(vdev);
>  	virtio_reset_device(vdev);
> +	vdev->config->del_vqs(vdev);

[Severity: High]
This is a pre-existing issue, but does this permanently leak DMA mappings for
unused virtqueue buffers because they are not detached before the virtqueues
are deleted?

System suspend/resume or driver unbinding will call virtsnd_freeze() or
virtsnd_remove(), which deletes the virtqueues without first detaching unused
buffers via virtqueue_detach_unused_buf().

Repeated suspend/resume cycles could continuously leak DMA mappings (such as
pending event buffers and PCM buffers) until the DMA space is exhausted.

>  
>  	for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) {
>  		struct virtio_pcm_substream *vss = &snd->substreams[i];

[ ... ]

> @@ -383,8 +383,8 @@ static int virtsnd_freeze(struct virtio_device *vdev)
>  	virtsnd_disable_event_vq(snd);
>  	virtsnd_ctl_msg_cancel_all(snd);

[Severity: High]
This is a pre-existing issue, but does prematurely cancelling and freeing
pending control messages here (and similarly in virtsnd_remove) before
resetting the device leave a window where the hardware can DMA to freed
memory?

For messages sent with nowait = true, virtsnd_ctl_msg_cancel_all() drops the
refcount to 0, immediately freeing the memory while the message is still
actively queued in the hardware virtqueue.

The device could then DMA write into the freed memory, or trigger an
interrupt where virtsnd_ctl_notify_cb() retrieves the freed pointer and calls
virtsnd_ctl_msg_complete() on it again, causing a double free.

>  
> -	vdev->config->del_vqs(vdev);
>  	virtio_reset_device(vdev);
> +	vdev->config->del_vqs(vdev);
>  
>  	for (i = 0; i < snd->nsubstreams; ++i)
>  		cancel_work_sync(&snd->substreams[i].elapsed_period);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911031121.1542502-1-oss.patchbox@gmail.com?part=1