Hello, On Fri, Jun 26, 2020 at 04:34:28PM -0400, Gabriel Krisman Bertazi wrote: > Hi, > > This is introduced by a Debian specific patch > features/all/lockdown/arm64-add-kernel-config-option-to-lock-down-when.patch > > The following patch fixes it.
Note, I'm not an expert in the area this patch modifies, just some
general feedback.
> >8
> From: Gabriel Krisman Bertazi <kris...@collabora.com>
> Subject: [PATCH] arm64: Don't disable EFI boot mode on linux,uefi-secure-boot
> table absence
>
> The Debian specific out-of-tree kernel patch titled ("arm64: add kernel
> config option to lock down when in Secure Boot mode") introduces a
> regression for EFI-booted systems that don't have a
> "linux,uefi-secure-boot" FDT entry.
>
> In these systems, when the table is not found, it causes the FDT
> function to error out and not return other UEFI tables, in particular
> the System Table, which makes the kernel think it is not running on EFI
> mode.
>
> Instead, let the EFI mode boot continue with the correct System Table,
> and consider the efi secureboot mode as unknown.
>
> This regression was found at least as early as the debian port to 5.4.19,
> but it still affects the most recent 5.7.6 debian kernel.
>
> Signed-off-by: Gabriel Krisman Bertazi <kris...@collabora.com>
> ---
> drivers/firmware/efi/arm-init.c | 2 +-
> drivers/firmware/efi/fdtparams.c | 18 +++++++++++-------
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
> index 78fcfbe3ddb9..fcb60320e77a 100644
> --- a/drivers/firmware/efi/arm-init.c
> +++ b/drivers/firmware/efi/arm-init.c
> @@ -206,7 +206,7 @@ void __init efi_init(void)
> {
> struct efi_memory_map_data data;
> u64 efi_system_table;
> - u32 secure_boot;
> + u32 secure_boot = efi_secureboot_mode_unknown;
>
> /* Grab UEFI information placed in FDT by stub */
> efi_system_table = efi_get_fdt_params(&data, &secure_boot);
I'd prefer to have the assignment in efi_get_fdt_params.
> diff --git a/drivers/firmware/efi/fdtparams.c
> b/drivers/firmware/efi/fdtparams.c
> index 152ca7cfccc9..78c36e582408 100644
> --- a/drivers/firmware/efi/fdtparams.c
> +++ b/drivers/firmware/efi/fdtparams.c
> @@ -96,13 +96,15 @@ u64 __init efi_get_fdt_params(struct efi_memory_map_data
> *mm, u32 *secure_boot)
> struct {
> void *var;
> int size;
> + int required;
> +
> } target[] = {
> - [SYSTAB] = { &systab, sizeof(systab) },
> - [MMBASE] = { &mm->phys_map, sizeof(mm->phys_map) },
> - [MMSIZE] = { &mm->size, sizeof(mm->size) },
> - [DCSIZE] = { &mm->desc_size, sizeof(mm->desc_size) },
> - [DCVERS] = { &mm->desc_version, sizeof(mm->desc_version) },
> - [SBMODE] = { secure_boot, sizeof(*secure_boot) },
> + [SYSTAB] = {&systab, sizeof(systab), 1},
> + [MMBASE] = {&mm->phys_map, sizeof(mm->phys_map), 1},
> + [MMSIZE] = {&mm->size, sizeof(mm->size), 1},
> + [DCSIZE] = {&mm->desc_size, sizeof(mm->desc_size), 1},
> + [DCVERS] = {&mm->desc_version, sizeof(mm->desc_version), 1},
> + [SBMODE] = {secure_boot, sizeof(*secure_boot), 0 },
Is the whitespace change intended here? I wonder if it would be easier
to drop this hunk and ...
> };
>
> BUILD_BUG_ON(ARRAY_SIZE(target) != ARRAY_SIZE(name));
> @@ -125,8 +127,10 @@ u64 __init efi_get_fdt_params(struct efi_memory_map_data
> *mm, u32 *secure_boot)
> continue;
> if (!j)
> goto notfound;
> +
> pr_err("Can't find property '%s' in DT!\n", pname);
> - return 0;
> + if (target[j].required)
> + return 0;
... do here
if (j != SBMODE)
return 0;
here.
Is it right that a missing linux,uefi-secure-boot property still results
in a pr_err?
I wonder what the upstream status of the broken patch is.
Best regards
Uwe
signature.asc
Description: PGP signature
