[PATCH v2 0/2] 9p: fix guest-triggered Treaddir/ACPI eject UAF

[PATCH v2 0/2] 9p: fix guest-triggered Treaddir/ACPI eject UAF

[Christian Schoenebeck]

A guest can trigger unplugging 9pfs server's virtio-pci device via ACPI
eject. As a consequence the device is unrealized, server's internal state
is freed while pending coroutines would still have access to them, causing
a potential heap-use-after-free.

Overview Patches:

 - Patch 1: this is the core fix, that drains all PDUs (i.e. coroutines
   that handle individual pending requests in parallel) before freeing
   server state.

 - Patch 2: fixes a similar identified issue with the Xen transport, even
   though not triggered via ACPI, it is also prone to UAF, plus a resource
   leak.

v2:
  - Patch 1: Make Jia the official author of this patch.
  - Drop prev. patch 2 ("hw/9pfs/virtio: disable hotpluggable property...")
  - Patch 2: defer explict xen_9pfs_disconnect() call from error paths of
    xen_9pfs_pdu_vmarshal() and xen_9pfs_pdu_vunmarshal().

Christian Schoenebeck (1):
  hw/9pfs/xen: drain in-flight PDUs before xen-9p disconnect

Jia Jia (1):
  hw/9pfs/virtio: drain in-flight PDUs before virtio-9p unrealize

 hw/9pfs/virtio-9p-device.c |  1 +
 hw/9pfs/xen-9p-backend.c   | 19 +++++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

-- 
2.47.3

[PATCH v2 1/2] hw/9pfs/virtio: drain in-flight PDUs before virtio-9p unrealize

[Christian Schoenebeck]

From: Jia Jia <physicalmtea@gmail.com>

A guest can trigger a heap-use-after-free in the virtio transport
unrealize path by submitting a Treaddir request and immediately
ejecting the device via ACPI PCI hotplug. The unrealize path frees
struct LocalData while a worker thread still holds a reference
on it, causing a UAF in local_open_nofollow().

Fix this by draining all in-flight 9p PDUs by calling v9fs_reset()
before final server cleanup. This ensures all coroutines completed,
all FIDs are closed, and no worker thread still holds references
on 9p server state when it is freed.

Fixes: 6cecf09373 ("virtio-9p-device: add minimal unrealize handler")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3937
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
[ Christian Schoenebeck: add commit log message. ]
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/virtio-9p-device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 50dc93091d..1ec48fc9e0 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -243,6 +243,7 @@ static void virtio_9p_device_unrealize(DeviceState *dev)
     V9fsVirtioState *v = VIRTIO_9P(dev);
     V9fsState *s = &v->state;
 
+    v9fs_reset(s);
     virtio_delete_queue(v->vq);
     virtio_cleanup(vdev);
     v9fs_device_unrealize_common(s);
-- 
2.47.3

[PATCH v2 2/2] hw/9pfs/xen: drain in-flight PDUs before xen-9p disconnect

[Christian Schoenebeck]

The xen-9p disconnect path has two issues:

1. It frees the Xen9pfsRing structures while in-flight PDUs may still
   reference them via pdu->tag to index rings[]. This causes a UAF
   in xen_9pfs_push_and_notify() when worker threads resume after
   completing filesystem operations.

2. It never calls v9fs_device_unrealize_common(), which means server
   state (struct LocalData, mountfd, FIDs) is never cleaned up on
   disconnect, causing a resource leak on every guest-initiated
   disconnect.

Fix both by draining in-flight PDUs via v9fs_reset() before tearing
down rings, and calling v9fs_device_unrealize_common() to clean up
server state.

Additionally, explicit calls of xen_9pfs_disconnect() in the error
paths of xen_9pfs_pdu_vmarshal() and xen_9pfs_pdu_vunmarshal() must
be deferred (via aio_bh_schedule_oneshot()), because
xen_9pfs_pdu_v(un)marshal() are running within a coroutine context
which makes them unsafe [1] for calling v9fs_reset() directly, as
the latter e.g. has a loop like:

    while (!QLIST_EMPTY(&s->active_list)) {
        aio_poll(qemu_get_aio_context(), true);
    }

which would a) never terminate (as the coroutine is on the
active_list) and b) aio_poll() is marked as no_coroutine_fn.

[1] https://lore.kernel.org/qemu-devel/3351181.5fSG56mABF@weasel/

And finally, add an idempotent guard to xen_9pfs_disconnect(),
just for the case.

Fixes: b37eeb0201 ("xen/9pfs: introduce Xen 9pfs backend")
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/xen-9p-backend.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 24c90d97ec..edb65a7afc 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -68,6 +68,11 @@ typedef struct Xen9pfsDev {
 
 static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
 
+static void xen_9pfs_disconnect_bh(void *opaque)
+{
+    xen_9pfs_disconnect(opaque);
+}
+
 static void xen_9pfs_in_sg(Xen9pfsRing *ring,
                            struct iovec *in_sg,
                            int *num,
@@ -150,7 +155,8 @@ static ssize_t xen_9pfs_pdu_vmarshal(V9fsPDU *pdu,
                       "Failed to encode VirtFS reply type %d\n",
                       pdu->id + 1);
         xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
-        xen_9pfs_disconnect(&xen_9pfs->xendev);
+        aio_bh_schedule_oneshot(qemu_get_aio_context(),
+                                xen_9pfs_disconnect_bh, &xen_9pfs->xendev);
     }
     return ret;
 }
@@ -173,7 +179,8 @@ static ssize_t xen_9pfs_pdu_vunmarshal(V9fsPDU *pdu,
         xen_pv_printf(&xen_9pfs->xendev, 0,
                       "Failed to decode VirtFS request type %d\n", pdu->id);
         xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing);
-        xen_9pfs_disconnect(&xen_9pfs->xendev);
+        aio_bh_schedule_oneshot(qemu_get_aio_context(),
+                                xen_9pfs_disconnect_bh, &xen_9pfs->xendev);
     }
     return ret;
 }
@@ -368,10 +375,18 @@ static void xen_9pfs_evtchn_event(void *opaque)
 static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev)
 {
     Xen9pfsDev *xen_9pdev = container_of(xendev, Xen9pfsDev, xendev);
+    V9fsState *s = &xen_9pdev->state;
     int i;
 
+    if (!xen_9pdev->rings) {
+        return;
+    }
+
     trace_xen_9pfs_disconnect(xendev->name);
 
+    v9fs_reset(s);
+    v9fs_device_unrealize_common(s);
+
     for (i = 0; i < xen_9pdev->num_rings; i++) {
         if (xen_9pdev->rings[i].evtchndev != NULL) {
             qemu_set_fd_handler(qemu_xen_evtchn_fd(xen_9pdev->rings[i].evtchndev),
-- 
2.47.3