| Message ID | 20220815132028.732531-1-ardb@kernel.org |
|---|---|
| State | Accepted |
| Commit | 6c3a9c9ae02a16295ea144dc431aaac2c20dbffd |
| Headers | show |
| Series | efi/x86-mixed: move unmitigated RET into .rodata | expand |
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
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
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 :/
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.
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
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(-)