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

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

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

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/efi_64.c       | 18 +++++++++++++-----
 arch/x86/platform/efi/efi_thunk_64.S |  8 +++++---
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 1f3675453a57..b36596bf0fc3 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -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;
 }
 
diff --git a/arch/x86/platform/efi/efi_thunk_64.S b/arch/x86/platform/efi/efi_thunk_64.S
index 4e5257a4811b..e436ce03741e 100644
--- a/arch/x86/platform/efi/efi_thunk_64.S
+++ b/arch/x86/platform/efi/efi_thunk_64.S
@@ -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
-- 
2.35.1

Re: [PATCH] efi/x86-mixed: move unmitigated RET into .rodata

[Borislav Petkov]

On Mon, Aug 15, 2022 at 03:20:28PM +0200, Ard Biesheuvel wrote:
> 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 <ardb@kernel.org>
> ---
>  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 <bp@suse.de>

For some reason, objtool is not happy here:

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

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] efi/x86-mixed: move unmitigated RET into .rodata

[Ard Biesheuvel]

On Mon, 15 Aug 2022 at 21:46, Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Aug 15, 2022 at 03:20:28PM +0200, Ard Biesheuvel wrote:
> > 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 <ardb@kernel.org>
> > ---
> >  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 <bp@suse.de>
>
> 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

Re: [PATCH] efi/x86-mixed: move unmitigated RET into .rodata

[Peter Zijlstra]

On Tue, Aug 16, 2022 at 09:04:56AM +0200, Ard Biesheuvel wrote:
> (cc Peter and Josh)
> 
> On Mon, 15 Aug 2022 at 22:18, Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Mon, Aug 15, 2022 at 09:57:50PM +0200, Ard Biesheuvel wrote:
> > > 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.
> >
> > > 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/20220815132028.732531-1-ardb@kernel.org/
> 
> On Mon, 15 Aug 2022 at 22:18, Borislav Petkov <bp@alien8.de> wrote:
> >
> > 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:
> 
> 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 :/

Re: [PATCH] efi/x86-mixed: move unmitigated RET into .rodata

[Ard Biesheuvel]

On Tue, 16 Aug 2022 at 12:17, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Aug 16, 2022 at 09:04:56AM +0200, Ard Biesheuvel wrote:
> > (cc Peter and Josh)
> >
> > On Mon, 15 Aug 2022 at 22:18, Borislav Petkov <bp@alien8.de> wrote:
...
> > > 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:
> >
> > 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'.
>

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.