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

[sashiko-bot@kernel.org]

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