scsi: mpi3mr: Fix use-after-free on tgt_dev->starget during target device refresh/update

mpi3mr_refresh_tgtdevs() and mpi3mr_devinfochg_evt_bh() read
tgt_dev->starget and immediately pass it to starget_for_each_device()
without holding mrioc->tgtdev_lock. Every writer of this field --
mpi3mr_target_alloc(), mpi3mr_target_destroy(), mpi3mr_slave_destroy()
and mpi3mr_sdev_init() -- correctly serializes access under tgtdev_lock,
but these two read sites do not, which leaves a check-then-use window
against the SCSI core's target teardown path (scsi_remove_target(),
invoked e.g. via a concurrent host reset, sysfs "delete", or SCSI EH
device offlining running independently of the fwevt workqueue).

Sequence observed on production hardware, triggered on the
mpi3mr0_fwevt_wrkr workqueue during a SAS topology change shortly after
a controller reset:

  BUG: kernel NULL pointer dereference, address: 0000000000000058
  RIP: scsi_is_host_device+0x7/0x20
  Call Trace:
   starget_for_each_device+0x34/0x100
   mpi3mr_refresh_tgtdevs+0x152/0x1d0 [mpi3mr]
   mpi3mr_fwevt_bh+0x514/0x6c0 [mpi3mr]
   mpi3mr_fwevt_worker+0x1a/0x50 [mpi3mr]
   process_one_work+0x194/0x380
   worker_thread+0x2fe/0x410

mpi3mr_refresh_tgtdevs() reads tgt_dev->starget as non-NULL, but by the
time starget_for_each_device() dereferences it, a concurrent
mpi3mr_target_destroy() has already cleared tgt_dev->starget under
tgtdev_lock and the SCSI/device core has freed the underlying
scsi_target (and its embedded struct device). The stale pointer is then
walked by dev_to_shost() -> scsi_is_host_device(), producing the
NULL/garbage dereference above.

Fix this by taking mrioc->tgtdev_lock around every read of
tgt_dev->starget, matching the existing writer-side discipline. Since
starget_for_each_device() and mpi3mr_update_sdev() can end up doing
non-atomic work (e.g. queue_limits_commit_update()), the lock cannot be
held across the whole call, so instead pin the target's device with
get_device() while holding the lock, drop the lock, then run
starget_for_each_device() against the pinned reference and put_device()
afterwards. This closes the TOCTOU window instead of merely narrowing
it.

The same unlocked read-and-dereference pattern also exists earlier in
mpi3mr_refresh_tgtdevs()'s first removal-scan loop
(tgt_dev->starget->hostdata); fix it the same way by holding tgtdev_lock
across that check, which is cheap since it only touches plain struct
fields.

Assisted-by: Claude:Sonnet5 [Claude Code]
Signed-off-by: Laurence Oberman <loberman@redhat.com>
Acked-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>
Link: https://patch.msgid.link/20260831120047.14690-1-loberman@redhat.com
Signed-off-by: Martin K. Petersen (Oracle) <mkp@kernel.org>

1 files changed, 35 insertions, 10 deletions

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index f80a21ec161b..0f7380448718 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -1094,10 +1094,13 @@ static void mpi3mr_refresh_tgtdevs(struct mpi3mr_ioc *mrioc)
 {
 	struct mpi3mr_tgt_dev *tgtdev, *tgtdev_next;
 	struct mpi3mr_stgt_priv_data *tgt_priv;
+	struct scsi_target *starget;
+	unsigned long flags;
 
 	dprint_reset(mrioc, "refresh target devices: check for removals\n");
 	list_for_each_entry_safe(tgtdev, tgtdev_next, &mrioc->tgtdev_list,
 	    list) {
+		spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
 		if (((tgtdev->dev_handle == MPI3MR_INVALID_DEV_HANDLE) ||
 		     tgtdev->is_hidden) &&
 		     tgtdev->host_exposed && tgtdev->starget &&
@@ -1106,6 +1109,7 @@ static void mpi3mr_refresh_tgtdevs(struct mpi3mr_ioc *mrioc)
 			tgt_priv->dev_removed = 1;
 			atomic_set(&tgt_priv->block_io, 0);
 		}
+		spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
 	}
 
 	list_for_each_entry_safe(tgtdev, tgtdev_next, &mrioc->tgtdev_list,
@@ -1127,15 +1131,25 @@ static void mpi3mr_refresh_tgtdevs(struct mpi3mr_ioc *mrioc)
 	tgtdev = NULL;
 	list_for_each_entry(tgtdev, &mrioc->tgtdev_list, list) {
 		if ((tgtdev->dev_handle != MPI3MR_INVALID_DEV_HANDLE) &&
-		    !tgtdev->is_hidden) {
-			if (!tgtdev->host_exposed)
+				!tgtdev->is_hidden) {
+			if (!tgtdev->host_exposed) {
 				mpi3mr_report_tgtdev_to_host(mrioc,
-							     tgtdev->perst_id);
-			else if (tgtdev->starget)
-				starget_for_each_device(tgtdev->starget,
-							(void *)tgtdev, mpi3mr_update_sdev);
-	}
+				     tgtdev->perst_id);
+				continue;
+			}
+			spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
+			starget = tgtdev->starget;
+			if (starget)
+				get_device(&starget->dev);
+			spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
+			if (starget) {
+				starget_for_each_device(starget, (void *)tgtdev,
+							mpi3mr_update_sdev);
+				put_device(&starget->dev);
+			}
+		}
 	}
+	dprint_reset(mrioc, "refresh target devices: done\n");
 }
 
 /**
@@ -1515,6 +1529,8 @@ static void mpi3mr_devinfochg_evt_bh(struct mpi3mr_ioc *mrioc,
 	struct mpi3_device_page0 *dev_pg0)
 {
 	struct mpi3mr_tgt_dev *tgtdev = NULL;
+	struct scsi_target *starget;
+	unsigned long flags;
 	u16 dev_handle = 0, perst_id = 0;
 
 	perst_id = le16_to_cpu(dev_pg0->persistent_id);
@@ -1535,9 +1551,18 @@ static void mpi3mr_devinfochg_evt_bh(struct mpi3mr_ioc *mrioc,
 		mpi3mr_report_tgtdev_to_host(mrioc, perst_id);
 	if (tgtdev->is_hidden && tgtdev->host_exposed)
 		mpi3mr_remove_tgtdev_from_host(mrioc, tgtdev);
-	if (!tgtdev->is_hidden && tgtdev->host_exposed && tgtdev->starget)
-		starget_for_each_device(tgtdev->starget, (void *)tgtdev,
-		    mpi3mr_update_sdev);
+	if (!tgtdev->is_hidden && tgtdev->host_exposed) {
+		spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
+		starget = tgtdev->starget;
+		if (starget)
+			get_device(&starget->dev);
+		spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
+		if (starget) {
+			starget_for_each_device(starget, (void *)tgtdev,
+						mpi3mr_update_sdev);
+			put_device(&starget->dev);
+		}
+	}
 out:
 	if (tgtdev)
 		mpi3mr_tgtdev_put(tgtdev);