Re: [PATCH v5 04/10] ACPI: APEI: GHES: move estatus cache helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The macro GHES_ESTATUS_CACHE_LEN() can overflow when calculating the cache allocation size if the CPER record length is maliciously large.
- [High] ghes_read_estatus() reads the CPER error status header from hardware memory twice, creating a Time-of-Check to Time-of-Use (TOCTOU) vulnerability.
- [High] cper_estatus_len() integer overflow bypasses length validation checks, leading to out-of-bounds reads.
--

commit f4f08f05fe70448a89bd543c421db6376b61be52
Author: Ahmed Tiba <ahmed.tiba@arm.com>

ACPI: APEI: GHES: move estatus cache helpers

Relocate the estatus cache allocation and lookup helpers from ghes.c into
ghes_cper.c. This code move keeps the logic intact while making the cache
implementation available to forthcoming users.

> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
> index 8080e0f76dac4..0a117f478afb7 100644
> --- a/drivers/acpi/apei/ghes_cper.c
> +++ b/drivers/acpi/apei/ghes_cper.c

[ ... ]

> +static struct ghes_estatus_cache *ghes_estatus_cache_alloc(
> +	struct acpi_hest_generic *generic,
> +	struct acpi_hest_generic_status *estatus)
> +{
> +	int alloced;
> +	u32 len, cache_len;
> +	struct ghes_estatus_cache *cache;
> +	struct acpi_hest_generic_status *cache_estatus;
> +
> +	alloced = atomic_add_return(1, &ghes_estatus_cache_alloced);
> +	if (alloced > GHES_ESTATUS_CACHE_ALLOCED_MAX) {
> +		atomic_dec(&ghes_estatus_cache_alloced);
> +		return NULL;
> +	}
> +	len = cper_estatus_len(estatus);

[Severity: High]
This is a pre-existing issue, but does cper_estatus_len() have an integer
overflow that bypasses length validation checks?

The cper_estatus_len() function calculates the record length by adding two
32-bit fields (such as raw_data_offset + raw_data_length or
sizeof(*estatus) + data_length).

Because the return type is u32, can an attacker controlling hardware memory
set large values that sum to a small wrapped result?

This small wrapped length could easily bypass the bounds check in
__ghes_check_estatus(). However, downstream functions like
cper_estatus_check() and apei_estatus_for_each_section() iterate up
to estatus->data_length without verifying it against the actual allocated
buffer size, which might cause the kernel to read far out-of-bounds of the
ghes->estatus heap buffer.

> +	cache_len = GHES_ESTATUS_CACHE_LEN(len);

[Severity: High]
This is a pre-existing issue, but could GHES_ESTATUS_CACHE_LEN() overflow
when calculating the cache allocation size if the CPER record length is
maliciously large?

GHES_ESTATUS_CACHE_LEN(len) adds sizeof(struct ghes_estatus_cache) to the
32-bit len. If an attacker bypasses the initial length validation, perhaps
via the double-fetch in ghes_read_estatus(), they could supply a
maliciously large length like 0xFFFFFFF0.

The sum would wrap around a 32-bit boundary, resulting in a tiny cache_len.
gen_pool_alloc() might successfully allocate a small buffer, but the
subsequent memcpy(cache_estatus, estatus, len) would copy a massive amount
of data based on the original un-truncated len.

Could this lead to a heap buffer overflow?

> +	cache = (void *)gen_pool_alloc(ghes_estatus_pool, cache_len);
> +	if (cache == NULL) {
> +		atomic_dec(&ghes_estatus_cache_alloced);
> +		return NULL;
> +	}
> +	cache_estatus = GHES_ESTATUS_FROM_CACHE(cache);
> +	memcpy(cache_estatus, estatus, len);
> +	cache->estatus_len = len;
> +	atomic_set(&cache->count, 0);
> +	cache->generic = generic;
> +	cache->time_in = sched_clock();
> +	return cache;
> +}

[ ... ]

> diff --git a/include/acpi/ghes_cper.h b/include/acpi/ghes_cper.h
> index 6b7632cfaf667..1b5dbeca9bb69 100644
> --- a/include/acpi/ghes_cper.h
> +++ b/include/acpi/ghes_cper.h

[ ... ]

> @@ -98,5 +101,8 @@ int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
>  			u64 buf_paddr, enum fixed_addresses fixmap_idx,
>  			size_t buf_len);
>  #endif

[Severity: High]
This isn't a bug introduced by this patch, but does ghes_read_estatus() read
the CPER error status header from hardware memory twice, potentially creating
a Time-of-Check to Time-of-Use (TOCTOU) situation?

It appears ghes_read_estatus() first copies the header via
__ghes_peek_estatus() to validate the record length using
__ghes_check_estatus(). It then calls __ghes_read_estatus() to copy the
entire record, which overwrites the previously validated header with whatever
is currently in hardware memory.

Could a malicious PCIe device or firmware race the kernel, altering length
fields between the two fetches?

This might completely bypass the bounds check, allowing unvalidated
out-of-bounds lengths to reach downstream parsing functions, which could
lead to out-of-bounds reads and memory corruption.

> +int ghes_estatus_cached(struct acpi_hest_generic_status *estatus);
> +void ghes_estatus_cache_add(struct acpi_hest_generic *generic,
> +			    struct acpi_hest_generic_status *estatus);
>  
>  #endif /* ACPI_APEI_GHES_CPER_H */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-topics-ahmtib01-ras_ffh_arm_internal_review-v5-0-2e0500d42642@arm.com?part=4