Re: [PATCH] scsi: sd: validate device-supplied mode sense lengths in cache_type_store

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Stack out-of-bounds read and write in `scsi_cdl_enable` due to unvalidated device-supplied mode sense lengths.
- [High] Data race on `struct scsi_disk` bitfields between `cache_type_store` and `sd_revalidate_disk`.
--

--- Patch [1]: [PATCH] scsi: sd: validate device-supplied mode sense lengths in cache_type_store ---
commit 58b82fd0109d95a6ff61d8f8d2790723466ad640
Author: Jay Vadayath <jay@artiphishell.com>

scsi: sd: validate device-supplied mode sense lengths in cache_type_store

This commit adds bounds checking to cache_type_store() to prevent an
out-of-bounds stack access when parsing device-supplied MODE SENSE lengths.
It explicitly verifies that the combined header and block descriptor
lengths do not exceed the on-stack buffer size before calculating offsets.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 599e75f333343..4234ce314c9d9 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c

[ ... ]

> @@ -268,10 +268,19 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
>  	if (scsi_mode_sense(sdp, 0x08, 8, 0, buffer, sizeof(buffer), SD_TIMEOUT,
>  			    sdkp->max_retries, &data, NULL))
>  		return -EINVAL;
> -	len = min_t(size_t, sizeof(buffer), data.length - data.header_length -
> -		  data.block_descriptor_length);
> -	buffer_data = buffer + data.header_length +
> -		data.block_descriptor_length;
> +
> +	/*
> +	 * The mode parameter header and block descriptor lengths are
> +	 * supplied by the device and must not be trusted (e.g. a malicious
> +	 * USB mass storage device).  Reject responses that would place the
> +	 * caching mode page (of which we touch the first three bytes)
> +	 * outside of the buffer to avoid an out-of-bounds access below.
> +	 */
> +	offset = data.header_length + data.block_descriptor_length;
> +	if (offset + 3 > sizeof(buffer))
> +		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does modifying bitfields in cache_type_store()
create a data race with other device events?

Earlier in this same function, the code directly modifies sdkp->cache_override:

drivers/scsi/sd.c:cache_type_store() {
    ...
    if (strncmp(buf, temp, sizeof(temp) - 1) == 0) {
        buf += sizeof(temp) - 1;
        sdkp->cache_override = 1;
    ...
}

Concurrently, during a device rescan or hotplug event, sd_read_cache_type()
modifies adjacent bitfields in the same struct:

drivers/scsi/sd.c:sd_read_cache_type() {
    ...
    sdkp->DPOFUA = (data.device_specific & 0x10) != 0;
    ...
}

Since bitfield updates are read-modify-write operations, could a concurrent
sysfs write to cache_type silently corrupt other bitfields like DPOFUA
or lbpme if they aren't protected by a shared lock?

[Severity: Critical]
This is a pre-existing issue, but does a structurally identical out-of-bounds
vulnerability exist in scsi_cdl_enable() in drivers/scsi/scsi.c?

Looking at scsi_cdl_enable(), the same device-supplied lengths are used to
calculate an offset into an on-stack buffer without bounds checking:

drivers/scsi/scsi.c:scsi_cdl_enable() {
    ...
    buf_data = buf + data.header_length + data.block_descriptor_length;
    ...
    buf_data[4] &= ~0x03;
    ...
}

If a malicious USB device returns a large block_descriptor_length, could
this calculation exceed the stack buffer bounds and lead to memory corruption,
just like the issue fixed here in cache_type_store()?

> +	len = min_t(size_t, sizeof(buffer) - offset, data.length - offset);
> +	buffer_data = buffer + offset;
>  	buffer_data[2] &= ~0x05;
>  	buffer_data[2] |= wce << 2 | rcd;
>  	sp = buffer_data[0] & 0x80 ? 1 : 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717191023.15011-1-jay@artiphishell.com?part=1