Re: [PATCH v6 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing minimum length validation for error section payloads in `cper_print_mem()` leading to out-of-bounds memory reads if firmware provides a heavily truncated record.
- [High] acpi_extlog silently drops valid UEFI 2.1/2.2 73-byte records.
--

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

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

This patch modifies cper_mem_err_pack() to conditionally copy extended, rank,
mem_array_handle, and mem_dev_handle fields based on their validation bits,
preventing out-of-bounds reads when processing 73-byte UEFI 2.1/2.2 records.

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 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: High]
This isn't a bug introduced by this patch, but does this code still perform
out-of-bounds reads if the firmware provides a heavily truncated record?

In cper_print_mem(), if the error data length is between 74 and 79 bytes,
the exact-match length check is bypassed:

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

Then, cper_print_mem() calls cper_mem_err_pack() which evaluates the
validation bits without verifying if the length is large enough to cover the
claimed fields. If firmware provides a short record (e.g., 74 bytes) and sets
CPER_MEM_VALID_MODULE_HANDLE, wouldn't we still read out-of-bounds from the
estatus buffer here?

Note that a later patch in this series (commit "ACPI: APEI: GHES: Validate
memory error section length before payload access") fixes this issue in
ghes_do_proc(), but the caller path through cper_estatus_print_section() to
cper_print_mem() appears to miss this length validation.

>  }
>  EXPORT_SYMBOL_GPL(cper_mem_err_pack);

[Severity: High]
This is a pre-existing issue, but since this patch series addresses legacy
73-byte records, does the size check in extlog_print() silently drop valid
UEFI 2.1/2.2 records?

drivers/acpi/acpi_extlog.c:extlog_print() {
    ...
    if (guid_equal(sec_type, &CPER_SEC_PLATFORM_MEM)) {
        struct cper_sec_mem_err *mem = acpi_hest_get_payload(gdata);

        if (gdata->error_data_length >= sizeof(*mem))
            trace_extlog_mem_event(mem, err_seq, fru_id, fru_text,
                                   (u8)gdata->error_severity);
    ...
}

Since sizeof(*mem) evaluates to 80 bytes (the size of the newer struct),
wouldn't this check silently reject 73-byte records and prevent them from
being traced?

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