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

On Wed, Jul 22, 2026 at 04:31:02AM +0000, [email protected] wrote:
> 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.

Agreed w this one. See my direct response.

> 
> Pre-existing issues:
> - [High] Missing bounds check in `sizeof_dcr` leads to potential out-of-bounds read when parsing the Control Region subtable.

I think this is true but not something that belongs in this patch.


> - [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.

This one is not obviously true. I think the question is should sizeof_spa()
take structure size from header.length rather than LOCATION_COOKIE_VALID?
I wonder if ACPI 6.4 permits a 64-byte SPA with LOCATION_COOKIE_VALID clear,
in which case the current logic would reject an otherwise valid table.
I didn't look into this, but I think you need to figure that out in
case this patch needs to change.

-- Alison


> --
> 
> --- 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
>