Re: [PATCH v4 11/13] ACPI: APEI: GHES: Validate memory error section length before payload access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unfixed out-of-bounds read and valid record rejection in `cper_print_mem()` for CPER_SEC_PLATFORM_MEM records.
--

--- Patch [11]: [PATCH v4 11/13] ACPI: APEI: GHES: Validate memory error section length before payload access ---
commit 550e022dfc62676e66fb5e0818106e122ddfcbf2
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: APEI: GHES: Validate memory error section length before payload access

This commit adds memory error section length validation to ghes_do_proc() 
to prevent out-of-bounds reads when handling CPER_SEC_PLATFORM_MEM payloads. 
It checks the validation bits to determine the minimum required length.

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

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c

[ ... ]

> @@ -940,6 +962,25 @@ static void ghes_do_proc(struct ghes *ghes,
>  		if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) {
>  			struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>  
> +			/*
> +			 * Check once for all three consumers below. The 73-byte
> +			 * UEFI 2.1/2.2 layout is the floor, matching
> +			 * cper_estatus_print_section() and making
> +			 * validation_bits safe to read.
> +			 */
> +			if (gdata->error_data_length <
> +			    sizeof(struct cper_sec_mem_err_old))
> +				continue;
> +
> +			/* Then require what the claimed fields actually need. */
> +			if (gdata->error_data_length <
> +			    ghes_mem_err_min_len(mem_err->validation_bits)) {
> +				pr_warn_ratelimited(FW_WARN GHES_PFX
> +						    "memory error section too small (%u) for the fields it claims\n",
> +						    gdata->error_data_length);
> +				continue;
> +			}

[Severity: High]
This is a pre-existing issue, but does this validation happen too late to
prevent out-of-bounds reads when printing the error?

Looking at ghes_proc(), it prints the error status before calling
ghes_do_proc():

drivers/acpi/apei/ghes.c:ghes_proc() {
    ...
    if (!ghes_estatus_cached(estatus)) {
        if (ghes_print_estatus(NULL, ghes->generic, estatus))
            ghes_estatus_cache_add(ghes->generic, estatus);
    }
    ghes_do_proc(ghes, estatus);
    ...
}

The call chain goes through ghes_print_estatus() ->
cper_estatus_print_section() -> cper_print_mem(), which has a strict
73-byte length check that allows lengths 74-79 to bypass validation:

drivers/firmware/efi/cper.c:cper_print_mem() {
    ...
    /* Don't trust UEFI 2.1/2.2 structure with bad validation bits */
    if (len == sizeof(struct cper_sec_mem_err_old) &&
        (mem->validation_bits & ~(CPER_MEM_VALID_RANK_NUMBER - 1))) {
        pr_err(FW_WARN "valid bits set for fields beyond structure\n");
        return;
    }
    ...
}

If a malformed payload has a length between 74 and 79 bytes and claims
extended fields, does this lead to an out-of-bounds read during
cper_mem_err_pack() before the new checks in ghes_do_proc() can reject it?

drivers/firmware/efi/cper.c:cper_mem_err_pack() {
    ...
    if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE)
        cmem->mem_dev_handle = mem->mem_dev_handle;
    ...
}

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