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

[Laurence Oberman <loberman@redhat.com>]

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