Re: [PATCH v6 2/5] scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] scsi: use-after-free on inquiry buffer for concurrent lockless readers
- [High] scsi: data race on struct scsi_device bitfields corrupts concurrent state updates
- [Medium] scsi: torn reads of INQUIRY strings in procfs and target during rescan
- [Medium] scsi: core: silent truncation of inq_len in scsi_update_inquiry_data()
--

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

    scsi: core: Add scsi_update_inquiry_data() for updating INQUIRY data
    
    Add a new function scsi_update_inquiry_data() that can safely update all
    INQUIRY-derived fields for an existing SCSI device:
    
    - Vendor, model, revision strings
    - Peripheral qualifier and device type
    - Capability flags (removable, lockable, tagged queuing support, etc.)
    - ATA device detection and allow_restart setting

> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 3af9e77847f70..b80378ecf4856 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
[ ... ]
> @@ -549,6 +550,183 @@ void scsi_attach_vpd(struct scsi_device *sdev)
[ ... ]
> +int scsi_update_inquiry_data(struct scsi_device *sdev,
> +			     unsigned char *inq_result, size_t inq_len)
> +{
[ ... ]
> +	had_prior_inquiry = (sdev->inquiry != NULL);
> +	old_type = sdev->type;
> +	old_periph_qual = sdev->inq_periph_qual;
> +
> +	kfree(sdev->inquiry);
> +	sdev->inquiry = new_inquiry;
> +	sdev->inquiry_len = inq_len;

[Severity: High]
This frees and replaces the existing sdev->inquiry buffer dynamically on a
live device. However, there are multiple existing paths that read the inquiry
buffer locklessly, such as inline subsystem helpers in
include/scsi/scsi_device.h:

scsi_device_enclosure() {
    return sdev->inquiry ? (sdev->inquiry[6] & (1<<6)) : 1;
}

Since these lockless readers do not acquire inquiry_mutex and there is no RCU
protection, can a concurrent I/O path or sysfs read dereference the freed
inquiry pointer, resulting in a use-after-free? Furthermore, do these
non-atomic updates to the pointer and its length expose lockless readers to
torn state?

[Severity: Medium]
sdev->inquiry_len is an unsigned char (8 bits), while inq_len is a size_t.
If a manual rescan passes an inq_len of 256, will this assignment silently
truncate the length to 0, causing the device's sysfs inquiry attribute to
return 0 bytes?

> +	strscpy(sdev->vendor, sdev->inquiry + INQUIRY_VENDOR_OFFSET);
> +	strscpy(sdev->model, sdev->inquiry + INQUIRY_MODEL_OFFSET);
> +	/*
> +	 * memcpy() instead of strscpy() because strscpy() would read past
> +	 * the end of sdev->inquiry if its length is exactly 36 bytes.
> +	 */
> +	memcpy(sdev->rev, sdev->inquiry + INQUIRY_REVISION_OFFSET,
> +	       INQUIRY_REVISION_LEN);

[Severity: Medium]
This patch makes the INQUIRY string arrays mutable at runtime. While the
updates are protected by inquiry_mutex, unprotected readers like procfs and
the PSCSI target read these character arrays without acquiring the newly
introduced inquiry_mutex:

proc_print_scsidevice() {
    for (i = 0; i < 8; i++) {
        if (sdev->vendor[i] >= 0x20)
            ...
}

Will unprotected readers race with these non-atomic strscpy() updates, causing
torn reads that expose mixed or garbled strings?

> +	sdev->rev[INQUIRY_REVISION_LEN] = '\0';
> +	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
[ ... ]
> +	/*
> +	 * Update device type from INQUIRY byte 0.
> +	 * BLIST_ISROM is a quirk for devices that report wrong type but should
> +	 * be treated as (removable) CD-ROM. Override to TYPE_ROM as exception.
> +	 */
> +	if (sdev->sdev_bflags & BLIST_ISROM) {
> +		sdev->type = TYPE_ROM;
> +		sdev->removable = 1;
> +	} else {
> +		sdev->type = inq_result[0] & 0x1f;
> +		sdev->removable = (inq_result[1] & 0x80) >> 7;

[Severity: High]
These assignments update capability bitfields (e.g., is_ata, removable,
lockable) on a live device while holding only inquiry_mutex. In
struct scsi_device, these capability bitfields are packed into the same memory
word as highly dynamic operational state bitfields:

unsigned is_ata:1;
unsigned removable:1;
unsigned changed:1;     /* Data invalid due to media change */
unsigned busy:1;        /* Used to prevent races */

Since other subsystems modify the operational bitfields without holding
inquiry_mutex (for example, scsi_test_unit_ready() sets sdev->changed = 1 in
softirq context), does this cause a non-atomic read-modify-write on the shared
word? Could this race overwrite and lose critical runtime state changes like
media change notifications?

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