[PATCH] nvme-tcp: fix use-after-free in nvme_tcp_free_ctrl()

[PATCH] nvme-tcp: fix use-after-free in nvme_tcp_free_ctrl()

[Christian Aurich Zanettini Martins]

nvme_tcp_free_ctrl() reads ctrl->ctrl.opts to know how many queue
mutexes to destroy. When creating the controller fails,
nvmf_create_ctrl() frees opts as soon as nvme_tcp_create_ctrl()
returns, but a concurrent open of the character device can hold the
last reference to the controller. With CONFIG_DEBUG_KOBJECT_RELEASE,
which delays the release, every failed connect reads freed memory:

  BUG: KASAN: slab-use-after-free in nvme_tcp_free_ctrl+0x38b/0x430
  Read of size 4 at addr ffff888005b93740 by task kworker/2:1/51

Keep the size of the queue array in the controller and use it instead.

Fixes: 2eeeaa9c1ff0 ("nvme-tcp: Validate the queue state under the queue lock in the address attribute")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
---
Based on nvme-7.4. Nilay pointed this out on the nvme-rdma thread and
suggested the same fix:
https://lore.kernel.org/linux-nvme/1408da28-a55e-448b-b493-d60fd5d20524@linux.ibm.com/

Reproduced in QEMU with KASAN and CONFIG_DEBUG_KOBJECT_RELEASE by
connecting to a port with no listener. Each failed connect reports
three use-after-free reads without the patch and none with it. Growing
attr_qid_max from 2 to 4 and 30 resets, both under I/O, still pass with
and without lockdep.

 drivers/nvme/host/tcp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index a8e94e4866db..d11fb52ce1a2 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -204,6 +204,7 @@ struct nvme_tcp_ctrl {
 	struct delayed_work	connect_work;
 	struct nvme_tcp_request async_req;
 	u32			io_queues[HCTX_MAX_TYPES];
+	unsigned int		nr_allocated_queues;
 	struct net		*net;
 };
 
@@ -2861,8 +2862,6 @@ static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
 static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
 {
 	struct nvme_tcp_ctrl *ctrl = to_tcp_ctrl(nctrl);
-	/* the same count used to size and initialize the queue array */
-	unsigned int nr_queues = NVME_TCP_OPT_NR_QUEUES(nctrl->opts);
 	unsigned int i;
 
 	if (list_empty_careful(&ctrl->list))
@@ -2875,7 +2874,7 @@ static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
 	nvmf_free_options(nctrl->opts);
 free_ctrl:
 	put_net(ctrl->net);
-	for (i = 0; i < nr_queues; i++)
+	for (i = 0; i < ctrl->nr_allocated_queues; i++)
 		nvme_tcp_exit_queue(ctrl, i);
 	kfree(ctrl->queues);
 	kfree(ctrl);
@@ -3269,13 +3268,14 @@ static struct nvme_tcp_ctrl *nvme_tcp_alloc_ctrl(struct device *dev,
 		goto out_free_ctrl;
 	}
 
-	ctrl->queues = kzalloc_objs(*ctrl->queues, ctrl->ctrl.queue_count);
+	ctrl->nr_allocated_queues = ctrl->ctrl.queue_count;
+	ctrl->queues = kzalloc_objs(*ctrl->queues, ctrl->nr_allocated_queues);
 	if (!ctrl->queues) {
 		ret = -ENOMEM;
 		goto out_free_ctrl;
 	}
 
-	for (i = 0; i < ctrl->ctrl.queue_count; i++)
+	for (i = 0; i < ctrl->nr_allocated_queues; i++)
 		nvme_tcp_init_queue(ctrl, i);
 
 	ctrl->net = get_net(current->nsproxy->net_ns);

base-commit: 89480d137bf0d943a82c64f86ae2407a15276b5e
-- 
2.43.0

Re: [PATCH] nvme-tcp: fix use-after-free in nvme_tcp_free_ctrl()

[Nilay Shroff]

On 10/30/26 10:31 PM, Christian Aurich Zanettini Martins wrote:
> nvme_tcp_free_ctrl() reads ctrl->ctrl.opts to know how many queue
> mutexes to destroy. When creating the controller fails,
> nvmf_create_ctrl() frees opts as soon as nvme_tcp_create_ctrl()
> returns, but a concurrent open of the character device can hold the
> last reference to the controller. With CONFIG_DEBUG_KOBJECT_RELEASE,
> which delays the release, every failed connect reads freed memory:
> 
>    BUG: KASAN: slab-use-after-free in nvme_tcp_free_ctrl+0x38b/0x430
>    Read of size 4 at addr ffff888005b93740 by task kworker/2:1/51
> 
> Keep the size of the queue array in the controller and use it instead.
> 
> Fixes: 2eeeaa9c1ff0 ("nvme-tcp: Validate the queue state under the queue lock in the address attribute")
> Cc:stable@vger.kernel.org
> Signed-off-by: Christian Aurich Zanettini Martins<christian.aurichzm@gmail.com>

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>