Re: [PATCH] ACPI: NFIT: validate subtable extents before parsing

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

New issues:
- [High] Rejecting the capability table if `highest_capability > 31` fails the entire NFIT driver probe, breaking forward compatibility.

Pre-existing issues:
- [High] Missing bounds check in `sizeof_dcr` leads to potential out-of-bounds read when parsing the Control Region subtable.
- [High] `sizeof_spa` incorrectly assumes that the SPA length is determined by the `LOCATION_COOKIE_VALID` flag, rejecting valid ACPI 6.4+ System Physical Address tables.
--

--- Patch [1]: [PATCH] ACPI: NFIT: validate subtable extents before parsing ---
commit d7436f5d72f81006ac23c35fc1e7248c7a9fe254
Author: Pengpeng Hou <[email protected]>

ACPI: NFIT: validate subtable extents before parsing

add_table() reads an NFIT subtable header after checking only that the
cursor is before the end of the table. It then advances by the advertised
subtable length without proving that either the header or the full
subtable is present. This patch validates the fixed header and advertised
length before dispatch, and ensures variable arrays fit their subtables.

> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index cb771d9cadb2a..711ab639cb147 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -705,6 +705,10 @@ int nfit_spa_type(struct acpi_nfit_system_address *spa)
>  
>  static size_t sizeof_spa(struct acpi_nfit_system_address *spa)
>  {
> +	if (spa->header.length <
> +	    offsetof(struct acpi_nfit_system_address, reserved))
> +		return 0;
> +
>  	if (spa->flags & ACPI_NFIT_LOCATION_COOKIE_VALID)
>  		return sizeof(*spa);
>  	return sizeof(*spa) - 8;
>  }

[Severity: High]
This is a pre-existing issue, but does this logic risk rejecting valid
ACPI 6.4+ System Physical Address tables?

If a compliant ACPI 6.4 firmware presents a 64-byte SPA table with the
LOCATION_COOKIE_VALID flag unset, sizeof_spa() will assume the length
should be 56.

In add_spa() in drivers/acpi/nfit/core.c, the exact length equality check
will then fail:

add_spa() {
    ...
    if (spa->header.length != sizeof_spa(spa))
        return false;
    ...
}

Will this bubble up as ERR_PTR(-ENOMEM) and abort the entire driver probe
on compliant platforms?

[Severity: High]
This is also a pre-existing issue, but while we're adding bounds checks,
does sizeof_dcr() in drivers/acpi/nfit/core.c have a similar missing check
that could lead to an out-of-bounds read?

When an ACPI table provides a DCR subtable with header.length exactly enough
to include the windows field (e.g., 32) and windows > 0, sizeof_dcr() calculates
the required size as 80 bytes:

sizeof_dcr() {
    ...
    if (dcr->windows)
        return sizeof(*dcr);
    ...
}

add_table() only verifies that 32 bytes remain in the mapped ACPI table. The
memcpy() in add_dcr() then reads 80 bytes:

add_dcr() {
    ...
    memcpy(nfit_dcr->dcr, dcr, sizeof_dcr(dcr));
    ...
}

Could this extend 48 bytes past the end of the subtable, potentially reading
unmapped memory and causing a kernel panic?

[ ... ]

> @@ -951,7 +969,16 @@ static bool add_platform_cap(struct acpi_nfit_desc *acpi_desc,
>  	struct device *dev = acpi_desc->dev;
>  	u32 mask;
>  
> -	mask = (1 << (pcap->highest_capability + 1)) - 1;
> +	if (pcap->header.length < sizeof(*pcap))
> +		return false;
> +	if (pcap->highest_capability > 31)
> +		return false;

[Severity: High]
Does returning false here break forward compatibility with future ACPI
firmware?

If a future firmware revision sets highest_capability > 31 to indicate support
for new features, this check will return false.

Since add_table() handles this by returning ERR_PTR(-ENOMEM):

add_table() {
    ...
    case ACPI_NFIT_TYPE_CAPABILITIES:
        if (!add_platform_cap(acpi_desc, table))
            return err;
    ...
}

Would this cause the entire NFIT driver probe to fail, rather than just
gracefully capping or ignoring the unknown capability bits?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1