[PATCH] scsi: ses: validate the page 1 geometry before walking it

[PATCH] scsi: ses: validate the page 1 geometry before walking it

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

ses_intf_add() stores the enclosure's page 1 type-descriptor pointer and
count (page1_types, page1_num_types) without bounding them against the
page 1 buffer.  The page 2 descriptor walks, ses_enclosure_data_process()
and the logical-id read in ses_show_id() all iterate or index with those
device-supplied values.  An enclosure that reports a short page 1 with an
inflated type count, or one too short for its logical id, makes those
reads run past the page 1 allocation -- an out-of-bounds read of adjacent
slab, some of which ses_show_id() hands back to user space.

Validate the page 1 geometry where it is parsed and reject a page too
short for its logical id or declaring more type descriptors than it
carries.  Conforming enclosures are unaffected.

Fixes: 9927c68864e9 ("[SCSI] ses: add new Enclosure ULD")
Closes: https://sashiko.dev/#/patchset/20260706-b4-disp-29a05ca3-v1-1-49591f469f60@proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Verified with an in-kernel KASAN model (7.2.0-rc1) that replays each
accessor over a real kmalloc'd page 1:
  - ses_show_id() read (page1+12, 8 bytes) over a 12-byte page 1
    -> BUG: KASAN: slab-out-of-bounds READ; a validated page 1 is clean.
  - a type_ptr walk with an inflated page1_num_types over a short page 1
    -> BUG: KASAN: slab-out-of-bounds READ; the geometry check stops it.
  - a conforming page 1 -> clean.
Reproducer available on request.
---
 drivers/scsi/ses.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 4c348645b04e..bbd8ff13cb2a 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -750,6 +750,22 @@ static int ses_intf_add(struct device *cdev)
 	ses_dev->page1_types = type_ptr;
 	ses_dev->page1_num_types = types;
 
+	/*
+	 * Validate the device-reported page 1 geometry once, here, before the
+	 * accessors walk it.  page1_types and page1_num_types come straight from
+	 * the enclosure, and the page 2 descriptor walks (ses_get_page2_descriptor(),
+	 * ses_set_page2_descriptor()), ses_enclosure_data_process() and the
+	 * logical-id read in ses_show_id() all trust them.  A page 1 shorter than
+	 * its logical-id field, or one that declares more type descriptors than it
+	 * carries, would make those reads run past the page 1 buffer.
+	 */
+	if (len < 8 + 4 + (int)sizeof(u64) ||
+	    ses_dev->page1_types > buf + len ||
+	    ses_dev->page1_num_types > (buf + len - ses_dev->page1_types) / 4) {
+		err = -EINVAL;
+		goto err_free;
+	}
+
 	for (i = 0; i < types && type_ptr < buf + len; i++, type_ptr += 4) {
 		if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
 		    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)

---
base-commit: dd3210c47e8d3ac6b4e9141fc68acc03b38c0ba3
change-id: 20260711-b4-disp-26cc3226-291ae14662de

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

[PATCH] scsi: ses: validate the page 1 geometry before walking it

[Bryam Vargas]

ses_intf_add() stores the enclosure's page 1 type-descriptor pointer and
count (page1_types, page1_num_types) without bounding them against the
page 1 buffer.  The page 2 descriptor walks, ses_enclosure_data_process()
and the logical-id read in ses_show_id() all iterate or index with those
device-supplied values.  An enclosure that reports a short page 1 with an
inflated type count, or one too short for its logical id, makes those
reads run past the page 1 allocation -- an out-of-bounds read of adjacent
slab, some of which ses_show_id() hands back to user space.

Validate the page 1 geometry where it is parsed and reject a page too
short for its logical id or declaring more type descriptors than it
carries.  Conforming enclosures are unaffected.

Fixes: 9927c68864e9 ("[SCSI] ses: add new Enclosure ULD")
Closes: https://sashiko.dev/#/patchset/20260706-b4-disp-29a05ca3-v1-1-49591f469f60@proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Verified with an in-kernel KASAN model (7.2.0-rc1) that replays each
accessor over a real kmalloc'd page 1:
  - ses_show_id() read (page1+12, 8 bytes) over a 12-byte page 1
    -> BUG: KASAN: slab-out-of-bounds READ; a validated page 1 is clean.
  - a type_ptr walk with an inflated page1_num_types over a short page 1
    -> BUG: KASAN: slab-out-of-bounds READ; the geometry check stops it.
  - a conforming page 1 -> clean.
Reproducer available on request.
---
 drivers/scsi/ses.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 4c348645b04e..bbd8ff13cb2a 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -750,6 +750,22 @@ static int ses_intf_add(struct device *cdev)
 	ses_dev->page1_types = type_ptr;
 	ses_dev->page1_num_types = types;
 
+	/*
+	 * Validate the device-reported page 1 geometry once, here, before the
+	 * accessors walk it.  page1_types and page1_num_types come straight from
+	 * the enclosure, and the page 2 descriptor walks (ses_get_page2_descriptor(),
+	 * ses_set_page2_descriptor()), ses_enclosure_data_process() and the
+	 * logical-id read in ses_show_id() all trust them.  A page 1 shorter than
+	 * its logical-id field, or one that declares more type descriptors than it
+	 * carries, would make those reads run past the page 1 buffer.
+	 */
+	if (len < 8 + 4 + (int)sizeof(u64) ||
+	    ses_dev->page1_types > buf + len ||
+	    ses_dev->page1_num_types > (buf + len - ses_dev->page1_types) / 4) {
+		err = -EINVAL;
+		goto err_free;
+	}
+
 	for (i = 0; i < types && type_ptr < buf + len; i++, type_ptr += 4) {
 		if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
 		    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)

---
base-commit: dd3210c47e8d3ac6b4e9141fc68acc03b38c0ba3
change-id: 20260711-b4-disp-26cc3226-291ae14662de

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

Re: [PATCH] scsi: ses: validate the page 1 geometry before walking it

[sashiko-bot]

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

New issues:
- [High] An integer truncation when storing the descriptor type count allows bypassing the new page 1 geometry validation, leading to an out-of-bounds read.

Pre-existing issues:
- [Critical] Diagnostic page 2 lacks length validation, leading to an integer underflow and massive out-of-bounds write in `ses_set_page2_descriptor`.
- [Critical] The page 2 descriptor iterations can access memory out of bounds if page 2 is smaller than the required size computed from page 1.
- [High] The newly added page 1 geometry validation is placed after the initial enclosure descriptor parsing loop, leaving the loop vulnerable to out-of-bounds reads.
- [High] The enclosure device is exposed to userspace before its private data (`edev->scratch`) is initialized, creating a race condition that leads to a NULL pointer dereference.
- [High] Diagnostic page 7 is parsed without adequate length validation, allowing out-of-bounds reads during device initialization.
--

--- Patch [1]: [PATCH] scsi: ses: validate the page 1 geometry before walking it ---
commit df60f2b0ae545a4bc27de56a34c9f4c56a73eb55
Author: Bryam Vargas <hexlabsecurity@proton.me>
Subject: scsi: ses: validate the page 1 geometry before walking it

This commit adds validation to the SCSI Enclosure Services (SES) driver to
verify the geometry of diagnostic page 1 before parsing it. It ensures the
page is long enough for its logical ID and bounds the number of type
descriptors to prevent out-of-bounds reads.

Link: https://sashiko.dev/#/patchset/20260706-b4-disp-29a05ca3-v1-1-49591f469f60@proton.me?part=1

> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index 4c348645b04ee..bbd8ff13cb2a5 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -750,6 +750,22 @@ static int ses_intf_add(struct device *cdev)
>  	ses_dev->page1_types = type_ptr;
>  	ses_dev->page1_num_types = types;

[Severity: High]
This is a pre-existing issue, but the loop just above this context:

	/* begin at the enclosure descriptor */
	type_ptr = buf + 8;
	/* skip all the enclosure descriptors */
	for (i = 0; i < num_enclosures && type_ptr < buf + len; i++) {
		types += type_ptr[2];
		type_ptr += type_ptr[3] + 4;
	}

iterates and reads type_ptr[2] and type_ptr[3] before the newly added validation
check. Is it possible for this to cause an out-of-bounds read if the buffer is
smaller than 12 bytes?

> +	/*
> +	 * Validate the device-reported page 1 geometry once, here, before the
> +	 * accessors walk it.  page1_types and page1_num_types come straight from
> +	 * the enclosure, and the page 2 descriptor walks (ses_get_page2_descriptor(),
> +	 * ses_set_page2_descriptor()), ses_enclosure_data_process() and the
> +	 * logical-id read in ses_show_id() all trust them.  A page 1 shorter than
> +	 * its logical-id field, or one that declares more type descriptors than it
> +	 * carries, would make those reads run past the page 1 buffer.
> +	 */
> +	if (len < 8 + 4 + (int)sizeof(u64) ||
> +	    ses_dev->page1_types > buf + len ||
> +	    ses_dev->page1_num_types > (buf + len - ses_dev->page1_types) / 4) {

[Severity: High]
Can assigning the integer types to ses_dev->page1_num_types (which is a short)
cause a truncation? If a malicious device reports up to 256 enclosures and 255
types per enclosure, types could reach 65,280. When assigned to page1_num_types,
it truncates to a negative number (e.g., -256).

This would cause the ses_dev->page1_num_types > ... check to evaluate as false,
bypassing the validation and potentially allowing an out-of-bounds read in the
subsequent loop.

> +		err = -EINVAL;
> +		goto err_free;
> +	}
> +
>  	for (i = 0; i < types && type_ptr < buf + len; i++, type_ptr += 4) {
>  		if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
>  		    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)

[Severity: High]
This is a pre-existing issue, but looking further down in ses_intf_add():

	edev = enclosure_register(cdev->parent, dev_name(&sdev->sdev_gendev),
				  components, &ses_enclosure_callbacks);
	if (IS_ERR(edev)) {
		err = PTR_ERR(edev);
		goto err_free;
	}

	kfree(hdr_buf);

	edev->scratch = ses_dev;

Are we exposing the device to userspace before initializing its private data
(edev->scratch)? It looks like this creates a race window where userspace
could immediately read the sysfs attributes (like id), and the ses_show_id()
callback might dereference a NULL edev->scratch.


[Severity: High]
This isn't a bug introduced by this patch, but looking at ses_enclosure_data_process():

	} else {
		desc_ptr = buf + 8;
		len = (desc_ptr[2] << 8) + desc_ptr[3];
		/* skip past overall descriptor */
		desc_ptr += len + 4;
	}

We seem to parse diagnostic page 7 without adequate length validation here.
If the device provides a short page (e.g., length of 0, making the allocated
buffer 5 bytes), desc_ptr is advanced past the allocated buffer and
unconditionally dereferenced. Could this lead to out-of-bounds reads during
device initialization?


[Severity: Critical]
This is a pre-existing issue, but looking at ses_set_page2_descriptor():

	unsigned char *desc_ptr = ses_dev->page2 + 8;

	/* Clear everything */
	memset(desc_ptr, 0, ses_dev->page2_len - 8);

What happens if page2_len is less than 8? The subtraction would underflow
and cause memset to overwrite a massive amount of kernel memory. Should there
be length validation for diagnostic page 2 before this memset?


[Severity: Critical]
This is a pre-existing issue, but also in ses_set_page2_descriptor():

	for (i = 0; i < ses_dev->page1_num_types; i++, type_ptr += 4) {
		for (j = 0; j < type_ptr[1]; j++) {
			desc_ptr += 4;

It appears desc_ptr is unconditionally advanced based on page1_num_types
without bounds checking against page2_len. If a device reports a large number
of types in page 1 but returns a truncated page 2, could this iterate out of
bounds and cause slab corruption during a subsequent memcpy?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-b4-disp-26cc3226-v1-1-e04c11c88996@proton.me?part=1