Re: [PATCH v3 10/10] ACPI: extlog: Validate elog record length before walking sections

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

Pre-existing issues:
- [High] The newly added bounds check using `cper_estatus_len()` can be bypassed due to an integer truncation, failing to prevent the out-of-bounds read it was intended to fix.
- [High] An integer overflow in `cper_estatus_check_header()` allows malformed records to bypass the bounds check on 32-bit systems.
- [High] The physical address supplied by firmware is not validated to fall within the memory-mapped `elog_addr` region, allowing out-of-bounds memory accesses.
- [High] The `fru_text` string field within the CPER record is missing a guaranteed null-terminator before being passed to trace events, causing `strlen()` to read out of bounds.
--

--- Patch [10]: [PATCH v3 10/10] ACPI: extlog: Validate elog record length before walking sections ---
commit a0127a0e48d0e90f30ee3978c528f569d5e624af
Author: Dave Jiang <[email protected]>
Subject: ACPI: extlog: Validate elog record length before walking sections

This commit attempts to fix an out-of-bounds read by adding bounds checks
on the firmware-controlled data_length before walking the elog sections.
Malformed records exceeding ELOG_ENTRY_LEN are now dropped with NOTIFY_DONE
instead of being processed.

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

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 1fbaf134c04e3..30863fd3f5b07 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -200,6 +200,13 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>  
>  	tmp = (struct acpi_hest_generic_status *)elog_buf;
>  
> +	/*
> +	 * data_length is firmware controlled, so make sure the record and its
> +	 * sections stay within elog_buf before anything walks them.
> +	 */
> +	if (cper_estatus_len(tmp) > ELOG_ENTRY_LEN || cper_estatus_check(tmp))

[Severity: High]
This is a pre-existing issue in the helper function, but does the 32-bit
truncation in cper_estatus_len() bypass this newly added bounds check?

Looking at cper_estatus_len():

drivers/acpi/apei/apei-internal.h:cper_estatus_len() {
    ...
    return sizeof(*estatus) + estatus->data_length;
}

If a malformed firmware record sets data_length to an extremely large value
like 0xFFFFFFFF, the 64-bit sum evaluates to 0x100000013. Because the
function returns a 32-bit u32, this is implicitly truncated to 19.

Since 19 is not greater than ELOG_ENTRY_LEN (4096), the check passes, and
cper_estatus_check() will validate and loop over the un-truncated large
data_length, allowing the parser to read out of bounds.


[Severity: High]
This is a pre-existing issue, but can malformed records bypass the bounds
check in cper_estatus_check_header() on 32-bit systems?

Looking at cper_estatus_check_header(), which is called by the new
cper_estatus_check() validation:

drivers/firmware/efi/cper.c:cper_estatus_check_header() {
    ...
    if (estatus->raw_data_length &&
        estatus->raw_data_offset < sizeof(*estatus) + estatus->data_length)
        return -EINVAL;
}

On 32-bit architectures, if data_length is close to the 32-bit maximum (e.g.,
0xFFFFFFEC), the calculation sizeof(*estatus) + estatus->data_length
overflows to a small value (e.g., 0).

The safety check then evaluates to false and passes, allowing the large
data_length to be used in section parsing and walking past the bounds of the
buffer.

> +		return NOTIFY_DONE;
> +
>  	if (!ras_userspace_consumers()) {
>  		print_extlog_rcd(NULL, tmp, cpu);
>  		goto out;


While reviewing the rest of the file, I noticed a few other pre-existing 
issues:

[Severity: High]
This is a pre-existing issue, but could a corrupted firmware physical address
lead to accessing arbitrary kernel memory?

In drivers/acpi/acpi_extlog.c:extlog_elog_entry_check():

    data &= EXT_ELOG_ENTRY_MASK;
    estatus = (struct acpi_hest_generic_status *)ELOG_ENTRY_ADDR(data);

    /* if no valid data in elog entry, just return */
    if (estatus->block_status == 0)

The physical address data is read directly from the firmware-controlled L1
directory and translated into a virtual address via pointer arithmetic against
the elog_addr mapping.

There appears to be no bounds check to ensure the address falls within the
intended elog_base and elog_size region. If firmware supplies a corrupted
address, the resulting pointer will be out-of-bounds, causing a potential
kernel panic upon dereferencing block_status.


[Severity: High]
This is a pre-existing issue, but is it possible for the tracepoint to read
out of bounds when processing the non-terminated fru_text string?

In drivers/acpi/acpi_extlog.c:extlog_print():

    if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
        fru_text = gdata->fru_text;
    ...
    trace_extlog_mem_event(mem, err_seq, fru_id, fru_text,
                           (u8)gdata->error_severity);

The fru_text string is extracted from a fixed 20-byte array without
enforcing a null terminator.

Since the tracepoint implicitly calls strlen() on fru_text, a firmware
record filled with non-null characters could cause strlen() to read out of
bounds into adjacent kernel heap memory, leaking memory contents to the
tracing infrastructure.

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