Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
From:   Yuanyuan Zhong <yzhong@purestorage.com>
To:     kbusch@kernel.org, axboe@kernel.dk, hch@lst.de, sagi@grimberg.me
Cc:     linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
        randyj@purestorage.com, hcoutinho@purestorage.com,
        Yuanyuan Zhong <yzhong@purestorage.com>
Subject: [PATCH] nvme-core: remove head->effects to fix use-after-free
Date:   Wed, 15 Nov 2023 11:54:39 -0700
Message-Id: <20231115185439.2616073-1-yzhong@purestorage.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

The head->effects stores a pointer to the controller's Command Sets
Supported and Effects log. When the namespace head is shared by multiple
controllers, if the particular controller is removed, dereferencing
head->effects causes use-after-free:

[  227.820066] ==================================================================
[  227.820069] BUG: KFENCE: use-after-free read in nvme_command_effects+0x1f/0x90 [nvme_core]

[  227.820091] Use-after-free read at 0x00000000c02dadcf (in kfence-#0):
[  227.820094]  nvme_command_effects+0x1f/0x90 [nvme_core]
[  227.820107]  nvme_passthru_start+0x19/0x80 [nvme_core]
[  227.820121]  nvme_submit_user_cmd+0xc7/0x170 [nvme_core]
[  227.820136]  nvme_user_cmd.constprop.16+0x152/0x1d0 [nvme_core]
[  227.820149]  nvme_ns_head_ioctl+0x92/0x140 [nvme_core]
[  227.820162]  blkdev_ioctl+0x1c5/0x280
[  227.820169]  __x64_sys_ioctl+0x93/0xd0
[  227.820174]  do_syscall_64+0x45/0xf0
[  227.820177]  entry_SYSCALL_64_after_hwframe+0x6e/0x76

[  227.820182] kfence-#0: 0x000000000fac1d5d-0x00000000a28a73c3, size=4096, cache=kmalloc-4k

[  227.820185] allocated by task 2559 on cpu 3 at 188.703118s:
[  227.820233]  __kmem_cache_alloc_node+0x3c9/0x410
[  227.820236]  kmalloc_trace+0x2a/0xa0
[  227.820238]  nvme_get_effects_log+0x68/0xd0 [nvme_core]
[  227.820251]  nvme_init_identify+0x5ef/0x640 [nvme_core]
[  227.820263]  nvme_init_ctrl_finish+0x8d/0x250 [nvme_core]
[  227.820275]  nvme_tcp_setup_ctrl+0x1ce/0x710 [nvme_tcp]
[  227.820281]  nvme_tcp_create_ctrl+0x359/0x450 [nvme_tcp]
[  227.820286]  nvmf_dev_write+0x203/0x3b0 [nvme_fabrics]
[  227.820292]  vfs_write+0xd2/0x3d0
[  227.820294]  ksys_write+0x5d/0xd0
[  227.820297]  do_syscall_64+0x45/0xf0
[  227.820298]  entry_SYSCALL_64_after_hwframe+0x6e/0x76

[  227.820302] freed by task 2521 on cpu 28 at 220.115945s:
[  227.820329]  nvme_free_ctrl+0xa6/0x260 [nvme_core]
[  227.820342]  device_release+0x37/0x90
[  227.820345]  kobject_release+0x57/0x120
[  227.820347]  nvme_sysfs_delete+0x39/0x50 [nvme_core]
[  227.820360]  kernfs_fop_write_iter+0x144/0x1e0
[  227.820362]  vfs_write+0x25f/0x3d0
[  227.820364]  ksys_write+0x5d/0xd0
[  227.820366]  do_syscall_64+0x45/0xf0
[  227.820368]  entry_SYSCALL_64_after_hwframe+0x6e/0x76

Fix this by removing head->effects. Use the Commands Supported and Effects log
in ctrl->cels directly.

Fixes: be93e87e7802 ("nvme: support for multiple Command Sets Supported and Effects log pages")
Signed-off-by: Yuanyuan Zhong <yzhong@purestorage.com>
Reviewed-by: Randy Jennings <randyj@purestorage.com>
Reviewed-by: Hamilton Coutinho <hcoutinho@purestorage.com>
---
 drivers/nvme/host/core.c | 17 ++++++++---------
 drivers/nvme/host/nvme.h |  1 -
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 88b54cdcbd68..14fdc2de3a75 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1085,7 +1085,9 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 	u32 effects = 0;
 
 	if (ns) {
-		effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
+		struct nvme_effects_log	*cel = xa_load(&ctrl->cels, ns->head->ids.csi);
+
+		effects = le32_to_cpu(cel->iocs[opcode]);
 		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
 			dev_warn_once(ctrl->device,
 				"IO command:%02x has unusual effects:%08x\n",
@@ -2872,7 +2874,8 @@ static int nvme_get_effects_log(struct nvme_ctrl *ctrl, u8 csi,
 
 	xa_store(&ctrl->cels, csi, cel, GFP_KERNEL);
 out:
-	*log = cel;
+	if (log)
+		*log = cel;
 	return 0;
 }
 
@@ -3388,13 +3391,6 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	head->shared = info->is_shared;
 	kref_init(&head->ref);
 
-	if (head->ids.csi) {
-		ret = nvme_get_effects_log(ctrl, head->ids.csi, &head->effects);
-		if (ret)
-			goto out_cleanup_srcu;
-	} else
-		head->effects = ctrl->effects;
-
 	ret = nvme_mpath_alloc_disk(ctrl, head);
 	if (ret)
 		goto out_cleanup_srcu;
@@ -3779,6 +3775,9 @@ static void nvme_scan_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	if (ret || !info.is_ready)
 		return;
 
+	if (info.ids.csi && nvme_get_effects_log(ctrl, info.ids.csi, NULL))
+		return;
+
 	ns = nvme_find_get_ns(ctrl, nsid);
 	if (ns) {
 		nvme_validate_ns(ns, &info);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 39a90b7cb125..38298c072ec3 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -445,7 +445,6 @@ struct nvme_ns_head {
 	struct kref		ref;
 	bool			shared;
 	int			instance;
-	struct nvme_effects_log *effects;
 
 	struct cdev		cdev;
 	struct device		cdev_device;
-- 
2.34.1