Re: [PATCH v5 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] The patch identifies but fails to fix an out-of-bounds read vulnerability in `cper_print_mem()` and `cper_mem_err_pack()` when handling malformed memory error sections.
--

commit af698921ff589a7f79e6f852052dbc3da7191b73
Author: Dave Jiang <dave.jiang@intel.com>

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

ghes_do_proc() hands the CPER_SEC_PLATFORM_MEM payload to the report chain,
arch_apei_report_mem_error() and ghes_handle_memory_failure() without
checking gdata->error_data_length. All three read validation_bits and
physical_addr, at offsets 0 and 16, so a shorter section reads past the
record.

Check the length once in ghes_do_proc(), before any consumer runs. Take the
73-byte struct cper_sec_mem_err_old as the floor: older firmware
legitimately emits that UEFI 2.1/2.2 layout, and it makes validation_bits
safe to read.

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

> The fields from "extended" on are absent from that layout, so also require
> whatever length the validation bits claim. Derive it per field rather than
> demanding the full 80 bytes: rank needs only 76, and the BANK_GROUP and
> BANK_ADDRESS bits describe "bank" at offset 38, which every record carries.
> Testing the whole mask against one size, as cper_print_mem() does, gets it
> wrong both ways.

[Severity: High]
This is a pre-existing issue, but since the commit message notes the flaw in
cper_print_mem(), should this patch also fix the out-of-bounds read there?

A CPER memory error record containing a CPER_SEC_PLATFORM_MEM section with a
length between 74 and 79 bytes and CPER_MEM_VALID_MODULE_HANDLE set can trigger
an out-of-bounds read of up to 6 bytes.

When cper_estatus_print_section() in drivers/firmware/efi/cper.c processes such
a record:

drivers/firmware/efi/cper.c:cper_estatus_print_section() {
...
		if (gdata->error_data_length >=
		    sizeof(struct cper_sec_mem_err_old))
			cper_print_mem(newpfx, mem_err,
				       gdata->error_data_length);
...
}

It calls cper_print_mem(), which bypasses its invalid bits check because
len != 73:

drivers/firmware/efi/cper.c:cper_print_mem() {
...
	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;
	}
...
	cper_mem_err_pack(mem, &cmem);
...
}

It then unconditionally calls cper_mem_err_pack(), which reads
mem->mem_dev_handle at offset 78 regardless of the record length:

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;
...
}

Can this lead to a kernel oops if the struct is located exactly at the end of a
mapped page, or leak kernel heap memory into dmesg via DMI handle lookups?

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