[PATCH 2/2] scsi: mvsas: fix iterator use-after-free in mvs_free() wq drain loop

[Sangyun Kim]

mvs_free() walks mvi->wq_list with list_for_each_entry() and calls
cancel_delayed_work_sync(&mwq->work_q) for each element.  If the
callback for the current node is already executing, the sync wait
allows mvs_work_queue() to remove its own list entry and kfree() the
mvs_wq:

  list_del(&mwq->entry);
  spin_unlock_irqrestore(&mvi->lock, flags);
  kfree(mwq);

When cancel_delayed_work_sync() returns, list_for_each_entry() in
mvs_free() advances with list_next_entry(mwq, entry), which reads
mwq->entry.next from the already-freed node.  This is a read
use-after-free in the remove path.

Switching the iterator to list_for_each_entry_safe() is not enough,
because any saved "next" cursor can itself be freed by its own
callback once cancel_delayed_work_sync() drops mvi->lock.

Drain wq_list one entry at a time under mvi->lock:

 - mvs_free() uses list_first_entry() + list_del_init() to detach
   the head, drops the lock, calls cancel_delayed_work_sync(), then
   kfree()s the entry, and retakes the lock for the next iteration.

 - mvs_work_queue() checks list_empty(&mwq->entry) under mvi->lock
   before doing its own list_del_init() + kfree().  If mvs_free()
   has already detached the entry, the callback skips the final
   free and teardown owns it.

This gives teardown and the callback a single-owner rule for the
struct mvs_wq free.

Fixes: 60cd16a3b743 ("scsi: mvsas: Fix use-after-free bugs in mvs_work_queue")
Signed-off-by: Sangyun Kim <sangyun.kim@xxxxxxxxx>
---
 drivers/scsi/mvsas/mv_init.c | 11 ++++++++++-
 drivers/scsi/mvsas/mv_sas.c  | 10 +++++++---
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 0c9c62c25987..c32f645deef3 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -85,6 +85,7 @@ static void mvs_phy_init(struct mvs_info *mvi, int phy_id)
 static void mvs_free(struct mvs_info *mvi)
 {
 	struct mvs_wq *mwq;
+	unsigned long flags;
 	int slot_nr;
 
 	if (!mvi)
@@ -120,8 +121,16 @@ static void mvs_free(struct mvs_info *mvi)
 		dma_free_coherent(mvi->dev, TRASH_BUCKET_SIZE,
 				  mvi->bulk_buffer1, mvi->bulk_buffer_dma1);
 
-	list_for_each_entry(mwq, &mvi->wq_list, entry)
+	spin_lock_irqsave(&mvi->lock, flags);
+	while (!list_empty(&mvi->wq_list)) {
+		mwq = list_first_entry(&mvi->wq_list, struct mvs_wq, entry);
+		list_del_init(&mwq->entry);
+		spin_unlock_irqrestore(&mvi->lock, flags);
 		cancel_delayed_work_sync(&mwq->work_q);
+		kfree(mwq);
+		spin_lock_irqsave(&mvi->lock, flags);
+	}
+	spin_unlock_irqrestore(&mvi->lock, flags);
 	MVS_CHIP_DISP->chip_iounmap(mvi);
 	if (mvi->shost)
 		scsi_host_put(mvi->shost);
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 359226e80eae..7df6964a76e8 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -1729,9 +1729,13 @@ static void mvs_work_queue(struct work_struct *work)
 				PORTE_BROADCAST_RCVD, GFP_ATOMIC);
 		mv_dprintk("phy%d Got Broadcast Change\n", phy_no);
 	}
-	list_del(&mwq->entry);
-	spin_unlock_irqrestore(&mvi->lock, flags);
-	kfree(mwq);
+	if (!list_empty(&mwq->entry)) {
+		list_del_init(&mwq->entry);
+		spin_unlock_irqrestore(&mvi->lock, flags);
+		kfree(mwq);
+	} else {
+		spin_unlock_irqrestore(&mvi->lock, flags);
+	}
 }
 
 static int mvs_handle_event(struct mvs_info *mvi, void *data, int handler)
-- 
2.34.1