Re: [PATCH v5 16/27] scsi-multipath: clear path when device is blocked

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `scsi_device_block()` due to lockless access to `sdev->scsi_mpath_dev` leads to NULL pointer dereferences and Use-After-Free vulnerabilities during device initialization, teardown, and initialization failure.
--

--- Patch [16]: [PATCH v5 16/27] scsi-multipath: clear path when device is blocked ---
commit 029c1109c722c76acac1333821c2190171087cb6
Author: John Garry <[email protected]>

scsi-multipath: clear path when device is blocked

Add scsi_mpath_dev_clear_path() to clear a device path when it becomes
blocked.

Signed-off-by: John Garry <[email protected]>

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index daeb3693fe559..a1ec11050090c 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c

[ ... ]

> @@ -3008,6 +3009,8 @@ static void scsi_device_block(struct scsi_device *sdev, void *data)
>  		scsi_stop_queue(sdev);
>  
>  	mutex_unlock(&sdev->state_mutex);
> +	if ((err == 0) && sdev->scsi_mpath_dev)
> +		scsi_mpath_dev_clear_path(sdev->scsi_mpath_dev);

[Severity: High]
Since sdev->scsi_mpath_dev is read locklessly after dropping state_mutex,
can this race with device teardown or initialization failures?

If a concurrent teardown or error path calls scsi_multipath_sdev_uninit() 
and frees sdev->scsi_mpath_dev, could this result in a use-after-free when 
evaluating or passing the pointer to scsi_mpath_dev_clear_path()?

>  	WARN_ONCE(err, "%s: failed to block %s in state %d\n",
>  		  __func__, dev_name(&sdev->sdev_gendev), state);

[ ... ]

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 5999c59bf7297..801d95652fc6b 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c

[ ... ]

> @@ -231,6 +231,16 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
>  	return 0;
>  }
>  
> +void scsi_mpath_dev_clear_path(struct scsi_mpath_device *scsi_mpath_dev)
> +{
> +       struct mpath_device *mpath_device = &scsi_mpath_dev->mpath_device;
> +       struct scsi_mpath_head *scsi_mpath_head = scsi_mpath_dev->scsi_mpath_head;
> +       struct mpath_head *mpath_head = &scsi_mpath_head->mpath_head;

[Severity: High]
Could scsi_mpath_dev->scsi_mpath_head be NULL here?

Looking at scsi_multipath_sdev_init(), sdev->scsi_mpath_dev is published
globally before the inner scsi_mpath_head structure is fully initialized:

scsi_multipath_sdev_init() {
    ...
    scsi_mpath_dev->sdev = sdev;
    sdev->scsi_mpath_dev = scsi_mpath_dev;
    ...
}

If a concurrent scsi_device_block() call sees the non-NULL wrapper, would
this dereference a NULL scsi_mpath_head?

> +
> +       if (mpath_clear_current_path(mpath_device))
> +               mpath_synchronize(mpath_head);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16