Re: [PATCH v2 1/5] ACPI: NFIT: validate subtable extents before dispatch

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] sizeof_spa() accesses spa->flags before validating the subtable length, causing an out-of-bounds read.
- [High] add_platform_cap() reads capability fields without verifying the subtable length, causing an out-of-bounds read.
- [High] Early termination in acpi_nfit_init() leaves internal device lists stranded on the stack, corrupting the device state and leaking resources.
- [High] sizeof_idt() calculates a size that can exceed the verified subtable length, causing an out-of-bounds read in add_idt().
- [High] sizeof_dcr() can return a size exceeding the verified subtable length when dcr->windows is set, causing an out-of-bounds read.
- [Medium] Bitwise shift operation in add_platform_cap() can cause undefined behavior if highest_capability >= 31.
--

--- Patch [1]: [PATCH v2 1/5] ACPI: NFIT: validate subtable extents before dispatch ---
commit 7124d3c49e2df0b214c3037c798664d5e4c0f3d3
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

ACPI: NFIT: validate subtable extents before dispatch

This commit strengthens NFIT table parsing by requiring a complete header and 
validating that the advertised subtable length fits within the remaining bytes
in the NFIT before dispatching it.

> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -963,14 +963,18 @@ static void *add_table(struct acpi_nfit_desc *acpi_desc,
>  	struct device *dev = acpi_desc->dev;
>  	struct acpi_nfit_header *hdr;
>  	void *err = ERR_PTR(-ENOMEM);
> +	size_t table_len;
>  
>  	if (table >= end)
>  		return NULL;
> +	table_len = end - table;
> +	if (table_len < sizeof(*hdr))
> +		return NULL;
>  

[Severity: High]
This isn't a bug introduced by this patch, but returning NULL here can cause
device state corruption during runtime updates.

If add_table() returns NULL early while parsing an ACPI update in
acpi_nfit_init(), the function jumps to out_unlock:

drivers/acpi/nfit/core.c:acpi_nfit_init() {
    ...
    goto out_unlock;
    ...
}

At that point, it exits without restoring nodes from the stack list prev back
to acpi_desc, which permanently empties the device's operational lists.

Does acpi_nfit_init() need to be updated to handle this early return safely?

>  	hdr = table;
> -	if (!hdr->length) {
> -		dev_warn(dev, "found a zero length table '%d' parsing nfit\n",
> -			hdr->type);
> +	if (hdr->length < sizeof(*hdr) || hdr->length > table_len) {
> +		dev_warn(dev, "invalid table length %u for type %u parsing nfit\n",
> +			 hdr->length, hdr->type);
>  		return NULL;
>  	}

[Severity: High]
This is a pre-existing issue, but while we are strengthening generic table
length checks here, does sizeof_spa() also need bounds checking?

If a malformed table provides a System Physical Address (SPA) subtable with a
length of 4 or 5, add_table() will accept it because hdr->length >= 4. However,
sizeof_spa() in drivers/acpi/nfit/core.c reads spa->flags before verifying the
subtable length is at least 8 bytes:

drivers/acpi/nfit/core.c:sizeof_spa() {
    if (spa->flags & ACPI_NFIT_LOCATION_COOKIE_VALID)
        ...
}

Can this lead to an out-of-bounds read if the table length is too small?

[Severity: High]
This is a pre-existing issue, but does add_platform_cap() similarly need
length validation?

A capabilities subtable might pass the generic length check here, but
add_platform_cap() in drivers/acpi/nfit/core.c reads capabilities without
verifying the subtable length is at least 16 bytes:

drivers/acpi/nfit/core.c:add_platform_cap() {
    mask = (1 << (pcap->highest_capability + 1)) - 1;
    acpi_desc->platform_cap = pcap->capabilities & mask;
}

Could this result in an out-of-bounds read?

[Severity: Medium]
This is also a pre-existing issue, but in the same code snippet above, if
highest_capability is 31 or greater, would the bitwise shift:

drivers/acpi/nfit/core.c:add_platform_cap() {
    mask = (1 << (pcap->highest_capability + 1)) - 1;
}

invoke undefined behavior by shifting a 32-bit signed integer by 32 or more?

[Severity: High]
This is a pre-existing issue, but does sizeof_idt() also need length bounds
checking to prevent out-of-bounds reads?

If a malformed table has a small hdr->length but a large line_count,
sizeof_idt() in drivers/acpi/nfit/core.c calculates a required size ignoring
hdr->length:

drivers/acpi/nfit/core.c:sizeof_idt() {
    return sizeof(*idt) + sizeof(u32) * idt->line_count;
}

Then add_idt() passes this unvalidated size to memcpy:

drivers/acpi/nfit/core.c:add_idt() {
    memcpy(nfit_idt->idt, idt, sizeof_idt(idt));
}

Could this copy past the mapped ACPI table boundary?

[Severity: High]
This is a pre-existing issue, but could sizeof_dcr() return a size exceeding
the verified subtable length?

If dcr->windows is set, sizeof_dcr() in drivers/acpi/nfit/core.c blindly
returns 80 bytes:

drivers/acpi/nfit/core.c:sizeof_dcr() {
    if (dcr->windows)
        return sizeof(*dcr);
}

Then add_dcr() copies the memory:

drivers/acpi/nfit/core.c:add_dcr() {
    memcpy(nfit_dcr->dcr, dcr, sizeof_dcr(dcr));
}

If the firmware provided less than 80 bytes for the control region table, does
this cause an out-of-bounds read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813150948.17239-1-pengpeng@iscas.ac.cn?part=1