Re: [PATCH v2 5/5] ACPI: NFIT: bound the platform capability mask

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

Pre-existing issues:
- [High] The `acpi_nfit_init` function fails to restore list elements on error paths, leading to memory leaks and complete state corruption for existing NVDIMM devices during hot-updates.
--

--- Patch [5]: [PATCH v2 5/5] ACPI: NFIT: bound the platform capability mask ---
commit bbbfd46d3c5bf4ec32e49479ec84620dc0f64973
Author: Pengpeng Hou <[email protected]>

ACPI: NFIT: bound the platform capability mask

The platform capability subtable parser reads highest_capability and
capabilities without first requiring the fixed structure to be present.
This commit adds checks to prevent out-of-bounds reads and undefined shifts.

> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 84c70f1941c57..7638d4dc8c6da 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -987,7 +987,14 @@ 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;

[Severity: High]
This is a pre-existing issue, but does adding this new early return expose
a state corruption bug during hot-updates?

In acpi_nfit_init(), existing NFIT resources are moved to a local stack
variable before parsing:

acpi_nfit_init() {
        ...
        list_cut_position(&prev.spas, &acpi_desc->spas,
                          acpi_desc->spas.prev);
        ...

If the newly added capability length check fails, it appears the table
parsing aborts:

        if (IS_ERR(data)) {
                dev_dbg(dev, "nfit table parsing error: %ld\n", PTR_ERR(data));
                rc = PTR_ERR(data);
                goto out_unlock;
        }

Which jumps directly to the exit label:

out_unlock:
        mutex_unlock(&acpi_desc->init_mutex);
        return rc;
}

Since the elements in the local prev lists are never restored to
acpi_desc on this error path, do these devm-managed items become permanently
leaked with dangling list pointers when prev goes out of scope?

> +
> +	if (pcap->highest_capability >= 31)
> +		mask = U32_MAX;
> +	else
> +		mask = (1U << (pcap->highest_capability + 1)) - 1;
> +
>  	acpi_desc->platform_cap = pcap->capabilities & mask;
>  	dev_dbg(dev, "cap: %#x\n", acpi_desc->platform_cap);
>  	return true;

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