Re: [PATCH v2 2/5] ACPI: NFIT: accept SPA structures with an unused location cookie

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

Pre-existing issues:
- [High] If `acpi_nfit_init()` encounters an error during table parsing (e.g., memory allocation failure) or detects unsupported deletions, it returns without restoring the remaining elements left in the stack-allocated `prev` lists. This corrupts the driver's active state by permanently losing tracking of those elements.
- [High] Pre-existing out-of-bounds reads in `add_dcr`, `add_flush`, and `add_idt` due to missing `header.length` validation against computed sizes.
--

--- Patch [2]: [PATCH v2 2/5] ACPI: NFIT: accept SPA structures with an unused location cookie ---
commit 1d01437bf3a7475500c2f72bf23fa46edef22d6c
Author: Pengpeng Hou <[email protected]>

ACPI: NFIT: accept SPA structures with an unused location cookie
    
ACPI 6.4 extended the System Physical Address Range Structure from 56
to 64 bytes by appending an eight-byte location cookie. The cookie-valid
flag describes whether that field contains usable data; it does not
select the structure length.

> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 4428adb6a1ab6..f68edfe649528 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -705,9 +705,20 @@ int nfit_spa_type(struct acpi_nfit_system_address *spa)
>  
>  static size_t sizeof_spa(struct acpi_nfit_system_address *spa)
>  {
> +	size_t legacy_size = offsetof(struct acpi_nfit_system_address,
> +				      location_cookie);
> +	size_t size = spa->header.length;
> +
> +	if (size == sizeof(*spa))
> +		return size;
> +
> +	if (size != legacy_size)
> +		return 0;
> +
>  	if (spa->flags & ACPI_NFIT_LOCATION_COOKIE_VALID)
> -		return sizeof(*spa);
> -	return sizeof(*spa) - 8;
> +		return 0;
> +
> +	return size;
>  }

[Severity: High]
This is a pre-existing issue, but since you are fixing the length logic for
SPA structures here, does similar code in sizeof_idt() and sizeof_flush() need
bounds checking against the header length?

In drivers/acpi/nfit/core.c:sizeof_idt():

static size_t sizeof_idt(struct acpi_nfit_interleave *idt)
{
	if (idt->header.length < sizeof(*idt))
		return 0;
	return sizeof(*idt) + sizeof(u32) * idt->line_count;
}

Can this lead to an out-of-bounds read if a malformed ACPI table provides an
idt->line_count that makes the computed size larger than idt->header.length?

The add_idt() function uses this unvalidated computed size directly for
memcpy() and memcmp() on the ACPI buffer. Similar logic is present in
sizeof_flush() and add_flush() with flush->hint_count.

[ ... ]

> @@ -716,23 +727,26 @@ static bool add_spa(struct acpi_nfit_desc *acpi_desc,
>  {
>  	struct device *dev = acpi_desc->dev;
>  	struct nfit_spa *nfit_spa;
> +	size_t size = sizeof_spa(spa);
>  
> -	if (spa->header.length != sizeof_spa(spa))
> +	if (!size)
>  		return false;

[Severity: High]
This isn't a bug introduced by this patch, but looking at acpi_nfit_init()
which manages the prev list passed into these add_*() functions, is there a
state corruption and memory leak on the error path?

If a hotplug event removes regions and acpi_nfit_check_deletions() fails, it
returns -ENXIO and jumps to out_unlock:

	rc = acpi_nfit_check_deletions(acpi_desc, &prev);
	if (rc)
		goto out_unlock;
    ...
 out_unlock:
	mutex_unlock(&acpi_desc->init_mutex);
	return rc;

Since prev is allocated on the stack in acpi_nfit_init(), does jumping to
out_unlock without splicing the remaining un-matched entries back onto the
acpi_desc lists permanently lose track of those items? 

It appears this would drop them from the driver's tracking lists while leaving
their list node pointers dangling to a destroyed stack frame.

>  	list_for_each_entry(nfit_spa, &prev->spas, list) {
> -		if (memcmp(nfit_spa->spa, spa, sizeof_spa(spa)) == 0) {
> +		if (sizeof_spa(nfit_spa->spa) != size)
> +			continue;
> +
> +		if (memcmp(nfit_spa->spa, spa, size) == 0) {
>  			list_move_tail(&nfit_spa->list, &acpi_desc->spas);
>  			return true;
>  		}
>  	}

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