Re: [PATCH v6 3/5] scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] scsi: core: Use-After-Free in lockless INQUIRY readers during rescan
- [High] scsi: core: Bitfield data race between INQUIRY capability updates and SCSI Error Handler
- [Medium] scsi: core: scsi_update_inquiry_data clobbers BLIST_NOT_LOCKABLE workaround during rescan
--

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

scsi: core: Refactor scsi_add_lun() to use scsi_update_inquiry_data()

This commit refactors scsi_add_lun() to use a new helper,
scsi_update_inquiry_data(), which centralizes INQUIRY parsing. It moves
several capability and identity assignments into the new helper and ensures
consistent handling between initial probe and runtime rescans.

> 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
[ ... ]
> +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;

[Severity: High]
Does this expose concurrent lockless readers to a use-after-free?

During a rescan, the old INQUIRY buffer is freed directly with kfree().

However, there are multiple helpers (like scsi_device_enclosure() used by
SES sysfs traversal, and scsi_device_tpgs() used by ALUA workqueues) that
dereference sdev->inquiry locklessly:

include/scsi/scsi_device.h:scsi_device_enclosure() {
    return sdev->inquiry ? (sdev->inquiry[6] & (1<<6)) : 1;
}

If a rescan is triggered concurrently with ALUA background polling or SES
sysfs reads, these lockless readers could dereference the freed memory.

> +	sdev->inquiry_len = inq_len;
[ ... ]
> +	/*
> +	 * Check if this is an ATA device (SATA emulation layer).
> +	 * ATA devices need allow_restart set to work around SATL power
> +	 * management specifications.
> +	 */
> +	if (strncmp(sdev->vendor, "ATA     ", 8) == 0) {
> +		sdev->is_ata = 1;
> +		sdev->allow_restart = 1;
> +	} else
> +		sdev->is_ata = 0;
> +
> +	/*
> +	 * 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]
Can modifying these bitfields during a rescan corrupt other bitfields via
a data race?

scsi_update_inquiry_data() modifies capability bitfields like is_ata,
removable, and lockable under sdev->inquiry_mutex. 

However, the SCSI Error Handler modifies operational state bitfields
asynchronously without this lock:

drivers/scsi/scsi_error.c:__scsi_report_device_reset() {
    sdev->was_reset = 1;
    sdev->expecting_cc_ua = 1;
}

All of these bitfields (is_ata, removable, was_reset, expecting_cc_ua) are
packed into the same machine word in struct scsi_device.

If a device rescan runs concurrently with a bus reset report, the compiler's
read-modify-write operations will race, potentially clobbering either the
device capabilities or the critical error recovery state.

[ ... ]
> +		}
> +	}
> +
> +	/*
> +	 * Set lockable to match removable. Devices with removable media
> +	 * can typically have their media locked/unlocked via the
> +	 * ALLOW_MEDIUM_REMOVAL command.
> +	 */
> +	sdev->lockable = sdev->removable;

[Severity: Medium]
Will this unconditionally clobber the BLIST_NOT_LOCKABLE workaround during
a rescan?

During initial device setup in scsi_scan.c:scsi_add_lun(), if a device has
BLIST_NOT_LOCKABLE set, the code forces sdev->lockable = 0. However, that
blacklist guard was not moved into scsi_update_inquiry_data() with the rest
of the logic.

When a blacklisted removable device is rescanned, this assignment will
unconditionally overwrite the blacklist override, setting lockable back to 1.

This could expose broken hardware to ALLOW_MEDIUM_REMOVAL commands,
potentially causing hardware hangs or I/O timeouts.

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