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

[sashiko-bot@kernel.org]

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