Re: [PATCH v4 01/13] efi/cper: Reject CPER records with an out-of-range error_data_length

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) double-fetch vulnerability in GHES record parsing allows bypassing length validation.
- [Medium] Out-of-bounds read in CPER record validation due to partial trailing sections.
--

--- Patch [1]: [PATCH v4 01/13] efi/cper: Reject CPER records with an out-of-range error_data_length ---
commit b61df2c17060fd5f4040a122aa6b8bc641dbf1f3
Author: Dave Jiang <dave.jiang@intel.com>

efi/cper: Reject CPER records with an out-of-range error_data_length

This patch prevents acpi_hest_get_record_size() from wrapping small when
adding a large error_data_length. By performing the arithmetic in u64, it
ensures the size check against data_len catches oversized records correctly.

Link: https://sashiko.dev/#/patchset/20260714231835.303081-1-dave.jiang@intel.com?part=1

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 06b4fdb59917a..ec092729caccb 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -752,7 +752,7 @@ EXPORT_SYMBOL_GPL(cper_estatus_check_header);
>  int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>  {

[Severity: High]
This is a pre-existing issue, but does cper_estatus_check() lack sufficient
context to prevent an out-of-bounds read if the record is modified
concurrently?

When drivers/acpi/apei/ghes.c:ghes_in_nmi_queue_one_entry() allocates the
buffer, it sizes it exactly to a peeked length. Then it fetches the record a
second time from physical memory via __ghes_read_estatus():

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

If the data_length is maliciously increased in physical memory between these
fetches, the newly copied header will contain an unvalidated length. Because
cper_estatus_check() doesn't verify the length against the originally peeked
allocation size, the loop below can iterate far beyond the bounds of the
allocated buffer.

Would it be possible to pass max_len into cper_estatus_check() to re-verify
it against the newly fetched data_length?

>  	struct acpi_hest_generic_data *gdata;
> -	unsigned int data_len, record_size;
> +	unsigned int data_len;
>  	int rc;

[ ... ]

> @@ -762,11 +762,21 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>  	data_len = estatus->data_length;
>  
>  	apei_estatus_for_each_section(estatus, gdata) {
> +		u64 record_size;
> +
>  		if (acpi_hest_get_size(gdata) > data_len)
>  			return -EINVAL;

[Severity: Medium]
This is a pre-existing issue, but does this loop safely handle trailing
partial sections?

If a malformed CPER record leaves fewer than the required bytes for the
header, calling acpi_hest_get_size(gdata) here will read gdata->revision
beyond the end of the estatus payload before checking against data_len.

Also, in include/acpi/ghes.h, apei_estatus_for_each_section() advances using
acpi_hest_get_next(section). Since the loop update calls
acpi_hest_get_record_size() which calls acpi_hest_get_size(), it could
similarly read out of bounds before the loop terminates if the remaining
data_len is too small.

Could data_len be checked against the minimum section header size before
attempting to access gdata fields?

> -		record_size = acpi_hest_get_record_size(gdata);
> -		if (record_size > data_len)
> +		/*
> +		 * acpi_hest_get_record_size() sums these as a signed int (see

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824174936.939059-1-dave.jiang@intel.com?part=1