EFI fixes for v5.7-rc

---
 drivers/firmware/efi/cper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/firmware/efi/cper.c +1 -1

index b1af0de2e100..9d2512913d25 100644
@@ -101,7 +101,7 @@ void cper_print_bits(const char *pfx, unsigned int bits,
 		if (!len)
 			len = snprintf(buf, sizeof(buf), "%s%s", pfx, str);
 		else
-			len += snprintf(buf+len, sizeof(buf)-len, ", %s", str);
+			len += scnprintf(buf+len, sizeof(buf)-len, ", %s", str);
 	}
 	if (len)
 		printk("%s\n", buf);

Signature · 1 line

2.17.1

Ard Biesheuvel

author maintainer

2020-04-09 13:05 UTC #2 lore raw

PATCH 2/9 efi/libstub/x86: remove redundant assignment to pointer hdr

From: Colin Ian King <[email protected]>

The pointer hdr is being assigned a value that is never read and it is being updated later with a new value. The assignment is redundant and can be removed.

Addresses-Coverity: ("Unused value")

Signed-off-by Colin Ian King <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 drivers/firmware/efi/libstub/x86-stub.c | 2 --
 1 file changed, 2 deletions(-)

drivers/firmware/efi/libstub/x86-stub.c +0 -2

index 8d3a707789de..e02ea51273ff 100644
@@ -392,8 +392,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	image_base = efi_table_attr(image, image_base);
 	image_offset = (void *)startup_32 - image_base;
 
-	hdr = &((struct boot_params *)image_base)->hdr;
-
 	status = efi_allocate_pages(0x4000, (unsigned long *)&boot_params, ULONG_MAX);
 	if (status != EFI_SUCCESS) {
 		efi_printk("Failed to allocate lowmem for boot params\n");

Signature · 1 line

2.17.1

Ard Biesheuvel

author maintainer

2020-04-09 13:05 UTC #3 lore raw

PATCH 3/9 efi/x86: Move efi stub globals from .bss to .data

[1] https://systemd.io/BOOT_LOADER_SPECIFICATION/#type-2-efi-unified-kernel-images

From: Arvind Sankar <[email protected]>

Commit

3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")

removed the .bss section from the bzImage.

However, while a PE loader is required to zero-initialize the .bss section before calling the PE entry point, the EFI handover protocol does not currently document any requirement that .bss be initialized by the bootloader prior to calling the handover entry.

When systemd-boot is used to boot a unified kernel image [1], the image is constructed by embedding the bzImage as a .linux section in a PE executable that contains a small stub loader from systemd together with additional sections and potentially an initrd. As the .bss section within the bzImage is no longer explicitly present as part of the file, it is not initialized before calling the EFI handover entry. Furthermore, as the size of the embedded .linux section is only the size of the bzImage file itself, the .bss section's memory may not even have been allocated.

In particular, this can result in efi_disable_pci_dma being true even when it was not specified via the command line or configuration option, which in turn causes crashes while booting on some systems.

To avoid issues, place all EFI stub global variables into the .data section instead of .bss. As of this writing, only boolean flags for a few command line arguments and the sys_table pointer were in .bss and will now move into the .data section.

Signed-off-by Arvind Sankar <[email protected]>

Reported-by Sergey Shatunov <[email protected]>

Fixes 3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")

Link https://lore.kernel.org/r/[email protected] open here

---
 drivers/firmware/efi/libstub/efistub.h  | 2 +-
 drivers/firmware/efi/libstub/x86-stub.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/firmware/efi/libstub/efistub.h +1 -1

index cc90a748bcf0..67d26949fd26 100644
@@ -25,7 +25,7 @@
 #define EFI_ALLOC_ALIGN		EFI_PAGE_SIZE
 #endif
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) || defined(CONFIG_X86)
 #define __efistub_global	__section(.data)
 #else
 #define __efistub_global

drivers/firmware/efi/libstub/x86-stub.c +1 -1

index e02ea51273ff..867a57e28980 100644
@@ -20,7 +20,7 @@
 /* Maximum physical address for 64-bit kernel with 4-level paging */
 #define MAXMEM_X86_64_4LEVEL (1ull << 46)
 
-static efi_system_table_t *sys_table;
+static efi_system_table_t *sys_table __efistub_global;
 extern const bool efi_is64;
 extern u32 image_offset;

Signature · 1 line

2.17.1

Brian Gerst

2020-04-09 20:05 UTC #4 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 72 lines · Ard Biesheuvel

From: Arvind Sankar <[email protected]>
Commit
3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")
removed the .bss section from the bzImage.
However, while a PE loader is required to zero-initialize the .bss section before calling the PE entry point, the EFI handover protocol does not currently document any requirement that .bss be initialized by the bootloader prior to calling the handover entry.
When systemd-boot is used to boot a unified kernel image [1], the image is constructed by embedding the bzImage as a .linux section in a PE executable that contains a small stub loader from systemd together with additional sections and potentially an initrd. As the .bss section within the bzImage is no longer explicitly present as part of the file, it is not initialized before calling the EFI handover entry. Furthermore, as the size of the embedded .linux section is only the size of the bzImage file itself, the .bss section's memory may not even have been allocated.
In particular, this can result in efi_disable_pci_dma being true even when it was not specified via the command line or configuration option, which in turn causes crashes while booting on some systems.
To avoid issues, place all EFI stub global variables into the .data section instead of .bss. As of this writing, only boolean flags for a few command line arguments and the sys_table pointer were in .bss and will now move into the .data section.
[1] https://systemd.io/BOOT_LOADER_SPECIFICATION/#type-2-efi-unified-kernel-images
Signed-off-by Arvind Sankar <[email protected]>
Reported-by Sergey Shatunov <[email protected]>
Fixes 3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Ard Biesheuvel <[email protected]>
---
 drivers/firmware/efi/libstub/efistub.h  | 2 +-
 drivers/firmware/efi/libstub/x86-stub.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/firmware/efi/libstub/efistub.h +1 -1
index cc90a748bcf0..67d26949fd26 100644
@@ -25,7 +25,7 @@
 #define EFI_ALLOC_ALIGN                EFI_PAGE_SIZE
 #endif
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) || defined(CONFIG_X86)
 #define __efistub_global       __section(.data)
 #else
 #define __efistub_global
drivers/firmware/efi/libstub/x86-stub.c +1 -2
index e02ea51273ff..867a57e28980 100644
@@ -20,7 +20,7 @@
 /* Maximum physical address for 64-bit kernel with 4-level paging */
 #define MAXMEM_X86_64_4LEVEL (1ull << 46)
 
-static efi_system_table_t *sys_table;
+static efi_system_table_t *sys_table __efistub_global;
 extern const bool efi_is64;
 extern u32 image_offset;
 
--

Quoted 2 lines

2.17.1

Can we use the -fno-zero-initialized-in-bss compiler flag instead of explicitly marking global variables?

--
Brian Gerst

Brian Gerst

2020-04-09 20:53 UTC #5 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 73 lines · Brian Gerst

Quoted 71 lines · Ard Biesheuvel
From: Arvind Sankar <[email protected]>
Commit
3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")
removed the .bss section from the bzImage.
However, while a PE loader is required to zero-initialize the .bss section before calling the PE entry point, the EFI handover protocol does not currently document any requirement that .bss be initialized by the bootloader prior to calling the handover entry.
When systemd-boot is used to boot a unified kernel image [1], the image is constructed by embedding the bzImage as a .linux section in a PE executable that contains a small stub loader from systemd together with additional sections and potentially an initrd. As the .bss section within the bzImage is no longer explicitly present as part of the file, it is not initialized before calling the EFI handover entry. Furthermore, as the size of the embedded .linux section is only the size of the bzImage file itself, the .bss section's memory may not even have been allocated.
In particular, this can result in efi_disable_pci_dma being true even when it was not specified via the command line or configuration option, which in turn causes crashes while booting on some systems.
To avoid issues, place all EFI stub global variables into the .data section instead of .bss. As of this writing, only boolean flags for a few command line arguments and the sys_table pointer were in .bss and will now move into the .data section.
[1] https://systemd.io/BOOT_LOADER_SPECIFICATION/#type-2-efi-unified-kernel-images
Signed-off-by Arvind Sankar <[email protected]>
Reported-by Sergey Shatunov <[email protected]>
Fixes 3ee372ccce4d ("x86/boot/compressed/64: Remove .bss/.pgtable from bzImage")
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Ard Biesheuvel <[email protected]>
---
 drivers/firmware/efi/libstub/efistub.h  | 2 +-
 drivers/firmware/efi/libstub/x86-stub.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/firmware/efi/libstub/efistub.h +1 -1
index cc90a748bcf0..67d26949fd26 100644
@@ -25,7 +25,7 @@
 #define EFI_ALLOC_ALIGN                EFI_PAGE_SIZE
 #endif
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) || defined(CONFIG_X86)
 #define __efistub_global       __section(.data)
 #else
 #define __efistub_global
drivers/firmware/efi/libstub/x86-stub.c +1 -1
index e02ea51273ff..867a57e28980 100644
@@ -20,7 +20,7 @@
 /* Maximum physical address for 64-bit kernel with 4-level paging */
 #define MAXMEM_X86_64_4LEVEL (1ull << 46)
 
-static efi_system_table_t *sys_table;
+static efi_system_table_t *sys_table __efistub_global;
 extern const bool efi_is64;
 extern u32 image_offset;
 

Quoted 6 lines

Quoted 3 lines
--
2.17.1
Can we use the -fno-zero-initialized-in-bss compiler flag instead of explicitly marking global variables?

Scratch that. Apparently it only works when a variable is explicitly initialized to zero.

--
Brian Gerst

Arvind Sankar

2020-04-09 21:08 UTC #6 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 8 lines · Brian Gerst

Quoted 2 lines
Can we use the -fno-zero-initialized-in-bss compiler flag instead of explicitly marking global variables?
Scratch that. Apparently it only works when a variable is explicitly initialized to zero.
--
Brian Gerst

Right, there doesn't seem to be a compiler option to turn off the use of .bss altogether.

Ard Biesheuvel

author maintainer

2020-04-10 08:29 UTC #7 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 5 lines · Arvind Sankar

Quoted 3 lines · Brian Gerst
Quoted 2 lines
Can we use the -fno-zero-initialized-in-bss compiler flag instead of explicitly marking global variables?

Quoted 8 lines

Quoted 5 lines
Scratch that. Apparently it only works when a variable is explicitly initialized to zero.
--
Brian Gerst
Right, there doesn't seem to be a compiler option to turn off the use of .bss altogether.

Yeah. I'll try to come up with a way to consolidate this a bit across architectures (which is a bit easier now that all of the EFI stub C code lives in the same place). It is probably easiest to use a section renaming trick similar to the one I added for ARM (as Arvind suggested as well, IIRC), and get rid of the per-symbol annotations altogether.

Arvind Sankar

2020-04-10 15:16 UTC #8 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 12 lines · Ard Biesheuvel

Quoted 11 lines · Arvind Sankar
Quoted 8 lines · Brian Gerst
Quoted 2 lines
Can we use the -fno-zero-initialized-in-bss compiler flag instead of explicitly marking global variables?
Scratch that. Apparently it only works when a variable is explicitly initialized to zero.
--
Brian Gerst

Quoted 8 lines

Quoted 2 lines
Right, there doesn't seem to be a compiler option to turn off the use of .bss altogether.
Yeah. I'll try to come up with a way to consolidate this a bit across architectures (which is a bit easier now that all of the EFI stub C code lives in the same place). It is probably easiest to use a section renaming trick similar to the one I added for ARM (as Arvind suggested as well, IIRC), and get rid of the per-symbol annotations altogether.

Does that work for 32-bit ARM, or does it need to be .data to tell the compiler to avoid generating GOT references? If that's fine, we don't actually need to rename sections -- linker script magic is enough. For eg, the below pulls the EFI stub bss into .data for x86 without the need for the annotations.

arch/x86/boot/compressed/vmlinux.lds.S +1 -0

index 508cfa6828c5..e324819c95bc 100644
@@ -52,6 +52,7 @@ SECTIONS
 		_data = . ;
 		*(.data)
 		*(.data.*)
+		drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
 		_edata = . ;
 	}
 	. = ALIGN(L1_CACHE_BYTES);

Ard Biesheuvel

author maintainer

2020-04-10 16:03 UTC #9 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 34 lines · Arvind Sankar

Quoted 12 lines · Ard Biesheuvel
Quoted 11 lines · Arvind Sankar
Quoted 8 lines · Brian Gerst
Quoted 2 lines
Can we use the -fno-zero-initialized-in-bss compiler flag instead of explicitly marking global variables?
Scratch that. Apparently it only works when a variable is explicitly initialized to zero.
--
Brian Gerst
Quoted 8 lines
Quoted 2 lines
Right, there doesn't seem to be a compiler option to turn off the use of .bss altogether.
Yeah. I'll try to come up with a way to consolidate this a bit across architectures (which is a bit easier now that all of the EFI stub C code lives in the same place). It is probably easiest to use a section renaming trick similar to the one I added for ARM (as Arvind suggested as well, IIRC), and get rid of the per-symbol annotations altogether.
Does that work for 32-bit ARM, or does it need to be .data to tell the compiler to avoid generating GOT references? If that's fine, we don't actually need to rename sections -- linker script magic is enough. For eg, the below pulls the EFI stub bss into .data for x86 without the need for the annotations.
arch/x86/boot/compressed/vmlinux.lds.S +0 -0
index 508cfa6828c5..e324819c95bc 100644

arch/x86/boot/compressed/vmlinux.lds.S SECTIONS · line 58

Quoted 7 lines

arch/x86/boot/compressed/vmlinux.lds.S +1 -0

@@ -52,6 +52,7 @@ SECTIONS
                _data = . ;
                *(.data)
                *(.data.*)
+               drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
                _edata = . ;
        }
        . = ALIGN(L1_CACHE_BYTES);

No, we can add this to ARM as well, and get rid of the __efistub_global annotations entirely.

We'll still need .data.efistub for the .data pieces, but that is a separate issue.

Arvind Sankar

2020-04-10 18:01 UTC #10 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 30 lines · Ard Biesheuvel

Quoted 29 lines · Arvind Sankar
Quoted 12 lines · Ard Biesheuvel
Quoted 11 lines · Arvind Sankar
Quoted 8 lines · Brian Gerst
Quoted 2 lines
Can we use the -fno-zero-initialized-in-bss compiler flag instead of explicitly marking global variables?
Scratch that. Apparently it only works when a variable is explicitly initialized to zero.
--
Brian Gerst
Quoted 8 lines
Quoted 2 lines
Right, there doesn't seem to be a compiler option to turn off the use of .bss altogether.
Yeah. I'll try to come up with a way to consolidate this a bit across architectures (which is a bit easier now that all of the EFI stub C code lives in the same place). It is probably easiest to use a section renaming trick similar to the one I added for ARM (as Arvind suggested as well, IIRC), and get rid of the per-symbol annotations altogether.
Does that work for 32-bit ARM, or does it need to be .data to tell the compiler to avoid generating GOT references? If that's fine, we don't actually need to rename sections -- linker script magic is enough. For eg, the below pulls the EFI stub bss into .data for x86 without the need for the annotations.

Quoted 15 lines

arch/x86/boot/compressed/vmlinux.lds.S SECTIONS · line 58

Quoted 12 lines

arch/x86/boot/compressed/vmlinux.lds.S +1 -0

index 508cfa6828c5..e324819c95bc 100644
@@ -52,6 +52,7 @@ SECTIONS
                _data = . ;
                *(.data)
                *(.data.*)
+               drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
                _edata = . ;
        }
        . = ALIGN(L1_CACHE_BYTES);

No, we can add this to ARM as well, and get rid of the __efistub_global annotations entirely.

Cool.

Quoted 3 lines

We'll still need .data.efistub for the .data pieces, but that is a separate issue.

You can avoid that by using an archive specification like above. i.e. adding
drivers/firmware/efi/libstub/lib.a:(.data .data.*)
to the .init.data output section will pull in just the .data input sections from the EFI stub into the .init.data section.

Ard Biesheuvel

author maintainer

2020-04-10 18:03 UTC #11 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 51 lines · Arvind Sankar

Quoted 30 lines · Ard Biesheuvel
Quoted 29 lines · Arvind Sankar
Quoted 12 lines · Ard Biesheuvel
Quoted 11 lines · Arvind Sankar
Quoted 8 lines · Brian Gerst
Quoted 2 lines
Can we use the -fno-zero-initialized-in-bss compiler flag instead of explicitly marking global variables?
Scratch that. Apparently it only works when a variable is explicitly initialized to zero.
--
Brian Gerst
Quoted 8 lines
Quoted 2 lines
Right, there doesn't seem to be a compiler option to turn off the use of .bss altogether.
Yeah. I'll try to come up with a way to consolidate this a bit across architectures (which is a bit easier now that all of the EFI stub C code lives in the same place). It is probably easiest to use a section renaming trick similar to the one I added for ARM (as Arvind suggested as well, IIRC), and get rid of the per-symbol annotations altogether.
Does that work for 32-bit ARM, or does it need to be .data to tell the compiler to avoid generating GOT references? If that's fine, we don't actually need to rename sections -- linker script magic is enough. For eg, the below pulls the EFI stub bss into .data for x86 without the need for the annotations.
Quoted 15 lines
arch/x86/boot/compressed/vmlinux.lds.S SECTIONS · line 58
Quoted 12 lines
arch/x86/boot/compressed/vmlinux.lds.S +1 -0
index 508cfa6828c5..e324819c95bc 100644
@@ -52,6 +52,7 @@ SECTIONS
                _data = . ;
                *(.data)
                *(.data.*)
+               drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
                _edata = . ;
        }
        . = ALIGN(L1_CACHE_BYTES);
No, we can add this to ARM as well, and get rid of the __efistub_global annotations entirely.
Cool.
Quoted 1 line

Quoted 8 lines

Quoted 2 lines
We'll still need .data.efistub for the .data pieces, but that is a separate issue.
You can avoid that by using an archive specification like above. i.e. adding
drivers/firmware/efi/libstub/lib.a:(.data .data.*)
to the .init.data output section will pull in just the .data input sections from the EFI stub into the .init.data section.

Sure. But the ARM decompressor linker script currently discards .data before this point in the linker script, and relies on this as a safety net to ensure that no new .data items get added to the decompressor binary (which runs after the stub)

Arvind Sankar

2020-04-10 19:04 UTC #12 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 61 lines · Ard Biesheuvel

Quoted 51 lines · Arvind Sankar
Quoted 30 lines · Ard Biesheuvel
Quoted 29 lines · Arvind Sankar
Quoted 12 lines · Ard Biesheuvel
Quoted 11 lines · Arvind Sankar
Quoted 8 lines · Brian Gerst
Quoted 2 lines
Can we use the -fno-zero-initialized-in-bss compiler flag instead of explicitly marking global variables?
Scratch that. Apparently it only works when a variable is explicitly initialized to zero.
--
Brian Gerst
Quoted 8 lines
Quoted 2 lines
Right, there doesn't seem to be a compiler option to turn off the use of .bss altogether.
Yeah. I'll try to come up with a way to consolidate this a bit across architectures (which is a bit easier now that all of the EFI stub C code lives in the same place). It is probably easiest to use a section renaming trick similar to the one I added for ARM (as Arvind suggested as well, IIRC), and get rid of the per-symbol annotations altogether.
Does that work for 32-bit ARM, or does it need to be .data to tell the compiler to avoid generating GOT references? If that's fine, we don't actually need to rename sections -- linker script magic is enough. For eg, the below pulls the EFI stub bss into .data for x86 without the need for the annotations.
Quoted 15 lines
arch/x86/boot/compressed/vmlinux.lds.S SECTIONS · line 58
Quoted 12 lines
arch/x86/boot/compressed/vmlinux.lds.S +1 -0
index 508cfa6828c5..e324819c95bc 100644
@@ -52,6 +52,7 @@ SECTIONS
                _data = . ;
                *(.data)
                *(.data.*)
+               drivers/firmware/efi/libstub/lib.a:(.bss .bss.*)
                _edata = . ;
        }
        . = ALIGN(L1_CACHE_BYTES);
No, we can add this to ARM as well, and get rid of the __efistub_global annotations entirely.
Cool.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
We'll still need .data.efistub for the .data pieces, but that is a separate issue.
You can avoid that by using an archive specification like above. i.e. adding
drivers/firmware/efi/libstub/lib.a:(.data .data.*)
to the .init.data output section will pull in just the .data input sections from the EFI stub into the .init.data section.

Quoted 4 lines

Sure. But the ARM decompressor linker script currently discards .data before this point in the linker script, and relies on this as a safety net to ensure that no new .data items get added to the decompressor binary (which runs after the stub)

Ah.

Arvind Sankar

2020-04-11 01:03 UTC #13 lore raw

Re: [PATCH 3/9] efi/x86: Move efi stub globals from .bss to .data

Quoted 10 lines · Ard Biesheuvel

Quoted 9 lines
Quoted 3 lines
We'll still need .data.efistub for the .data pieces, but that is a separate issue.
You can avoid that by using an archive specification like above. i.e. adding
drivers/firmware/efi/libstub/lib.a:(.data .data.*)
to the .init.data output section will pull in just the .data input sections from the EFI stub into the .init.data section.

Quoted 4 lines

Sure. But the ARM decompressor linker script currently discards .data before this point in the linker script, and relies on this as a safety net to ensure that no new .data items get added to the decompressor binary (which runs after the stub)

You should be able to use EXCLUDE_FILE to skip discarding the .data section from libstub. That also supports the archive: syntax according to the docs though I haven't tried it.

Ard Biesheuvel

author maintainer

2020-04-09 13:05 UTC #14 lore raw

PATCH 4/9 efi/x86: Always relocate the kernel for EFI handover entry

[1] https://systemd.io/BOOT_LOADER_SPECIFICATION/#type-2-efi-unified-kernel-images

From: Arvind Sankar <[email protected]>

Commit

d5cdf4cfeac9 ("efi/x86: Don't relocate the kernel unless necessary")

tries to avoid relocating the kernel in the EFI stub as far as possible.

However, when systemd-boot is used to boot a unified kernel image [1], the image is constructed by embedding the bzImage as a .linux section in a PE executable that contains a small stub loader from systemd that will call the EFI stub handover entry, together with additional sections and potentially an initrd. When this image is constructed, by for example dracut, the initrd is placed after the bzImage without ensuring that at least init_size bytes are available for the bzImage. If the kernel is not relocated by the EFI stub, this could result in the compressed kernel's startup code in head_{32,64}.S overwriting the initrd.

To prevent this, unconditionally relocate the kernel if the EFI stub was entered via the handover entry point.

Signed-off-by Arvind Sankar <[email protected]>

Reported-by Sergey Shatunov <[email protected]>

Fixes d5cdf4cfeac9 ("efi/x86: Don't relocate the kernel unless necessary")

Link https://lore.kernel.org/r/[email protected] open here

---
 drivers/firmware/efi/libstub/x86-stub.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

drivers/firmware/efi/libstub/x86-stub.c +10 -4

index 867a57e28980..05ccb229fb45 100644
@@ -740,8 +740,15 @@ unsigned long efi_main(efi_handle_t handle,
 	 * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
 	 * KASLR uses.
 	 *
-	 * Also relocate it if image_offset is zero, i.e. we weren't loaded by
-	 * LoadImage, but we are not aligned correctly.
+	 * Also relocate it if image_offset is zero, i.e. the kernel wasn't
+	 * loaded by LoadImage, but rather by a bootloader that called the
+	 * handover entry. The reason we must always relocate in this case is
+	 * to handle the case of systemd-boot booting a unified kernel image,
+	 * which is a PE executable that contains the bzImage and an initrd as
+	 * COFF sections. The initrd section is placed after the bzImage
+	 * without ensuring that there are at least init_size bytes available
+	 * for the bzImage, and thus the compressed kernel's startup code may
+	 * overwrite the initrd unless it is moved out of the way.
 	 */
 
 	buffer_start = ALIGN(bzimage_addr - image_offset,

@@ -751,8 +758,7 @@ unsigned long efi_main(efi_handle_t handle,
 	if ((buffer_start < LOAD_PHYSICAL_ADDR)				     ||
 	    (IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE)    ||
 	    (IS_ENABLED(CONFIG_X86_64) && buffer_end > MAXMEM_X86_64_4LEVEL) ||
-	    (image_offset == 0 && !IS_ALIGNED(bzimage_addr,
-					      hdr->kernel_alignment))) {
+	    (image_offset == 0)) {
 		status = efi_relocate_kernel(&bzimage_addr,
 					     hdr->init_size, hdr->init_size,
 					     hdr->pref_address,

Signature · 1 line

2.17.1

Ard Biesheuvel

author maintainer

2020-04-09 13:05 UTC #15 lore raw

PATCH 5/9 efi/arm: Deal with ADR going out of range in efi_enter_kernel()

Commit

0698fac4ac2a ("efi/arm: Clean EFI stub exit code from cache instead of avoiding it")

introduced a PC-relative reference to 'call_cache_fn' into efi_enter_kernel(), which lives way at the end of head.S. In some cases, the ARM version of the ADR instruction does not have sufficient range, resulting in a build error:

arch/arm/boot/compressed/head.S:1453: Error: invalid constant (fffffffffffffbe4) after fixup

ARM defines an alternative with a wider range, called ADRL, but this does not exist for Thumb-2. At the same time, the ADR instruction in Thumb-2 has a wider range, and so it does not suffer from the same issue.

So let's switch to ADRL for ARM builds, and keep the ADR for Thumb-2 builds.

Tested-by Arnd Bergmann <[email protected]>

---
 arch/arm/boot/compressed/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm/boot/compressed/head.S +2 -1

index 04f77214f050..61e6ee3ba75f 100644
@@ -1454,7 +1454,8 @@ ENTRY(efi_enter_kernel)
 		@ running beyond the PoU, and so calling cache_off below from
 		@ inside the PE/COFF loader allocated region is unsafe unless
 		@ we explicitly clean it to the PoC.
-		adr	r0, call_cache_fn		@ region of code we will
+ ARM(		adrl	r0, call_cache_fn	)
+ THUMB(		adr	r0, call_cache_fn	)	@ region of code we will
 		adr	r1, 0f				@ run with MMU off
 		bl	cache_clean_flush
 		bl	cache_off

Signature · 1 line

2.17.1

tip-bot2 for Ard Biesheuvel

merge bot Applied

2020-04-14 08:23 UTC #16 lore raw

[tip: efi/urgent] efi/arm: Deal with ADR going out of range in efi_enter_kernel()

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     a94691680bace7e1404e4f235badb74e30467e86
Gitweb:        https://git.kernel.org/tip/a94691680bace7e1404e4f235badb74e30467e86
Author:        Ard Biesheuvel <[email protected]>
AuthorDate:    Thu, 09 Apr 2020 15:04:30 +02:00
Committer:     Ingo Molnar <[email protected]>
CommitterDate: Tue, 14 Apr 2020 08:32:14 +02:00

efi/arm: Deal with ADR going out of range in efi_enter_kernel()

Commit

0698fac4ac2a ("efi/arm: Clean EFI stub exit code from cache instead of avoiding it")

arch/arm/boot/compressed/head.S:1453: Error: invalid constant (fffffffffffffbe4) after fixup

So let's switch to ADRL for ARM builds, and keep the ADR for Thumb-2 builds.

Tested-by Arnd Bergmann <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 arch/arm/boot/compressed/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm/boot/compressed/head.S +2 -1

index cabdd8f..e8e1c86 100644
@@ -1450,7 +1450,8 @@ ENTRY(efi_enter_kernel)
 		@ running beyond the PoU, and so calling cache_off below from
 		@ inside the PE/COFF loader allocated region is unsafe unless
 		@ we explicitly clean it to the PoC.
-		adr	r0, call_cache_fn		@ region of code we will
+ ARM(		adrl	r0, call_cache_fn	)
+ THUMB(		adr	r0, call_cache_fn	)	@ region of code we will
 		adr	r1, 0f				@ run with MMU off
 		bl	cache_clean_flush
 		bl	cache_off

Ard Biesheuvel

author maintainer

2020-04-09 13:05 UTC #17 lore raw

PATCH 6/9 Documentation: efi/x86: clarify EFI handover protocol and its requirements

The EFI handover protocol was introduced on x86 to permit the boot loader to pass a populated boot_params structure as an additional function argument to the entry point. This allows the bootloader to pass the base and size of a initrd image, which is more flexible than relying on the EFI stub's file I/O routines, which can only access the file system from which the kernel image itself was loaded from firmware.

This approach requires a fair amount of internal knowledge regarding the layout of the boot_params structure on the part of the boot loader, as well as knowledge regarding the allowed placement of the initrd in memory, and so it has been deprecated in favour of a new initrd loading method that is based on existing UEFI protocols and best practices.

So update the x86 boot protocol documentation to clarify that the EFI handover protocol has been deprecated, and while at it, add a note that invoking the EFI handover protocol still requires the PE/COFF image to be loaded properly (as opposed to simply being copied into memory). Also, drop the code32_start header field from the list of values that need to be provided, as this is no longer required.

Reviewed-by Borislav Petkov <[email protected]>

---
 Documentation/x86/boot.rst | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Documentation/x86/boot.rst +18 -3

index fa7ddc0428c8..5325c71ca877 100644
@@ -1399,8 +1399,8 @@ must have read/write permission; CS must be __BOOT_CS and DS, ES, SS
 must be __BOOT_DS; interrupt must be disabled; %rsi must hold the base
 address of the struct boot_params.
 
-EFI Handover Protocol
-=====================
+EFI Handover Protocol (deprecated)
+==================================
 
 This protocol allows boot loaders to defer initialisation to the EFI
 boot stub. The boot loader is required to load the kernel/initrd(s)

@@ -1408,6 +1408,12 @@ from the boot media and jump to the EFI handover protocol entry point
 which is hdr->handover_offset bytes from the beginning of
 startup_{32,64}.
 
+The boot loader MUST respect the kernel's PE/COFF metadata when it comes
+to section alignment, the memory footprint of the executable image beyond
+the size of the file itself, and any other aspect of the PE/COFF header
+that may affect correct operation of the image as a PE/COFF binary in the
+execution context provided by the EFI firmware.
+
 The function prototype for the handover entry point looks like this::
 
     efi_main(void *handle, efi_system_table_t *table, struct boot_params *bp)

@@ -1419,9 +1425,18 @@ UEFI specification. 'bp' is the boot loader-allocated boot params.
 
 The boot loader *must* fill out the following fields in bp::
 
-  - hdr.code32_start
   - hdr.cmd_line_ptr
   - hdr.ramdisk_image (if applicable)
   - hdr.ramdisk_size  (if applicable)
 
 All other fields should be zero.
+
+NOTE: The EFI Handover Protocol is deprecated in favour of the ordinary PE/COFF
+      entry point, combined with the LINUX_EFI_INITRD_MEDIA_GUID based initrd
+      loading protocol (refer to [0] for an example of the bootloader side of
+      this), which removes the need for any knowledge on the part of the EFI
+      bootloader regarding the internal representation of boot_params or any
+      requirements/limitations regarding the placement of the command line
+      and ramdisk in memory, or the placement of the kernel image itself.
+
+[0] https://github.com/u-boot/u-boot/commit/ec80b4735a593961fe701cc3a5d717d4739b0fd0

Signature · 1 line

2.17.1

tip-bot2 for Ard Biesheuvel

merge bot Applied

2020-04-14 08:23 UTC #18 lore raw

[tip: efi/urgent] Documentation/x86, efi/x86: Clarify EFI handover protocol and its requirements

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     8b84769a7a1505b279b337dae83d16390e83f5c1
Gitweb:        https://git.kernel.org/tip/8b84769a7a1505b279b337dae83d16390e83f5c1
Author:        Ard Biesheuvel <[email protected]>
AuthorDate:    Thu, 09 Apr 2020 15:04:31 +02:00
Committer:     Ingo Molnar <[email protected]>
CommitterDate: Tue, 14 Apr 2020 08:32:15 +02:00

Documentation/x86, efi/x86: Clarify EFI handover protocol and its requirements

Reviewed-by Borislav Petkov <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 Documentation/x86/boot.rst | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Documentation/x86/boot.rst +18 -3

index fa7ddc0..5325c71 100644
@@ -1399,8 +1399,8 @@ must have read/write permission; CS must be __BOOT_CS and DS, ES, SS
 must be __BOOT_DS; interrupt must be disabled; %rsi must hold the base
 address of the struct boot_params.
 
-EFI Handover Protocol
-=====================
+EFI Handover Protocol (deprecated)
+==================================
 
 This protocol allows boot loaders to defer initialisation to the EFI
 boot stub. The boot loader is required to load the kernel/initrd(s)

@@ -1408,6 +1408,12 @@ from the boot media and jump to the EFI handover protocol entry point
 which is hdr->handover_offset bytes from the beginning of
 startup_{32,64}.
 
+The boot loader MUST respect the kernel's PE/COFF metadata when it comes
+to section alignment, the memory footprint of the executable image beyond
+the size of the file itself, and any other aspect of the PE/COFF header
+that may affect correct operation of the image as a PE/COFF binary in the
+execution context provided by the EFI firmware.
+
 The function prototype for the handover entry point looks like this::
 
     efi_main(void *handle, efi_system_table_t *table, struct boot_params *bp)

@@ -1419,9 +1425,18 @@ UEFI specification. 'bp' is the boot loader-allocated boot params.
 
 The boot loader *must* fill out the following fields in bp::
 
-  - hdr.code32_start
   - hdr.cmd_line_ptr
   - hdr.ramdisk_image (if applicable)
   - hdr.ramdisk_size  (if applicable)
 
 All other fields should be zero.
+
+NOTE: The EFI Handover Protocol is deprecated in favour of the ordinary PE/COFF
+      entry point, combined with the LINUX_EFI_INITRD_MEDIA_GUID based initrd
+      loading protocol (refer to [0] for an example of the bootloader side of
+      this), which removes the need for any knowledge on the part of the EFI
+      bootloader regarding the internal representation of boot_params or any
+      requirements/limitations regarding the placement of the command line
+      and ramdisk in memory, or the placement of the kernel image itself.
+
+[0] https://github.com/u-boot/u-boot/commit/ec80b4735a593961fe701cc3a5d717d4739b0fd0

Ard Biesheuvel

author maintainer

2020-04-09 13:05 UTC #19 lore raw

PATCH 7/9 efi/libstub/file: merge filename buffers to reduce stack usage

Arnd reports that commit

9302c1bb8e47 ("efi/libstub: Rewrite file I/O routine")

reworks the file I/O routines in a way that triggers the following warning:

drivers/firmware/efi/libstub/file.c:240:1: warning: the frame size
of 1200 bytes is larger than 1024 bytes [-Wframe-larger-than=]

We can work around this issue dropping an instance of efi_char16_t[256] from the stack frame, and reusing the 'filename' field of the file info struct that we use to obtain file information from EFI (which contains the filename even though we already know it since we used it to open the file in the first place)

---
 drivers/firmware/efi/libstub/file.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

drivers/firmware/efi/libstub/file.c +14 ~~-13~~

index d4c7e5f59d2c..ea66b1f16a79 100644
@@ -29,30 +29,31 @@
  */
 #define EFI_READ_CHUNK_SIZE	SZ_1M
 
+struct finfo {
+	efi_file_info_t info;
+	efi_char16_t	filename[MAX_FILENAME_SIZE];
+};
+
 static efi_status_t efi_open_file(efi_file_protocol_t *volume,
-				  efi_char16_t *filename_16,
+				  struct finfo *fi,
 				  efi_file_protocol_t **handle,
 				  unsigned long *file_size)
 {
-	struct {
-		efi_file_info_t info;
-		efi_char16_t	filename[MAX_FILENAME_SIZE];
-	} finfo;
 	efi_guid_t info_guid = EFI_FILE_INFO_ID;
 	efi_file_protocol_t *fh;
 	unsigned long info_sz;
 	efi_status_t status;
 
-	status = volume->open(volume, &fh, filename_16, EFI_FILE_MODE_READ, 0);
+	status = volume->open(volume, &fh, fi->filename, EFI_FILE_MODE_READ, 0);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err("Failed to open file: ");
-		efi_char16_printk(filename_16);
+		efi_char16_printk(fi->filename);
 		efi_printk("\n");
 		return status;
 	}
 
-	info_sz = sizeof(finfo);
-	status = fh->get_info(fh, &info_guid, &info_sz, &finfo);
+	info_sz = sizeof(struct finfo);
+	status = fh->get_info(fh, &info_guid, &info_sz, fi);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err("Failed to get file info\n");
 		fh->close(fh);

@@ -60,7 +61,7 @@ static efi_status_t efi_open_file(efi_file_protocol_t *volume,
 	}
 
 	*handle = fh;
-	*file_size = finfo.info.file_size;
+	*file_size = fi->info.file_size;
 	return EFI_SUCCESS;
 }

@@ -146,13 +147,13 @@ static efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 
 	alloc_addr = alloc_size = 0;
 	do {
-		efi_char16_t filename[MAX_FILENAME_SIZE];
+		struct finfo fi;
 		unsigned long size;
 		void *addr;
 
 		offset = find_file_option(cmdline, cmdline_len,
 					  optstr, optstr_size,
-					  filename, ARRAY_SIZE(filename));
+					  fi.filename, ARRAY_SIZE(fi.filename));
 
 		if (!offset)
 			break;

@@ -166,7 +167,7 @@ static efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 				return status;
 		}
 
-		status = efi_open_file(volume, filename, &file, &size);
+		status = efi_open_file(volume, &fi, &file, &size);
 		if (status != EFI_SUCCESS)
 			goto err_close_volume;

Signature · 1 line

2.17.1

tip-bot2 for Ard Biesheuvel

merge bot Applied

2020-04-14 08:22 UTC #20 lore raw

[tip: efi/urgent] efi/libstub/file: Merge file name buffers to reduce stack usage

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     464fb126d98a047953040cc9c754801dbda54e5d
Gitweb:        https://git.kernel.org/tip/464fb126d98a047953040cc9c754801dbda54e5d
Author:        Ard Biesheuvel <[email protected]>
AuthorDate:    Thu, 09 Apr 2020 15:04:32 +02:00
Committer:     Ingo Molnar <[email protected]>
CommitterDate: Tue, 14 Apr 2020 08:32:15 +02:00

efi/libstub/file: Merge file name buffers to reduce stack usage

Arnd reports that commit

9302c1bb8e47 ("efi/libstub: Rewrite file I/O routine")

reworks the file I/O routines in a way that triggers the following warning:

drivers/firmware/efi/libstub/file.c:240:1: warning: the frame size
of 1200 bytes is larger than 1024 bytes [-Wframe-larger-than=]

We can work around this issue dropping an instance of efi_char16_t[256] from the stack frame, and reusing the 'filename' field of the file info struct that we use to obtain file information from EFI (which contains the file name even though we already know it since we used it to open the file in the first place)

Signed-off-by Ingo Molnar <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 drivers/firmware/efi/libstub/file.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

drivers/firmware/efi/libstub/file.c +14 ~~-13~~

index d4c7e5f..ea66b1f 100644
@@ -29,30 +29,31 @@
  */
 #define EFI_READ_CHUNK_SIZE	SZ_1M
 
+struct finfo {
+	efi_file_info_t info;
+	efi_char16_t	filename[MAX_FILENAME_SIZE];
+};
+
 static efi_status_t efi_open_file(efi_file_protocol_t *volume,
-				  efi_char16_t *filename_16,
+				  struct finfo *fi,
 				  efi_file_protocol_t **handle,
 				  unsigned long *file_size)
 {
-	struct {
-		efi_file_info_t info;
-		efi_char16_t	filename[MAX_FILENAME_SIZE];
-	} finfo;
 	efi_guid_t info_guid = EFI_FILE_INFO_ID;
 	efi_file_protocol_t *fh;
 	unsigned long info_sz;
 	efi_status_t status;
 
-	status = volume->open(volume, &fh, filename_16, EFI_FILE_MODE_READ, 0);
+	status = volume->open(volume, &fh, fi->filename, EFI_FILE_MODE_READ, 0);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err("Failed to open file: ");
-		efi_char16_printk(filename_16);
+		efi_char16_printk(fi->filename);
 		efi_printk("\n");
 		return status;
 	}
 
-	info_sz = sizeof(finfo);
-	status = fh->get_info(fh, &info_guid, &info_sz, &finfo);
+	info_sz = sizeof(struct finfo);
+	status = fh->get_info(fh, &info_guid, &info_sz, fi);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err("Failed to get file info\n");
 		fh->close(fh);

@@ -60,7 +61,7 @@ static efi_status_t efi_open_file(efi_file_protocol_t *volume,
 	}
 
 	*handle = fh;
-	*file_size = finfo.info.file_size;
+	*file_size = fi->info.file_size;
 	return EFI_SUCCESS;
 }

@@ -146,13 +147,13 @@ static efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 
 	alloc_addr = alloc_size = 0;
 	do {
-		efi_char16_t filename[MAX_FILENAME_SIZE];
+		struct finfo fi;
 		unsigned long size;
 		void *addr;
 
 		offset = find_file_option(cmdline, cmdline_len,
 					  optstr, optstr_size,
-					  filename, ARRAY_SIZE(filename));
+					  fi.filename, ARRAY_SIZE(fi.filename));
 
 		if (!offset)
 			break;

@@ -166,7 +167,7 @@ static efi_status_t handle_cmdline_files(efi_loaded_image_t *image,
 				return status;
 		}
 
-		status = efi_open_file(volume, filename, &file, &size);
+		status = efi_open_file(volume, &fi, &file, &size);
 		if (status != EFI_SUCCESS)
 			goto err_close_volume;

Ard Biesheuvel

author maintainer

2020-04-09 13:05 UTC #21 lore raw

PATCH 8/9 efi/x86: Fix the deletion of variables in mixed mode

From: Gary Lin <[email protected]>

efi_thunk_set_variable() treated the NULL "data" pointer as an invalid parameter, and this broke the deletion of variables in mixed mode. This commit fixes the check of data so that the userspace program can delete a variable in mixed mode.

Fixes 8319e9d5ad98ffcc ("efi/x86: Handle by-ref arguments covering multiple pages in mixed mode")

Cc [email protected]

Cc Ard Biesheuvel <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Thomas Gleixner <[email protected]>

Signed-off-by Gary Lin <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 arch/x86/platform/efi/efi_64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/platform/efi/efi_64.c +2 -2

index 211bb9358b73..e0e2e8136cf5 100644
@@ -638,7 +638,7 @@ efi_thunk_set_variable(efi_char16_t *name, efi_guid_t *vendor,
 	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_data = virt_to_phys_or_null_size(data, data_size);
 
-	if (!phys_name || !phys_data)
+	if (!phys_name || (data && !phys_data))
 		status = EFI_INVALID_PARAMETER;
 	else
 		status = efi_thunk(set_variable, phys_name, phys_vendor,

@@ -669,7 +669,7 @@ efi_thunk_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 	phys_vendor = virt_to_phys_or_null(vnd);
 	phys_data = virt_to_phys_or_null_size(data, data_size);
 
-	if (!phys_name || !phys_data)
+	if (!phys_name || (data && !phys_data))
 		status = EFI_INVALID_PARAMETER;
 	else
 		status = efi_thunk(set_variable, phys_name, phys_vendor,

Signature · 1 line

2.17.1

Ard Biesheuvel

author maintainer

2020-04-09 13:05 UTC #22 lore raw

PATCH 9/9 efi/x86: Don't remap text<->rodata gap read-only for mixed mode

Commit

d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")

updated the code that creates the 1:1 memory mapping to use read-only attributes for the 1:1 alias of the kernel's text and rodata sections, to protect it from inadvertent modification. However, it failed to take into account that the unused gap between text and rodata is given to the page allocator for general use.

If the vmap'ed stack happens to be allocated from this region, any by-ref output arguments passed to EFI runtime services that are allocated on the stack (such as the 'datasize' argument taken by GetVariable() when invoked from efivar_entry_size()) will be referenced via a read-only mapping, resulting in a page fault if the EFI code tries to write to it:

  BUG: unable to handle page fault for address: 00000000386aae88
  #PF: supervisor write access in kernel mode
  #PF: error_code(0x0003) - permissions violation
  PGD fd61063 P4D fd61063 PUD fd62063 PMD 386000e1
  Oops: 0003 [#1] SMP PTI
  CPU: 2 PID: 255 Comm: systemd-sysv-ge Not tainted 5.6.0-rc4-default+ #22
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
  RIP: 0008:0x3eaeed95
  Code: ...  <89> 03 be 05 00 00 80 a1 74 63 b1 3e 83 c0 48 e8 44 d2 ff ff eb 05
  RSP: 0018:000000000fd73fa0 EFLAGS: 00010002
  RAX: 0000000000000001 RBX: 00000000386aae88 RCX: 000000003e9f1120
  RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000001
  RBP: 000000000fd73fd8 R08: 00000000386aae88 R09: 0000000000000000
  R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
  R13: ffffc0f040220000 R14: 0000000000000000 R15: 0000000000000000
  FS:  00007f21160ac940(0000) GS:ffff9cf23d500000(0000) knlGS:0000000000000000
  CS:  0008 DS: 0018 ES: 0018 CR0: 0000000080050033
  CR2: 00000000386aae88 CR3: 000000000fd6c004 CR4: 00000000003606e0
  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  Call Trace:
  Modules linked in:
  CR2: 00000000386aae88
  ---[ end trace a8bfbd202e712834 ]---

Let's fix this by remapping text and rodata individually, and leave the gaps mapped read-write.

Fixes d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")

Reported-by Jiri Slaby <[email protected]>

Tested-by Jiri Slaby <[email protected]>

---
 arch/x86/platform/efi/efi_64.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/x86/platform/efi/efi_64.c +10 -2

index e0e2e8136cf5..c5e393f8bb3f 100644
@@ -202,7 +202,7 @@ virt_to_phys_or_null_size(void *va, unsigned long size)
 
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
-	unsigned long pfn, text, pf;
+	unsigned long pfn, text, pf, rodata;
 	struct page *page;
 	unsigned npages;
 	pgd_t *pgd = efi_mm.pgd;

@@ -256,7 +256,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 
 	efi_scratch.phys_stack = page_to_phys(page + 1); /* stack grows down */
 
-	npages = (__end_rodata_aligned - _text) >> PAGE_SHIFT;
+	npages = (_etext - _text) >> PAGE_SHIFT;
 	text = __pa(_text);
 	pfn = text >> PAGE_SHIFT;

@@ -266,6 +266,14 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 		return 1;
 	}
 
+	npages = (__end_rodata - __start_rodata) >> PAGE_SHIFT;
+	rodata = __pa(__start_rodata);
+	pfn = rodata >> PAGE_SHIFT;
+	if (kernel_map_pages_in_pgd(pgd, pfn, rodata, npages, pf)) {
+		pr_err("Failed to map kernel rodata 1:1\n");
+		return 1;
+	}
+
 	return 0;
 }

Signature · 1 line

2.17.1

tip-bot2 for Ard Biesheuvel

merge bot Applied

2020-04-14 08:22 UTC #23 lore raw

[tip: efi/urgent] efi/x86: Don't remap text<->rodata gap read-only for mixed mode

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     f6103162008dfd37567f240b50e5e1ea7cf2e00c
Gitweb:        https://git.kernel.org/tip/f6103162008dfd37567f240b50e5e1ea7cf2e00c
Author:        Ard Biesheuvel <[email protected]>
AuthorDate:    Thu, 09 Apr 2020 15:04:34 +02:00
Committer:     Ingo Molnar <[email protected]>
CommitterDate: Tue, 14 Apr 2020 08:32:17 +02:00

efi/x86: Don't remap text<->rodata gap read-only for mixed mode

Commit

d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")

  BUG: unable to handle page fault for address: 00000000386aae88
  #PF: supervisor write access in kernel mode
  #PF: error_code(0x0003) - permissions violation
  PGD fd61063 P4D fd61063 PUD fd62063 PMD 386000e1
  Oops: 0003 [#1] SMP PTI
  CPU: 2 PID: 255 Comm: systemd-sysv-ge Not tainted 5.6.0-rc4-default+ #22
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
  RIP: 0008:0x3eaeed95
  Code: ...  <89> 03 be 05 00 00 80 a1 74 63 b1 3e 83 c0 48 e8 44 d2 ff ff eb 05
  RSP: 0018:000000000fd73fa0 EFLAGS: 00010002
  RAX: 0000000000000001 RBX: 00000000386aae88 RCX: 000000003e9f1120
  RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000001
  RBP: 000000000fd73fd8 R08: 00000000386aae88 R09: 0000000000000000
  R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
  R13: ffffc0f040220000 R14: 0000000000000000 R15: 0000000000000000
  FS:  00007f21160ac940(0000) GS:ffff9cf23d500000(0000) knlGS:0000000000000000
  CS:  0008 DS: 0018 ES: 0018 CR0: 0000000080050033
  CR2: 00000000386aae88 CR3: 000000000fd6c004 CR4: 00000000003606e0
  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  Call Trace:
  Modules linked in:
  CR2: 00000000386aae88
  ---[ end trace a8bfbd202e712834 ]---

Let's fix this by remapping text and rodata individually, and leave the gaps mapped read-write.

Fixes d9e3d2c4f10320 ("efi/x86: Don't map the entire kernel text RW for mixed mode")

Reported-by Jiri Slaby <[email protected]>

Tested-by Jiri Slaby <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
 arch/x86/platform/efi/efi_64.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/x86/platform/efi/efi_64.c +10 -2

index e0e2e81..c5e393f 100644
@@ -202,7 +202,7 @@ virt_to_phys_or_null_size(void *va, unsigned long size)
 
 int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 {
-	unsigned long pfn, text, pf;
+	unsigned long pfn, text, pf, rodata;
 	struct page *page;
 	unsigned npages;
 	pgd_t *pgd = efi_mm.pgd;

@@ -256,7 +256,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 
 	efi_scratch.phys_stack = page_to_phys(page + 1); /* stack grows down */
 
-	npages = (__end_rodata_aligned - _text) >> PAGE_SHIFT;
+	npages = (_etext - _text) >> PAGE_SHIFT;
 	text = __pa(_text);
 	pfn = text >> PAGE_SHIFT;

@@ -266,6 +266,14 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 		return 1;
 	}
 
+	npages = (__end_rodata - __start_rodata) >> PAGE_SHIFT;
+	rodata = __pa(__start_rodata);
+	pfn = rodata >> PAGE_SHIFT;
+	if (kernel_map_pages_in_pgd(pgd, pfn, rodata, npages, pf)) {
+		pr_err("Failed to map kernel rodata 1:1\n");
+		return 1;
+	}
+
 	return 0;
 }

Theodore Y. Ts'o

2020-04-09 19:01 UTC #24 lore raw

https://github.com/tytso/xfstests-bld/blob/master/kvm-xfstests/test-appliance/files/usr/local/lib/gce-kexec#L146

Quoted 7 lines · Ard Biesheuvel

The following changes since commit 594e576d4b93b8cda3247542366b47e1b2ddc4dc:
efi/libstub/arm: Fix spurious message that an initrd was loaded (2020-03-29 12:08:18 +0200)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-urgent

Hi Ard,

By any chance does this series fix a kexec failure which I bisected down to 0a67361dcdaa ("efi/x86: Remove runtime table address from
kexec EFI setup data")? Or if it doesn't, is this a known failure?

I'm currently building Linus's latest branch to see if it's been fixed since v5.6-11114-g9c94b39560c3 (which is where I first noticed it) and while I was waiting for v5.6-12349-g87ebc45d2d32 to finish building so I could test it, I noticed these patches, and so I figured I'd fire off this quick question.

Thanks,

- Ted

Ard Biesheuvel

author maintainer

2020-04-09 19:04 UTC #25 lore raw

Quoted 8 lines · Theodore Y. Ts'o

Quoted 6 lines · Ard Biesheuvel
The following changes since commit 594e576d4b93b8cda3247542366b47e1b2ddc4dc:
efi/libstub/arm: Fix spurious message that an initrd was loaded (2020-03-29 12:08:18 +0200)
are available in the Git repository at:

Quoted 8 lines

Quoted 1 line
git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-urgent
Hi Ard,
By any chance does this series fix a kexec failure which I bisected down to 0a67361dcdaa ("efi/x86: Remove runtime table address from
kexec EFI setup data")? Or if it doesn't, is this a known failure?

Hi Ted,

I wasn't aware of this issue, and this series will most likely not fix it.

Quoted 6 lines

I'm currently building Linus's latest branch to see if it's been fixed since v5.6-11114-g9c94b39560c3 (which is where I first noticed it) and while I was waiting for v5.6-12349-g87ebc45d2d32 to finish building so I could test it, I noticed these patches, and so I figured I'd fire off this quick question.

I think we might be able to downright revert that patch if the underlying assumption on my part is inaccurate, which was that the fact that the boot code no longer uses the runtime table address implies that there is no longer a reason to pass it.

Please involve me in the discussions on this issue.

Theodore Y. Ts'o

2020-04-09 20:16 UTC #26 lore raw

Quoted 8 lines · Ard Biesheuvel

Quoted 7 lines
I'm currently building Linus's latest branch to see if it's been fixed since v5.6-11114-g9c94b39560c3 (which is where I first noticed it) and while I was waiting for v5.6-12349-g87ebc45d2d32 to finish building so I could test it, I noticed these patches, and so I figured I'd fire off this quick question.

Quoted 4 lines

I think we might be able to downright revert that patch if the underlying assumption on my part is inaccurate, which was that the fact that the boot code no longer uses the runtime table address implies that there is no longer a reason to pass it.

Unfortunately, it doesn't cleanly revert, which is why I started checking to see if it might be fixed already before trying to figure out how to do a manual revert. I just tested and the tip of Linus's tree still has the failure.

The short description of the failure: I'm using Debian Stable (Buster) with a 4.19 distro kernel and kexec-tools 2.0.18 to kexec into the kernel under test. I'm using a Google Compute Engine VM, and the actual kexec command is here:

What happens is that the kexec'ed kernel immediately crashes, at which point we drop back into the BIOS, and then it boots the Debain 4.19.0 distro kernel instead of the kernel to be tested boot. Since we lose the boot command line that was used from the kexec, the gce-xfstests image retries the kexec, which fails, and the failing kexec repeats until I manually kill the VM.

The bisect fingred v5.6-rc1-59-g0a67361dcdaa ("efi/x86: Remove runtime table address from kexec EFI setup data") as the first failing commit. Its immediate parent commit, v5.6-rc1-58-g06c0bd93434c works just fine.

Is there any further debugging information that would be useful?

Thanks,

- Ted

Ard Biesheuvel

author maintainer

2020-04-09 21:29 UTC #27 lore raw

Quoted 34 lines · Theodore Y. Ts'o

Quoted 8 lines · Ard Biesheuvel
Quoted 7 lines
I'm currently building Linus's latest branch to see if it's been fixed since v5.6-11114-g9c94b39560c3 (which is where I first noticed it) and while I was waiting for v5.6-12349-g87ebc45d2d32 to finish building so I could test it, I noticed these patches, and so I figured I'd fire off this quick question.
Quoted 4 lines
I think we might be able to downright revert that patch if the underlying assumption on my part is inaccurate, which was that the fact that the boot code no longer uses the runtime table address implies that there is no longer a reason to pass it.
Unfortunately, it doesn't cleanly revert, which is why I started checking to see if it might be fixed already before trying to figure out how to do a manual revert. I just tested and the tip of Linus's tree still has the failure.
The short description of the failure: I'm using Debian Stable (Buster) with a 4.19 distro kernel and kexec-tools 2.0.18 to kexec into the kernel under test. I'm using a Google Compute Engine VM, and the actual kexec command is here:
https://github.com/tytso/xfstests-bld/blob/master/kvm-xfstests/test-appliance/files/usr/local/lib/gce-kexec#L146
What happens is that the kexec'ed kernel immediately crashes, at which point we drop back into the BIOS, and then it boots the Debain 4.19.0 distro kernel instead of the kernel to be tested boot. Since we lose the boot command line that was used from the kexec, the gce-xfstests image retries the kexec, which fails, and the failing kexec repeats until I manually kill the VM.

Quoted 7 lines

The bisect fingred v5.6-rc1-59-g0a67361dcdaa ("efi/x86: Remove runtime table address from kexec EFI setup data") as the first failing commit. Its immediate parent commit, v5.6-rc1-58-g06c0bd93434c works just fine.
Is there any further debugging information that would be useful?

Does this help at all?

arch/x86/include/asm/efi.h +0 -0

index 781170d36f50..52f8138243df 100644
@@ -180,6 +180,7 @@ extern void __init

efi_uv1_memmap_phys_epilog(pgd_t *save_pgd);

 struct efi_setup_data {
        u64 fw_vendor;
+       u64 __unused;
        u64 tables;
        u64 smbios;
        u64 reserved[8];

Theodore Y. Ts'o

Tested-by

2020-04-09 23:57 UTC #28 lore raw

Quoted 21 lines · Ard Biesheuvel

Quoted 6 lines
What happens is that the kexec'ed kernel immediately crashes, at which point we drop back into the BIOS, and then it boots the Debain 4.19.0 distro kernel instead of the kernel to be tested boot. Since we lose the boot command line that was used from the kexec, the gce-xfstests image retries the kexec, which fails, and the failing kexec repeats until I manually kill the VM.
Does this help at all?
arch/x86/include/asm/efi.h +1 -0
index 781170d36f50..52f8138243df 100644
@@ -180,6 +180,7 @@ extern void __init
 efi_uv1_memmap_phys_epilog(pgd_t *save_pgd);
 
 struct efi_setup_data {
        u64 fw_vendor;
+       u64 __unused;
        u64 tables;
        u64 smbios;

Quoted 1 line

        u64 reserved[8];

Tested-by Theodore Ts'o <[email protected]>

Yep, that fixed it. Thanks!!

I wonder if this structure definition should be moved something like arch/x86/include/uapi/asm/efi.h so it's more obvious that the structure layout is used externally to the kernel?

- Ted

Ard Biesheuvel

author maintainer

2020-04-10 07:08 UTC #29 lore raw

Quoted 18 lines · Theodore Y. Ts'o

Quoted 16 lines · Ard Biesheuvel
Quoted 6 lines
What happens is that the kexec'ed kernel immediately crashes, at which point we drop back into the BIOS, and then it boots the Debain 4.19.0 distro kernel instead of the kernel to be tested boot. Since we lose the boot command line that was used from the kexec, the gce-xfstests image retries the kexec, which fails, and the failing kexec repeats until I manually kill the VM.
Does this help at all?
arch/x86/include/asm/efi.h +0 -0
index 781170d36f50..52f8138243df 100644
@@ -180,6 +180,7 @@ extern void __init
 efi_uv1_memmap_phys_epilog(pgd_t *save_pgd);
 

Quoted 10 lines

Quoted 6 lines

arch/x86/include/asm/efi.h +1 -0

 struct efi_setup_data {
        u64 fw_vendor;
+       u64 __unused;
        u64 tables;
        u64 smbios;

        u64 reserved[8];

Tested-by Theodore Ts'o <[email protected]>

OK, I'll spin a proper patch

Quoted 6 lines

Yep, that fixed it. Thanks!!
I wonder if this structure definition should be moved something like arch/x86/include/uapi/asm/efi.h so it's more obvious that the structure layout is used externally to the kernel?

Well, 95% of the data structures used by EFI are based on the UEFI spec, so the base assumption is really that we cannot make changes like these to begin with. But I'll add a DON'T TOUCH comment here in any case.

Dave Young

2020-04-10 13:55 UTC #30 lore raw

Cc kexec list.

Quoted 39 lines · Ard Biesheuvel

Quoted 18 lines · Theodore Y. Ts'o
Quoted 16 lines · Ard Biesheuvel
Quoted 6 lines
What happens is that the kexec'ed kernel immediately crashes, at which point we drop back into the BIOS, and then it boots the Debain 4.19.0 distro kernel instead of the kernel to be tested boot. Since we lose the boot command line that was used from the kexec, the gce-xfstests image retries the kexec, which fails, and the failing kexec repeats until I manually kill the VM.
Does this help at all?
arch/x86/include/asm/efi.h +0 -0
index 781170d36f50..52f8138243df 100644
@@ -180,6 +180,7 @@ extern void __init
 efi_uv1_memmap_phys_epilog(pgd_t *save_pgd);
 
Quoted 10 lines
Quoted 6 lines
arch/x86/include/asm/efi.h +1 -0
 struct efi_setup_data {
        u64 fw_vendor;
+       u64 __unused;
        u64 tables;
        u64 smbios;
        u64 reserved[8];
Tested-by Theodore Ts'o <[email protected]>
OK, I'll spin a proper patch
Quoted 6 lines
Yep, that fixed it. Thanks!!
I wonder if this structure definition should be moved something like arch/x86/include/uapi/asm/efi.h so it's more obvious that the structure layout is used externally to the kernel?

Quoted 5 lines

Well, 95% of the data structures used by EFI are based on the UEFI spec, so the base assumption is really that we cannot make changes like these to begin with. But I'll add a DON'T TOUCH comment here in any case.

The runtime cleanup looks a very good one, but I also missed that, userspace kexec-tools will break with the efi setup_data changes. But kexec_file_load will just work with the cleanup applied.

Ard, could you add kexec list in cc when you send the fix out?

Thanks
Dave

Theodore Y. Ts'o

2020-04-11 19:44 UTC #31 lore raw

Quoted 4 lines · Dave Young

The runtime cleanup looks a very good one, but I also missed that, userspace kexec-tools will break with the efi setup_data changes. But kexec_file_load will just work with the cleanup applied.

Hmmm, I wonder if there could be some kselftest or kunit tests that would make it easier to pick up these sorts of regressions earlier?

- Ted

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Dave Young

2020-04-12 03:51 UTC #32 lore raw

Quoted 8 lines · Theodore Y. Ts'o

Quoted 4 lines · Dave Young
The runtime cleanup looks a very good one, but I also missed that, userspace kexec-tools will break with the efi setup_data changes. But kexec_file_load will just work with the cleanup applied.
Hmmm, I wonder if there could be some kselftest or kunit tests that would make it easier to pick up these sorts of regressions earlier?

I thought about that before, but did not go with any actual actions. kexec test needs a system reboot, Kdump is even harder to test, that is the reason I hesitated about.

But since the breakage happens here and there frequently, it is time to try it. I think I will play with it, but I might be slow because of other things, welcome to post patches if anyone is interested :)

Thanks
Dave

David Howells

Tested-by

2020-04-13 14:07 UTC #33 lore raw