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

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

[Alberto Carboneri]

scsi_cdl_enable() uses length fields returned by MODE SENSE to locate the
ATA feature mode page in a 64-byte stack buffer. A target can report a
total length shorter than its mode header and block descriptors. The
unsigned subtraction used for the MODE SELECT length can wrap, and the
separately computed buf_data can point beyond buf.

During automatic scan, enable is false, so the read-modify-write of
buf_data[4] can clear the low two bits of a target-selected out-of-bounds
stack byte. scsi_mode_select() can then copy up to 64 bytes from outside
the buffer into the outgoing MODE SELECT payload, disclosing stack contents
to the target.

This is reachable while scanning a USB storage device that identifies as
an ATA device and advertises CDL support. No filesystem mount or userspace
access to the block device is required.

On upstream commit cee9395acd80 ("Linux 7.3-rc1"), a build-specific,
one-vCPU QEMU/Raw Gadget proof using QEMU-only multi-UDC allocator
sampling executed a fixed proof command inside the guest and created a
UID-0-owned marker during automatic enumeration, with KASLR and NX
enabled.

The issue was independently found during security research at Drivesec
S.r.l.

Cap the available length to the buffer size. Validate and consume the mode
header and block descriptor lengths before using the page, and require the
five bytes needed to access the CDL field.

Fixes: 1b22cfb14142 ("scsi: core: Allow enabling and disabling command duration limits")
Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Closes: 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/
Cc: stable@vger.kernel.org
Assisted-by: LLM
Co-developed-by: Pimen Flavian Dei (Drivesec S.r.l.) <fdei@drivesec.com>
Signed-off-by: Pimen Flavian Dei (Drivesec S.r.l.) <fdei@drivesec.com>
Signed-off-by: Alberto Carboneri (Drivesec S.r.l.) <acarboneri@drivesec.com>
---
Tested on x86-64:
- GCC 13.3.0, W=1 drivers/scsi/scsi.o
- Clang 18.1.3, W=1 drivers/scsi/scsi.o
- Malformed BDL92 MODE SENSE regression, 3/3 fresh QEMU boots:
  scan continued with no MODE SELECT, disclosure, Oops, or panic
- Applies cleanly to v6.12.107

 drivers/scsi/scsi.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 76cdad063f7b..f285521d9de6 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -727,6 +727,7 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
 		struct scsi_mode_data data;
 		struct scsi_sense_hdr sshdr;
 		char *buf_data;
+		size_t avail, offset;
 		int len;
 
 		ret = scsi_mode_sense(sdev, 0x08, 0x0a, 0xf2, buf, sizeof(buf),
@@ -735,11 +736,24 @@ int scsi_cdl_enable(struct scsi_device *sdev, bool enable)
 			return -EINVAL;
 
 		/* Enable or disable CDL using the ATA feature page */
-		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

base-commit: af8c27375733fb6a6df9fa484cda77cc3dd0cb80
-- 
2.43.0

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

[sashiko-bot]

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