Re: [PATCH v6 4/5] scsi: core: Add device reprobe support to scsi_rescan_device()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] scsi: error handling skips drv->rescan and returns false success
- [High] scsi: unsynchronized sdev->inquiry_len update causes OOB read in sysfs
- [High] scsi: concurrent lockless access to sdev->inquiry causes UAF during rescan
- [High] scsi: core: Use-After-Free of dev->mutex on reprobe error path
--

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

scsi: core: Add device reprobe support to scsi_rescan_device()

This patch updates INQUIRY data on rescan and calls device_reprobe() if
the peripheral qualifier or type changed. This handles ALUA unavailable
states.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 93a8719fcf265..ddb92282d49ff 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
[ ... ]
> @@ -1677,18 +1758,52 @@ int scsi_rescan_device(struct scsi_device *sdev)
>  		goto unlock;
>  	}
>  
> -	scsi_attach_vpd(sdev);
> -	scsi_cdl_check(sdev);
> -
> -	if (sdev->handler && sdev->handler->rescan)
> -		sdev->handler->rescan(sdev);
> +	/*
> +	 * Rescan standard INQUIRY data to detect changes in device
> +	 * properties (vendor, model, rev, peripheral qualifier, device type, etc.)
> +	 */
> +	inq_result = kmalloc(result_len, GFP_KERNEL);
> +	if (inq_result) {

[Severity: High]
Could this inadvertently break error handling by skipping the rescan hooks
and returning a false success?

If the kmalloc() fails, or if the device is unresponsive and scsi_probe_lun()
returns an error like -EIO, the function skips the entire block. Because
ret is still initialized to 0, it returns success to the caller.

Crucially, the fallback to drv->rescan(dev) (which was previously executed
unconditionally) is now bypassed. For SCSI disks, sd_rescan() calls
sd_revalidate_disk(). If the device is dead, this revalidation is what
zeroes the disk capacity. Because this patch skips the rescan hook on IO
or memory failure, does this leave dead disks with stale capacity and
geometry data in sysfs and the block layer?

> +		if (scsi_probe_lun(sdev, inq_result, result_len,
> +				   &bflags) == 0) {

[Severity: High]
Can calling scsi_probe_lun() on a live, sysfs-exposed device introduce an
out-of-bounds read in sysfs?

scsi_probe_lun() directly mutates sdev->inquiry_len without acquiring
sdev->inquiry_mutex:

drivers/scsi/scsi_scan.c:scsi_probe_lun() {
    ...
    sdev->inquiry_len = min(try_inquiry_len, response_len);
    ...
}

If the device responds with more data than previously (e.g., length jumps
from 36 to 255 bytes), sdev->inquiry_len is increased immediately. However,
the actual sdev->inquiry buffer is not reallocated until
scsi_update_inquiry_data() runs later in the flow.

If a concurrent read to the /sys/bus/scsi/devices/.../inquiry sysfs file
occurs, show_inquiry() will acquire the mutex and read the newly increased
sdev->inquiry_len bytes from the old, smaller sdev->inquiry buffer,
potentially leaking adjacent heap memory to userspace.

> +			ret = __scsi_reprobe_inquiry(sdev, inq_result,
> +						     max_t(size_t, sdev->inquiry_len, 36),
> +						     &need_reprobe);

[Severity: High]
Is it possible that triggering the INQUIRY buffer replacement on an active
device causes a use-after-free due to concurrent lockless readers?

__scsi_reprobe_inquiry() eventually calls scsi_update_inquiry_data(),
which frees the old buffer using a raw kfree() without RCU deferral:

drivers/scsi/scsi.c:scsi_update_inquiry_data() {
    ...
    kfree(sdev->inquiry);
    sdev->inquiry = new_inquiry;
    ...
}

Several kernel paths access sdev->inquiry without acquiring
sdev->inquiry_mutex or using RCU. For example, scsi_device_protection()
(called during block revalidation) reads from sdev->inquiry locklessly:

include/scsi/scsi_device.h:scsi_device_protection() {
    return sdev->scsi_level > SCSI_2 && sdev->inquiry[5] & (1<<0);
}

If one of these lockless readers executes concurrently with
scsi_rescan_device(), it could dereference the sdev->inquiry pointer
exactly as it is being freed.

> +			if (ret < 0) {
> +				kfree(inq_result);
> +				goto unlock;
> +			}
> +		}
> +		kfree(inq_result);
> +	}
[ ... ]
> +	if (need_reprobe) {
> +		get_device(dev);
> +		device_unlock(dev);
> +		ret = device_reprobe(dev);
> +		device_lock(dev);
> +
> +		if (sdev->sdev_state == SDEV_CANCEL ||
> +		    sdev->sdev_state == SDEV_DEL) {
> +			put_device(dev);
> +			ret = -ENODEV;
> +			goto unlock;
> +		}

[Severity: High]
Might this error path trigger a use-after-free on dev->mutex?

scsi_evt_thread() (via scsi_evt_emit()) calls scsi_rescan_device() without
holding an explicit get_device() reference, relying solely on
cancel_work_sync(&sdev->event_work) during scsi_device_dev_release() to
prevent device destruction.

If scsi_remove_device() runs concurrently during the unlocked reprobe
window above, it will drop its references. This leaves the temporary
get_device(dev) taken in scsi_rescan_device() as the sole remaining
reference.

When put_device(dev) is called here (or at the end of the block), it
drops that final reference from within the worker thread. This invokes
scsi_device_dev_release() inline, which calls cancel_work_sync() on the
currently executing work. The workqueue core detects the self-flush and
returns immediately, allowing scsi_device_dev_release() to free the sdev.

The code then jumps to the unlock label and executes device_unlock(dev)
on the freed memory.

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