Re: [PATCH v5 10/13] efi/cper: Read only validated fields in cper_mem_err_pack()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] cper_mem_err_pack() remains vulnerable to out-of-bounds reads because cper_print_mem() bypasses length validation for records slightly larger than the base structure.
--

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

efi/cper: Read only validated fields in cper_mem_err_pack()

cper_mem_err_pack() copies extended, rank, mem_array_handle and
mem_dev_handle unconditionally. Those live at offsets 73 to 79, past the
end of struct cper_sec_mem_err_old, the 73-byte UEFI 2.1/2.2 layout that
older firmware still emits and that cper_estatus_print_section() admits.
On such a record the copy reads up to seven bytes past the payload, and off
the end of the error status block when that section is the last one.

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

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 3dcef618ad39f..13b3e72c3b62f 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -389,10 +389,28 @@ void cper_mem_err_pack(const struct cper_sec_mem_err *mem,
>  	cmem->requestor_id = mem->requestor_id;
>  	cmem->responder_id = mem->responder_id;
>  	cmem->target_id = mem->target_id;
> -	cmem->extended = mem->extended;
> -	cmem->rank = mem->rank;
> -	cmem->mem_array_handle = mem->mem_array_handle;
> -	cmem->mem_dev_handle = mem->mem_dev_handle;
> +
> +	/*
> +	 * These four sit past the end of the UEFI 2.1/2.2 layout, which older
> +	 * firmware still emits, so reading them unconditionally runs off a
> +	 * short record. Every consumer of the compact record gates them on the
> +	 * same validation bits, so leave them zero when firmware does not
> +	 * claim them.
> +	 */
> +	cmem->extended = 0;
> +	cmem->rank = 0;
> +	cmem->mem_array_handle = 0;
> +	cmem->mem_dev_handle = 0;
> +
> +	if (mem->validation_bits &
> +	    (CPER_MEM_VALID_ROW_EXT | CPER_MEM_VALID_CHIP_ID))
> +		cmem->extended = mem->extended;
> +	if (mem->validation_bits & CPER_MEM_VALID_RANK_NUMBER)
> +		cmem->rank = mem->rank;
> +	if (mem->validation_bits & CPER_MEM_VALID_CARD_HANDLE)
> +		cmem->mem_array_handle = mem->mem_array_handle;
> +	if (mem->validation_bits & CPER_MEM_VALID_MODULE_HANDLE)
> +		cmem->mem_dev_handle = mem->mem_dev_handle;

[Severity: Medium]
This is a pre-existing issue, but does this conditional read still allow an
out-of-bounds read if the record length is between 74 and 79 bytes?

In cper_print_mem(), the length validation only checks if the length is
exactly sizeof(struct cper_sec_mem_err_old) (which is 73 bytes):

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;
	}
	...
	cper_mem_err_pack(mem, &cmem);
	...
}

If the length is slightly larger, such as 74 bytes, this check is completely
bypassed. If firmware provides a record of length 74 with extended validation
bits set (e.g., CPER_MEM_VALID_MODULE_HANDLE), cper_mem_err_pack() will trust
these validation bits and read out of bounds up to 6 bytes, leaking adjacent
memory content.

While a follow-up patch ("ACPI: APEI: GHES: Validate memory error section
length before payload access") fixed this vulnerability for consumers reached
via ghes_do_proc(), cper_print_mem() remains unchanged.

Could cper_print_mem() be updated to ensure the length covers the fields
indicated by the validation bits?

>  }
>  EXPORT_SYMBOL_GPL(cper_mem_err_pack);

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