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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] cper_estatus_check() fails to verify remaining data_len before calling acpi_hest_get_size(gdata), leading to an out-of-bounds read of gdata->revision.
- [High] Missing bounds check on physical address provided by firmware, leading to arbitrary memory read and write.
--

--- Patch [3]: [PATCH v4 03/13] ACPI: extlog: Validate elog record length before walking sections ---
commit b1da2ee38edf690076a81fda05c21b313452f06b
Author: Dave Jiang <dave.jiang@intel.com>

ACPI: extlog: Validate elog record length before walking sections

extlog_print() copies a fixed 4096 bytes from the elog record into
elog_buf and walks the sections using data_length. This patch rejects
records longer than ELOG_ENTRY_LEN and runs cper_estatus_check() to
validate the records and prevent walking past the buffer.

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..9ad0052aa20c9 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -208,6 +208,10 @@ static int extlog_print(struct notifier_block *nb, unsigned long val,
>  

[Severity: High]
This is a pre-existing issue, but is the physical address provided by firmware
validated before it is accessed here?

Looking at extlog_elog_entry_check() called earlier in this function:

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

The virtual address computed by ELOG_ENTRY_ADDR() doesn't appear to be
checked against the mapped bounds.

Furthermore, in extlog_print(), this unvalidated pointer is read from and
written to:

drivers/acpi/acpi_extlog.c:extlog_print() {
    ...
    memcpy(elog_buf, (void *)estatus, ELOG_ENTRY_LEN);
    /* clear record status to enable BIOS to update it again */
    estatus->block_status = 0;

Could a malformed firmware L1 directory provide an out-of-bounds physical
address, leading to arbitrary memory read and write when extlog_print() copies
the buffer and clears block_status?

>  	tmp = (struct acpi_hest_generic_status *)elog_buf;
>  
> +	/* Keep the firmware-controlled data_length inside elog_buf. */
> +	if (cper_estatus_len(tmp) > ELOG_ENTRY_LEN || cper_estatus_check(tmp))

[Severity: High]
This is a pre-existing issue, but does cper_estatus_check() have an
out-of-bounds read when verifying the sections?

Looking at cper_estatus_check():

drivers/firmware/efi/cper.c:cper_estatus_check() {
    ...
    apei_estatus_for_each_section(estatus, gdata) {
        ...
        if (acpi_hest_get_size(gdata) > data_len)
            return -EINVAL;

And acpi_hest_get_size() calls acpi_hest_get_version(), which unconditionally
reads gdata->revision:

include/acpi/ghes.h:acpi_hest_get_size() {
    ...
    if (acpi_hest_get_version(gdata) >= 3)

If a firmware-provided malformed record contains a section where the
remaining data_len is less than the size of the section header, wouldn't
this dereference memory past the end of the payload?

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

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