[PATCH 2/6] nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()

[Guixin Liu]

Commit b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
switched the loop to list_for_each_entry_safe() to allow dropping the
rport lock for the nvme_fc_ctrl_put() of a non-matching controller.
But the safe iterator only protects the current entry within one
iteration: the next entry is cached before the body runs, and while
the lock is dropped that entry can run through its own final put into
nvme_fc_ctrl_free(), which removes it from the list and frees it.
The next iteration then dereferences freed memory.

The safe variant is not needed here at all. The reference the loop
takes on the current entry pins it on the list until the iteration
moves on, so a plain list_for_each_entry() tolerates the lock drop:
while the lock is held the current entry cannot be unlinked, and once
the lock is dropped the cached ->next of the pinned entry still points
at a valid list member or the head.

Switch back to the plain iterator.

Fixes: b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
Cc: stable at vger.kernel.org
Signed-off-by: Guixin Liu <kanie at linux.alibaba.com>

---

Reproduced under KASAN in a VM through fcloop with three or more
controllers on one remote port, a tbkas force-reconnect racing
concurrent controller disconnects.  The lock-drop window was widened
with a test-only mdelay(200) inserted at the same place in both the
buggy and the fixed version so the iterator is the only variable; the
stock microsecond window did not trigger in 400 iterations of the
same workload.

  BUG: KASAN: slab-use-after-free in nvme_fc_match_disconn_ls
  Read of size 8 by task kworker
  Workqueue: events nvme_fc_handle_ls_rqst_work
  Call Trace:
   nvme_fc_match_disconn_ls
   nvme_fc_ls_disconnect_assoc
   nvme_fc_handle_ls_rqst_work
   process_one_work

The object was allocated by nvme_fc_alloc_ctrl() on connect and freed
by nvme_free_ctrl() on the concurrent teardown of the cached next
entry.  With the plain iterator the identical amplified sequence
reports nothing and survives; the read hits the pinned current entry,
which cannot be freed while the loop holds its reference.
---
 drivers/nvme/host/fc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 0b9ff50d26ff..edf192055618 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1470,14 +1470,14 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
 {
 	struct fcnvme_ls_disconnect_assoc_rqst *rqst =
 					&lsop->rqstbuf->rq_dis_assoc;
-	struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
+	struct nvme_fc_ctrl *ctrl, *ret = NULL;
 	struct nvmefc_ls_rcv_op *oldls = NULL;
 	u64 association_id = be64_to_cpu(rqst->associd.association_id);
 	unsigned long flags;
 
 	spin_lock_irqsave(&rport->lock, flags);
 
-	list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
+	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
 		if (!nvme_fc_ctrl_get(ctrl))
 			continue;
 		spin_lock(&ctrl->lock);
-- 
2.43.7