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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A 32-bit integer overflow in `cper_estatus_check_header()` allows malicious hardware/firmware to bypass bounds checks, leading to an out-of-bounds read during CPER parsing.
--

--- Patch [2]: [PATCH v7 02/10] ACPI: APEI: GHES: move CPER read helpers ---
commit 04015aac8e37e710180920d3ccbdeddf2e3b4a43
Author: Ahmed Tiba <ahmed.tiba@arm.com>

ACPI: APEI: GHES: move CPER read helpers

Relocate 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.
This commit only shuffles code; behavior stays the same.

> diff --git a/drivers/acpi/apei/ghes_cper.c b/drivers/acpi/apei/ghes_cper.c
> new file mode 100644
> index 0000000000000..0c3f52b2d7f1c
> --- /dev/null
> +++ b/drivers/acpi/apei/ghes_cper.c

[ ... ]

> +int __ghes_read_estatus(struct acpi_hest_generic_status *estatus,
> +			u64 buf_paddr, enum fixed_addresses fixmap_idx,
> +			size_t buf_len)
> +{
> +	u32 len;
> +
> +	ghes_copy_tofrom_phys(estatus, buf_paddr, buf_len, 1, fixmap_idx);
> +
> +	len = cper_estatus_len(estatus);
> +	if (len < sizeof(*estatus)) {
> +		pr_warn_ratelimited(FW_WARN GHES_PFX "Truncated error status block!\n");
> +		return -EIO;
> +	}
> +
> +	if (len > buf_len) {
> +		pr_warn_ratelimited(FW_WARN GHES_PFX "Invalid error status block length!\n");
> +		return -EIO;
> +	}
> +
> +	if (cper_estatus_check(estatus)) {

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds read
when parsing the CPER block?

On 32-bit architectures, a maliciously crafted CPER block with an extremely
large data_length (e.g., 0xFFFFFFFF), a small raw_data_offset (e.g., 20),
and a non-zero raw_data_length (e.g., 1) could bypass the header validation
check.

In drivers/firmware/efi/cper.c:cper_estatus_check_header():

    if (estatus->raw_data_length &&
        estatus->raw_data_offset < sizeof(*estatus) + estatus->data_length)
            return -EINVAL;

The addition of sizeof(*estatus) and estatus->data_length can integer
overflow. For example, 20 + 0xFFFFFFFF = 19. The validation 20 < 19
evaluates to false, allowing it to pass.

Because cper_estatus_len() would just return the offset + raw_data_length
(21 in this case), the len > buf_len check above also passes.

Finally, when cper_estatus_check() is called,
apei_estatus_for_each_section() will iterate using the massive data_length
over unmapped memory, resulting in an out-of-bounds read and likely a
kernel panic.

Could we add safeguards against this overflow in the header validation?

> +		pr_warn_ratelimited(FW_WARN GHES_PFX
> +				    "Failed to read error status block!\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-topics-ahmtib01-ras_ffh_arm_internal_review-v7-0-8b3a85216cef@arm.com?part=2