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

Klaus Jensen posted 1 patch 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260618-fix-heap-uaf-ctrl-reset-v1-1-806ec08dd951@samsung.com
Maintainers: Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>, Jesper Devantier <foss@defmacro.it>
There is a newer version of this series
hw/nvme/ctrl.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
[PATCH] hw/nvme: fix potential use-after-free doing controller reset
Posted by Klaus Jensen 1 month, 1 week 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>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 815f39173c8a..75768272f241 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7906,10 +7906,40 @@ static void nvme_ctrl_reset(NvmeCtrl *n, NvmeResetType rst)
     }
 
     for (i = 0; i < n->params.max_ioqpairs + 1; i++) {
-        if (n->sq[i] != NULL) {
-            nvme_free_sq(n->sq[i], n);
+        NvmeRequest *req, *next;
+
+        NvmeSQueue *sq = n->sq[i];
+        NvmeCQueue *cq;
+
+        if (!sq) {
+            continue;
         }
+
+        /* cancel any outstanding requests */
+        while (!QTAILQ_EMPTY(&sq->out_req_list)) {
+            req = QTAILQ_FIRST(&sq->out_req_list);
+            assert(req->aiocb);
+            req->status = NVME_NO_COMPLETE;
+            blk_aio_cancel(req->aiocb);
+        }
+
+        assert(QTAILQ_EMPTY(&sq->out_req_list));
+
+        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->params.max_ioqpairs + 1; i++) {
         if (n->cq[i] != NULL) {
             nvme_free_cq(n->cq[i], n);

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

Best regards,
-- 
Klaus Jensen <k.jensen@samsung.com>
Re: [PATCH] hw/nvme: fix potential use-after-free doing controller reset
Posted by Philippe Mathieu-Daudé 1 month, 1 week ago
Hi Klaus,

On 18/6/26 09:56, Klaus Jensen wrote:
> 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>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/nvme/ctrl.c | 34 ++++++++++++++++++++++++++++++++--
>   1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 815f39173c8a..75768272f241 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -7906,10 +7906,40 @@ static void nvme_ctrl_reset(NvmeCtrl *n, NvmeResetType rst)
>       }
>   
>       for (i = 0; i < n->params.max_ioqpairs + 1; i++) {
> -        if (n->sq[i] != NULL) {
> -            nvme_free_sq(n->sq[i], n);
> +        NvmeRequest *req, *next;
> +
> +        NvmeSQueue *sq = n->sq[i];
> +        NvmeCQueue *cq;
> +
> +        if (!sq) {
> +            continue;
>           }
> +
> +        /* cancel any outstanding requests */
> +        while (!QTAILQ_EMPTY(&sq->out_req_list)) {
> +            req = QTAILQ_FIRST(&sq->out_req_list);
> +            assert(req->aiocb);
> +            req->status = NVME_NO_COMPLETE;
> +            blk_aio_cancel(req->aiocb);
> +        }
> +
> +        assert(QTAILQ_EMPTY(&sq->out_req_list));
> +
> +        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);
>       }

Could we factor common code with nvme_del_sq() and call it instead?

> +
>       for (i = 0; i < n->params.max_ioqpairs + 1; i++) {
>           if (n->cq[i] != NULL) {
>               nvme_free_cq(n->cq[i], n);
> 
> ---
> base-commit: c7cf7c810153d6f5f31aa2d5c0dee9087f6b4dff
> change-id: 20260618-fix-heap-uaf-ctrl-reset-cf05cbc0e729
> 
> Best regards,