Re: [PATCH v6 5/5] scsi: core: Handle reprobe for existing devices during SCSI scan

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: sysfs duplicate filename and list corruption from duplicate scsi_device allocation
- [High] scsi: UAF and NULL pointer dereference in __scsi_reprobe_inquiry due to missing device_lock
- [Medium] scsi: scsi_device reference leak on reprobe of pseudo devices
--

commit ba325f494bfb24c08935735dfa22078230cda945
Author: Brian Bunker <brian@purestorage.com>

scsi: core: Handle reprobe for existing devices during SCSI scan

Complement scsi_rescan_device() reprobe by handling the scan path.
Update INQUIRY data and reprobe existing devices when standard INQUIRY
data changed.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index ddb92282d49ff..6df140d3c7482 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1215,7 +1216,32 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,
>  	 */
>  	sdev = scsi_device_lookup_by_target(starget, lun);
>  	if (sdev) {
> -		if (rescan != SCSI_SCAN_INITIAL || !scsi_device_created(sdev)) {
> +		if (rescan == SCSI_SCAN_INITIAL && scsi_device_created(sdev)) {
> +			/*
> +			 * Initial scan found device in CREATED state (being probed
> +			 * by another thread). Drop reference and allocate new -
> +			 * the other thread will complete setup of the original.
> +			 */
> +			scsi_device_put(sdev);
> +			sdev = scsi_alloc_sdev(starget, lun, hostdata);
> +			if (!sdev)
> +				goto out;

[Severity: High]
Does allocating a new device here create a duplicate device for the same LUN?

Since scsi_alloc_sdev() blindly adds the new device to the starget->devices
list and the original device is still on the list, both threads will proceed
to call scsi_add_lun() and invoke device_add(). This guarantees a sysfs
duplicate filename collision and failure. Additionally, subsequent lookups
using __scsi_device_lookup_by_target() will permanently intercept and return
the dummy device at the head of the list instead of the successfully probed
one.

[ ... ]

> +		} else if (rescan != SCSI_SCAN_INITIAL && !scsi_device_created(sdev)) {
> +			/*
> +			 * Manual rescan of fully initialized device.
> +			 * Reprobe to detect peripheral qualifier or device type
> +			 * changes (e.g., ALUA state transitions).
> +			 */
> +			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
> +				"scsi scan: device exists (type %d, PQ %d), reprobing\n",
> +				sdev->type, sdev->inq_periph_qual));
> +			is_reprobe = true;
> +		} else {

[ ... ]

>  	if (scsi_device_is_pseudo_dev(sdev)) {
>  		if (bflagsp)
>  			*bflagsp = BLIST_NOLUN;
>  		return SCSI_SCAN_LUN_PRESENT;
>  	}

[Severity: Medium]
Will this early return leak a scsi_device reference when is_reprobe is true?

When a manual rescan encounters an existing pseudo device, the code sets
is_reprobe to true and retains the extra kobject reference acquired by
scsi_device_lookup_by_target(). By returning directly here, it bypasses the
cleanup block at the end of the function where scsi_device_put(sdev) would
drop the lookup reference.

[ ... ]

> +	/*
> +	 * For reprobe scenarios, update the inquiry data with fresh
> +	 * INQUIRY results. The device already exists in sysfs, so we
> +	 * don't call scsi_add_lun() which would try to add it again.
> +	 */
> +	if (is_reprobe) {
> +		bool need_reprobe = false;
> +		int update_ret = __scsi_reprobe_inquiry(sdev, result, result_len,
> +							&need_reprobe);

[Severity: High]
Does this __scsi_reprobe_inquiry() call lack the required device_lock()?

This reprobe path calls __scsi_reprobe_inquiry() while holding only
shost->scan_mutex. Inside __scsi_reprobe_inquiry(), dev->driver is checked
and then dev->driver->owner is accessed for try_module_get() before invoking
the driver's rescan callback. Because device_lock(&sdev->sdev_gendev) is
not held, a concurrent sysfs unbind operation or module unload can detach the
driver and clear dev->driver immediately after the check, leading to a NULL
pointer dereference or use-after-free on the freed driver module.

> +
> +		if (update_ret < 0) {
> +			res = SCSI_SCAN_NO_RESPONSE;
> +			goto out_free_result;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012210511.16061-1-brian@purestorage.com?part=5