Re: [PATCH v2 7/7] ACPI: APEI: GHES: Validate memory error section length before payload access

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

New issues:
- [High] The newly added length check is overly strict and incorrectly rejects legitimate memory error payloads generated by older UEFI firmware, silently dropping valid hardware errors.

Pre-existing issues:
- [High] The payload length check added in `ghes_handle_memory_failure()` is an incomplete fix and placed too late, leaving other consumers of the same payload vulnerable to out-of-bounds reads.
--

--- Patch [7]: [PATCH v2 7/7] ACPI: APEI: GHES: Validate memory error section length before payload access ---
commit e9b5dfb37c8ab1d8db8c3e63d015c33fc4b4b12f
Author: Dave Jiang <[email protected]>

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

This commit attempts to prevent out-of-bounds accesses when handling memory
errors by validating that the CPER section payload length is large enough
to contain a struct cper_sec_mem_err before dereferencing it.

> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index b36f6d268c9fd..e8b2980bd5fea 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c

[ ... ]

> @@ -535,6 +535,9 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>  	int sec_sev = ghes_severity(gdata->error_severity);
>  	struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>  
> +	if (gdata->error_data_length < sizeof(*mem_err))
> +		return false;
> +

[Severity: High]
Does this length check accidentally reject legitimate memory error payloads
from older UEFI firmware?

If the firmware uses the older v2.1/v2.2 layout (which is 73 bytes long),
comparing gdata->error_data_length against sizeof(*mem_err) (which evaluates
to 80 bytes) will cause this condition to return false.

Could this regression lead to silently dropping valid hardware errors and
preventing the kernel from gracefully offlining broken memory pages?

[Severity: High]
This is a pre-existing issue, but does validating the payload length here in
ghes_handle_memory_failure() leave other consumers vulnerable to
out-of-bounds reads?

Looking at the call chain, the payload is distributed to other consumers
before this function is ever reached:

drivers/acpi/apei/ghes.c:ghes_do_proc() {
        ...
        struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
        atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err);
        arch_apei_report_mem_error(sev, mem_err);
        queued = ghes_handle_memory_failure(gdata, sev, sync);
        ...
}

If the payload is truncated, won't callbacks like
ghes_edac_report_mem_error() and apei_mce_report_mem_error() access fields
out of bounds when they check mem_err->validation_bits, potentially
interpreting adjacent garbage memory as a valid physical address?

>  	if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
>  		return false;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7