[PATCH v2 2/5] nvme-fc: fix use-after-free in nvme_fc_match_disconn_ls()

[Guixin Liu]

Commit b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
dropped the rport lock inside the loop to put the reference of a
non-matching controller, because nvme_fc_ctrl_free() takes the rport
lock to unlink the controller. But no iterator is sound across that
drop: with list_for_each_entry_safe() the prefetched next entry can be
freed in the window, and with a plain iterator the loop's own
nvme_fc_ctrl_put() can be the final reference, freeing the current
entry before the iterator reads its ->next pointer.

The reference dance is unnecessary in the first place. An entry cannot
leave ctrl_list while rport->lock is held, as the final put unlinks it
under that very lock, so association_id can be inspected under
ctrl->lock without holding a reference. Take a reference only for the
matching controller, still under the rport lock, and hand it to the
caller as before. The loop then no longer needs to drop the lock at
all, which also removes the reason the lock was dropped for in the
first place.

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 several controllers
on one remote port and a tbkas force-reconnect racing concurrent
controller disconnects; the lock-drop window was widened with a
test-only mdelay(200) to make the microsecond window deterministic:

  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 freed object was the prefetched next entry, allocated by
nvme_fc_alloc_ctrl() on connect and freed by nvme_free_ctrl() on the
concurrent teardown. The loop increment dereferences it before
nvme_fc_ctrl_get() gets a chance to reject it, which is why the
get-unless-zero check in the loop body does not help. The reworked
loop does not drop the lock, so no such window is left to widen.
---
 drivers/nvme/host/fc.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 0b9ff50d26ff..e28eafc362db 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1470,29 +1470,31 @@ 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) {
-		if (!nvme_fc_ctrl_get(ctrl))
-			continue;
+	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
 		spin_lock(&ctrl->lock);
 		if (association_id == ctrl->association_id) {
-			oldls = ctrl->rcv_disconn;
-			ctrl->rcv_disconn = lsop;
-			ret = ctrl;
+			/*
+			 * Entries cannot leave the list while rport->lock is
+			 * held, so they can be inspected without a reference.
+			 * Only the match needs one, for the caller.
+			 */
+			if (nvme_fc_ctrl_get(ctrl)) {
+				oldls = ctrl->rcv_disconn;
+				ctrl->rcv_disconn = lsop;
+				ret = ctrl;
+			}
 		}
 		spin_unlock(&ctrl->lock);
 		if (ret)
 			/* leave the ctrl get reference */
 			break;
-		spin_unlock_irqrestore(&rport->lock, flags);
-		nvme_fc_ctrl_put(ctrl);
-		spin_lock_irqsave(&rport->lock, flags);
 	}
 
 	spin_unlock_irqrestore(&rport->lock, flags);
-- 
2.43.7