Re: [PATCH v6 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] Unprotected, lockless access to `sdev->scsi_mpath_dev` in `scsi_device_block()` causes NULL pointer dereferences and Use-After-Free (UAF) bugs when racing with device initialization or teardown.
--

--- Patch [16]: [PATCH v6 16/27] scsi-multipath: clear path when device is blocked ---
commit 8f07d9f873129f9ecc9b8d3d02fee5bce667c745
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.

> 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]
Is it safe to access sdev->scsi_mpath_dev without holding state_mutex?

When scsi_alloc_sdev() adds a new device to shost->__devices, it becomes
globally visible. Then scsi_mpath_dev_alloc() sets sdev->scsi_mpath_dev but
leaves scsi_mpath_head as NULL while issuing SCSI VPD queries.

If a concurrent error on the bus triggers scsi_device_block(), it could
read the partially initialized sdev->scsi_mpath_dev here outside of any locks.

Additionally, if scsi_mpath_dev_alloc() encounters an error and frees
sdev->scsi_mpath_dev, could reading it here cause a use-after-free?

During teardown, if __scsi_remove_device() is called concurrently with
scsi_device_block(), the teardown thread might set scsi_mpath_head to NULL.
Can this lead to a NULL pointer dereference when evaluating
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 0d439cac6447a..b23cd6f12c602 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c

[ ... ]

> @@ -238,6 +238,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]
Does this code dereference a NULL pointer if it races with initialization or
teardown?

If scsi_mpath_head is NULL due to the races mentioned above, deriving
mpath_head from it will calculate an invalid pointer.

> +
> +       if (mpath_clear_current_path(mpath_device))

[Severity: High]
Could calling mpath_clear_current_path() with a partially initialized
mpath_device lead to a NULL pointer dereference?

If mpath_device->mpath_head is NULL, it will trigger an immediate dereference
in the multipath code:

lib/multipath.c:mpath_clear_current_path() {
    ...
	struct mpath_head *mpath_head = mpath_device->mpath_head;
    ...
		if (mpath_device ==
			rcu_access_pointer(mpath_head->current_path[node])) {
    ...
}

> +               mpath_synchronize(mpath_head);
> +}

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