[PATCH v2] hw/nvme: fix potential use-after-free doing controller reset

Klaus Jensen posted 1 patch 3 weeks, 2 days ago
Failed in applying to current master (apply log)
hw/nvme/ctrl.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 52 insertions(+), 10 deletions(-)
[PATCH v2] hw/nvme: fix potential use-after-free doing controller reset
Posted by Klaus Jensen 3 weeks, 2 days ago
From: Klaus Jensen <k.jensen@samsung.com>

A controller reset may race with inflight DMA I/O. SQ deletion (when
done by the host through the Delete I/O Submission Queue command)
handles this, but the controller reset path does not, which may lead to
the freeing request resources prior to the DMA completing in certain
conditions under large and heavy I/O pressure.

Cc: qemu-stable@nongnu.org
Reported-by: Jia Jia <physicalmtea@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3398
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
Changes in v2:
- Factor out code from nvme_del_sq and reuse (Philippe)
- Link to v1: https://lore.kernel.org/qemu-devel/20260618-fix-heap-uaf-ctrl-reset-v1-1-806ec08dd951@samsung.com
---
 hw/nvme/ctrl.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 10 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index a67e1598891c..7ff0fef41e46 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4840,6 +4840,20 @@ static void nvme_free_sq(NvmeSQueue *sq, NvmeCtrl *n)
     }
 }
 
+static void nvme_iosq_cancel_all(NvmeSQueue *sq, uint16_t status)
+{
+    assert(sq->sqid);
+
+    while (!QTAILQ_EMPTY(&sq->out_req_list)) {
+        NvmeRequest *req = QTAILQ_FIRST(&sq->out_req_list);
+
+        assert(req->aiocb);
+        blk_aio_cancel(req->aiocb);
+
+        req->status = status;
+    }
+}
+
 static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
 {
     NvmeDeleteQ *c = (NvmeDeleteQ *)&req->cmd;
@@ -4856,14 +4870,8 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
     trace_pci_nvme_del_sq(qid);
 
     sq = n->sq[qid];
-    while (!QTAILQ_EMPTY(&sq->out_req_list)) {
-        r = QTAILQ_FIRST(&sq->out_req_list);
-        assert(r->aiocb);
-        r->status = NVME_CMD_ABORT_SQ_DEL;
-        blk_aio_cancel(r->aiocb);
-    }
 
-    assert(QTAILQ_EMPTY(&sq->out_req_list));
+    nvme_iosq_cancel_all(sq, NVME_CMD_ABORT_SQ_DEL);
 
     if (!nvme_check_cqid(n, sq->cqid)) {
         cq = n->cq[sq->cqid];
@@ -8022,11 +8030,45 @@ static void nvme_ctrl_reset(NvmeCtrl *n, NvmeResetType rst)
         nvme_ns_drain(ns);
     }
 
-    for (i = 0; i < n->num_queues; i++) {
-        if (n->sq[i] != NULL) {
-            nvme_free_sq(n->sq[i], n);
+    /*
+     * The ASQ can be immediately freed since the only requests that may linger
+     * in the out_req_list are AERs.
+     */
+    if (n->sq[0]) {
+        nvme_free_sq(n->sq[0], n);
+    }
+
+    /*
+     * The IOSQs need more care since they may have AIOCBs lingering that needs
+     * to be completed before we can free the sqes.
+     */
+    for (i = 1; i < n->num_queues; i++) {
+        NvmeRequest *req, *next;
+
+        NvmeSQueue *sq = n->sq[i];
+        NvmeCQueue *cq;
+
+        if (!sq) {
+            continue;
         }
+
+        nvme_iosq_cancel_all(sq, NVME_NO_COMPLETE);
+
+        cq = n->cq[sq->cqid];
+
+        /* blk_aio_cancel may enqueue completions on the cq; drop them */
+        QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
+            if (req->sq != sq) {
+                continue;
+            }
+
+            QTAILQ_REMOVE(&cq->req_list, req, entry);
+            QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
+        }
+
+        nvme_free_sq(n->sq[i], n);
     }
+
     for (i = 0; i < n->num_queues; i++) {
         if (n->cq[i] != NULL) {
             nvme_free_cq(n->cq[i], n);

---
base-commit: ef18d154e626a5c4f868493b6d7f722065fa16b9
change-id: 20260618-fix-heap-uaf-ctrl-reset-cf05cbc0e729

Best regards,
-- 
Klaus Jensen <k.jensen@samsung.com>