Re: [PATCH v6 03/13] ACPI: extlog: Validate elog record length before walking sections

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on firmware-supplied physical address allows out-of-bounds memory read (and potential kernel panic) when mapped to a virtual address.
--

commit 7d4af53bdefb9af34e8feff4c7e1e31654f51d71
Author: Dave Jiang <dave.jiang@intel.com>
Subject: ACPI: extlog: Validate elog record length before walking sections

This patch adds validation to extlog_print() to bound the length of elog
records and check them via cper_estatus_check() before walking the sections.
It drops malformed records to prevent out-of-bounds reads if data_length
extends past elog_buf.

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

> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index 7ad3b36013cc6..046e92ee195f9 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -208,6 +208,15 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>  
>  	tmp = (struct acpi_hest_generic_status *)elog_buf;
>  
> +	/*
> +	 * Bound the length before cper_estatus_check() walks the sections: it
> +	 * iterates over data_length, which is not yet known to fit elog_buf.
> +	 * cper_estatus_check_header() then rejects a length that wrapped, which
> +	 * the bound cannot see.
> +	 */
> +	if (cper_estatus_len(tmp) > ELOG_ENTRY_LEN || cper_estatus_check(tmp))
> +		return NOTIFY_DONE;
> +

[Severity: High]
This is a pre-existing issue, but since the goal of this code is to protect
against malformed firmware records, there appears to be a missing bounds
check earlier in this path.

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

    data = ELOG_ENTRY_DATA(idx);
    if ((data & ELOG_ENTRY_VALID) == 0)
        return NULL;

    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)
        return NULL;

The physical address read from the firmware-controlled L1 directory into
data is converted to a virtual pointer via the ELOG_ENTRY_ADDR() macro. It
is then immediately dereferenced by estatus->block_status.

Is there anything verifying that this physical address falls within the
mapped elog_base to elog_base + elog_size boundaries?

If the resulting virtual address points to an unmapped page in the vmalloc
or ioremap region, could this dereference cause a kernel page fault and
panic?

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

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