6.12-stable review patch. If anyone has any objections, please let me know. ------------------ From: Maurizio Lombardi [ Upstream commit f1a8846e06388113dfdbb89dee005083fa9afdf9 ] The function nvmet_subsys_attr_qid_max_store() can race against nvmet_alloc_ctrl() when a subsystem's max_qid limit is modified. Suppose max_qid is currently 64. If nvmet_alloc_ctrl() executes: ctrl->sqs = kzalloc_objs(struct nvmet_sq *, subsys->max_qid + 1); and at this exact point, a userspace process changes max_qid to 128, nvmet_subsys_attr_qid_max_store() will set the new max_qid value. It attempts to delete active controllers to force a reconnect, but the new controller won't be deleted because it hasn't been added to the subsys->ctrls list yet. nvmet_alloc_ctrl() then proceeds and adds the new controller to the subsys->ctrls list. Later, when nvmet_install_queue() is called, it will see max_qid set to 128, but the memory allocated for sqs is only sized for 64 entries. This results in a KASAN out-of-bounds warning and potential memory corruptions. Fix this by protecting the queue allocations and list insertion in nvmet_alloc_ctrl() with down_read(&nvmet_config_sem). Because nvmet_subsys_attr_qid_max_store() acquires down_write(&nvmet_config_sem) to modify the attribute, this safely prevents the configfs writer from modifying max_qid during controller creation. Copy the max_qid from the subsystem to the controller's structure during the allocation; ctrl->max_qid never changes as long as the controller remains in LIVE state, so this will prevent similar race conditions. Fixes: 3e980f5995e0 ("nvmet: expose max queues to configfs") Reported-by: syzbot+2626e846cd2585c9aa67@syzkaller.appspotmail.com Signed-off-by: Maurizio Lombardi Signed-off-by: Keith Busch Signed-off-by: Sasha Levin --- drivers/nvme/target/admin-cmd.c | 8 +++--- drivers/nvme/target/core.c | 41 ++++++++++++++++++------------- drivers/nvme/target/fabrics-cmd.c | 2 +- drivers/nvme/target/nvmet.h | 6 +++++ 4 files changed, 35 insertions(+), 22 deletions(-) diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c index 0f1956bb6d3c9..72a5de1f8252d 100644 --- a/drivers/nvme/target/admin-cmd.c +++ b/drivers/nvme/target/admin-cmd.c @@ -828,7 +828,7 @@ u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask) void nvmet_execute_set_features(struct nvmet_req *req) { - struct nvmet_subsys *subsys = nvmet_req_subsys(req); + struct nvmet_ctrl *ctrl = nvmet_req_ctrl(req); u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10); u32 cdw11 = le32_to_cpu(req->cmd->common.cdw11); u16 status = 0; @@ -847,7 +847,7 @@ void nvmet_execute_set_features(struct nvmet_req *req) break; } nvmet_set_result(req, - (subsys->max_qid - 1) | ((subsys->max_qid - 1) << 16)); + (ctrl->max_qid - 1) | ((ctrl->max_qid - 1) << 16)); break; case NVME_FEAT_KATO: status = nvmet_set_feat_kato(req); @@ -902,7 +902,7 @@ void nvmet_get_feat_async_event(struct nvmet_req *req) void nvmet_execute_get_features(struct nvmet_req *req) { - struct nvmet_subsys *subsys = nvmet_req_subsys(req); + struct nvmet_ctrl *ctrl = nvmet_req_ctrl(req); u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10); u16 status = 0; @@ -939,7 +939,7 @@ void nvmet_execute_get_features(struct nvmet_req *req) break; case NVME_FEAT_NUM_QUEUES: nvmet_set_result(req, - (subsys->max_qid-1) | ((subsys->max_qid-1) << 16)); + (ctrl->max_qid-1) | ((ctrl->max_qid-1) << 16)); break; case NVME_FEAT_KATO: nvmet_get_feat_kato(req); diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c index 5950635d6f58d..fa011c16c9b1c 100644 --- a/drivers/nvme/target/core.c +++ b/drivers/nvme/target/core.c @@ -1479,21 +1479,6 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn, if (!ctrl->changed_ns_list) goto out_free_ctrl; - ctrl->sqs = kcalloc(subsys->max_qid + 1, - sizeof(struct nvmet_sq *), - GFP_KERNEL); - if (!ctrl->sqs) - goto out_free_changed_ns_list; - - ret = ida_alloc_range(&cntlid_ida, - subsys->cntlid_min, subsys->cntlid_max, - GFP_KERNEL); - if (ret < 0) { - status = NVME_SC_CONNECT_CTRL_BUSY | NVME_STATUS_DNR; - goto out_free_sqs; - } - ctrl->cntlid = ret; - /* * Discovery controllers may use some arbitrary high value * in order to cleanup stale discovery sessions @@ -1507,13 +1492,33 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn, ctrl->err_counter = 0; spin_lock_init(&ctrl->error_lock); - nvmet_start_keep_alive_timer(ctrl); - + down_read(&nvmet_config_sem); mutex_lock(&subsys->lock); + + ctrl->max_qid = subsys->max_qid; + + ctrl->sqs = kcalloc(ctrl->max_qid + 1, + sizeof(struct nvmet_sq *), + GFP_KERNEL); + if (!ctrl->sqs) + goto out_free_changed_ns_list; + + ret = ida_alloc_range(&cntlid_ida, + subsys->cntlid_min, subsys->cntlid_max, + GFP_KERNEL); + if (ret < 0) { + status = NVME_SC_CONNECT_CTRL_BUSY | NVME_STATUS_DNR; + goto out_free_sqs; + } + ctrl->cntlid = ret; + list_add_tail(&ctrl->subsys_entry, &subsys->ctrls); nvmet_setup_p2p_ns_map(ctrl, req); nvmet_debugfs_ctrl_setup(ctrl); mutex_unlock(&subsys->lock); + up_read(&nvmet_config_sem); + + nvmet_start_keep_alive_timer(ctrl); *ctrlp = ctrl; return 0; @@ -1521,6 +1526,8 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn, out_free_sqs: kfree(ctrl->sqs); out_free_changed_ns_list: + mutex_unlock(&subsys->lock); + up_read(&nvmet_config_sem); kfree(ctrl->changed_ns_list); out_free_ctrl: kfree(ctrl); diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c index a593003c1c20c..66d0dbf7ac733 100644 --- a/drivers/nvme/target/fabrics-cmd.c +++ b/drivers/nvme/target/fabrics-cmd.c @@ -350,7 +350,7 @@ static void nvmet_execute_io_connect(struct nvmet_req *req) goto out; } - if (unlikely(qid > ctrl->subsys->max_qid)) { + if (unlikely(qid > ctrl->max_qid)) { pr_warn("invalid queue id (%d)\n", qid); status = NVME_SC_CONNECT_INVALID_PARAM | NVME_STATUS_DNR; req->cqe->result.u32 = IPO_IATTR_CONNECT_SQE(qid); diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h index 25bba2d097626..408c3cda47436 100644 --- a/drivers/nvme/target/nvmet.h +++ b/drivers/nvme/target/nvmet.h @@ -204,6 +204,7 @@ struct nvmet_ctrl { uuid_t hostid; u16 cntlid; + u16 max_qid; u32 kato; struct nvmet_port *port; @@ -642,6 +643,11 @@ static inline struct nvmet_subsys *nvmet_req_subsys(struct nvmet_req *req) return req->sq->ctrl->subsys; } +static inline struct nvmet_ctrl *nvmet_req_ctrl(struct nvmet_req *req) +{ + return req->sq->ctrl; +} + static inline bool nvmet_is_disc_subsys(struct nvmet_subsys *subsys) { return subsys->type != NVME_NQN_NVME; -- 2.53.0