Re: [PATCH] efi/x86: use naked RET on mixed mode call wrapper

[Ard Biesheuvel]

On Mon, 18 Jul 2022 at 20:46, Borislav Petkov <bp@xxxxxxx> wrote:
>
> On Mon, Jul 18, 2022 at 11:34:02AM -0700, Linus Torvalds wrote:
> > Why would we have to protect the kernel from EFI?
>
> Yes, we cleared this up on IRC in the meantime.
>
> This was raised as a concern in case we don't trust EFI. But we cannot
> not (double negation on purpose) trust EFI because it can do whatever it
> likes anyway, "underneath" the OS.
>
> I'm keeping the UNTRAIN_RET-in-C diff in my patches/ folder, though - I
> get the feeling we might need it soon for something else.
>
> :-)
>

The code in question is a little trampoline that executes from the EFI
mixed mode 1:1 mapping of the kernel text, and never via the kernel
mapping, so we should just move it into .rodata instead (and fix up
the mixed mode virtual address map logic accordingly). I don't think
mapping the kernel text and rodata into the 1;1 EFI map is needed at
all, tbh, and the only thing we ever access via that mapping is that
little trampoline.

Something like

--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -176,7 +176,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, rodata;
+       unsigned long pfn, pf, rodata;
        struct page *page;
        unsigned npages;
        pgd_t *pgd = efi_mm.pgd;
@@ -222,7 +222,7 @@ int __init efi_setup_page_tables(unsigned long
pa_memmap, unsigned num_pages)
        /*
         * When making calls to the firmware everything needs to be 1:1
         * mapped and addressable with 32-bit pointers. Map the kernel
-        * text and allocate a new stack because we can't rely on the
+        * rodata and allocate a new stack because we can't rely on the
         * stack pointer being < 4GB.
         */
        if (!efi_is_mixed())
@@ -236,21 +236,11 @@ int __init efi_setup_page_tables(unsigned long
pa_memmap, unsigned num_pages)

        efi_mixed_mode_stack_pa = page_to_phys(page + 1); /* stack grows down */

-       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");
-               return 1;
-       }
-
        npages = (__end_rodata - __start_rodata) >> PAGE_SHIFT;
        rodata = __pa(__start_rodata);
        pfn = rodata >> PAGE_SHIFT;

-       pf = _PAGE_NX | _PAGE_ENC;
+       pf = _PAGE_ENC;
        if (kernel_map_pages_in_pgd(pgd, pfn, rodata, npages, pf)) {
                pr_err("Failed to map kernel rodata 1:1\n");
                return 1;
diff --git a/arch/x86/platform/efi/efi_thunk_64.S
b/arch/x86/platform/efi/efi_thunk_64.S
index 854dd81804b7..d4ee75ebfad6 100644
--- a/arch/x86/platform/efi/efi_thunk_64.S
+++ b/arch/x86/platform/efi/efi_thunk_64.S
@@ -71,7 +71,9 @@ STACK_FRAME_NON_STANDARD __efi64_thunk
        pushq   $__KERNEL32_CS
        pushq   %rdi                    /* EFI runtime service address */
        lretq
+SYM_FUNC_END(__efi64_thunk)

+       .section ".rodata", "a"
 1:     movq    0x20(%rsp), %rsp
        pop     %rbx
        pop     %rbp
@@ -81,7 +83,6 @@ STACK_FRAME_NON_STANDARD __efi64_thunk
 2:     pushl   $__KERNEL_CS
        pushl   %ebp
        lret
-SYM_FUNC_END(__efi64_thunk)

        .bss
        .balign 8