Re: [PATCH v4 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:
- [High] Out-of-bounds read in cper_mem_err_pack() triggered by cper_print_mem() for records of length 74-79 bytes.
--

--- Patch [10]: [PATCH v4 10/13] efi/cper: Read only validated fields in cper_mem_err_pack() ---
commit bef2c0319d994ade71f221a3c685230d96b2ab0b
Author: Dave Jiang <dave.jiang@intel.com>

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

This patch updates cper_mem_err_pack() to only copy extended memory error
fields when their respective validation bits are set. This addresses
out-of-bounds reads on older UEFI 2.1/2.2 73-byte layouts where these
fields don't exist. By zeroing unvalidated fields, it also prevents reading
uninitialized stack data.

Closes: 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 ea1c999089bc1..6c64a0f06a4e6 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: High]
This is a pre-existing issue, but if firmware provides a CPER record with a
length between 74 and 79 bytes and sets validation bits for fields beyond its
length, can this still cause an out-of-bounds read?

In drivers/firmware/efi/cper.c:cper_print_mem(), the validation bits are only
sanitized if the length is exactly 73 bytes:

	if (len == sizeof(struct cper_sec_mem_err_old) &&
	    (mem->validation_bits & ~(CPER_MEM_VALID_RANK_NUMBER - 1))) {
		...

If the length is 74 to 79 bytes, this sanitization is bypassed.

Then, here in cper_mem_err_pack(), we trust the validation bits without length
context. If the CPER record is at the end of the mapped estatus block and
aligned at a page boundary, could reading mem_dev_handle cross into an
unmapped page and result in a kernel panic?

In drivers/acpi/apei/ghes.c:ghes_proc(), ghes_print_estatus() is called before
length validation is performed in ghes_do_proc():

	if (ghes_print_estatus(NULL, ghes->generic, estatus))
		ghes_estatus_cache_add(ghes->generic, estatus);
	}
	ghes_do_proc(ghes, estatus);

Does cper_mem_err_pack() need to consider the length of the record, or should
cper_print_mem() be updated to handle lengths between 74 and 79 bytes?

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