## Host environment - Operating system: Linux (Debian/gLinux) - Architecture: x86_64 - QEMU flavor: `qemu-system-x86_64` - QEMU version: `master` (`v11.0.0-rc0`, base commit `8db19f3ad9`) - Reported by: Hui Peng <benquike@gmail.com> - QEMU command line: ```sh ./qemu-system-x86_64 -M q35 -m 1G -drive file=/tmp/nvme0.img,if=none,id=drv0 -device nvme,serial=deadbeef,id=nvme0 -device nvme-ns,drive=drv0,bus=nvme0,nsid=1 ``` ## Impact **Heap buffer overflow**, **heap use-after-free**, and **NULL pointer dereference** (`CWE-787`, `CWE-416`, `CWE-476`). ## Description of problem Multiple memory-safety vulnerabilities exist in NVMe controller emulation (`hw/nvme/ctrl.c`, `hw/nvme/ns.c`, `hw/nvme/subsys.c`): ### 1. Cross-namespace and concurrent-format `nvme_copy` heap buffer overflow (`#027`, `#028`, `#134`, `#156`, `#220`) - `nvme_copy_matching_ns_format()` failed to check `sns->lbaf.ds == dns->lbaf.ds` (logical block data size power-of-two). When copying from a source namespace with a smaller LBA size to a destination namespace with a larger LBA size (or when a namespace is reformatted via `NVME_ADM_CMD_FORMAT_NVM` while an async `nvme_copy` is in flight), `nvme_copy_in_completed_cb()` / `nvme_copy_out_cb()` compute `nvme_l2b(dns, nlb)` exceeding `iocb->blen`, overflowing the `iocb->bounce` heap buffer. - `nvme_do_format()` now sets `iocb->ns->status = NVME_FORMAT_IN_PROGRESS` before `nvme_ns_drain(iocb->ns)` and `nvme_copy_*_cb` validates `lbaf.ds` and `nvme_l2b(..., nlb) + mlen <= iocb->blen` before every bounce buffer access. ### 2. Duplicate `dbbuf` `ioeventfd` registration heap Use-After-Free (`#050` / `a8d8de43f5`) Issuing `NVME_ADM_CMD_DBBUF_CONFIG` multiple times (or when `cq`/`sq` already has an active `EventNotifier`) re-initializes `sq->notifier` / `cq->notifier` and registers a duplicate handler on the EventNotifier list without cleaning up the existing registration, leading to a heap Use-After-Free when the queue is later deleted. ### 3. FDP NULL `ns` dereference, ZRWA underflow, and CMB/PMR DMA validation (`#120`, `#156`, `#169`) - Fixes NULL `ns` dereference in FDP commands (`#120`), ZRWA range underflows and CMB/PMR DMA bounds (`#169`), and submission/completion queue bounds (`#156`). ## Patch The proposed consolidated patch applies cleanly to current `master` (`8db19f3ad9`). ```diff diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index c641226e27..ff0d9c45bb 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -687,8 +687,9 @@ static void nvme_irq_assert(NvmeCtrl *n, NvmeCQueue *cq) msix_notify(pci, cq->vector); } else { trace_pci_nvme_irq_pin(); - assert(cq->vector < 32); - n->irq_status |= 1 << cq->vector; + if (cq->vector < 32) { + n->irq_status |= 1U << cq->vector; + } nvme_irq_check(n); } } else { @@ -702,9 +703,8 @@ static void nvme_irq_deassert(NvmeCtrl *n, NvmeCQueue *cq) if (msix_enabled(PCI_DEVICE(n))) { return; } else { - assert(cq->vector < 32); - if (!n->cq_pending) { - n->irq_status &= ~(1 << cq->vector); + if (!n->cq_pending && cq->vector < 32) { + n->irq_status &= ~(1U << cq->vector); } nvme_irq_check(n); } @@ -1324,6 +1324,10 @@ static uint16_t nvme_tx_interleaved(NvmeCtrl *n, NvmeSg *sg, uint8_t *ptr, assert(sg->flags & NVME_SG_ALLOC); while (len) { + if (sg_idx >= (dma ? sg->qsg.nsg : sg->iov.niov)) { + return NVME_INVALID_FIELD | NVME_DNR; + } + sge_len = dma ? sg->qsg.sg[sg_idx].len : sg->iov.iov[sg_idx].iov_len; if (sge_len - offset < 0) { @@ -1343,18 +1347,23 @@ static uint16_t nvme_tx_interleaved(NvmeCtrl *n, NvmeSg *sg, uint8_t *ptr, if (dma) { addr = sg->qsg.sg[sg_idx].base + offset; - } else { - addr = (hwaddr)(uintptr_t)sg->iov.iov[sg_idx].iov_base + offset; - } + if (dir == NVME_TX_DIRECTION_TO_DEVICE) { + ret = nvme_addr_read(n, addr, ptr, trans_len); + } else { + ret = nvme_addr_write(n, addr, ptr, trans_len); + } - if (dir == NVME_TX_DIRECTION_TO_DEVICE) { - ret = nvme_addr_read(n, addr, ptr, trans_len); + if (ret) { + return NVME_DATA_TRAS_ERROR; + } } else { - ret = nvme_addr_write(n, addr, ptr, trans_len); - } + uint8_t *iov_ptr = (uint8_t *)sg->iov.iov[sg_idx].iov_base + offset; - if (ret) { - return NVME_DATA_TRAS_ERROR; + if (dir == NVME_TX_DIRECTION_TO_DEVICE) { + memcpy(ptr, iov_ptr, trans_len); + } else { + memcpy(iov_ptr, ptr, trans_len); + } } ptr += trans_len; @@ -1508,8 +1517,13 @@ static void nvme_update_cq_eventidx(const NvmeCQueue *cq) static void nvme_update_cq_head(NvmeCQueue *cq) { - ldl_le_pci_dma(PCI_DEVICE(cq->ctrl), cq->db_addr, &cq->head, + uint32_t head; + + ldl_le_pci_dma(PCI_DEVICE(cq->ctrl), cq->db_addr, &head, MEMTXATTRS_UNSPECIFIED); + if (head < cq->size) { + cq->head = head; + } trace_pci_nvme_update_cq_head(cq->cqid, cq->head); } @@ -2068,12 +2082,23 @@ static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns, return NVME_SUCCESS; } + if ((flags & NVME_ZRM_ZRWA) && !(zone->d.za & NVME_ZA_ZRWA_VALID)) { + status = nvme_zns_check_resources(ns, 0, 0, 1); + if (status) { + return status; + } + } + nvme_assign_zone_state(ns, zone, NVME_ZONE_STATE_EXPLICITLY_OPEN); /* fallthrough */ case NVME_ZONE_STATE_EXPLICITLY_OPEN: - if (flags & NVME_ZRM_ZRWA) { + if ((flags & NVME_ZRM_ZRWA) && !(zone->d.za & NVME_ZA_ZRWA_VALID)) { + status = nvme_zns_check_resources(ns, 0, 0, 1); + if (status) { + return status; + } ns->zns.numzrwa--; zone->d.za |= NVME_ZA_ZRWA_VALID; @@ -2775,6 +2800,7 @@ typedef struct NvmeCopyAIOCB { int idx; uint8_t *bounce; + size_t blen; QEMUIOVector iov; struct { BlockAcctCookie read; @@ -2998,6 +3024,11 @@ static void nvme_copy_out_cb(void *opaque, int ret) &nlb, NULL, NULL, NULL, NULL); mlen = nvme_m2b(dns, nlb); + if (nvme_l2b(dns, nlb) + mlen > iocb->blen) { + iocb->ret = -1; + req->status = NVME_CMD_INCOMP_NS_OR_FMT | NVME_DNR; + goto out; + } mbounce = iocb->bounce + nvme_l2b(dns, nlb); qemu_iovec_reset(&iocb->iov); @@ -3046,6 +3077,11 @@ static void nvme_copy_in_completed_cb(void *opaque, int ret) trace_pci_nvme_copy_out(iocb->slba, nlb); len = nvme_l2b(sns, nlb); + if (sns->lbaf.ds != dns->lbaf.ds || + len + MAX(nvme_m2b(sns, nlb), nvme_m2b(dns, nlb)) > iocb->blen) { + status = NVME_CMD_INCOMP_NS_OR_FMT | NVME_DNR; + goto invalid; + } if (NVME_ID_NS_DPS_TYPE(sns->id_ns.dps)) { copy = (NvmeCopyCmd *)&req->cmd; @@ -3151,6 +3187,11 @@ static void nvme_copy_in_cb(void *opaque, int ret) &nlb, NULL, NULL, NULL, NULL); mlen = nvme_m2b(sns, nlb); + if (nvme_l2b(sns, nlb) + mlen > iocb->blen) { + iocb->ret = -1; + iocb->req->status = NVME_CMD_INCOMP_NS_OR_FMT | NVME_DNR; + goto out; + } qemu_iovec_reset(&iocb->iov); qemu_iovec_add(&iocb->iov, iocb->bounce + nvme_l2b(sns, nlb), mlen); @@ -3198,7 +3239,8 @@ static bool nvme_copy_matching_ns_format(NvmeNamespace *sns, NvmeNamespace *dns, static inline bool nvme_copy_corresp_pi_match(NvmeNamespace *sns, NvmeNamespace *dns) { - return sns->lbaf.ms == 0 && + return sns->lbaf.ds == dns->lbaf.ds && + sns->lbaf.ms == 0 && ((dns->lbaf.ms == 8 && dns->pif == 0) || (dns->lbaf.ms == 16 && dns->pif == 1)); } @@ -3245,6 +3287,11 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb) goto done; } + if (dns->status) { + status = dns->status; + goto invalid; + } + if (iocb->format == 2 || iocb->format == 3) { nvme_copy_source_range_parse(iocb->ranges, iocb->idx, iocb->format, &slba, &nlb, &snsid, NULL, NULL, NULL); @@ -3259,7 +3306,12 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb) status = NVME_INVALID_FIELD | NVME_DNR; goto invalid; } + if (iocb->sns->status) { + status = iocb->sns->status; + goto invalid; + } } else { + iocb->sns = dns; if (((slba + nlb) > iocb->slba) && ((slba + nlb) < (iocb->slba + iocb->tcl))) { status = NVME_CMD_OVERLAP_IO_RANGE | NVME_DNR; @@ -3267,6 +3319,7 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb) } } } else { + iocb->sns = dns; nvme_copy_source_range_parse(iocb->ranges, iocb->idx, iocb->format, &slba, &nlb, NULL, NULL, NULL, NULL); } @@ -3357,6 +3410,7 @@ static void nvme_do_copy(NvmeCopyAIOCB *iocb) sns->lbasz + MAX(sns->lbaf.ms, dns->lbaf.ms))); iocb->bounce = g_malloc(blen); + iocb->blen = blen; qemu_iovec_reset(&iocb->iov); assert(len <= blen); @@ -3748,6 +3802,7 @@ static void nvme_do_write_fdp(NvmeCtrl *n, NvmeRequest *req, uint64_t slba, !nvme_parse_pid(ns, pid, &ph, &rg)) { ph = 0; rg = 0; + pid = 0; } ruhid = ns->fdp.phs[ph]; @@ -3763,7 +3818,9 @@ static void nvme_do_write_fdp(NvmeCtrl *n, NvmeRequest *req, uint64_t slba, } nlb -= ru->ruamw; - nvme_update_ruh(n, ns, pid); + if (!nvme_update_ruh(n, ns, pid) || ru->ruamw == 0) { + break; + } } } @@ -3872,10 +3929,6 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append, if (status) { goto invalid; } - - if (!(zone->d.za & NVME_ZA_ZRWA_VALID)) { - zone->w_ptr += nlb; - } } else if (ns->endgrp && ns->endgrp->fdp.enabled) { nvme_do_write_fdp(n, req, slba, nlb); } @@ -3883,7 +3936,12 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append, data_offset = nvme_l2b(ns, slba); if (NVME_ID_NS_DPS_TYPE(ns->id_ns.dps)) { - return nvme_dif_rw(n, req); + status = nvme_dif_rw(n, req); + if (status == NVME_NO_COMPLETE && ns->params.zoned && + !(zone->d.za & NVME_ZA_ZRWA_VALID)) { + zone->w_ptr += nlb; + } + return status; } if (!wrz) { @@ -3891,7 +3949,13 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append, if (status) { goto invalid; } + } + if (ns->params.zoned && !(zone->d.za & NVME_ZA_ZRWA_VALID)) { + zone->w_ptr += nlb; + } + + if (!wrz) { block_acct_start(blk_get_stats(blk), &req->acct, data_size, BLOCK_ACCT_WRITE); nvme_blk_write(blk, data_offset, BDRV_SECTOR_SIZE, nvme_rw_cb, req); @@ -4258,7 +4322,8 @@ static uint16_t nvme_zone_mgmt_send_zrwa_flush(NvmeCtrl *n, NvmeZone *zone, return NVME_INVALID_FIELD | NVME_DNR; } - if (elba < wp || elba > wp + ns->zns.zrwas) { + if (elba < wp || elba >= wp + ns->zns.zrwas || + elba >= nvme_zone_wr_boundary(zone)) { return NVME_ZONE_BOUNDARY_ERROR | NVME_DNR; } @@ -6627,7 +6692,9 @@ static uint16_t nvme_set_feature_fdp_events(NvmeCtrl *n, NvmeNamespace *ns, g_autofree uint8_t *events = NULL; NvmeRuHandle *ruh = NULL; - assert(ns); + if (!ns) { + return NVME_FEAT_NOT_NS_SPEC | NVME_DNR; + } if (!n->subsys || !n->subsys->endgrp.fdp.enabled) { return NVME_FDP_DISABLED | NVME_DNR; @@ -7130,6 +7197,10 @@ done: static uint16_t nvme_format_check(NvmeNamespace *ns, uint8_t lbaf, uint8_t pi) { + if (ns->status) { + return ns->status; + } + if (ns->params.zoned) { return NVME_INVALID_FORMAT | NVME_DNR; } @@ -7154,7 +7225,6 @@ static void nvme_do_format(NvmeFormatAIOCB *iocb) NvmeRequest *req = iocb->req; NvmeCtrl *n = nvme_ctrl(req); uint32_t dw10 = le32_to_cpu(req->cmd.cdw10); - uint8_t lbaf = dw10 & 0xf; uint8_t pi = (dw10 >> 5) & 0x7; uint16_t status; int i; @@ -7177,13 +7247,14 @@ static void nvme_do_format(NvmeFormatAIOCB *iocb) goto done; } - status = nvme_format_check(iocb->ns, lbaf, pi); + status = nvme_format_check(iocb->ns, iocb->lbaf, pi); if (status) { req->status = status; goto done; } iocb->ns->status = NVME_FORMAT_IN_PROGRESS; + nvme_ns_drain(iocb->ns); nvme_format_ns_cb(iocb, 0); return; @@ -7438,7 +7509,8 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const NvmeRequest *req) sq->ei_addr = eis_addr + (i << 3); stl_le_pci_dma(pci, sq->db_addr, sq->tail, MEMTXATTRS_UNSPECIFIED); - if (n->params.ioeventfd && sq->sqid != 0) { + if (n->params.ioeventfd && sq->sqid != 0 && + !sq->ioeventfd_enabled) { if (!nvme_init_sq_ioeventfd(sq)) { sq->ioeventfd_enabled = true; } @@ -7451,7 +7523,8 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const NvmeRequest *req) cq->ei_addr = eis_addr + (i << 3) + (1 << 2); stl_le_pci_dma(pci, cq->db_addr, cq->head, MEMTXATTRS_UNSPECIFIED); - if (n->params.ioeventfd && cq->cqid != 0) { + if (n->params.ioeventfd && cq->cqid != 0 && + !cq->ioeventfd_enabled) { if (!nvme_init_cq_ioeventfd(cq)) { cq->ioeventfd_enabled = true; } @@ -7797,8 +7870,13 @@ static void nvme_update_sq_eventidx(const NvmeSQueue *sq) static void nvme_update_sq_tail(NvmeSQueue *sq) { - ldl_le_pci_dma(PCI_DEVICE(sq->ctrl), sq->db_addr, &sq->tail, + uint32_t tail; + + ldl_le_pci_dma(PCI_DEVICE(sq->ctrl), sq->db_addr, &tail, MEMTXATTRS_UNSPECIFIED); + if (tail < sq->size) { + sq->tail = tail; + } trace_pci_nvme_update_sq_tail(sq->sqid, sq->tail); } @@ -8615,7 +8693,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val) } if (cq->tail == cq->head) { - if (cq->irq_enabled) { + if (cq->irq_enabled && n->cq_pending) { n->cq_pending--; } ```