Re: [PATCH v3 02/10] efi/cper: Reject CPER records with an out-of-range error_data_length

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

Pre-existing issues:
- [High] The `extlog_print` path iterates over CPER sections without validating the firmware data bounds or lengths, leaving it vulnerable to out-of-bounds reads and integer overflows.
- [High] A malformed CPER record with trailing bytes less than the size of a section header causes an out-of-bounds read in `cper_estatus_check()`.
--

--- Patch [2]: [PATCH v3 02/10] efi/cper: Reject CPER records with an out-of-range error_data_length ---
commit dc1e1608d652687fa8e464b2ae6a04fa5c4f8848
Author: Dave Jiang <[email protected]>

efi/cper: Reject CPER records with an out-of-range error_data_length

This commit fixes an integer overflow vulnerability in the shared CPER length
check. It ensures that error_data_length is not negative and does not exceed
the remaining data length before size arithmetic.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 06b4fdb59917a..99a86b2675e39 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -765,6 +765,16 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
>  		if (acpi_hest_get_size(gdata) > data_len)
>  			return -EINVAL;
>  

[Severity: High]
This is a pre-existing issue, but could the acpi_hest_get_size() call perform
an out-of-bounds read if data_len is very small?

If a malformed record leaves a small number of trailing bytes (for example,
data_len drops to 1), the iteration macro continues because the section
offset is still less than the total data_length.

The acpi_hest_get_size() helper calls acpi_hest_get_version(), which
dereferences gdata->revision at offset 20. Would this read memory past the
end of the validated CPER data region before we check if data_len is large
enough to contain the generic header itself?

[Severity: High]
This isn't a bug introduced by this patch, but is the extlog_print() path
bypassing these safety checks entirely?

Looking at extlog_print(), it iterates over CPER sections without validating
the firmware data bounds:

drivers/acpi/acpi_extlog.c:extlog_print() {
    ...
    tmp = (struct acpi_hest_generic_status *)elog_buf;
    ...
    apei_estatus_for_each_section(tmp, gdata) {
    ...
}

It copies up to ELOG_ENTRY_LEN (4096) bytes of estatus from firmware into
elog_buf, but doesn't appear to call cper_estatus_check() to validate it.

If firmware provides a data_length > 4096, will this loop read past the
bounds of elog_buf? Furthermore, does this leave the unvalidated path
vulnerable to the same error_data_length integer overflow issue being fixed
here?

> +		/*
> +		 * error_data_length reaches record_size below as a signed int
> +		 * (see <acpi/ghes.h>), so a value with the sign bit set can
> +		 * wrap record_size small and slip past the bound check. Reject
> +		 * it before the arithmetic.
> +		 */
> +		if (acpi_hest_get_error_length(gdata) < 0 ||
> +		    acpi_hest_get_error_length(gdata) > data_len)
> +			return -EINVAL;
> +
>  		record_size = acpi_hest_get_record_size(gdata);
>  		if (record_size > data_len)
>  			return -EINVAL;

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