Commit 1a3f540d authored by Israel Rukshin's avatar Israel Rukshin Committed by Keith Busch

nvmet: Fix controller use after free

After nvmet_install_queue() sets sq->ctrl calling to nvmet_sq_destroy()
reduces the controller refcount. In case nvmet_install_queue() fails,
calling to nvmet_ctrl_put() is done twice (at nvmet_sq_destroy and
nvmet_execute_io_connect/nvmet_execute_admin_connect) instead of once for
the queue which leads to use after free of the controller. Fix this by set
NULL at sq->ctrl in case of a failure at nvmet_install_queue().

The bug leads to the following Call Trace:

[65857.994862] refcount_t: underflow; use-after-free.
[65858.108304] Workqueue: events nvmet_rdma_release_queue_work [nvmet_rdma]
[65858.115557] RIP: 0010:refcount_warn_saturate+0xe5/0xf0
[65858.208141] Call Trace:
[65858.211203]  nvmet_sq_destroy+0xe1/0xf0 [nvmet]
[65858.216383]  nvmet_rdma_release_queue_work+0x37/0xf0 [nvmet_rdma]
[65858.223117]  process_one_work+0x167/0x370
[65858.227776]  worker_thread+0x49/0x3e0
[65858.232089]  kthread+0xf5/0x130
[65858.235895]  ? max_active_store+0x80/0x80
[65858.240504]  ? kthread_bind+0x10/0x10
[65858.244832]  ret_from_fork+0x1f/0x30
[65858.249074] ---[ end trace f82d59250b54beb7 ]---

Fixes: bb1cc747 ("nvmet: implement valid sqhd values in completions")
Fixes: 1672ddb8 ("nvmet: Add install_queue callout")
Signed-off-by: default avatarIsrael Rukshin <israelr@mellanox.com>
Reviewed-by: default avatarMax Gurtovoy <maxg@mellanox.com>
Reviewed-by: default avatarChristoph Hellwig <hch@lst.de>
Signed-off-by: default avatarKeith Busch <kbusch@kernel.org>
parent 0b87a2b7
...@@ -109,6 +109,7 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req) ...@@ -109,6 +109,7 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
u16 qid = le16_to_cpu(c->qid); u16 qid = le16_to_cpu(c->qid);
u16 sqsize = le16_to_cpu(c->sqsize); u16 sqsize = le16_to_cpu(c->sqsize);
struct nvmet_ctrl *old; struct nvmet_ctrl *old;
u16 ret;
old = cmpxchg(&req->sq->ctrl, NULL, ctrl); old = cmpxchg(&req->sq->ctrl, NULL, ctrl);
if (old) { if (old) {
...@@ -119,7 +120,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req) ...@@ -119,7 +120,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
if (!sqsize) { if (!sqsize) {
pr_warn("queue size zero!\n"); pr_warn("queue size zero!\n");
req->error_loc = offsetof(struct nvmf_connect_command, sqsize); req->error_loc = offsetof(struct nvmf_connect_command, sqsize);
return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR; ret = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
goto err;
} }
/* note: convert queue size from 0's-based value to 1's-based value */ /* note: convert queue size from 0's-based value to 1's-based value */
...@@ -132,16 +134,19 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req) ...@@ -132,16 +134,19 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
} }
if (ctrl->ops->install_queue) { if (ctrl->ops->install_queue) {
u16 ret = ctrl->ops->install_queue(req->sq); ret = ctrl->ops->install_queue(req->sq);
if (ret) { if (ret) {
pr_err("failed to install queue %d cntlid %d ret %x\n", pr_err("failed to install queue %d cntlid %d ret %x\n",
qid, ctrl->cntlid, ret); qid, ctrl->cntlid, ret);
return ret; goto err;
} }
} }
return 0; return 0;
err:
req->sq->ctrl = NULL;
return ret;
} }
static void nvmet_execute_admin_connect(struct nvmet_req *req) static void nvmet_execute_admin_connect(struct nvmet_req *req)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment