Re: [PATCH v9 17/29] scsi-multipath: clear path when device is blocked

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] scsi: multipath: UAF and NULL deref in scsi_device_block() on
  removal race
- [High] scsi: multipath: NULL deref and UAF in scsi_device_block()
  during LUN scan
--

commit 6f23ec119648804e57ce4572c175483a5109ea7a
Author: John Garry <john.garry@linux.dev>

scsi-multipath: clear path when device is blocked

This commit introduces scsi_mpath_dev_clear_path() and calls it from
scsi_device_block() to clear the active multipath device path when a SCSI
device is blocked.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 36bc995e4e250..1950034f580f4 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
[ ... ]
> @@ -3037,6 +3038,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);
>  
>  	WARN_ONCE(err, "%s: failed to block %s in state %d\n",
>  		  __func__, dev_name(&sdev->sdev_gendev), state);

[Severity: High]
Can calling scsi_mpath_dev_clear_path() after dropping sdev->state_mutex
race with __scsi_remove_device() and cause a use-after-free or NULL
pointer dereference on the multipath head?

scsi_block_targets() iterates target devices via starget_for_each_device()
without holding shost->scan_mutex.  Once scsi_device_block() unlocks
sdev->state_mutex, __scsi_remove_device() can transition sdev->sdev_state
from SDEV_BLOCK to SDEV_DEL and tear down the multipath head while
starget_for_each_device() still holds a reference to sdev:

CPU0                                  CPU1
scsi_device_block()
  mutex_lock(&sdev->state_mutex)
  __scsi_internal_device_block_nowait()
  mutex_unlock(&sdev->state_mutex)
                                      __scsi_remove_device()
                                        mutex_lock(&sdev->state_mutex)
                                        sdev->sdev_state = SDEV_DEL
                                        mutex_unlock(&sdev->state_mutex)
                                        ...
                                        scsi_mpath_remove_device()
                                          scsi_mpath_remove_head()
                                            scsi_mpath_put_head()
                                            scsi_mpath_dev->scsi_mpath_head = NULL
  scsi_mpath_dev_clear_path()
    mpath_clear_current_path()        <-- use-after-free on mpath_head
    mpath_synchronize(mpath_head)     <-- NULL deref or use-after-free

Because scsi_mpath_remove_head() drops the reference on scsi_mpath_head
and sets scsi_mpath_dev->scsi_mpath_head to NULL while leaving
mpath_device.mpath_head dangling, mpath_clear_current_path() reads the
freed mpath_device->mpath_head and mpath_synchronize() dereferences a NULL
or freed mpath_head.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 8e229357e62c5..67e9010d54782 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -240,6 +240,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;
> +
> +       if (mpath_clear_current_path(mpath_device))
> +               mpath_synchronize(mpath_head);
> +}
> +
>  static void scsi_mpath_clone_end_io(struct bio *clone)
>  {
>  	struct bio *master_bio = clone->bi_private;

[Severity: High]
What prevents scsi_device_block() from calling scsi_mpath_dev_clear_path()
while scsi_mpath_dev_alloc() is still initializing or freeing
sdev->scsi_mpath_dev during LUN scanning?

sdev is already linked onto starget->devices and set to SDEV_RUNNING in
scsi_add_lun() before scsi_mpath_dev_alloc() runs.  Inside
scsi_mpath_dev_alloc(), scsi_multipath_sdev_init() publishes
sdev->scsi_mpath_dev before scsi_mpath_head and mpath_device.mpath_head
are populated, and the error path in scsi_multipath_sdev_uninit() frees
sdev->scsi_mpath_dev before clearing the pointer:

CPU0                                  CPU1
scsi_add_lun()
  scsi_mpath_dev_alloc()
    scsi_multipath_sdev_init()
      sdev->scsi_mpath_dev = ...
                                      scsi_block_targets()
                                        ...
                                        scsi_device_block()
                                          scsi_mpath_dev_clear_path()
                                            mpath_clear_current_path()
                                              mpath_head->current_path[node]
                                              <-- NULL pointer dereference
    ...
    sdev->scsi_mpath_dev->mpath_device.mpath_head = ...

If scsi_mpath_dev_alloc() fails (for example when a LUN has no VPD page 83
device ID) and calls scsi_multipath_sdev_uninit(), a concurrent
scsi_device_block() also risks a use-after-free on sdev->scsi_mpath_dev.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108204733.557995-1-john.garry@linux.dev?part=17