[PATCH] scsi: ses: skip an enclosure status page shorter than its header

[PATCH] scsi: ses: skip an enclosure status page shorter than its header

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

ses_intf_add() trusts the length an enclosure reports for its diagnostic
page 2. ses_set_page2_descriptor() clears everything past the eight-byte
status-page header; when the enclosure reports a shorter page,
page2_len - 8 underflows to a huge size_t and the clear runs off the
undersized allocation -- a heap out-of-bounds write, reached through a
sysfs component-control write, that panics the host.

Skip a page 2 too short for its header, as a page 2 whose RECEIVE
DIAGNOSTIC fails is already skipped: the enclosure still registers from
page 1, and ses_page2_supported() then reports no page 2, so the clear
is never reached. Conforming enclosures are unaffected.

Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Reproduced on v7.2-rc1 under KASAN (CONFIG_KASAN_INLINE=y,
kasan.fault=panic). ses_set_page2_descriptor() is static and reached only
through the sysfs component-control path, so the clear is exercised
directly by an in-kernel litmus (page2 = kzalloc(4), then
memset(page2 + 8, 0, page2_len - 8) with page2_len = 4):

  A (no guard): BUG: KASAN: out-of-bounds write of size
    18446744073709551612 at a 4-byte kmalloc-8 region; kernel panic.
  B (this patch): the short page 2 is skipped; page2 stays NULL,
    ses_page2_supported() is false, the clear is never reached. Clean.
  Control (page2_len = 64): the clear stays in-bounds. Clean.
  m32/m64: ASan negative-size-param (size = -4), 4 bytes past the region.

The guard bails before ses_dev->page2 is set, so no accessor reaches
ses_set_page2_descriptor(); every accessor is gated by
ses_page2_supported() (page2 != NULL).

This fixes only the header underflow. It does not bound the descriptor
walk against the page-1 element count -- the separate concern of commit
801ab13d50cf ("scsi: ses: Fix possible desc_ptr out-of-bounds accesses")
and its siblings. No Fixes: tag: the flaw dates to the 2008 ses ULD,
unreachable in a grafted tree, and that series set the Cc: stable@-without-

Fixes: precedent for this class.
---
 drivers/scsi/ses.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 4c348645b04e..f77054695e12 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -766,6 +766,9 @@ static int ses_intf_add(struct device *cdev)
 		goto page2_not_supported;
 
 	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	/* a status page too short for its 8-byte header carries no page 2 */
+	if (len < 8)
+		goto page2_not_supported;
 	buf = kzalloc(len, GFP_KERNEL);
 	if (!buf)
 		goto err_free;

---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260705-b4-disp-e1aca452-8afe6b970864

Best regards,
-- 
Bryam Vargas <hexlabsecurity@proton.me>

Re: [PATCH] scsi: ses: skip an enclosure status page shorter than its header

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch prevents a `memset` length underflow by skipping Page 2 processing if `len < 8`, but fails to ensure that `page2` is large enough to hold all component descriptors defined in Page 1, leaving out-of-bounds reads and writes unpatched.
- [High] Missing length validation for Page 1 leads to out-of-bounds reads during descriptor parsing, leading to artificially large component counts.
- [High] Missing bounds check on Page 7 length leads to an unconditional out-of-bounds read.
- [High] Reading the `id` sysfs attribute leaks up to 8 bytes of uninitialized or adjacent kernel heap memory to user-space.
--

commit 30f64f6103a88369da925d5a9a969b26cc05a865
Author: Bryam Vargas <hexlabsecurity@proton.me>

scsi: ses: skip an enclosure status page shorter than its header

This commit prevents a heap out-of-bounds write by ensuring that the diagnostic
page 2 length is at least 8 bytes. It skips the page if the length is shorter
than its header.

> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index 4c348645b04ee..f77054695e125 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -766,6 +766,9 @@ static int ses_intf_add(struct device *cdev)
>  		goto page2_not_supported;
>  
>  	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
> +	/* a status page too short for its 8-byte header carries no page 2 */
> +	if (len < 8)
> +		goto page2_not_supported;

[Severity: High]
This is a pre-existing issue, but does this check go far enough to prevent
out-of-bounds accesses for page 2?

While this ensures the buffer is at least 8 bytes, it doesn't guarantee it is
large enough to hold all component descriptors defined in page 1.

In both ses_set_page2_descriptor() and ses_get_page2_descriptor(), the loops
advance desc_ptr based on page 1 element counts:

drivers/scsi/ses.c:ses_set_page2_descriptor() {
    ...
				memcpy(desc_ptr, desc, 4);
				/* set select */
				desc_ptr[0] |= 0x80;
    ...
}

If page2_len is 8, but page 1 defined multiple components, could these loops
still read or write past the end of the page 2 allocation?

[Severity: High]
This is a pre-existing issue, but does page 1 also suffer from missing length
validation that can lead to out-of-bounds reads?

When parsing page 1 elements earlier in ses_intf_add(), the loop condition only
checks the pointer address:

	for (i = 0; i < num_enclosures && type_ptr < buf + len; i++) {
		types += type_ptr[2];
		type_ptr += type_ptr[3] + 4;
	}

If len is short (e.g., 9 bytes), the loop body unconditionally accesses
type_ptr[2] and type_ptr[3], reading past the end of the allocation.

This could populate page1_num_types (the types variable) with untrusted heap
data.

Later in ses_enclosure_data_process(), the code iterates using this untrusted
types count without verifying against page1_len:

	for (i = 0; i < types; i++, type_ptr += 4) {
		for (j = 0; j < type_ptr[1]; j++) {

Could this lead to further out-of-bounds memory accesses?

[Severity: High]
This is a pre-existing issue, but is there a similar missing bounds check on
page 7?

In ses_enclosure_data_process(), the page 7 length is derived from its header.

If the device returns a length smaller than 8, the buffer is allocated too
small, but the code still unconditionally reads offsets 10 and 11:

		desc_ptr = buf + 8;
		len = (desc_ptr[2] << 8) + desc_ptr[3];

Could this lead to an unconditional out-of-bounds read if the page 7 buffer is
smaller than 12 bytes?

[Severity: High]
This is a pre-existing issue, but can ses_show_id() leak adjacent kernel heap
memory to user-space?

If a device returns a page 1 shorter than 20 bytes, ses_intf_add() will allocate
a small buffer.

However, ses_show_id() unconditionally reads 8 bytes from offset 12:

	unsigned long long id = get_unaligned_be64(ses_dev->page1+8+4);

	return sprintf(buf, "%#llx\n", id);

If page1_len is not validated, could this expose uninitialized or adjacent
heap memory via the id sysfs attribute?

>  	buf = kzalloc(len, GFP_KERNEL);
>  	if (!buf)
>  		goto err_free;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-b4-disp-e1aca452-v1-1-1dab8448608c@proton.me?part=1