Re: [PATCH v4 02/10] ACPI: APEI: GHES: move CPER read helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in CPER error status block reads allows Out-of-Bounds memory access.
- [High] Integer overflow in `cper_estatus_len()` allows parsing of uninitialized kernel heap memory.
- [High] Bitwise logic error in `ghes_ack_error()` unconditionally clears bits outside the shifted `read_ack_preserve` mask, causing hardware register corruption.
--

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

ACPI: APEI: GHES: move CPER read helpers

This commit relocates the CPER buffer mapping, peek, and clear helpers
from ghes.c into ghes_cper.c. This allows sharing the CPER handling
flow with other firmware-first error sources. The commit states that
behavior remains the same.

> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
> new file mode 100644
> index 0000000000000..7bb72fe578384
> --- /dev/null
> +++ b/drivers/acpi/apei/ghes_cper.c
> @@ -0,0 +1,195 @@

[ ... ]

> +static void ghes_ack_error(struct acpi_hest_generic_v2 *gv2)
> +{
> +	int rc;
> +	u64 val = 0;
> +
> +	rc = apei_read(&val, &gv2->read_ack_register);
> +	if (rc)
> +		return;
> +
> +	val &= gv2->read_ack_preserve << gv2->read_ack_register.bit_offset;

This wasn't introduced by this patch, but does this bitwise logic
unconditionally clear the lower bits of the register?

Shifting the preserve mask left by bit_offset introduces zeros into the
lowest bits. The bitwise AND will then clear bits 0 through bit_offset - 1
of val to zero, regardless of the read_ack_preserve value.

Can this destroy unrelated hardware configurations residing in the lower
bits when val is written back?

> +	val |= gv2->read_ack_write    << gv2->read_ack_register.bit_offset;
> +
> +	apei_write(val, &gv2->read_ack_register);
> +}

[ ... ]

> +/* Check the top-level record header has an appropriate size. */
> +int __ghes_check_estatus(struct ghes *ghes,
> +			 struct acpi_hest_generic_status *estatus)
> +{
> +	u32 len = cper_estatus_len(estatus);

This wasn't introduced by this patch, but does cper_estatus_len() have an
integer overflow risk?

cper_estatus_len() calculates the total block length by adding
estatus->raw_data_offset and estatus->raw_data_length. Since both are
32-bit integers, could they wrap around to a small length?

If it wraps to a small length (e.g., 24 bytes), it might pass the
cper_estatus_check_header() checks but cause __ghes_read_estatus() to
copy only a few bytes, leaving the rest uninitialized.

Could the subsequent cper_estatus_check() loop then use a malicious
data_length to parse uninitialized kernel heap memory?

> +	u32 max_len = min(ghes->generic->error_block_length,
> +			  ghes->estatus_length);

[ ... ]

> +int ghes_read_estatus(struct ghes *ghes,
> +		      struct acpi_hest_generic_status *estatus,
> +		      u64 *buf_paddr, enum fixed_addresses fixmap_idx)
> +{
> +	int rc;
> +
> +	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));

This wasn't introduced by this patch, but is there a double-fetch issue
between peek and read?

__ghes_peek_estatus() fetches the CPER header from physical memory into
the estatus buffer, and __ghes_check_estatus() validates this header
against max_len.

However, __ghes_read_estatus() performs a second copy from the same
physical memory, overwriting the previously validated header.

If the underlying memory is modified via DMA between the first and second
fetches, could the new header bypass the max_len validation and lead to
out-of-bounds reads in cper_estatus_check() past the allocated buffer?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-topics-ahmtib01-ras_ffh_arm_internal_review-v4-0-42698675ba61@arm.com?part=2