[PATCH 0/2] EFI mixed mode fixes

[PATCH 0/2] EFI mixed mode fixes

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

Two fixes for mixed mode:
- run EFI boot services from the firmware's stack, to avoid overruns
- don't wipe BSS twice in mixed mode

Whether or not this fixes Clayton's issue remains to be seen, but these
need to be fixed in any case.

Link: https://lkml.kernel.org/r/20240321144541.GD8211%40craftyguy.net

Cc: Clayton Craft <clayton@craftyguy.net>

Ard Biesheuvel (2):
  x86/efistub: Call mixed mode boot services on the firmware's stack
  x86/efistub: Don't clear BSS twice in mixed mode

 arch/x86/boot/compressed/efi_mixed.S    | 10 +++++++++-
 drivers/firmware/efi/libstub/x86-stub.c |  3 ++-
 2 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.44.0.396.g6e790dbe36-goog

[PATCH 1/2] x86/efistub: Call mixed mode boot services on the firmware's stack

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

Normally, the EFI stub calls into the EFI boot services using the stack
that was live when the stub was entered. According to the UEFI spec,
this stack needs to be at least 128k in size - this might seem large but
all asynchronous processing and event handling in EFI runs from the same
stack and so quite a lot of space may be used in practice.

In mixed mode, the situation is a bit different: the bootloader calls
the 32-bit EFI stub entry point, which calls the decompressor's 32-bit
entry point, where the boot stack is set up, using a fixed allocation
of 16k. This stack is still in use when the EFI stub is started in
64-bit mode, and so all calls back into the EFI firmware will be using
the decompressor's limited boot stack.

Due to the placement of the boot stack right after the boot heap, any
stack overruns have gone unnoticed. However, commit

  5c4feadb0011983b ("x86/decompressor: Move global symbol references to C code")

moved the definition of the boot heap into C code, and now the boot
stack is placed right at the base of BSS, where any overruns will
corrupt the end of the .data section.

While it would be possible to work around this by increasing the size of
the boot stack, doing so would affect all x86 systems, and mixed mode
systems are a tiny (and shrinking) fraction of the x86 installed base.

So instead, record the firmware stack pointer value when entering from
the 32-bit firmware, and switch to this stack every time a EFI boot
service call is made.

Cc: <stable@kernel.org> # v6.1+
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/efi_mixed.S | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/efi_mixed.S b/arch/x86/boot/compressed/efi_mixed.S
index f4e22ef774ab..c21afc9c84f1 100644
--- a/arch/x86/boot/compressed/efi_mixed.S
+++ b/arch/x86/boot/compressed/efi_mixed.S
@@ -83,6 +83,10 @@ SYM_FUNC_START(__efi64_thunk)
 	movq	0x38(%rsp), %rbx
 	movq	0x40(%rsp), %rax
 
+	/* Switch to the firmware's stack */
+	movq	%rsp, %r11
+	movl	efi32_boot_sp(%rip), %esp
+
 	/*
 	 * Convert x86-64 ABI params to i386 ABI
 	 */
@@ -120,7 +124,7 @@ SYM_FUNC_START(__efi64_thunk)
 	pushq	%rax
 	lretq
 
-1:	addq	$64, %rsp
+1:	movq	%r11, %rsp
 	movq	%rdi, %rax
 
 	pop	%rbx
@@ -254,6 +258,9 @@ SYM_FUNC_START_LOCAL(efi32_entry)
 	/* Store firmware IDT descriptor */
 	sidtl	(efi32_boot_idt - 1b)(%ebx)
 
+	/* Store firmware stack pointer */
+	movl	%esp, (efi32_boot_sp - 1b)(%ebx)
+
 	/* Store boot arguments */
 	leal	(efi32_boot_args - 1b)(%ebx), %ebx
 	movl	%ecx, 0(%ebx)
@@ -318,5 +325,6 @@ SYM_DATA_END(efi32_boot_idt)
 
 SYM_DATA_LOCAL(efi32_boot_cs, .word 0)
 SYM_DATA_LOCAL(efi32_boot_ds, .word 0)
+SYM_DATA_LOCAL(efi32_boot_sp, .long 0)
 SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0, 0)
 SYM_DATA(efi_is64, .byte 1)
-- 
2.44.0.396.g6e790dbe36-goog

[PATCH 2/2] x86/efistub: Don't clear BSS twice in mixed mode

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

Clearing BSS should only be done once, at the very beginning.
efi_pe_entry() is the entrypoint from the firmware, which may not clear
BSS and so it is done explicitly. However, efi_pe_entry() is also used
as an entrypoint by the mixed mode startup code, in which case BSS will
already have been cleared, and doing it again at this point will corrupt
global variables holding the firmware's GDT/IDT and segment selectors.

So make the memset() conditional on whether the EFI stub is running in
native mode.

Fixes: b3810c5a2cc4a666 ("x86/efistub: Clear decompressor BSS in native EFI entrypoint")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/x86-stub.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 2096ae09438e..1edf93e63897 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -476,7 +476,8 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	efi_status_t status;
 	char *cmdline_ptr;
 
-	memset(_bss, 0, _ebss - _bss);
+	if (efi_is_native())
+		memset(_bss, 0, _ebss - _bss);
 
 	efi_system_table = sys_table_arg;
 
-- 
2.44.0.396.g6e790dbe36-goog