Re: [PATCH 14/21] scsi: mpi3mr: Fix target private data use after free in BSG passthrough

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mpi3mr: scsi_target UAF in mpi3mr_tgtdev_block_io_dec() for SAS targets
- [High] scsi: mpi3mr: leaked block_io in mpi3mr_tgtdev_block_io_dec() on LUN removal
--

commit 5bf2d57a06044ae754274ac7045b1778169812fc
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

scsi: mpi3mr: Fix target private data use after free in BSG passthrough

This commit introduces mpi3mr_tgtdev_block_io_inc() and
mpi3mr_tgtdev_block_io_dec() to update block_io on the SCSI target private
data under mrioc->tgtdev_lock while holding a reference to tgtdev across the
BSG command wait, and moves kfree(starget->hostdata) inside mrioc->tgtdev_lock
in mpi3mr_target_destroy().

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 99928fcc5bd79..e7833b24c5097 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -788,6 +788,58 @@ struct mpi3mr_tgt_dev *mpi3mr_get_tgtdev_by_handle(
>  	return tgtdev;
>  }
>  
> +/**
> + * mpi3mr_tgtdev_block_io_inc - Block I/O on the SCSI target of a tgtdev
> + * @mrioc: Adapter instance reference
> + * @tgtdev: Target device reference, held by the caller
> + *
> + * Increment block_io of the SCSI target private data. The lookup and the
> + * increment are done under tgtdev_lock, which mpi3mr_target_destroy holds
> + * while it frees the private data.
> + *
> + * Return: The private data that was incremented, only to be handed back to
> + * mpi3mr_tgtdev_block_io_dec. NULL if the tgtdev has no SCSI target.
> + */
> +struct mpi3mr_stgt_priv_data *mpi3mr_tgtdev_block_io_inc(struct mpi3mr_ioc *mrioc,
> +							 struct mpi3mr_tgt_dev *tgtdev)
> +{
> +	struct mpi3mr_stgt_priv_data *stgt_priv = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
> +	if (tgtdev->starget && tgtdev->starget->hostdata) {
> +		stgt_priv = tgtdev->starget->hostdata;
> +		atomic_inc(&stgt_priv->block_io);
> +	}
> +	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
> +	return stgt_priv;
> +}
> +
> +/**
> + * mpi3mr_tgtdev_block_io_dec - Undo mpi3mr_tgtdev_block_io_inc
> + * @mrioc: Adapter instance reference
> + * @tgtdev: Target device reference, held by the caller
> + * @stgt_priv: Value returned by mpi3mr_tgtdev_block_io_inc
> + *
> + * Decrement block_io only if the SCSI target still has the same private
> + * data. If the target was removed meanwhile the private data is gone
> + * and there is nothing to undo.
> + */
> +void mpi3mr_tgtdev_block_io_dec(struct mpi3mr_ioc *mrioc,
> +				struct mpi3mr_tgt_dev *tgtdev,
> +				struct mpi3mr_stgt_priv_data *stgt_priv)
> +{
> +	unsigned long flags;
> +
> +	if (!stgt_priv)
> +		return;
> +
> +	spin_lock_irqsave(&mrioc->tgtdev_lock, flags);
> +	if (tgtdev->starget && tgtdev->starget->hostdata == stgt_priv)
> +		atomic_dec_if_positive(&stgt_priv->block_io);
> +	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
> +}

[Severity: High]
Can mpi3mr_tgtdev_block_io_dec() leak a block_io count and permanently wedge
I/O to the target when the last LUN is removed while a BSG task management
command is in flight?

In mpi3mr_sdev_destroy(), when the last LUN on a target is destroyed,
tgt_dev->starget is cleared even though starget and stgt_priv remain alive:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sdev_destroy() {
	...
	if (tgt_dev && (!scsi_tgt_priv_data->num_luns))
		tgt_dev->starget = NULL;
	...
}

For example, during __scsi_scan_target(), probing LUN 0 can find peripheral
qualifier 1 or 3 and call __scsi_remove_device() on the temporary LUN 0
scsi_device before scanning the remaining LUNs, or the last LUN can be
removed while a concurrent scan holds starget->reap_ref.

If mpi3mr_bsg_process_mpt_cmds() increments stgt_priv->block_io via
mpi3mr_tgtdev_block_io_inc() before mpi3mr_sdev_destroy() sets
tgt_dev->starget = NULL, and then completes while tgt_dev->starget is NULL,
mpi3mr_tgtdev_block_io_dec() will see tgtdev->starget == NULL and skip
atomic_dec_if_positive(&stgt_priv->block_io) (a regression that remains even
after later commit 23e409e23e8d fixes the separate stgt_priv slab reuse).

When mpi3mr_sdev_init() subsequently initializes another LUN on the same
starget, it reattaches starget without resetting block_io:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_sdev_init() {
	...
	if (tgt_dev) {
		if (tgt_dev->starget == NULL)
			tgt_dev->starget = starget;
	...
}

Wouldn't that leave stgt_priv->block_io permanently elevated and cause
mpi3mr_qcmd() to reject all subsequent commands with
SCSI_MLQUEUE_DEVICE_BUSY?

[ ... ]

> @@ -4953,10 +5005,9 @@ static void mpi3mr_target_destroy(struct scsi_target *starget)
>  		mpi3mr_tgtdev_put(tgt_dev);
>  		mpi3mr_tgtdev_put(tgt_dev);
>  	}
> -	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
> -
>  	kfree(starget->hostdata);
>  	starget->hostdata = NULL;
> +	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
>  }

[Severity: High]
Could mpi3mr_tgtdev_block_io_dec() and mpi3mr_tgtdev_block_io_inc()
dereference a freed struct scsi_target via tgtdev->starget->hostdata for SAS
transport targets?

In mpi3mr_target_alloc(), SAS transport targets (where
mrioc->sas_transport_enabled && !starget->channel) have tgt_dev->starget =
starget assigned, but starget->id is assigned by scsi_transport_sas
(rphy->scsi_target_id) rather than tgt_dev->perst_id.

When a SAS device is removed in mpi3mr_remove_device_by_sas_address(), tgtdev
is removed from mrioc->tgtdev_list before mpi3mr_remove_tgtdev_from_host()
runs:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_remove_device_by_sas_address() {
	...
	if (tgtdev) {
		if (!list_empty(&tgtdev->list)) {
			was_on_tgtdev_list = 1;
			if (tgtdev->state == MPI3MR_DEV_REMOVE_HS_STARTED) {
				list_del_init(&tgtdev->list);
				mpi3mr_tgtdev_put(tgtdev);
			}
		}
	}
	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
	if (was_on_tgtdev_list) {
		if (tgtdev->host_exposed)
			mpi3mr_remove_tgtdev_from_host(mrioc, tgtdev);
	...
}

Because tgtdev is no longer on mrioc->tgtdev_list,
__mpi3mr_get_tgtdev_by_addr_and_rphy() in mpi3mr_sdev_destroy() returns NULL
and does not clear tgt_dev->starget (or if target scanning fails before adding
a LUN, mpi3mr_sdev_destroy() never runs).

Then, when scsi_target_destroy() calls mpi3mr_target_destroy(),
tgt_dev->starget is only cleared when tgt_dev->perst_id == starget->id:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_target_destroy() {
	...
	tgt_dev = __mpi3mr_get_tgtdev_from_tgtpriv(mrioc, scsi_tgt_priv_data);
	if (tgt_dev && (tgt_dev->starget == starget) &&
	    (tgt_dev->perst_id == starget->id))
		tgt_dev->starget = NULL;
	...
}

When tgt_dev->perst_id != starget->id, tgt_dev->starget is left pointing to
starget after scsi_target_destroy() drops the final device reference and frees
starget. While mpi3mr_bsg_process_mpt_cmds() holds its reference to tgtdev
across wait_for_completion_timeout(), won't evaluating
tgtdev->starget->hostdata in mpi3mr_tgtdev_block_io_dec() (or in
mpi3mr_tgtdev_block_io_inc() after a failed target scan) read from the freed
struct scsi_target?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104120103.503871-1-ranjan.kumar@broadcom.com?part=14