Re: [GIT PULL] SCSI fixes for 7.3-rc1+

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected traversal of `tgtdev_list` in `mpi3mr_refresh_tgtdevs` allows data races with concurrent list modifications.
--

commit 4a7c6ef4ce1e621008a2fb66b4611720ab5e8ceb
Author: James Bottomley <James.Bottomley@HansenPartnership.com>

SCSI fixes for 7.3-rc1+

This commit brings in several SCSI subsystem fixes, including enhancements for
Intel 4.0 controllers and a variety of driver fixes. Notable changes address
a target use-after-free in mpi3mr and a TOCTOU fix for io_uring passthrough
in bsg.

> 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 is it safe to traverse mrioc->tgtdev_list
without holding the tgtdev_lock across the iteration steps?

Because this loop (and the subsequent loops in mpi3mr_refresh_tgtdevs())
iterates without holding mrioc->tgtdev_lock, could concurrent modifications
to the list result in data races? 

For instance, mpi3mr_refresh_tgtdevs() runs in a workqueue thread, but a
top-half interrupt handler can process MPI3_EVENT_DEVICE_ADDED, subsequently
calling mpi3mr_create_tgtdev() -> mpi3mr_tgtdev_add_to_list() to add elements.

Since standard linked list operations are not atomic, couldn't iterating
here without the lock cause torn reads or traversal of invalid pointers if
an interrupt modifies the list concurrently?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/d7c2465525ad601d8569fa7219a8add38bf4aa26.camel@HansenPartnership.com?part=1