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

[sashiko-bot@kernel.org]

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