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

[sashiko-bot@kernel.org]

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