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 <[email protected]> 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/[email protected]?part=17
