[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