linux-kernel - Re: [PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMj1kXG4A4h3=bZC6kSrwsZa7p4RZ-uN5N67pZUFLOQ2RJE64w@mail.gmail.com>
Date: Mon, 25 Nov 2024 18:24:57 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Yeoreum Yun <yeoreum.yun@....com>
Cc: broonie@...nel.org, sami.mujawar@....com, sudeep.holla@....com, 
	pierre.gondois@....com, hagarhem@...zon.com, catalin.marinas@....com, 
	will@...nel.org, guohanjun@...wei.com, Jonathan.Cameron@...wei.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-efi@...r.kernel.org
Subject: Re: [PATCH v2 2/2] efi/fdt: ignore dtb when acpi option is used with force

On Mon, 25 Nov 2024 at 18:08, Yeoreum Yun <yeoreum.yun@....com> wrote:
>
> Since acpi=force doesn't use dt fallback, it's meaningless to load dt
> from comaand line option or from configuration table.
> Skip loading dt when acpi=force option is used.
> otherwise it could produce unnecessary error message
> while scanning dt if passed dt's format is invalid.
>
> Signed-off-by: Yeoreum Yun <yeoreum.yun@....com>

I take it this is working around buggy firmware that passes both a DT
and ACPI tables, and the DT in question is broken?

If so, this should be fixed in the firmware. The EFI stub does not
reason at all about ACPI boot vs DT boot, and I would prefer to keep
it that way, especially because this code is shared with other
architectures. For instance, the meaning of acpi= could differ between
architectures, or they may not implement ACPI in the first place.

If not, I don't think there is anything to solve here. Those error
messages are not fatal, right? Note that older GRUB builds might use
the DT to pass initrd information even when booting via ACPI.



> ---
>  drivers/firmware/efi/libstub/fdt.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 6a337f1f8787..27291ef7c773 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -231,6 +231,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
>         struct exit_boot_struct priv;
>         unsigned long fdt_addr = 0;
>         unsigned long fdt_size = 0;
> +       bool acpi_force = false;
> +
>
>         if (!efi_novamap) {
>                 status = efi_alloc_virtmap(&priv.runtime_map, &desc_size,
> @@ -241,13 +243,17 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
>                 }
>         }
>
> +       if (strstr(cmdline_ptr, "acpi=force"))
> +               acpi_force = true;
> +
>         /*
>          * Unauthenticated device tree data is a security hazard, so ignore
>          * 'dtb=' unless UEFI Secure Boot is disabled.  We assume that secure
>          * boot is enabled if we can't determine its state.
>          */
>         if (!IS_ENABLED(CONFIG_EFI_ARMSTUB_DTB_LOADER) ||
> -           efi_get_secureboot() != efi_secureboot_mode_disabled) {
> +           efi_get_secureboot() != efi_secureboot_mode_disabled ||
> +                       acpi_force) {
>                 if (strstr(cmdline_ptr, "dtb="))
>                         efi_err("Ignoring DTB from command line.\n");
>         } else {
> @@ -261,7 +267,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
>
>         if (fdt_addr) {
>                 efi_info("Using DTB from command line\n");
> -       } else {
> +       } else if (!acpi_force) {
>                 /* Look for a device tree configuration table entry. */
>                 fdt_addr = (uintptr_t)get_fdt(&fdt_size);
>                 if (fdt_addr)
> --
> LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>