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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A double fetch (TOCTOU) vulnerability when reading the CPER error status block from firmware memory can lead to out-of-bounds reads.
- [High] ghes_ack_error() incorrectly shifts read_ack_preserve and read_ack_write by the register bit_offset, unconditionally clearing bits below bit_offset and potentially clobbering other hardware state in the register.
--

commit 6a836df1ccfeca08e11c00049d33e954958fa36f
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 so they can be shared with other
firmware-first providers. It only shuffles code without changing
behavior.

Reviewed-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Ahmed Tiba <ahmed.tiba@arm.com>

> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
> new file mode 100644
> index 0000000000000..b365c42efce49
> --- /dev/null
> +++ b/drivers/acpi/apei/ghes_cper.c
> @@ -0,0 +1,196 @@
[ ... ]
> +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;
> +	val |= gv2->read_ack_write    << gv2->read_ack_register.bit_offset;

[Severity: High]
This is a pre-existing issue, but does this code unintentionally clobber the
lower bits of the hardware register?

When ghes_ack_error() masks val using:

val &= gv2->read_ack_preserve << gv2->read_ack_register.bit_offset;

Any bits in val below bit_offset are unconditionally cleared to zero.

If there is hardware state in those lower bits, wouldn't writing back the
modified val via apei_write() overwrite and destroy it?

> +
> +	apei_write(val, &gv2->read_ack_register);
> +}
[ ... ]
> +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));

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential double
fetch issue in the error status read path?

Looking at the sequence in ghes_read_estatus(), we first fetch and validate the
header:

__ghes_peek_estatus() copies the header from physical memory into estatus.

__ghes_check_estatus() validates estatus->data_length.

Then we call __ghes_read_estatus() which copies from the same physical memory
again, overwriting the previously validated estatus header:

__ghes_read_estatus(...) {
    ghes_copy_tofrom_phys(estatus, buf_paddr, buf_len, 1, fixmap_idx);
    if (cper_estatus_check(estatus)) {
    ...
}

If the underlying firmware memory is modified between the peek and the read,
the local estatus will get a new, unvalidated data_length.

Since cper_estatus_check() relies on data_length to iterate over the sections,
could this new unvalidated length cause an out-of-bounds read?

> +}

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