Re: [PATCH v2 4/5] ACPI: NFIT: validate control region extent

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

Pre-existing issues:
- [High] State corruption and list_head dangling pointers on NFIT table parsing error.
- [High] Out-of-bounds read and undefined behavior via bit shift in add_platform_cap().
--

--- Patch [4]: [PATCH v2 4/5] ACPI: NFIT: validate control region extent ---
commit 0bc177f6743d37dbd5255c0f7a268009d181d358
Author: Pengpeng Hou <[email protected]>

ACPI: NFIT: validate control region extent

This patch modifies sizeof_dcr() to ensure that the advertised subtable size
is large enough to contain all remaining fields, preventing a potential
out-of-bounds read during the memory copy in add_dcr(). It computes the
required size based on the windows count and validates it against the
provided subtable length.

> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index bf4ddc56494e3..84c70f1941c57 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -817,12 +817,20 @@ EXPORT_SYMBOL_GPL(nfit_get_smbios_id);
>   */
>  static size_t sizeof_dcr(struct acpi_nfit_control_region *dcr)
>  {
> +	size_t size;
> +
>  	if (dcr->header.length < offsetof(struct acpi_nfit_control_region,
>  				window_size))
>  		return 0;
>  	if (dcr->windows)
> -		return sizeof(*dcr);
> -	return offsetof(struct acpi_nfit_control_region, window_size);
> +		size = sizeof(*dcr);
> +	else
> +		size = offsetof(struct acpi_nfit_control_region, window_size);
> +
> +	if (size > dcr->header.length)
> +		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but returning 0 here can trigger
a state corruption and memory leak in the caller's error path.

If sizeof_dcr() returns 0, add_dcr() returns false, which causes add_table()
to fail and jump to the error path in acpi_nfit_init():

drivers/acpi/nfit/core.c:acpi_nfit_init() {
    ...
    while (!IS_ERR_OR_NULL(data))
        data = add_table(acpi_desc, &prev, data, end);

    if (IS_ERR(data)) {
        dev_dbg(dev, "nfit table parsing error: %ld\n", PTR_ERR(data));
        rc = PTR_ERR(data);
        goto out_unlock;
    }
    ...
 out_unlock:
    mutex_unlock(&acpi_desc->init_mutex);
    return rc;
}

When jumping to out_unlock, existing list elements that were temporarily
moved to the stack variable prev are not spliced back to acpi_desc. They
become orphaned, and their list_head pointers are left dangling to invalid
stack memory. Does this need to be cleaned up or spliced back on error?

> +
> +	return size;
>  }
>  
>  static bool add_dcr(struct acpi_nfit_desc *acpi_desc,

[Severity: High]
This is a pre-existing issue, but is there an out-of-bounds read and undefined
behavior when parsing the Capabilities table?

In add_table(), a table is only validated to be at least 4 bytes long
(sizeof(struct acpi_nfit_header)). However, add_platform_cap() accesses fields
beyond the header:

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

If a malformed Capabilities subtable is provided with a length of 4 bytes,
could this result in an out-of-bounds read of pcap->highest_capability and
pcap->capabilities?

Additionally, if pcap->highest_capability is 30 or greater, does the bit shift
result in signed integer overflow or shift-out-of-bounds undefined behavior?

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