Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.bugs.dist > #1014270 > unrolled thread

Bug#963033: linux-image-arm64: kexec loses EFI system tables with Debian kernels

Back to article view | Back to linux.debian.bugs.dist

Contents

  Bug#963033: linux-image-arm64: kexec loses EFI system tables with Debian kernels Da Xue <da+debianbug@lessconfused.com> - 2020-06-18 07:00 +0200
    Bug#963033: linux-image-arm64: kexec loses EFI system tables with Debian kernels Gabriel Krisman Bertazi <krisman@collabora.com> - 2020-06-26 22:40 +0200
      Bug#963033: linux-image-arm64: kexec loses EFI system tables with Debian kernels Uwe Kleine-König <uwe@kleine-koenig.org> - 2020-07-22 21:30 +0200
        Bug#963033: linux-image-arm64: kexec loses EFI system tables with Debian kernels Gabriel Krisman Bertazi <krisman@collabora.com> - 2020-07-22 21:40 +0200
          Bug#963033: linux-image-arm64: kexec loses EFI system tables with Debian kernels Romain Perier <romain.perier@gmail.com> - 2020-07-24 15:20 +0200

#1014270 — Bug#963033: linux-image-arm64: kexec loses EFI system tables with Debian kernels

Package: linux-image-arm64
Version: 4.19+105+deb10u4
Severity: important
Tags: d-i

Dear Maintainer,

I am testing with ARM64 systems with u-boot. Debian is booted in EFI mode and properly detects the EFI system tables. 

[    0.000000] efi: EFI v2.80 by Das U-Boot                                                                                                 
[    0.000000] efi:  RTPROP=0x3aef9040  SMBIOS=0x3aeee000  RNG=0x39b81040  MEMRESERVE=0x39b80040                                            
[    0.000000] efi: seeding entropy pool  


Anytime I kexec the same or another Debian kernel, I get the following message:

[    0.000000] efi: Getting EFI parameters from FDT:
[    0.000000] efi: Can't find 'System Table' in device tree!

This problem is occuring with both 4.19, 5.4, 5.6, and 5.7 from Debian. It does not occur with SUSE or Fedora kernels. 

-- System Information:
Debian Release: 10.4
  APT prefers stable-updates
  APT policy: (500, 'stable-updates'), (500, 'stable')
Architecture: arm64 (aarch64)

Kernel: Linux 4.19.0-9-arm64 (SMP w/4 CPU cores)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), LANGUAGE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages linux-image-arm64 depends on:
ii  linux-image-4.19.0-9-arm64  4.19.118-2+deb10u1

linux-image-arm64 recommends no packages.

linux-image-arm64 suggests no packages.

-- no debconf information

[toc] | [next] | [standalone]

#1015423

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.

>8
From: Gabriel Krisman Bertazi <krisman@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 <krisman@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);
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 },
 	};
 
 	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;
 		}
 		return systab;
 	}
-- 
2.27.0

[toc] | [prev] | [next] | [standalone]

#1018929

[Multipart message — attachments visible in raw view] — view raw

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 <krisman@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 <krisman@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

[toc] | [prev] | [next] | [standalone]

#1018933

Uwe Kleine-König <uwe@kleine-koenig.org> writes:

> 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.

Hi, thanks for the review.

>> 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?

argh.

> 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.

I don't think this makes the patch more readable, quite the opposite.
But I don't oppose changing it.
>
> Is it right that a missing linux,uefi-secure-boot property still results
> in a pr_err?

It think it is reasonable to print the message in the log, so board
developers know there is something missing for their board.  We could
make it a pr_warn, as long as it doesn't fail silently, works for me.

> I wonder what the upstream status of the broken patch is.

It never got merged upstream.  There are some lockdown features merged
already, but this is not part of it, as far as I know.

-- 
Gabriel Krisman Bertazi

[toc] | [prev] | [next] | [standalone]

#1019131

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jul 22, 2020 at 03:28:39PM -0400, Gabriel Krisman Bertazi wrote:

Hi,

Well, after an analyze on my side, the EFI mode is detected based on the EFI stub, right ?
Then the kernel populates the FDT with EFI properties into its own address space (based on
what was previously found from the stub). Kexec simply does not preserve the EFI mode at
all for arm64, it is only present for x86.

With the original debian patch, we suppose that the "secure-boot" property will always be
set in the FDT, simply because this information should be found in the EFI stub (unset,
disabled, enabled, etc...), so the corresponding FDT property will always have a value and
will always be present. So assume that it is always the case is correct, imho.

Now, if the EFI stubs informations are not passed to the second stage
kernel, either nor "system table" or "secure-boot" or the rest will be
found, explaining the issue of the first comment (read the first comment
it is "System table" that is not found).

We should have a "setup_efi_info" like it is the case in the kexec-tools
code base for x86, except we should have something similar for arm64.

Regards,
Romain

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.bugs.dist

csiph-web