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

[PATCH v2] 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); the page 2 walks,
ses_enclosure_data_process() and the logical-id read in ses_show_id() all
trust them.  Three device-supplied values go unbounded: the enclosure-
descriptor skip loop reads a descriptor's length bytes while only checking
its first byte is in range; the stored count is a device sum unrelated to
what page 1 carries; and it is truncated into a short before being range-
checked.  A short or inflated page 1 thus drives an out-of-bounds read of
adjacent slab, some of which ses_show_id() returns to user space.

Bound the skip loop to a whole descriptor and validate the geometry once
where it is parsed: reject a page too short for its logical id, a type
array that ran past the buffer, or a count larger than the array holds --
checking it before it is stored also keeps it inside the short.  Conforming
enclosures are unaffected.

Fixes: 9927c68864e9 ("[SCSI] ses: add new Enclosure ULD")
Closes: https://sashiko.dev/#/patchset/20260711-b4-disp-26cc3226-v1-1-e04c11c88996@proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Changes since v1 [1], addressing the Sashiko review of that posting:
 - Bound the enclosure-descriptor skip loop itself (type_ptr + 4 <= buf +
   len).  v1 validated only after that loop ran, so a page 1 a few bytes
   short of a descriptor was still read out of bounds inside the loop
   before the check.
 - Validate the int type count before it is stored, not the short it
   truncates to.  A device count of 65280 wraps a signed short to -256,
   which slipped past the v1 range check; the int is bounded to
   (buf+len-type_ptr)/4, below the short's range, so it cannot wrap once
   stored.

Verified with an in-kernel KASAN model (7.2.0-rc1, kasan.fault=report) that
replays the skip loop over a real kzalloc'd page 1:
 - old guard, 10-byte page 1 -> BUG: KASAN: slab-out-of-bounds read, 1 byte,
   0 bytes to the right of the region; the tightened guard stops before it.
 - count 65280 -> the pre-store int check rejects it, the post-store short
   check (v1) did not.
 - a conforming page 1 walks unchanged.
Reproducer available on request.

[1] https://lore.kernel.org/all/20260711-b4-disp-26cc3226-v1-1-e04c11c88996@proton.me
---
 drivers/scsi/ses.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 4c348645b04e..907bb489252a 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -742,11 +742,28 @@ static int ses_intf_add(struct device *cdev)
 	/* 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++) {
+	for (i = 0; i < num_enclosures && type_ptr + 4 <= buf + len; i++) {
 		types += type_ptr[2];
 		type_ptr += type_ptr[3] + 4;
 	}
 
+	/*
+	 * Validate the device-reported page 1 geometry before the accessors
+	 * walk it.  page1_types and page1_num_types come straight from the
+	 * enclosure; 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.  Reject a page 1
+	 * shorter than its logical id, one whose descriptors ran past the end,
+	 * or one declaring more type descriptors than it carries.  Bounding the
+	 * count before it is stored also keeps it within its short.
+	 */
+	if (len < 8 + 4 + (int)sizeof(u64) ||
+	    type_ptr > buf + len ||
+	    types > (buf + len - type_ptr) / 4) {
+		err = -EINVAL;
+		goto err_free;
+	}
+
 	ses_dev->page1_types = type_ptr;
 	ses_dev->page1_num_types = types;
 

---
base-commit: dd3210c47e8d3ac6b4e9141fc68acc03b38c0ba3
change-id: 20260711-b4-disp-414d08d4-70013715f352

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

[PATCH v2] 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); the page 2 walks,
ses_enclosure_data_process() and the logical-id read in ses_show_id() all
trust them.  Three device-supplied values go unbounded: the enclosure-
descriptor skip loop reads a descriptor's length bytes while only checking
its first byte is in range; the stored count is a device sum unrelated to
what page 1 carries; and it is truncated into a short before being range-
checked.  A short or inflated page 1 thus drives an out-of-bounds read of
adjacent slab, some of which ses_show_id() returns to user space.

Bound the skip loop to a whole descriptor and validate the geometry once
where it is parsed: reject a page too short for its logical id, a type
array that ran past the buffer, or a count larger than the array holds --
checking it before it is stored also keeps it inside the short.  Conforming
enclosures are unaffected.

Fixes: 9927c68864e9 ("[SCSI] ses: add new Enclosure ULD")
Closes: https://sashiko.dev/#/patchset/20260711-b4-disp-26cc3226-v1-1-e04c11c88996@proton.me?part=1
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Changes since v1 [1], addressing the Sashiko review of that posting:
 - Bound the enclosure-descriptor skip loop itself (type_ptr + 4 <= buf +
   len).  v1 validated only after that loop ran, so a page 1 a few bytes
   short of a descriptor was still read out of bounds inside the loop
   before the check.
 - Validate the int type count before it is stored, not the short it
   truncates to.  A device count of 65280 wraps a signed short to -256,
   which slipped past the v1 range check; the int is bounded to
   (buf+len-type_ptr)/4, below the short's range, so it cannot wrap once
   stored.

Verified with an in-kernel KASAN model (7.2.0-rc1, kasan.fault=report) that
replays the skip loop over a real kzalloc'd page 1:
 - old guard, 10-byte page 1 -> BUG: KASAN: slab-out-of-bounds read, 1 byte,
   0 bytes to the right of the region; the tightened guard stops before it.
 - count 65280 -> the pre-store int check rejects it, the post-store short
   check (v1) did not.
 - a conforming page 1 walks unchanged.
Reproducer available on request.

[1] https://lore.kernel.org/all/20260711-b4-disp-26cc3226-v1-1-e04c11c88996@proton.me
---
 drivers/scsi/ses.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 4c348645b04e..907bb489252a 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -742,11 +742,28 @@ static int ses_intf_add(struct device *cdev)
 	/* 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++) {
+	for (i = 0; i < num_enclosures && type_ptr + 4 <= buf + len; i++) {
 		types += type_ptr[2];
 		type_ptr += type_ptr[3] + 4;
 	}
 
+	/*
+	 * Validate the device-reported page 1 geometry before the accessors
+	 * walk it.  page1_types and page1_num_types come straight from the
+	 * enclosure; 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.  Reject a page 1
+	 * shorter than its logical id, one whose descriptors ran past the end,
+	 * or one declaring more type descriptors than it carries.  Bounding the
+	 * count before it is stored also keeps it within its short.
+	 */
+	if (len < 8 + 4 + (int)sizeof(u64) ||
+	    type_ptr > buf + len ||
+	    types > (buf + len - type_ptr) / 4) {
+		err = -EINVAL;
+		goto err_free;
+	}
+
 	ses_dev->page1_types = type_ptr;
 	ses_dev->page1_num_types = types;
 

---
base-commit: dd3210c47e8d3ac6b4e9141fc68acc03b38c0ba3
change-id: 20260711-b4-disp-414d08d4-70013715f352

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Diagnostic Page 2 Length Vulnerabilities: Missing bounds checks and signed integer overflows lead to massive out-of-bounds reads and writes.
- [High] Missing minimum length check for the page 7 buffer leads to an out-of-bounds read when skipping the overall descriptor.
- [High] The enclosure device is exposed to sysfs and other subsystems before its driver-private data is initialized, leading to a NULL pointer dereference.
- [High] The enclosure private data is freed before the device is unregistered, causing a Use-After-Free or NULL pointer dereference for concurrent users.
- [Medium] Concurrent disk probes trigger simultaneous, lockless modifications to a shared diagnostic buffer, causing data corruption.
--

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

scsi: ses: validate the page 1 geometry before walking it

This patch bounds the skip loop to a whole descriptor and validates the page 1
geometry once where it is parsed. It rejects pages that are too short for
their logical id, type arrays that run past the buffer, or counts larger
than the array holds. This prevents out-of-bounds reads during the enclosure
descriptor skip loop.

> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index 4c348645b04ee..907bb489252ad 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -742,11 +742,28 @@ static int ses_intf_add(struct device *cdev)
>  	/* 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++) {
> +	for (i = 0; i < num_enclosures && type_ptr + 4 <= buf + len; i++) {
>  		types += type_ptr[2];
>  		type_ptr += type_ptr[3] + 4;
>  	}
>  
> +	/*
> +	 * Validate the device-reported page 1 geometry before the accessors
> +	 * walk it.  page1_types and page1_num_types come straight from the
> +	 * enclosure; 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.  Reject a page 1
> +	 * shorter than its logical id, one whose descriptors ran past the end,
> +	 * or one declaring more type descriptors than it carries.  Bounding the
> +	 * count before it is stored also keeps it within its short.
> +	 */
> +	if (len < 8 + 4 + (int)sizeof(u64) ||
> +	    type_ptr > buf + len ||
> +	    types > (buf + len - type_ptr) / 4) {
> +		err = -EINVAL;
> +		goto err_free;
> +	}
> +
>  	ses_dev->page1_types = type_ptr;
>  	ses_dev->page1_num_types = types;
>  

[Severity: High]
This is a pre-existing issue, but does this code leave the page 2 length
unvalidated?

In drivers/scsi/ses.c:ses_intf_add(), the length is initialized directly
from the device's diagnostic page:

    len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
    // ...
    ses_dev->page2_len = len;

Since struct ses_device declares short page2_len, a length greater than
32767 overflows into a negative value, and a length less than 8 causes
underflows.

Can this result in a massive out-of-bounds write via
memset(desc_ptr, 0, ses_dev->page2_len - 8) in ses_set_page2_descriptor(),
and subsequent out-of-bounds accesses when advancing pointers in
ses_get_page2_descriptor()?

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing minimum
length check for the page 7 buffer?

Looking at drivers/scsi/ses.c:ses_enclosure_data_process(), if the device
returns a page 7 length less than 8 (e.g., indicating 0 bytes of payload),
kzalloc() allocates a 5-byte buffer:

    page7_len = len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
    buf = kzalloc(len + 1, GFP_KERNEL);
    // ...
    } else {
        desc_ptr = buf + 8;
        len = (desc_ptr[2] << 8) + desc_ptr[3];

Does the unconditional desc_ptr = buf + 8 lead to a slab out-of-bounds read
when checking desc_ptr[2]?

[Severity: High]
This is a pre-existing issue, but is the enclosure device exposed before its
private data is initialized?

In drivers/scsi/ses.c:ses_intf_add(), the device is registered before the
scratch payload is attached:

    edev = enclosure_register(...);
    // ...
    edev->scratch = ses_dev;

If a user process reads the sysfs id file or an asynchronous scsi probe
matches the enclosure before edev->scratch is set, could this trigger a null
pointer dereference?

For example, in drivers/scsi/ses.c:ses_show_id():

    struct ses_device *ses_dev = edev->scratch;
    unsigned long long id = get_unaligned_be64(ses_dev->page1+8+4);

Will this unconditionally dereference a null edev->scratch?

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence
cause a use-after-free or null pointer dereference?

In drivers/scsi/ses.c:ses_intf_remove_enclosure(), the private data is freed
before the device is unregistered:

    ses_dev = edev->scratch;
    edev->scratch = NULL;

    kfree(ses_dev->page10);
    kfree(ses_dev->page1);
    kfree(ses_dev->page2);
    kfree(ses_dev);
    // ...
    enclosure_unregister(edev);

Can concurrent sysfs reads or asynchronous scsi probes operating on the
device during this window dereference the cleared edev->scratch, resulting
in a crash?

[Severity: Medium]
This is a pre-existing issue, but do concurrent disk probes trigger
data races on the shared diagnostic buffer?

In drivers/scsi/ses.c:ses_enclosure_data_process():

    /* re-read page 10 */
    if (ses_dev->page10)
        ses_recv_diag(sdev, 10, ses_dev->page10, ses_dev->page10_len);

If multiple scsi devices probe concurrently, does this unconditionally
issue ses_recv_diag() without locking, causing simultaneous hardware dma
writes and parser reads on the exact same buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-b4-disp-414d08d4-v2-1-784a01e8e2dd@proton.me?part=1