Linux SCSI subsystem development
 help / color / mirror / Atom feed
* [PATCH] scsi: sd: validate device-supplied mode sense lengths in cache_type_store
@ 2026-07-17 19:10 Jay Vadayath
  2026-07-17 19:23 ` sashiko-bot
  2026-07-17 19:24 ` James Bottomley
  0 siblings, 2 replies; 3+ messages in thread
From: Jay Vadayath @ 2026-07-17 19:10 UTC (permalink / raw)
  To: Martin K. Petersen, James E.J. Bottomley
  Cc: linux-scsi, linux-kernel, Jay Vadayath

A malicious/emulated USB mass storage device (or any SCSI target) can
return a MODE SENSE(6/10) response whose header_length plus
block_descriptor_length is >= the size of the on-stack 64-byte buffer.

cache_type_store() computed
	buffer_data = buffer + data.header_length + data.block_descriptor_length
and then dereferenced buffer_data[0] and buffer_data[2] without ever
checking that this offset still lies within buffer[]. With a reported
block_descriptor_length of 60 and a 4-byte MODE SENSE(6) header the
offset becomes 64 (== sizeof(buffer)), so buffer_data[0]/buffer_data[2]
read and write past the end of the stack buffer. The subsequent length
computation could also underflow and let buffer_data + len run past the
buffer when handed to scsi_mode_select().

KASAN report from an unprivileged user writing to the sysfs cache_type
attribute of a device backed by a raw-gadget mass storage emulator:

  BUG: KASAN: stack-out-of-bounds in cache_type_store+0x8ba/0x8f0
  Read of size 1 at addr ffff888003097c72 by task poc/57
  Call Trace:
   dump_stack_lvl+0x53/0x70
   print_report+0xce/0x610
   kasan_report+0xce/0x100
   cache_type_store+0x8ba/0x8f0
   kernfs_fop_write_iter+0x384/0x4f0
   vfs_write+0x5c7/0xe60
   ksys_write+0xf7/0x1c0
   do_syscall_64+0x61/0x480
   entry_SYSCALL_64_after_hwframe+0x76/0x7e

Reject responses whose mode page offset does not leave room for the
three caching-mode-page bytes we touch, and bound the length by the
space actually remaining in the buffer, mirroring the careful bounds
checking already done in sd_read_cache_type().

This bug was discovered by Artiphishell's vTriage pipeline, which
generated a userspace raw-gadget reproducer that reliably triggers the
KASAN report on an unpatched kernel. The fix below was drafted with the
Claude coding assistant; a userspace reproducer is available on
request.

Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Jay Vadayath <jay@artiphishell.com>

---
 drivers/scsi/sd.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -228,7 +228,7 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
 	struct scsi_mode_data data;
 	struct scsi_sense_hdr sshdr;
 	static const char temp[] = "temporary ";
-	int len, ret;
+	int len, offset, ret;

 	if (sdp->type != TYPE_DISK && sdp->type != TYPE_ZBC)
 		/* no cache control on RBC devices; theoretically they
@@ -265,13 +265,22 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
 		return count;
 	}

 	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;
+	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;

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] scsi: sd: validate device-supplied mode sense lengths in cache_type_store
  2026-07-17 19:10 [PATCH] scsi: sd: validate device-supplied mode sense lengths in cache_type_store Jay Vadayath
@ 2026-07-17 19:23 ` sashiko-bot
  2026-07-17 19:24 ` James Bottomley
  1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-07-17 19:23 UTC (permalink / raw)
  To: Jay Vadayath; +Cc: linux-scsi

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] scsi: sd: validate device-supplied mode sense lengths in cache_type_store
  2026-07-17 19:10 [PATCH] scsi: sd: validate device-supplied mode sense lengths in cache_type_store Jay Vadayath
  2026-07-17 19:23 ` sashiko-bot
@ 2026-07-17 19:24 ` James Bottomley
  1 sibling, 0 replies; 3+ messages in thread
From: James Bottomley @ 2026-07-17 19:24 UTC (permalink / raw)
  To: Jay Vadayath, Martin K. Petersen; +Cc: linux-scsi, linux-kernel

On Fri, 2026-07-17 at 12:10 -0700, Jay Vadayath wrote:
> A malicious/emulated USB mass storage device (or any SCSI target) can
> return a MODE SENSE(6/10) response whose header_length plus
> block_descriptor_length is >= the size of the on-stack 64-byte
> buffer.

Malicious devices aren't usually part of our threat model because
checking every device action would result in enormous performance
drops.  There is an exception for USB devices because an arbitrary evil
maid could plug one into your laptop, but the usual threat model here
is unassisted data exfiltration or login bypass, but because of the
performance issue, you need to demonstrate a viable exploit.

Regards,

James

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-07-17 19:24 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-17 19:10 [PATCH] scsi: sd: validate device-supplied mode sense lengths in cache_type_store Jay Vadayath
2026-07-17 19:23 ` sashiko-bot
2026-07-17 19:24 ` James Bottomley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox