[PATCH v3 0/5] nvme: a few error-path and validation fixes

[PATCH v3 0/5] nvme: a few error-path and validation fixes

[Guixin Liu]

This series collects independent fixes found while auditing error and
command-processing paths in the NVMe host and target drivers. Each is
standalone; they only share the same subsystem.

  1. nvmet: an Identify CNS 07h (active NS list for a command set)
     dereferences req->ns, which is always NULL on that path -> NULL
     pointer oops. Also fixes the filter to test the iterated ns->csi.

  2. nvmet: nvmet_ns_enable() ignores the percpu_ref_init() return value
     and reports success to userspace when it fails, leaving the ns dead.

  3. nvme-pci: the per-NUMA-node descriptor DMA pools, created lazily on
     admin tag set allocation, are only freed in nvme_remove(); a probe
     failure after that point leaks them.

  4. nvme: nvme_query_fdp_info() trusts the device-supplied nruhsd count
     and can read past the fixed-size RUH status buffer (heap OOB read).

  5. nvme: follow-up to #4, suggested by Kanchan -- the RUH status buffer
     and the placement handle clamp used the odd S8_MAX - 1 value; raise
     the cap to the meaningful U8_MAX (bio->bi_write_stream is u8) and
     warn on overflow.

Found by code audit. #1 and #2 are functional/crash fixes; #3 is a
resource leak; #4 is device-triggered hardening; #5 tidies up the cap
introduced by #4.

A blktests regression test for #1 (Identify CNS 07h) will be sent
separately to the blktests list.

v2 -> v3:
  - patch 5: addressed Kanchan's review -- move the overflow check below
    the zero-length check and fold the clamp into the warn branch (drop
    min_t, assign NVME_MAX_PLIDS directly); collected his Reviewed-by.

v1 -> v2:
  - Collected Reviewed-by tags from Hannes, Christoph, Kanchan and Nilay,
    thanks.
  - patch 1: added the observed NULL-deref oops splat to the commit log.
  - Added patch 5 (raise the FDP handle cap to U8_MAX and warn on
    overflow), as discussed on patch 4 with Keith, Christoph and Kanchan.

Guixin Liu (5):
  nvmet: fix NULL pointer dereference in nvmet_execute_identify_nslist()
  nvmet: propagate percpu_ref_init() failure in nvmet_ns_enable()
  nvme-pci: release descriptor pools on probe failure
  nvme: clamp FDP placement handle count to the buffer size
  nvme: raise FDP placement handle cap to U8_MAX and warn on overflow

 drivers/nvme/host/core.c        | 16 +++++++++++++++-
 drivers/nvme/host/pci.c         |  1 +
 drivers/nvme/target/admin-cmd.c |  2 +-
 drivers/nvme/target/core.c      |  3 ++-
 4 files changed, 19 insertions(+), 3 deletions(-)


base-commit: 11028ab62899e4191e074ee364c712b77823a9c4
-- 
2.43.7

[PATCH v3 1/5] nvmet: fix NULL pointer dereference in nvmet_execute_identify_nslist()

[Guixin Liu]

When a host issues an Identify command with CNS 07h (Active Namespace ID
List for a specific I/O Command Set), nvmet_execute_identify_nslist() is
called with match_css set. The command-set filter dereferences req->ns,
but this handler never calls nvmet_req_find_ns(), so req->ns is always
NULL (nvmet_req_init() resets it to NULL). As soon as an enabled
namespace with an NSID greater than the requested value exists,
req->ns->csi dereferences a NULL pointer and oopses.

Besides the crash, the comparison is logically wrong: to filter the list
by command set it must test the command set of the namespace being
iterated, not a single fixed value. Use the loop variable ns->csi.

Fixes: 61c9967cd634 ("nvmet: implement active command set ns list")
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/admin-cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 01b799e92ae6..ab6a0a98dd5d 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -958,7 +958,7 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req, bool match_css)
 	nvmet_for_each_enabled_ns(&ctrl->subsys->namespaces, idx, ns) {
 		if (ns->nsid <= min_nsid)
 			continue;
-		if (match_css && req->ns->csi != req->cmd->identify.csi)
+		if (match_css && ns->csi != req->cmd->identify.csi)
 			continue;
 		list[i++] = cpu_to_le32(ns->nsid);
 		if (i == buf_size / sizeof(__le32))
-- 
2.43.7

[PATCH v3 2/5] nvmet: propagate percpu_ref_init() failure in nvmet_ns_enable()

[Guixin Liu]

The return value of percpu_ref_init() is discarded. At this point ret is
0 from the preceding successful steps, so when the allocation inside
percpu_ref_init() fails the code jumps to the out_pr_exit cleanup chain
which ends with "return ret", i.e. reports success. The configfs enable
store then tells userspace the namespace was enabled even though it was
not and its backing device has already been torn down.

Capture the return value so the failure is propagated.

Fixes: 408232680707 ("nvmet: Fix crash when a namespace is disabled")
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/target/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 4477c4d6b1ee..c8e01dda4121 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -610,7 +610,8 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
 			goto out_dev_put;
 	}
 
-	if (percpu_ref_init(&ns->ref, nvmet_destroy_namespace, 0, GFP_KERNEL))
+	ret = percpu_ref_init(&ns->ref, nvmet_destroy_namespace, 0, GFP_KERNEL);
+	if (ret)
 		goto out_pr_exit;
 
 	nvmet_ns_changed(subsys, ns->nsid);
-- 
2.43.7

[PATCH v3 3/5] nvme-pci: release descriptor pools on probe failure

[Guixin Liu]

The per-NUMA-node descriptor DMA pools are created lazily from
nvme_init_hctx_common() once the admin tag set is allocated, but they are
only destroyed in nvme_remove() via nvme_release_descriptor_pools(). Any
probe failure after the admin tag set has been allocated unwinds through
the out_disable label and nvme_pci_free_ctrl(), neither of which releases
the pools, leaking the dma_pool objects.

Release the descriptor pools in the out_disable error path. It must not
be added to nvme_pci_free_ctrl(), as that would double-free against
nvme_remove() on the normal teardown path.

Fixes: d977506f8863 ("nvme-pci: make PRP list DMA pools per-NUMA-node")
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/host/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 69932d640b53..f1b24a53aa02 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3838,6 +3838,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	nvme_dev_remove_admin(dev);
 	nvme_dbbuf_dma_free(dev);
 	nvme_free_queues(dev, 0);
+	nvme_release_descriptor_pools(dev);
 out_release_iod_mempool:
 	mempool_destroy(dev->dmavec_mempool);
 out_dev_unmap:
-- 
2.43.7

[PATCH v3 4/5] nvme: clamp FDP placement handle count to the buffer size

[Guixin Liu]

nvme_query_fdp_info() allocates the RUH status buffer for at most
S8_MAX - 1 descriptors and caps the io-mgmt-receive transfer to that
size. head->nr_plids, however, is taken verbatim from the device-supplied
nruhsd field, which can be up to 65535. If a non-conformant or malicious
device reports more descriptors than the buffer holds, the copy loop
reads past the end of the ruhs buffer (heap out-of-bounds read).

Clamp nr_plids to the number of descriptors the buffer can actually hold.

Fixes: 30b5f20bb2dd ("nvme: register fdp parameters with the block layer")
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/host/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 453c1f0b2dd0..b1f444cd3daf 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2358,6 +2358,7 @@ static int nvme_query_fdp_info(struct nvme_ns *ns, struct nvme_ns_info *info)
 	}
 
 	head->nr_plids = le16_to_cpu(ruhs->nruhsd);
+	head->nr_plids = min_t(u16, head->nr_plids, S8_MAX - 1);
 	if (!head->nr_plids)
 		goto free;
 
-- 
2.43.7

[PATCH v3 5/5] nvme: raise FDP placement handle cap to U8_MAX and warn on overflow

[Guixin Liu]

The RUH status buffer and the placement-handle clamp used S8_MAX - 1
(126) as the maximum descriptor count. That value was picked only so the
io-mgmt-receive result fit in a page, not because of any protocol or
driver restriction.

The meaningful upper bound is U8_MAX: write hints (bio->bi_write_stream)
are u8, so placement handles beyond U8_MAX can never be selected. Size
the buffer and clamp nr_plids to U8_MAX, and emit a dev_warn() when the
device reports more handles than the driver tracks so the truncation is
observable.

Suggested-by: Kanchan Joshi <joshi.k@samsung.com>
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/core.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b1f444cd3daf..14832be67779 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -33,6 +33,13 @@
 
 #define NVME_MINORS		(1U << MINORBITS)
 
+/*
+ * Write hints (bio->bi_write_stream) are u8, so FDP placement handles beyond
+ * U8_MAX can never be selected. Cap the handle count to bound both the RUH
+ * status buffer and the per-head plids array.
+ */
+#define NVME_MAX_PLIDS		U8_MAX
+
 struct nvme_ns_info {
 	struct nvme_ns_ids ids;
 	u32 nsid;
@@ -2342,7 +2349,7 @@ static int nvme_query_fdp_info(struct nvme_ns *ns, struct nvme_ns_info *info)
 	if (!info->runs)
 		return ret;
 
-	size = struct_size(ruhs, ruhsd, S8_MAX - 1);
+	size = struct_size(ruhs, ruhsd, NVME_MAX_PLIDS);
 	ruhs = kzalloc(size, GFP_KERNEL);
 	if (!ruhs)
 		return -ENOMEM;
@@ -2358,10 +2365,16 @@ static int nvme_query_fdp_info(struct nvme_ns *ns, struct nvme_ns_info *info)
 	}
 
 	head->nr_plids = le16_to_cpu(ruhs->nruhsd);
-	head->nr_plids = min_t(u16, head->nr_plids, S8_MAX - 1);
 	if (!head->nr_plids)
 		goto free;
 
+	if (head->nr_plids > NVME_MAX_PLIDS) {
+		dev_warn(ctrl->device,
+			 "FDP RUH status reports %u placement handles, capping at %u\n",
+			 head->nr_plids, NVME_MAX_PLIDS);
+		head->nr_plids = NVME_MAX_PLIDS;
+	}
+
 	head->plids = kcalloc(head->nr_plids, sizeof(*head->plids),
 			      GFP_KERNEL);
 	if (!head->plids) {
-- 
2.43.7

Re: [PATCH v3 5/5] nvme: raise FDP placement handle cap to U8_MAX and warn on overflow

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3 5/5] nvme: raise FDP placement handle cap to U8_MAX and warn on overflow

[Nilay Shroff]

On 8/4/26 7:49 AM, Guixin Liu wrote:
> he RUH status buffer and the placement-handle clamp used S8_MAX - 1
> (126) as the maximum descriptor count. That value was picked only so the
> io-mgmt-receive result fit in a page, not because of any protocol or
> driver restriction.
> 
> The meaningful upper bound is U8_MAX: write hints (bio->bi_write_stream)
> are u8, so placement handles beyond U8_MAX can never be selected. Size
> the buffer and clamp nr_plids to U8_MAX, and emit a dev_warn() when the
> device reports more handles than the driver tracks so the truncation is
> observable.
> 
> Suggested-by: Kanchan Joshi<joshi.k@samsung.com>
> Signed-off-by: Guixin Liu<kanie@linux.alibaba.com>
> Reviewed-by: Kanchan Joshi<joshi.k@samsung.com>

Looks good to me.

Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>