[PATCH 3/3] efi_loader: add ACPI table dump EFI app

[Simon Glass]

Hi Heinrich,

On 2026-05-21T18:50:22, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
> efi_loader: add ACPI table dump EFI app
>
> Provide an EFI binary that can dump all ACPI tables to files
> <table_name>.dat. These can then be further analyzed by decompiling
> with iasl.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> Acked-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> Tested-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>
> lib/efi_loader/Makefile   |   1 +
>  lib/efi_loader/acpidump.c | 999 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 1000 insertions(+)

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,999 @@
> +#include <efi_api.h>
> +#include <part.h>
> +#include <string.h>
> +#include <acpi/acpi_table.h>

At 999 lines this is the third copy of the same EFI-app skeleton
(after smbiosdump.c and dtbdump.c). color(), print(), cls(), error(),
efi_input_yn(), efi_input(), skip_whitespace(), starts_with(),
open_file_system(), save_file(), command_loop(), get_load_options()
and the efi_main() boilerplate are all near-verbatim from
smbiosdump.c.

Please factor the shared pieces into a small header/object (e.g.
lib/efi_loader/efi_app_helper.c) so the new app only contains the
ACPI-specific code. Otherwise every future fix has to be made in three
places.

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,999 @@
> +     rsdp = get_acpi_rsdp_addr();
> +     print(u"RSDP address: ");
> +     printp(rsdp);
> +     print(u'\r\n');
> +     if (!rsdp) {
> +             error(u"No ACPI configuration table\r\n");
> +             return EFI_NOT_FOUND;
> +     }

When there is no ACPI table we still print 'RSDP address:
0x0000000000000000' before the error. Please move the NULL check ahead
of the printing.

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,999 @@
> +             if (!memcmp(table->signature, 'FACP', 4)) {
> +                     struct acpi_fadt *fadt = (struct acpi_fadt *)table;
> +                     uintptr_t dsdt_addr;
> +                     uintptr_t facs_addr;
> +
> +                     dsdt_addr = read_u64(&fadt->x_dsdt);
> +                     if (!dsdt_addr)
> +                             dsdt_addr = read_u32(&fadt->dsdt);

x_dsdt and x_firmware_ctrl live past offset 0x8c - an ACPI 1.0 FADT
has a length below that, so this reads beyond the table's declared
(and checksummed) extent. Do we care about old tables? If so, please
gate the 64-bit reads on len >= offsetofend(struct acpi_fadt, x_dsdt)
(and similarly for x_firmware_ctrl).

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,999 @@
> +                             dsdt = (struct acpi_table_header *)dsdt_addr;
> +                             dsdt_len = read_u32(&dsdt->length);
> +                             if (dsdt_len < sizeof(*dsdt)) {
> +                                     error(u"Invalid DSDT length\r\n");
> +                                     ret = EFI_LOAD_ERROR;
> +                                     goto err_out;
> +                             }
> +                             if (!checksum_ok(dsdt, dsdt_len)) {
> +                                     error(u"Invalid DSDT checksum\r\n");
> +                                     ret = EFI_LOAD_ERROR;
> +                                     goto err_out;
> +                             }

For FACS you verify the signature before trusting the length, but not
for DSDT - a corrupt FADT pointing at random memory could pass length
and checksum here. Please add a memcmp(dsdt->signature, 'DSDT', 4)
check.

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,999 @@
> +             memcpy(buf, (void *)tables[i].addr, tables[i].len);
> +             ret = save_file(filename, buf, tables[i].len);
> +             if (ret == EFI_SUCCESS) {
> +                     print(filename);
> +                     print(u" written\r\n");
> +             }
> +
> +             bs->free_pool(buf);
> +             if (ret != EFI_SUCCESS)
> +                     break;

If the user answers 'n' to the overwrite prompt for one table,
save_file() returns EFI_ACCESS_DENIED and the whole dump aborts,
silently skipping the rest. Is that intended?

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,999 @@
> + * starts_with() - check if @string starts with @keyword
> + *
> + * @string:  string to search for keyword
> + * @keyword: keyword to be searched
> + * Return:   true fi @string starts with the keyword
> + */

'fi' should be 'if'

> diff --git a/lib/efi_loader/acpidump.c b/lib/efi_loader/acpidump.c
> @@ -0,0 +1,999 @@
> +     ret = collect_tables(&tables, &table_count);
> +     if (ret != EFI_SUCCESS)
> +             return ret;
> +
> +     if (!table_count) {
> +             print(u"No ACPI tables found\r\n");
> +             return EFI_NOT_FOUND;
> +     }

collect_tables() always seeds the array with RSDP + root before
returning EFI_SUCCESS, so this branch is unreachable - and if it ever
did fire it would leak 'tables'. Either drop the check or free
'tables' on the way out.

Regards,
Simon