Re: [PATCH v6 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] Memory Leak / Resource Exhaustion in ghes_estatus_cache_add()
- [High] Time-of-Check to Time-of-Use (TOCTOU) / Double Fetch in ghes_read_estatus()
- [High] Missing bounds checks on gdata->error_data_length causing Out-of-Bounds Reads
--

commit a78b7d6a2bbf24ad4d5f5d8876b30cdec8135faf
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 c6e20fdc6964e..5218fc57e5622 100644
> --- a/drivers/acpi/apei/ghes_cper.c
> +++ b/drivers/acpi/apei/ghes_cper.c
[ ... ]
> +void ghes_estatus_cache_add(struct acpi_hest_generic *generic,
> +			    struct acpi_hest_generic_status *estatus)
> +{
> +	unsigned long long now, duration, period, max_period = 0;
> +	struct ghes_estatus_cache *cache, *new_cache;
> +	struct ghes_estatus_cache __rcu *victim;
> +	int i, slot = -1, count;
> +
> +	new_cache = ghes_estatus_cache_alloc(generic, estatus);
> +	if (!new_cache)
> +		return;
> +
> +	rcu_read_lock();
> +	now = sched_clock();
> +	for (i = 0; i < GHES_ESTATUS_CACHES_SIZE; i++) {
> +		cache = rcu_dereference(ghes_estatus_caches[i]);
> +		if (cache == NULL) {
> +			slot = i;
> +			break;
> +		}
> +		duration = now - cache->time_in;
> +		if (duration >= GHES_ESTATUS_IN_CACHE_MAX_NSEC) {
> +			slot = i;
> +			break;
> +		}
> +		count = atomic_read(&cache->count);
> +		period = duration;
> +		do_div(period, (count + 1));
> +		if (period > max_period) {
> +			max_period = period;
> +			slot = i;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	if (slot != -1) {

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential memory
leak here.

If all slots are occupied and period <= max_period for all entries (e.g., if
multiple errors occur in the same timer tick causing duration < count + 1,
making period = 0), slot remains -1.

In this case, the function skips the insertion block and returns without
freeing new_cache or decrementing the ghes_estatus_cache_alloced counter.
Since GHES_ESTATUS_CACHE_ALLOCED_MAX is only 6, could 6 occurrences of this
condition permanently exhaust the pool and disable the deduplication mechanism?

> +		/*
> +		 * Use release semantics to ensure that ghes_estatus_cached()
[ ... ]


I also wanted to ask about a couple of other pre-existing issues.

[Severity: High]
This is a pre-existing issue, but is there a double-fetch vulnerability in
ghes_read_estatus()?

drivers/acpi/apei/ghes_cper.c:ghes_read_estatus() {
    ...
	rc = __ghes_peek_estatus(ghes, estatus, buf_paddr, fixmap_idx);
	if (rc)
		return rc;

	rc = __ghes_check_estatus(ghes, estatus);
	if (rc)
		return rc;

	return __ghes_read_estatus(estatus, *buf_paddr, fixmap_idx,
				   cper_estatus_len(estatus));
}

The code peeks the error status header from physical memory and validates its
length via __ghes_check_estatus(). However, __ghes_read_estatus() then
performs a second read from the same physical memory to copy the full block.

If a hardware source modifies the data_length field between these two reads,
could the newly copied header overwrite the validated length with an
unvalidated size, leading to out-of-bounds heap reads during subsequent
parsing?


[Severity: High]
This is a pre-existing issue, but are there missing bounds checks on
gdata->error_data_length in ghes_handle_aer() and ghes_cper_handle_status()?

drivers/acpi/apei/ghes_cper.c:ghes_handle_aer() {
    ...
	struct cper_sec_pcie *pcie_err = acpi_hest_get_payload(gdata);

	if (pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
    ...
}

And similarly in ghes_cper_handle_status():

drivers/acpi/apei/ghes_cper.c:ghes_cper_handle_status() {
    ...
		} else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);

			cxl_cper_post_event(CXL_CPER_EVENT_GEN_MEDIA, rec);
    ...
}

The payload is cast and dereferenced without verifying that
gdata->error_data_length is large enough to contain the expected structure
size. If a valid section UUID is provided with a short error_data_length
near the end of the estatus buffer, could this read past the end of the
allocated kernel buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-topics-ahmtib01-ras_ffh_arm_internal_review-v6-0-91f725174aa0@arm.com?part=4