[PATCH] scsi: mpi3mr: fix use-after-free on tgt_dev->starget during target device refresh/update

[PATCH] scsi: mpi3mr: fix use-after-free on tgt_dev->starget during target device refresh/update

[Laurence Oberman]

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>
---
 drivers/scsi/mpi3mr/mpi3mr_os.c | 45 +++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 402d1f35d214..922b28c3fe12 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);
-- 
2.55.0

Re: [PATCH] scsi: mpi3mr: fix use-after-free on tgt_dev->starget during target device refresh/update

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fixes the unprotected check-then-use window for `tgtdev->starget` in `mpi3mr_refresh_tgtdevs()` and `mpi3mr_devinfochg_evt_bh()`, but identical vulnerable patterns remain in other functions like `mpi3mr_remove_tgtdev_from_host()`, `mpi3mr_update_tgtdev()`, and various event/BSG handlers.
- [High] Unprotected traversal of `mrioc->tgtdev_list` without holding `tgtdev_lock` can lead to list corruption and crashes due to concurrent insertions.
- [High] An AB-BA lock inversion exists between `mrioc->tgtdev_lock` and the SCSI host lock (`shost->host_lock`).
--

commit 22ac835bdc4964e1608be9181c0e9ec7c639df2b
Author: Laurence Oberman <loberman@redhat.com>

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

This commit addresses a check-then-use window against the SCSI core's target
teardown path in mpi3mr_refresh_tgtdevs() and mpi3mr_devinfochg_evt_bh(). It
protects reads of tgt_dev->starget by acquiring tgtdev_lock and pinning the
target's device with get_device(), closing a TOCTOU race condition that could
lead to a NULL pointer dereference.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index f80a21ec161b6..0f73804487188 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) {

[Severity: High]
This is a pre-existing issue, but does iterating over mrioc->tgtdev_list
without holding tgtdev_lock risk list corruption?

If the top-half event handler (mpi3mr_os_handle_events()) receives a
device addition event concurrently, it can call mpi3mr_create_tgtdev(),
which mutates the list using list_add_tail() inside
mpi3mr_tgtdev_add_to_list() while holding the lock. The lockless reader
here will race with the writer.

The same unprotected traversal appears again on the third loop in
mpi3mr_refresh_tgtdevs() and in mpi3mr_invalidate_devhandles().

> +		spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
>  		if (((tgtdev->dev_handle == MPI3MR_INVALID_DEV_HANDLE) ||
>  		     tgtdev->is_hidden) &&
>  		     tgtdev->host_exposed && tgtdev->starget &&

[ ... ]

> @@ -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);

[Severity: High]
This is a pre-existing issue, but does acquiring tgtdev_lock here
create a lock inversion against shost->host_lock?

In mpi3mr_set_qd_for_all_vd_in_tg(), the driver acquires
mrioc->tgtdev_lock and then calls starget_for_each_device(), which
acquires shost->host_lock via __scsi_iterate_devices().

Concurrently, a target teardown (e.g., via sysfs or EH) acquires
shost->host_lock before calling the driver's ->target_destroy callback
(mpi3mr_target_destroy()). That callback then attempts to acquire
mrioc->tgtdev_lock, creating a potential AB-BA deadlock.

> +			starget = tgtdev->starget;
> +			if (starget)
> +				get_device(&starget->dev);
> +			spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);

[Severity: High]
This isn't a bug introduced by this patch, but while this change fixes
the unprotected check-then-use window for tgtdev->starget in this function,
are there identical vulnerable patterns remaining elsewhere?

For example, mpi3mr_remove_tgtdev_from_host() reads tgtdev->starget
and tgtdev->starget->hostdata locklessly:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_remove_tgtdev_from_host() {
    ...
    if (tgtdev->starget && tgtdev->starget->hostdata) {
        tgt_priv = tgtdev->starget->hostdata;
    ...
}

Similar unprotected reads occur in mpi3mr_update_tgtdev(),
mpi3mr_invalidate_devhandles(), mpi3mr_pcietopochg_evt_th(), and
mpi3mr_bsg_process_mpt_cmds(). Can concurrent target teardowns
trigger use-after-free conditions in these locations as well?

> +			if (starget) {
> +				starget_for_each_device(starget, (void *)tgtdev,
> +							mpi3mr_update_sdev);
> +				put_device(&starget->dev);
> +			}
> +		}
>  	}
> +	dprint_reset(mrioc, "refresh target devices: done\n");
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831120047.14690-1-loberman@redhat.com?part=1

Re: [PATCH] scsi: mpi3mr: fix use-after-free on tgt_dev->starget during target device refresh/update

[Laurence Oberman]

On Mon, 2026-08-31 at 07:59 -0400, Laurence Oberman wrote:
> 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>
> ---
>  drivers/scsi/mpi3mr/mpi3mr_os.c | 45 +++++++++++++++++++++++++------
> --
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c
> b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 402d1f35d214..922b28c3fe12 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_updat
> e_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);
> 
> 
> Re:

sashiko found Pre-existing issues:
- [High] The patch fixes the unprotected check-then-use window for
`tgtdev->starget` in `mpi3mr_refresh_tgtdevs()` and
`mpi3mr_devinfochg_evt_bh()`, but identical vulnerable patterns remain
in other functions like `mpi3mr_remove_tgtdev_from_host()`,
`mpi3mr_update_tgtdev()`, and various event/BSG handlers.
- [High] Unprotected traversal of `mrioc->tgtdev_list` without holding
`tgtdev_lock` can lead to list corruption and crashes due to concurrent
insertions.
- [High] An AB-BA lock inversion exists between `mrioc->tgtdev_lock`
and the SCSI host lock (`shost->host_lock`).

The sashiko reviews were all for pre-existing issues and won't be fixed
in the specific patch submission here.

I will make an attempt to deal with those in a seperate submission as
already agreed with Chandrekanth 

Thanks
Laurence

Re: [PATCH] scsi: mpi3mr: fix use-after-free on tgt_dev->starget during target device refresh/update

[Chandrakanth Patil]

[-- Attachment #1: Type: text/plain, Size: 8727 bytes --]

On Mon, Aug 31, 2026 at 6:21 PM Laurence Oberman <loberman@redhat.com> wrote:
>
> On Mon, 2026-08-31 at 07:59 -0400, Laurence Oberman wrote:
> > 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>
> > ---
> >  drivers/scsi/mpi3mr/mpi3mr_os.c | 45 +++++++++++++++++++++++++------
> > --
> >  1 file changed, 35 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c
> > b/drivers/scsi/mpi3mr/mpi3mr_os.c
> > index 402d1f35d214..922b28c3fe12 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_updat
> > e_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);
> >
> >
> > Re:
>
> sashiko found Pre-existing issues:
> - [High] The patch fixes the unprotected check-then-use window for
> `tgtdev->starget` in `mpi3mr_refresh_tgtdevs()` and
> `mpi3mr_devinfochg_evt_bh()`, but identical vulnerable patterns remain
> in other functions like `mpi3mr_remove_tgtdev_from_host()`,
> `mpi3mr_update_tgtdev()`, and various event/BSG handlers.
> - [High] Unprotected traversal of `mrioc->tgtdev_list` without holding
> `tgtdev_lock` can lead to list corruption and crashes due to concurrent
> insertions.
> - [High] An AB-BA lock inversion exists between `mrioc->tgtdev_lock`
> and the SCSI host lock (`shost->host_lock`).
>
> The sashiko reviews were all for pre-existing issues and won't be fixed
> in the specific patch submission here.
>
> I will make an attempt to deal with those in a seperate submission as
> already agreed with Chandrekanth
>
> Thanks
> Laurence
>

Looks good to me. Thanks.

Acked-by: Chandrakanth Patil <chandrakanth.patil@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5493 bytes --]

Re: [PATCH] scsi: mpi3mr: fix use-after-free on tgt_dev->starget during target device refresh/update

[Martin K. Petersen (Oracle)]

On Mon, 31 Aug 2026 07:59:17 -0400, Laurence Oberman wrote:

> 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).
> 
> [...]

Applied to 7.3/scsi-fixes, thanks!

[1/1] scsi: mpi3mr: fix use-after-free on tgt_dev->starget during target device refresh/update
      https://git.kernel.org/mkp/scsi/c/139f57343b3d

-- 
Martin K. Petersen