[PATCH] scsi: ses: bound the page 2 descriptor walk to the page 2 buffer

[PATCH] scsi: ses: bound the page 2 descriptor walk to the page 2 buffer

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

ses_get_page2_descriptor() and ses_set_page2_descriptor() walk the
enclosure status descriptors by advancing desc_ptr four bytes per page 1
element without bounding it against the page 2 allocation.  The
descriptor count comes from page 1 while page 2 is a separately sized
diagnostic page, both supplied by the enclosure device; a device that
reports more descriptors than page 2 can hold walks desc_ptr past the
buffer.  The get path then dereferences it, a four-byte out-of-bounds
heap read reachable through the component sysfs attributes, and the set
path writes four bytes past the buffer.

Stop each walk once the next descriptor would extend past the end of
page 2: the get path returns NULL, which its callers already handle, and
the set path stops before the copy.  Well-formed enclosures are
unaffected.  The unbounded walk was flagged by the Sashiko AI review
(https://sashiko.dev) of the page 2 header length fix.

Fixes: 9927c68864e9 ("[SCSI] ses: add new Enclosure ULD")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Reproduced with an in-kernel KASAN module that models the page 2 walk: a
page 1 declaring one component with an 8-byte page 2 makes the first
descriptor land at page2+12, four bytes past the kmalloc-8 object.  KASAN
reports slab-out-of-bounds Write (ses_set_page2_descriptor) and Read
(ses_get_page2_descriptor); the read returns adjacent heap rather than the
zeroed in-bounds bytes.  With the patch both walks stop before that access,
and a page 2 large enough for the page 1 descriptors is unchanged.

The pages are only device-supplied, so this needs a malicious or
malfunctioning SES enclosure; the write path additionally needs a
privileged write to the component sysfs attributes.

There is a separate, already-posted fix on this file for the page 2
status-page-header underflow ("scsi: ses: skip an enclosure status page
shorter than its header").  This one is independent -- the hunks do not
overlap and it applies on v7.2-rc1 either way.
---
 drivers/scsi/ses.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 4c348645b04e..7e9c33809131 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -184,12 +184,21 @@ static int ses_set_page2_descriptor(struct enclosure_device *edev,
 	struct ses_device *ses_dev = edev->scratch;
 	unsigned char *type_ptr = ses_dev->page1_types;
 	unsigned char *desc_ptr = ses_dev->page2 + 8;
+	unsigned char *page2_end = ses_dev->page2 + ses_dev->page2_len;
 
 	/* Clear everything */
 	memset(desc_ptr, 0, ses_dev->page2_len - 8);
 	for (i = 0; i < ses_dev->page1_num_types; i++, type_ptr += 4) {
 		for (j = 0; j < type_ptr[1]; j++) {
 			desc_ptr += 4;
+			/*
+			 * The descriptor count comes from page 1 while page 2
+			 * is a separately sized diagnostic page; a device that
+			 * reports more descriptors than page 2 can hold would
+			 * walk desc_ptr past the buffer, so stop here.
+			 */
+			if (desc_ptr + 4 > page2_end)
+				goto out;
 			if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
 			    type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 				continue;
@@ -203,6 +212,7 @@ static int ses_set_page2_descriptor(struct enclosure_device *edev,
 		}
 	}
 
+out:
 	return ses_send_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
 }
 
@@ -214,6 +224,7 @@ static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev,
 	struct ses_device *ses_dev = edev->scratch;
 	unsigned char *type_ptr = ses_dev->page1_types;
 	unsigned char *desc_ptr = ses_dev->page2 + 8;
+	unsigned char *page2_end = ses_dev->page2 + ses_dev->page2_len;
 
 	if (ses_recv_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len))
 		return NULL;
@@ -221,6 +232,8 @@ static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev,
 	for (i = 0; i < ses_dev->page1_num_types; i++, type_ptr += 4) {
 		for (j = 0; j < type_ptr[1]; j++) {
 			desc_ptr += 4;
+			if (desc_ptr + 4 > page2_end)
+				return NULL;
 			if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
 			    type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 				continue;

---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260706-b4-disp-29a05ca3-c1fe7cd9693a

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

Re: [PATCH] scsi: ses: bound the page 2 descriptor walk to the page 2 buffer

[sashiko-bot]

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

New issues:
- [Medium] Silent failure when the new bounds check triggers during a device state update.

Pre-existing issues:
- [Critical] Massive out-of-bounds heap write in `ses_set_page2_descriptor` due to `memset` size underflow.
- [High] Unbounded `type_ptr` walk in descriptor parsing loops allows out-of-bounds heap reads of the `page1` buffer.
- [High] Buffer overread during page 1 enclosure descriptor parsing in `ses_intf_add`.
- [High] Page 2 descriptor traversal logic fails to account for overall control elements, misaligning descriptor selection.
- [High] Use-After-Free via concurrent sysfs callbacks due to premature freeing of enclosure scratch memory.
- [High] Concurrent sysfs accesses to enclosure attributes cause data races on the shared `ses_dev->page2` buffer.
- [High] Buffer overread during page 7 overall descriptor parsing in `ses_enclosure_data_process`.
- [High] Uninitialized heap memory leak via the `ses_show_id` sysfs attribute.
--

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

scsi: ses: bound the page 2 descriptor walk to the page 2 buffer

This commit introduces bounds checking in the page 2 descriptor walk for both
getting and setting enclosure status. It stops the walk if the next
descriptor would exceed the allocated page 2 diagnostic buffer, preventing
out-of-bounds memory accesses.

Link: https://sashiko.dev

> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> index 4c348645b04ee..7e9c33809131f 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -184,12 +184,21 @@ static int ses_set_page2_descriptor(struct enclosure_device *edev,
>  	struct ses_device *ses_dev = edev->scratch;
>  	unsigned char *type_ptr = ses_dev->page1_types;
>  	unsigned char *desc_ptr = ses_dev->page2 + 8;
> +	unsigned char *page2_end = ses_dev->page2 + ses_dev->page2_len;
>  
>  	/* Clear everything */
>  	memset(desc_ptr, 0, ses_dev->page2_len - 8);

[Severity: Critical]
This is a pre-existing issue, but does this calculation underflow the size
passed to memset?

If a USB enclosure device reports a page 2 length less than 8,
ses_dev->page2_len - 8 will underflow the size_t argument to memset. Could
this cause a massive out-of-bounds write over the kernel heap when userspace
writes to a sysfs attribute like fault?

[Severity: High]
This is a pre-existing issue, but does this function modify and send the
shared ses_dev->page2 buffer without any locks?

If multiple threads read or write enclosure sysfs attributes concurrently,
could they issue data transfers into, and memset operations upon, the shared
ses_dev->page2 buffer without any serialization locks?

>  	for (i = 0; i < ses_dev->page1_num_types; i++, type_ptr += 4) {

[Severity: High]
This is a pre-existing issue, but can type_ptr advance past the bounds of
the page1 buffer?

In ses_intf_add(), the page1_num_types count is calculated without capping
it against the buffer length:

drivers/scsi/ses.c:ses_intf_add() {
    ...
    ses_dev->page1_num_types = types;
    ...
}

If a device reports a large number of types, could this loop advance
type_ptr past the bounds of page1 and dereference it, leading to an
out-of-bounds heap read?

>  		for (j = 0; j < type_ptr[1]; j++) {
>  			desc_ptr += 4;

[Severity: High]
This is a pre-existing issue, but does this iteration logic fail to skip the
overall control element present for subsequent types?

According to the enclosure specification, page 2 contains a 4-byte overall
control element for each element type, followed by the individual control
elements.

Does incrementing desc_ptr by 4 for each individual element skip the overall
element for the first type, but fail to skip it for subsequent types, causing
it to treat the overall element as an individual element?

> +			/*
> +			 * The descriptor count comes from page 1 while page 2
> +			 * is a separately sized diagnostic page; a device that
> +			 * reports more descriptors than page 2 can hold would
> +			 * walk desc_ptr past the buffer, so stop here.
> +			 */
> +			if (desc_ptr + 4 > page2_end)
> +				goto out;

[Severity: Medium]
When the new bounds check triggers here and jumps to out, will this silently
return success to userspace?

The function returns the result of ses_send_diag(), which sends the zeroed
buffer. Could this send a diagnostic page with all select bits set to 0,
acting as a hardware no-op, and return success (0) despite ignoring the state
change? Should this return an error code instead?

[ ... ]

> @@ -214,6 +224,7 @@ static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev,
>  	struct ses_device *ses_dev = edev->scratch;
>  	unsigned char *type_ptr = ses_dev->page1_types;
>  	unsigned char *desc_ptr = ses_dev->page2 + 8;
> +	unsigned char *page2_end = ses_dev->page2 + ses_dev->page2_len;
>  
>  	if (ses_recv_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len))
>  		return NULL;

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
device removal?

In ses_intf_remove_enclosure(), the driver frees the ses_dev and its pages
before unregistering the enclosure:

drivers/scsi/ses.c:ses_intf_remove_enclosure() {
    ...
    kfree(ses_dev->page10);
    kfree(ses_dev->page1);
    kfree(ses_dev->page2);
    kfree(ses_dev);

    if (edev->components)
        kfree(edev->component[0].scratch);

    put_device(&edev->edev);
    enclosure_unregister(edev);
}

Since enclosure_unregister() is what disables the sysfs interfaces, could a
concurrent access to a sysfs attribute like fault during device removal
invoke ses_get_page2_descriptor() and dereference the already freed
edev->scratch?

[Severity: High]
This is a pre-existing issue, but does this loop overread the buffer during
page 1 enclosure descriptor parsing?

If a device provides a page 1 diagnostic page where the length allows
entering the loop with fewer than 4 bytes remaining:

drivers/scsi/ses.c:ses_intf_add() {
    ...
    for (i = 0; i < num_enclosures && type_ptr < buf + len; i++) {
        types += type_ptr[2];
    ...
}

Does the loop unconditionally access type_ptr[2] and type_ptr[3], reading out
of bounds?

[Severity: High]
This is a pre-existing issue, but is there a buffer overread during page 7
overall descriptor parsing?

If the device provides a page 7 length smaller than 8 bytes, buf is allocated
with fewer than 8 bytes:

drivers/scsi/ses.c:ses_enclosure_data_process() {
    ...
    desc_ptr = buf + 8;
    len = (desc_ptr[2] << 8) + desc_ptr[3];
    ...
}

Does this read desc_ptr[2] out of bounds before any bounds validation takes
place?

[Severity: High]
This is a pre-existing issue, but does ses_show_id() leak uninitialized heap
memory?

The ses_show_id function reads an 8-byte identifier from ses_dev->page1 at
offset 12:

drivers/scsi/ses.c:ses_show_id() {
    ...
    unsigned long long id = get_unaligned_be64(ses_dev->page1+8+4);
    ...
}

If the device-provided page 1 is not verified to be at least 20 bytes long,
could a shorter page result in reading out-of-bounds adjacent kernel heap
memory and exposing it via the sysfs output?

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