[PATCH 0/2] virtio: synchronize callbacks during device reset

[PATCH 0/2] virtio: synchronize callbacks during device reset

[Karl Mehltretter]

virtio_reset_device() documents that vq callbacks are not in progress
once it returns. virtio-pci delivers that by ending vp_reset() with
vp_synchronize_vectors(), virtio-mmio does not, and a driver that frees
state its callback uses then hits a use after free.

Patch 1 brings virtio-mmio in line with virtio-pci.

Patch 2 makes virtio_input reset before unregistering its input device
and stop its callback once teardown has begun.

Together these patches fix the use after free on virtio-pci and
virtio-mmio. Classic virtio-ccw and other transports still
do not provide the documented guarantee.

The use-after-free was reproduced under KASAN with the race window
artificially widened.

Callback synchronization was tested in an arm64 QEMU guest with a
virtio-input device over virtio-mmio and four vCPUs. The event callback
used a busy delay, not a sleep, and an in-progress counter tracked
overlap with unbind.

The driver was unbound and rebound 120 times under continuous input.
With patch 2 alone, virtio_reset_device() returned before the callback
finished in all 109 races, each in under 1 ms. With both patches,
vm_reset() waited for the callback in all 111 races, taking 24 to 86 ms.

Karl Mehltretter (2):
  virtio-mmio: synchronize callbacks during device reset
  virtio_input: stop callbacks before unregistering input device

 drivers/virtio/virtio_input.c | 8 ++++++--
 drivers/virtio/virtio_mmio.c  | 6 ++++++
 2 files changed, 12 insertions(+), 2 deletions(-)


base-commit: 3eb40771c00a8488fa6ed2cc1fe203477908bf38
-- 
2.53.0

[PATCH 1/2] virtio-mmio: synchronize callbacks during device reset

[Karl Mehltretter]

virtio_reset_device() promises that vq callbacks are not in progress
once it returns, but vm_reset() only writes 0 to the status register.
A callback that has already entered vring_interrupt() keeps running
while the driver tears down the state it uses.

Wait for a status read to return 0, so the device has stopped raising
interrupts, then synchronize_irq() as vp_reset() does with
vp_synchronize_vectors().

Fixes: edfd52e63672 ("virtio: Add platform bus driver for memory mapped virtio device")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/virtio/virtio_mmio.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 510b7c4efdff8..a2ca03dbe803d 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -55,6 +55,7 @@
 #define pr_fmt(fmt) "virtio-mmio: " fmt
 
 #include <linux/acpi.h>
+#include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/highmem.h>
 #include <linux/interrupt.h>
@@ -254,6 +255,11 @@ static void vm_reset(struct virtio_device *vdev)
 
 	/* 0 status means a reset. */
 	writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
+
+	/* Wait for reset completion before flushing callbacks. */
+	while (vm_get_status(vdev))
+		fsleep(1000);
+	synchronize_irq(platform_get_irq(vm_dev->pdev, 0));
 }
 
 
-- 
2.53.0

[PATCH 2/2] virtio_input: stop callbacks before unregistering input device

[Karl Mehltretter]

virtinput_remove() unregisters the input device, which can free it,
before resetting the virtio device that can still deliver events.

virtinput_recv_events() drops vi->lock around input_event() and samples
vi->ready only on loop entry. A callback that has passed that check can
therefore use the freed device, requeue a buffer, and kick the queue
after reset has begun.

A KASAN run with the race window widened reproduced the use-after-free.

Reset the device first, as virtinput_freeze() already does. Recheck
vi->ready after reacquiring the lock so the callback does not requeue or
kick once teardown starts.

Fixes: 271c865161c5 ("Add virtio-input driver.")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 drivers/virtio/virtio_input.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index deec24e8e6828..12ff33b178ea3 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c
@@ -49,9 +49,12 @@ static void virtinput_recv_events(struct virtqueue *vq)
 				    le16_to_cpu(event->code),
 				    le32_to_cpu(event->value));
 			spin_lock_irqsave(&vi->lock, flags);
+			if (!vi->ready)
+				break;
 			virtinput_queue_evtbuf(vi, event);
 		}
-		virtqueue_kick(vq);
+		if (vi->ready)
+			virtqueue_kick(vq);
 	}
 	spin_unlock_irqrestore(&vi->lock, flags);
 }
@@ -350,8 +353,9 @@ static void virtinput_remove(struct virtio_device *vdev)
 	vi->ready = false;
 	spin_unlock_irqrestore(&vi->lock, flags);
 
-	input_unregister_device(vi->idev);
+	/* Stop callbacks before unregistering the input device. */
 	virtio_reset_device(vdev);
+	input_unregister_device(vi->idev);
 	while ((buf = virtqueue_detach_unused_buf(vi->sts)) != NULL)
 		kfree(buf);
 	vdev->config->del_vqs(vdev);
-- 
2.53.0

Re: [PATCH 2/2] virtio_input: stop callbacks before unregistering input device

[sashiko-bot]

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

New issues:
- [High] Breaking early from the event processing loop truncates the input packet, violating input subsystem synchronization rules and leading to stuck keys.

Pre-existing issues:
- [High] Missing virtqueue_detach_unused_buf() for the 'events' virtqueue leads to DMA mapping leaks during device teardown and suspend.
--

--- Patch [2]: [PATCH 2/2] virtio_input: stop callbacks before unregistering input device ---
commit 8b35f1100c300e75de17028044efd92590e5c2f0
Author: Karl Mehltretter <kmehltretter@gmail.com>

virtio_input: stop callbacks before unregistering input device

This commit alters the teardown sequence in virtinput_remove() to reset the
virtio device prior to unregistering the input device. It also adds checks
for vi->ready in virtinput_recv_events() to prevent requeuing buffers or
kicking the queue during teardown.

> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> index deec24e8e6828..b6843601d0aea 100644
> --- a/drivers/virtio/virtio_input.c
> +++ b/drivers/virtio/virtio_input.c
> @@ -49,9 +49,12 @@ static void virtinput_recv_events(struct virtqueue *vq)
>  				    le16_to_cpu(event->code),
>  				    le32_to_cpu(event->value));
>  			spin_lock_irqsave(&vi->lock, flags);
> +			if (!vi->ready)
> +				break;

[Severity: High]
Does breaking out of this loop truncate the input packet?

If the device begins teardown or suspend while the loop is midway through
processing a packet, breaking early aborts reading the remaining events from
the used ring. If an EV_KEY release was processed just before the break, its
corresponding EV_SYN is left in the queue and subsequently destroyed when
the device resets.

Because EV_SYN is required to flush the state to userspace handlers, this
can lead to lost button releases and stuck keys. Should this use "continue"
instead of "break" to safely process the full packet while preventing buffer
requeue during teardown?

>  			virtinput_queue_evtbuf(vi, event);
>  		}
> -		virtqueue_kick(vq);
> +		if (vi->ready)
> +			virtqueue_kick(vq);
>  	}
>  	spin_unlock_irqrestore(&vi->lock, flags);
>  }

[ ... ]

> @@ -350,8 +353,9 @@ static void virtinput_remove(struct virtio_device *vdev)
>  	vi->ready = false;
>  	spin_unlock_irqrestore(&vi->lock, flags);
>  
> -	input_unregister_device(vi->idev);
> +	/* Stop callbacks before unregistering the input device. */
>  	virtio_reset_device(vdev);
> +	input_unregister_device(vi->idev);
>  	while ((buf = virtqueue_detach_unused_buf(vi->sts)) != NULL)

[Severity: High]
This is a pre-existing issue, but does this teardown sequence leak DMA
mappings for the events virtqueue?

During initialization, vi->evt is populated with buffers that establish DMA
mappings. During virtinput_remove() (and similarly in virtinput_freeze()),
the driver correctly drains vi->sts using virtqueue_detach_unused_buf() to
trigger dma_unmap_page(), but it skips vi->evt entirely before calling
vdev->config->del_vqs(vdev).

Since vring_del_virtqueue() only frees the queue descriptors and does not
automatically unmap unused buffers left inside the ring, skipping vi->evt
leaks its DMA mappings on every suspend/resume or driver unbind.

>  		kfree(buf);
>  	vdev->config->del_vqs(vdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818040433.66986-1-kmehltretter@gmail.com?part=2