History

nvmet: fix max_qid race between configfs and controller allocation

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.

Maurizio Lombardi committed 3 months ago

Commit f1a88461 parent 22eb6315 files · +39 −28Browse files

Unified · Split

5 files
  1. drivers/nvme/target/admin-cmd.c +4 −4
  2. drivers/nvme/target/core.c +27 −22
  3. drivers/nvme/target/fabrics-cmd.c +1 −1
  4. drivers/nvme/target/nvmet.h +6 −0
  5. drivers/nvme/target/pci-epf.c +1 −1
drivers/nvme/target/admin-cmd.c+4−4
@@ -1337,7 +1337,7 @@ static u16 nvmet_set_feat_arbitration(struct nvmet_req *req) · 1336 unmodified lines
13371337
13381338 void nvmet_execute_set_features(struct nvmet_req *req)
13391339 {
1340− struct nvmet_subsys *subsys = nvmet_req_subsys(req);
1340+ struct nvmet_ctrl *ctrl = nvmet_req_ctrl(req);
13411341 u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
13421342 u32 cdw11 = le32_to_cpu(req->cmd->common.cdw11);
13431343 u16 status = 0;
@@ -1359,7 +1359,7 @@ void nvmet_execute_set_features(struct nvmet_req *req) · 15 unmodified lines
13591359 break;
13601360 }
13611361 nvmet_set_result(req,
1362− (subsys->max_qid - 1) | ((subsys->max_qid - 1) << 16));
1362+ (ctrl->max_qid - 1) | ((ctrl->max_qid - 1) << 16));
13631363 break;
13641364 case NVME_FEAT_IRQ_COALESCE:
13651365 status = nvmet_set_feat_irq_coalesce(req);
@@ -1496,7 +1496,7 @@ void nvmet_get_feat_async_event(struct nvmet_req *req) · 130 unmodified lines
14961496
14971497 void nvmet_execute_get_features(struct nvmet_req *req)
14981498 {
1499− struct nvmet_subsys *subsys = nvmet_req_subsys(req);
1499+ struct nvmet_ctrl *ctrl = nvmet_req_ctrl(req);
15001500 u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
15011501 u16 status = 0;
15021502
@@ -1536,7 +1536,7 @@ void nvmet_execute_get_features(struct nvmet_req *req) · 33 unmodified lines
15361536 break;
15371537 case NVME_FEAT_NUM_QUEUES:
15381538 nvmet_set_result(req,
1539− (subsys->max_qid-1) | ((subsys->max_qid-1) << 16));
1539+ (ctrl->max_qid-1) | ((ctrl->max_qid-1) << 16));
15401540 break;
15411541 case NVME_FEAT_KATO:
15421542 nvmet_get_feat_kato(req);
drivers/nvme/target/core.c+27−22
@@ -878,7 +878,7 @@ u16 nvmet_check_cqid(struct nvmet_ctrl *ctrl, u16 cqid, bool create) · 877 unmodified lines
878878 if (!ctrl->cqs)
879879 return NVME_SC_INTERNAL | NVME_STATUS_DNR;
880880
881− if (cqid > ctrl->subsys->max_qid)
881+ if (cqid > ctrl->max_qid)
882882 return NVME_SC_QID_INVALID | NVME_STATUS_DNR;
883883
884884 if ((create && ctrl->cqs[cqid]) || (!create && !ctrl->cqs[cqid]))
@@ -926,7 +926,7 @@ u16 nvmet_check_sqid(struct nvmet_ctrl *ctrl, u16 sqid, · 41 unmodified lines
926926 if (!ctrl->sqs)
927927 return NVME_SC_INTERNAL | NVME_STATUS_DNR;
928928
929− if (sqid > ctrl->subsys->max_qid)
929+ if (sqid > ctrl->max_qid)
930930 return NVME_SC_QID_INVALID | NVME_STATUS_DNR;
931931
932932 if ((create && ctrl->sqs[sqid]) ||
@@ -1655,11 +1655,29 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args) · 722 unmodified lines
16551655 if (!ctrl->changed_ns_list)
16561656 goto out_free_ctrl;
16571657
1658− ctrl->sqs = kzalloc_objs(struct nvmet_sq *, subsys->max_qid + 1);
1658+ /*
1659+ * Discovery controllers may use some arbitrary high value
1660+ * in order to cleanup stale discovery sessions
1661+ */
1662+ if (nvmet_is_disc_subsys(ctrl->subsys) && !kato)
1663+ kato = NVMET_DISC_KATO_MS;
1664+
1665+ /* keep-alive timeout in seconds */
1666+ ctrl->kato = DIV_ROUND_UP(kato, 1000);
1667+
1668+ ctrl->err_counter = 0;
1669+ spin_lock_init(&ctrl->error_lock);
1670+
1671+ down_read(&nvmet_config_sem);
1672+ mutex_lock(&subsys->lock);
1673+
1674+ ctrl->max_qid = subsys->max_qid;
1675+
1676+ ctrl->sqs = kzalloc_objs(struct nvmet_sq *, ctrl->max_qid + 1);
16591677 if (!ctrl->sqs)
16601678 goto out_free_changed_ns_list;
16611679
1662− ctrl->cqs = kzalloc_objs(struct nvmet_cq *, subsys->max_qid + 1);
1680+ ctrl->cqs = kzalloc_objs(struct nvmet_cq *, ctrl->max_qid + 1);
16631681 if (!ctrl->cqs)
16641682 goto out_free_sqs;
16651683
@@ -1672,22 +1690,6 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args) · 6 unmodified lines
16721690 }
16731691 ctrl->cntlid = ret;
16741692
1675− /*
1676− * Discovery controllers may use some arbitrary high value
1677− * in order to cleanup stale discovery sessions
1678− */
1679− if (nvmet_is_disc_subsys(ctrl->subsys) && !kato)
1680− kato = NVMET_DISC_KATO_MS;
1681−
1682− /* keep-alive timeout in seconds */
1683− ctrl->kato = DIV_ROUND_UP(kato, 1000);
1684−
1685− ctrl->err_counter = 0;
1686− spin_lock_init(&ctrl->error_lock);
1687−
1688− nvmet_start_keep_alive_timer(ctrl);
1689−
1690− mutex_lock(&subsys->lock);
16911693 ret = nvmet_ctrl_init_pr(ctrl);
16921694 if (ret)
16931695 goto init_pr_fail;
@@ -1695,6 +1697,9 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args) · 1 unmodified line
16951697 nvmet_setup_p2p_ns_map(ctrl, args->p2p_client);
16961698 nvmet_debugfs_ctrl_setup(ctrl);
16971699 mutex_unlock(&subsys->lock);
1700+ up_read(&nvmet_config_sem);
1701+
1702+ nvmet_start_keep_alive_timer(ctrl);
16981703
16991704 if (args->hostid)
17001705 uuid_copy(&ctrl->hostid, args->hostid);
@@ -1724,14 +1729,14 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args) · 23 unmodified lines
17241729 return ctrl;
17251730
17261731 init_pr_fail:
1727− mutex_unlock(&subsys->lock);
1728− nvmet_stop_keep_alive_timer(ctrl);
17291732 ida_free(&cntlid_ida, ctrl->cntlid);
17301733 out_free_cqs:
17311734 kfree(ctrl->cqs);
17321735 out_free_sqs:
17331736 kfree(ctrl->sqs);
17341737 out_free_changed_ns_list:
1738+ mutex_unlock(&subsys->lock);
1739+ up_read(&nvmet_config_sem);
17351740 kfree(ctrl->changed_ns_list);
17361741 out_free_ctrl:
17371742 kfree(ctrl);
drivers/nvme/target/fabrics-cmd.c+1−1
@@ -370,7 +370,7 @@ static void nvmet_execute_io_connect(struct nvmet_req *req) · 369 unmodified lines
370370 goto out;
371371 }
372372
373− if (unlikely(qid > ctrl->subsys->max_qid)) {
373+ if (unlikely(qid > ctrl->max_qid)) {
374374 pr_warn("invalid queue id (%d)\n", qid);
375375 status = NVME_SC_CONNECT_INVALID_PARAM | NVME_STATUS_DNR;
376376 req->cqe->result.u32 = IPO_IATTR_CONNECT_SQE(qid);
drivers/nvme/target/nvmet.h+6−0
@@ -268,6 +268,7 @@ struct nvmet_ctrl { · 267 unmodified lines
268268
269269 uuid_t hostid;
270270 u16 cntlid;
271+ u16 max_qid;
271272 u32 kato;
272273
273274 struct nvmet_port *port;
@@ -756,6 +757,11 @@ static inline struct nvmet_subsys *nvmet_req_subsys(struct nvmet_req *req) · 482 unmodified lines
756757 return req->sq->ctrl->subsys;
757758 }
758759
760+static inline struct nvmet_ctrl *nvmet_req_ctrl(struct nvmet_req *req)
761+{
762+ return req->sq->ctrl;
763+}
764+
759765 static inline bool nvmet_is_disc_subsys(struct nvmet_subsys *subsys)
760766 {
761767 return subsys->type != NVME_NQN_NVME;
drivers/nvme/target/pci-epf.c+1−1
@@ -2081,7 +2081,7 @@ static int nvmet_pci_epf_create_ctrl(struct nvmet_pci_epf *nvme_epf, · 2080 unmodified lines
20812081 }
20822082
20832083 /* Allocate our queues, up to the maximum number. */
2084− ctrl->nr_queues = min(ctrl->tctrl->subsys->max_qid + 1, max_nr_queues);
2084+ ctrl->nr_queues = min(ctrl->tctrl->max_qid + 1, max_nr_queues);
20852085 ret = nvmet_pci_epf_alloc_queues(ctrl);
20862086 if (ret)
20872087 goto out_put_ctrl;

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t