Re: [PATCH] scsi: core: Validate MODE SENSE lengths in scsi_cdl_enable()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Stack out-of-bounds write and information leak in cache_type_store() due to unsigned subtraction wrap and missing bounds checks on MODE SENSE length fields.
- [High] scsi_cdl_enable() sends a malformed MODE SELECT command if the device returns a non-zero block descriptor length.
- [High] Heap out-of-bounds read in sd_read_app_tag_own() due to unvalidated block descriptor length.
- [High] Massive heap out-of-bounds read in sd_read_io_hints() due to unvalidated MODE SENSE length.
--

commit 6d4bd3fe431e6d359b82e4420cd2b054307e0a8c
Author: Alberto Carboneri <acarboneri@drivesec.com>

scsi: core: Validate MODE SENSE lengths in scsi_cdl_enable()

This patch attempts to fix a stack out-of-bounds vulnerability in
scsi_cdl_enable() by verifying lengths and bounding them to the stack
buffer size. It correctly caps the buffer size and ensures the mode
header and block descriptor lengths fit within the available buffer.

Link: https://lore.kernel.org/linux-scsi/20260717192313.93D791F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-scsi/20260717222931.AC4EE1F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-scsi/df13ec87ac9b28e3b0a2d9eb26477e276ff0278a.camel@HansenPartnership.com/

> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 76cdad063f7bc..f285521d9de6d 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -735,11 +736,24 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
[ ... ]
> -		len = min_t(size_t, sizeof(buf),
> -			    data.length - data.header_length -
> -			    data.block_descriptor_length);
> -		buf_data = buf + data.header_length +
> -			data.block_descriptor_length;
> +		avail = min_t(size_t, data.length, sizeof(buf));
> +		if (data.header_length > avail)
> +			return -EINVAL;
> +
> +		offset = data.header_length;
> +		avail -= data.header_length;
> +
> +		if (data.block_descriptor_length > avail)
> +			return -EINVAL;
> +
> +		offset += data.block_descriptor_length;
> +		avail -= data.block_descriptor_length;
> +
> +		if (avail < 5)
> +			return -EINVAL;
> +
> +		buf_data = buf + offset;
> +		len = avail;
>  
>  		/*
>  		 * If we want to enable CDL and CDL is already enabled on the

While reviewing this patch, a few related pre-existing regressions
were found in the surrounding mode sense handling code.

In drivers/scsi/scsi.c:scsi_cdl_enable():

> 		if (enable) {
> 			if ((buf_data[4] & 0x03) == 0x02)
> 				goto out;
> 			buf_data[4] &= ~0x03;
> 			buf_data[4] |= 0x02;
> 		} else {
> 			if ((buf_data[4] & 0x03) == 0x00)
> 				goto out;
> 			buf_data[4] &= ~0x03;
> 		}
> 
> 		ret = scsi_mode_select(sdev, 1, 0, buf_data, len, 5 * HZ, 3,
> 				       &data, &sshdr);

[Severity: High]
This isn't a bug introduced by this patch, but does this regression
still allow a malformed mode select command to be sent if the device
returns a non-zero block descriptor length?

If the target returns a mode page with data.block_descriptor_length > 0,
the command constructed by scsi_mode_select() will have a header
claiming block descriptors exist, but the payload will be the mode page
itself.

Can the target misinterpret the mode page as a block descriptor,
potentially modifying sector size or disk formatting?


In drivers/scsi/sd.c:cache_type_store():

> 	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;
> 	buffer_data[2] &= ~0x05;

[Severity: Critical]
This is a pre-existing issue, but can this regression cause a stack
out-of-bounds write and information leak?

If a device returns a short data.length (e.g., 0) and a large
block_descriptor_length (e.g., 255) during cache_type_store(), the
unsigned subtraction data.length - data.header_length -
data.block_descriptor_length underflows to a huge value.

While len is clamped to sizeof(buffer) (64), buffer_data is advanced
by up to 259 bytes.

Does this cause buffer_data to point outside the 64-byte stack buffer,
modifying buffer_data[2] out of bounds, and allowing scsi_mode_select()
to leak the stack memory?


In drivers/scsi/sd.c:sd_read_app_tag_own():

> 	res = scsi_mode_sense(sdp, 1, 0x0a, 0, buffer, 36, SD_TIMEOUT,
> 			      sdkp->max_retries, &data, &sshdr);
> 
> 	if (res < 0 || !data.header_length ||
> 	    data.length < 6) {
[ ... ]
> 		return;
> 	}
> 
> 	offset = data.header_length + data.block_descriptor_length;
> 
> 	if ((buffer[offset] & 0x3f) != 0x0a) {

[Severity: High]
This isn't a bug introduced by this patch, but does this regression
lead to a heap out-of-bounds read?

During disk enumeration, sd_revalidate_disk() calls
sd_read_app_tag_own() on a device returning a malformed mode sense
response.

If sdev->use_10_for_ms is set, the device can return a 10-byte mode
sense response with a block_descriptor_length up to 65535, making the
offset reach up to 65543.

Since the buffer passed by sd_revalidate_disk() is only 512 bytes,
will accessing buffer[offset] read far outside the heap allocation?


In drivers/scsi/sd.c:sd_read_io_hints():

> 	res = scsi_mode_sense(sdp, /*dbd=*/0x8, /*modepage=*/0x0a,
> 			      /*subpage=*/0x05, buffer, SD_BUF_SIZE, SD_TIMEOUT,
> 			      sdkp->max_retries, &data, &sshdr);
> 	if (res < 0)
> 		return;
> 	start = (void *)buffer + data.header_length + 16;
> 	end = (void *)buffer + ALIGN_DOWN(data.header_length + data.length,
> 					  sizeof(*end));
[ ... ]
> 	for (desc = start; desc < end; desc++)
> 		if (!desc->st_enble || !sd_is_perm_stream(sdkp, desc - start))

[Severity: High]
This is a pre-existing issue, but could this regression cause a
massive heap out-of-bounds read?

scsi_mode_sense() is called with a 512-byte buffer (SD_BUF_SIZE),
which unconditionally forces use_10_for_ms = 1 since it exceeds 255
bytes.

The device can thus return a data.length up to 65537, causing the end
pointer to be calculated using this unvalidated length.

Does this cause the end pointer to point up to 65KB past the end of
the 512-byte heap buffer, which is then dereferenced during the loop
iteration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904135410.360314-1-acarboneri@drivesec.com?part=1