efi/x86-mixed: move unmitigated RET into .rodata

PATCH In review Ard Biesheuvel 3 participants 5 messages Aug 15, 2022 – Aug 16, 2022 2 messages not in the archive

Subsystems (2)

x86 EFI

lore

Share & tools

lore

Ard Biesheuvel

author maintainer

2022-08-15 13:21 UTC #0 lore raw

PATCH 1/1 efi/x86-mixed: move unmitigated RET into .rodata

Move the EFI mixed mode return trampoline RET into .rodata, so it is normally mapped without executable permissions. And given that this snippet of code is really the only kernel code that we ever execute via this 1:1 mapping, let's unmap the 1:1 mapping of the kernel .text, and only map the page that covers the return trampoline with executable permissions.

Note that the remainder of .rodata needs to remain mapped into the 1:1 mapping with RO/NX permissions, as literal GUIDs and strings may be passed to the variable routines.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/platform/efi/efi_64.c       | 18 +++++++++++++-----
 arch/x86/platform/efi/efi_thunk_64.S |  8 +++++---
 2 files changed, 18 insertions(+), 8 deletions(-)

arch/x86/platform/efi/efi_64.c +13 -5

index 1f3675453a57..b36596bf0fc3 100644
@@ -176,7 +176,8 @@ 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, rodata;
+	extern const u8 __efi64_thunk_ret_tramp[];
+	unsigned long pfn, text, pf, rodata, tramp;
 	struct page *page;
 	unsigned npages;
 	pgd_t *pgd = efi_mm.pgd;

@@ -238,11 +239,9 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 
 	npages = (_etext - _text) >> PAGE_SHIFT;
 	text = __pa(_text);
-	pfn = text >> PAGE_SHIFT;
 
-	pf = _PAGE_ENC;
-	if (kernel_map_pages_in_pgd(pgd, pfn, text, npages, pf)) {
-		pr_err("Failed to map kernel text 1:1\n");
+	if (kernel_unmap_pages_in_pgd(pgd, text, npages)) {
+		pr_err("Failed to unmap kernel text 1:1 mapping\n");
 		return 1;
 	}

@@ -256,6 +255,15 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 		return 1;
 	}
 
+	tramp = __pa(__efi64_thunk_ret_tramp);
+	pfn = tramp >> PAGE_SHIFT;
+
+	pf = _PAGE_ENC;
+	if (kernel_map_pages_in_pgd(pgd, pfn, tramp, 1, pf)) {
+		pr_err("Failed to map mixed mode return trampoline\n");
+		return 1;
+	}
+
 	return 0;
 }

arch/x86/platform/efi/efi_thunk_64.S +5 -3

index 4e5257a4811b..e436ce03741e 100644
@@ -23,7 +23,6 @@
 #include <linux/objtool.h>
 #include <asm/page_types.h>
 #include <asm/segment.h>
-#include <asm/nospec-branch.h>
 
 	.text
 	.code64

@@ -72,11 +71,14 @@ STACK_FRAME_NON_STANDARD __efi64_thunk
 	pushq	$__KERNEL32_CS
 	pushq	%rdi			/* EFI runtime service address */
 	lretq
+SYM_FUNC_END(__efi64_thunk)
 
+	.section ".rodata", "a", @progbits
+	.balign	16
+SYM_DATA_START(__efi64_thunk_ret_tramp)
 1:	movq	0x20(%rsp), %rsp
 	pop	%rbx
 	pop	%rbp
-	ANNOTATE_UNRET_SAFE
 	ret
 	int3

@@ -84,7 +86,7 @@ STACK_FRAME_NON_STANDARD __efi64_thunk
 2:	pushl	$__KERNEL_CS
 	pushl	%ebp
 	lret
-SYM_FUNC_END(__efi64_thunk)
+SYM_DATA_END(__efi64_thunk_ret_tramp)
 
 	.bss
 	.balign 8

Signature · 1 line

2.35.1

Borislav Petkov

maintainer Acked-by on v1 1/1

2022-08-15 22:28 UTC #1 lore raw

Quoted 11 lines · Ard Biesheuvel

Move the EFI mixed mode return trampoline RET into .rodata, so it is normally mapped without executable permissions. And given that this snippet of code is really the only kernel code that we ever execute via this 1:1 mapping, let's unmap the 1:1 mapping of the kernel .text, and only map the page that covers the return trampoline with executable permissions.
Note that the remainder of .rodata needs to remain mapped into the 1:1 mapping with RO/NX permissions, as literal GUIDs and strings may be passed to the variable routines.

Quoted 5 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/platform/efi/efi_64.c       | 18 +++++++++++++-----
 arch/x86/platform/efi/efi_thunk_64.S |  8 +++++---
 2 files changed, 18 insertions(+), 8 deletions(-)

Acked-by Borislav Petkov <[email protected]>

For some reason, objtool is not happy here:

vmlinux.o: warning: objtool: efi_thunk_query_variable_info_nonblocking+0x1ba: unreachable instruction

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Ard Biesheuvel

author maintainer

2022-08-15 23:03 UTC #2 lore raw

Quoted 19 lines · Borislav Petkov

Quoted 11 lines · Ard Biesheuvel
Move the EFI mixed mode return trampoline RET into .rodata, so it is normally mapped without executable permissions. And given that this snippet of code is really the only kernel code that we ever execute via this 1:1 mapping, let's unmap the 1:1 mapping of the kernel .text, and only map the page that covers the return trampoline with executable permissions.
Note that the remainder of .rodata needs to remain mapped into the 1:1 mapping with RO/NX permissions, as literal GUIDs and strings may be passed to the variable routines.
Quoted 5 lines
Signed-off-by Ard Biesheuvel <[email protected]>
---
 arch/x86/platform/efi/efi_64.c       | 18 +++++++++++++-----
 arch/x86/platform/efi/efi_thunk_64.S |  8 +++++---
 2 files changed, 18 insertions(+), 8 deletions(-)

Quoted 6 lines

Acked-by Borislav Petkov <[email protected]>
For some reason, objtool is not happy here:
vmlinux.o: warning: objtool: efi_thunk_query_variable_info_nonblocking+0x1ba: unreachable instruction

I'm not seeing that warning. Any config in particular beyond x86_64_defconfig that you have enabled?
I'm using Debian GCC 12.1.0 btw

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): CAMj1kXHUyAZQqLbT8uqKJTb9QHq1A4ZUbVnx+J0yQfZihJGjbg@mail.gmail.com

Peter Zijlstra

2022-08-16 10:51 UTC #5 lore raw

Quoted 28 lines · Ard Biesheuvel

(cc Peter and Josh)
Quoted 11 lines · Borislav Petkov
Quoted 2 lines · Ard Biesheuvel
I'm not seeing that warning. Any config in particular beyond x86_64_defconfig that you have enabled?
It is my workstation's tailored config. Attached.
Quoted 1 line
I'm using Debian GCC 12.1.0 btw
gcc (Debian 11.2.0-19) 11.2.0
Complete thread here:
https://lore.kernel.org/all/[email protected]/
Quoted 3 lines · Borislav Petkov
For some reason, objtool is not happy here:
vmlinux.o: warning: objtool: efi_thunk_query_variable_info_nonblocking+0x1ba: unreachable instruction
[which is the instruction right after the call to __efi_thunk64()]
However, with the same config but without the patch (i.e., v6.0-rc1 with nothing on top), I see:

Quoted 6 lines

vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction
It appears that objtool is making inferences about whether or not __efi_thunk64() returns, even though it is marked as STACK_FRAME_NON_STANDARD. And note that I am not seeing any of these with x86_64_defconfig, only with Boris's config (attached)

STACK_FRAME_NON_STANDARD has no bearing on a call to that symbol being noreturn or not.

noreturn is a bit of a pain point in that the compiler leaves no clue in the object file. Objtool has a bunch of heuristics to guess at noreturn, but the only surefire way to make things consistent is to annotate the function with __noreturn and add it to the global_noreturns[] array in tools/objtool/check.c

Alternatively, if objtool guesses wrong, you can annotate the assembler with 'REACHABLE'.

Josh; should we create a config file for objtool to contain many of this stuff? Then again, parsing a config file over and over and over again isn't going to make it faster :/

Ard Biesheuvel

author maintainer

2022-08-16 11:02 UTC #6 lore raw

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines · Ard Biesheuvel
(cc Peter and Josh)
On Mon, 15 Aug 2022 at 22:18, Borislav Petkov <[email protected]> wrote:

...

Quoted 18 lines

Quoted 8 lines
Quoted 2 lines
For some reason, objtool is not happy here:
vmlinux.o: warning: objtool: efi_thunk_query_variable_info_nonblocking+0x1ba: unreachable instruction
[which is the instruction right after the call to __efi_thunk64()]
However, with the same config but without the patch (i.e., v6.0-rc1 with nothing on top), I see:
Quoted 6 lines
vmlinux.o: warning: objtool: sme_enable+0x71: unreachable instruction
It appears that objtool is making inferences about whether or not __efi_thunk64() returns, even though it is marked as STACK_FRAME_NON_STANDARD. And note that I am not seeing any of these with x86_64_defconfig, only with Boris's config (attached)
STACK_FRAME_NON_STANDARD has no bearing on a call to that symbol being noreturn or not.

Quoted 9 lines

noreturn is a bit of a pain point in that the compiler leaves no clue in the object file. Objtool has a bunch of heuristics to guess at noreturn, but the only surefire way to make things consistent is to annotate the function with __noreturn and add it to the global_noreturns[] array in tools/objtool/check.c
Alternatively, if objtool guesses wrong, you can annotate the assembler with 'REACHABLE'.

So the problem here is that the function is *not* __noreturn, and objtool thinks it is, resulting in the instruction after the call to be misidentified as unreachable.
The function is called from C code, so we cannot easily annotate the instruction after the call as REACHABLE.

I've patched it up by adding a dummy, unreachable RET into __efi64_thunk(), which seems to make objtool happy for now.