Skip to contentSkip to conversation

ALSA: virtio: reset device before deleting virtqueues

PATCH In review Yuho Choi 3 participants 3 messages Sep 11 – Sep 13

Subsystems (4)
Stable branch Sound virtio core virtio sound driver
lore
Share & tools
lore

Yuho Choi

author
#0 lore raw

PATCH 1/1 ALSA: virtio: reset device before deleting virtqueues

virtsnd_remove() and virtsnd_freeze() delete the virtqueues before resetting the device. del_vqs() frees the vring backing, but does not provide a generic device quiesce operation. In particular, modern virtio-pci keeps enabled queues active until the device is reset.

Reset the device before deleting the virtqueues so it can no longer access the vring memory when that memory is released. This also covers probe failures after DRIVER_OK, which unwind through virtsnd_remove().

Fixes de3a9980d8c3 ("ALSA: virtio: add virtio sound driver")
Fixes 575483e90a32 ("ALSA: virtio: introduce device suspend/resume support")
Cc [email protected]
Signed-off-by Yuho Choi <[email protected]>
---
 sound/virtio/virtio_card.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
sound/virtio/virtio_card.c +2 -2
index 647190f4d5afc..6f35276416fed 100644
@@ -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);
 
 	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);
 
-	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);
Signature · 1 line
2.43.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

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 <[email protected]>

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.

sound/virtio/virtio_card.c static void virtsnd_remove(struct virtio_device *vdev) · line 358

Quoted 11 lines
sound/virtio/virtio_card.c +1 -1
index 647190f4d5afc..6f35276416fed 100644
@@ -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.

Quoted 3 lines
sound/virtio/virtio_card.c +0 -0
 	for (i = 0; snd->substreams && i < snd->nsubstreams; ++i) {
 		struct virtio_pcm_substream *vss = &snd->substreams[i];

[ ... ]

sound/virtio/virtio_card.c static int virtsnd_freeze(struct virtio_device *vdev) · line 384

Quoted 3 lines
sound/virtio/virtio_card.c +0 -0
@@ -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.

Quoted 7 lines
sound/virtio/virtio_card.c +1 -1
-	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);
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#2 lore raw
In reply to Yuho Choi
Quoted 6 lines · Yuho Choi

virtsnd_remove() and virtsnd_freeze() delete the virtqueues before resetting the device. del_vqs() frees the vring backing, but does not provide a generic device quiesce operation. In particular, modern virtio-pci keeps enabled queues active until the device is reset.

Quoted 8 lines

Reset the device before deleting the virtqueues so it can no longer access the vring memory when that memory is released. This also covers probe failures after DRIVER_OK, which unwind through virtsnd_remove().

Fixes de3a9980d8c3 ("ALSA: virtio: add virtio sound driver")
Fixes 575483e90a32 ("ALSA: virtio: introduce device suspend/resume support")
Cc [email protected]
Signed-off-by Yuho Choi <[email protected]>

Applied now. Thanks.

Takashi