Hi,
On 3/25/24 9:39 AM, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
>
> The native EFI stub entry point does not take a struct boot_params from
> the boot loader, but creates it from scratch, and populates only the
> fields that still have meaning in this context (command line, initrd
> base and size, etc)
>
> The original mixed mode implementation used the EFI handover protocol,
> where the boot loader (i.e., GRUB) populates a struct boot_params and
> passes it to a special EFI entry point that takes the struct boot_params
> pointer as the third argument.
>
> When the new mixed mode implementation was introduced, using a special
> 32-bit PE entrypoint in the 64-bit kernel, it adopted the usual
> prototype, and relied on the EFI stub to create the struct boot_params
> as usual. This is preferred because it makes the bootloader side much
> easier to implement, as it does not need any x86-specific knowledge on
> how struct boot_params and struct setup_header are put together.
>
> However, one thing was missed: EFI mixed mode goes through startup_32()
> *before* entering the 64-bit EFI stub, which is difficult to avoid given
> that 64-bit execution requires page tables, which can only be populated
> using 32-bit code, and this piece is what the mixed mode EFI stub relies
> on. startup_32() accesses a couple of struct boot_params fields to
> decide where to place the page tables.
>
> startup_32() turns out to be quite tolerant to bogus struct boot_params,
> given that ESI used to contain junk when entering via the new mixed mode
> protocol. Only when commit
>
> e2ab9eab324c ("x86/boot/compressed: Move 32-bit entrypoint code into .text section")
>
> started to zero ESI explicitly when entering via this boot path, boot
> failures started to appear on some systems, presumably ones that unmap
> page 0x0 or map it read-only.
>
> The solution is to pass a special, temporary struct boot_params to
> startup_32() via ESI, one that is sufficient for getting it to create
> the page tables correctly and is discarded right after. This means
> setting a minimal alignment of 4k, only to get the statically allocated
> page tables line up correctly, and setting init_size to the executable
> image size (_end - startup_32). This ensures that the page tables are
> covered by the static footprint of the PE image.
>
> Given that EFI boot no longer calls the decompressor and no longer pads
> the image to permit the decompressor to execute in place, the same
> temporary struct boot_params should be used in the EFI handover protocol
> based mixed mode implementation as well, to prevent the page tables from
> being placed outside of allocated memory.
>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Fixes: e2ab9eab324c ("x86/boot/compressed: Move 32-bit entrypoint code into .text section")
> Closes: https://lore.kernel.org/all/20240321150510.GI8211@xxxxxxxxxxxxx/
> Reported-by: Clayton Craft <clayton@xxxxxxxxxxxxx>
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
I have given this a test run (on top of 6.9-rc1) on one of my
Bay Trail mixed mode tablets and the tablet still boots fine:
Tested-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Regards,
Hans
> ---
> arch/x86/boot/compressed/efi_mixed.S | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/efi_mixed.S b/arch/x86/boot/compressed/efi_mixed.S
> index 07873f269b7b..c7c108c0bcf0 100644
> --- a/arch/x86/boot/compressed/efi_mixed.S
> +++ b/arch/x86/boot/compressed/efi_mixed.S
> @@ -15,10 +15,12 @@
> */
>
> #include <linux/linkage.h>
> +#include <asm/asm-offsets.h>
> #include <asm/msr.h>
> #include <asm/page_types.h>
> #include <asm/processor-flags.h>
> #include <asm/segment.h>
> +#include <asm/setup.h>
>
> .code64
> .text
> @@ -155,6 +157,7 @@ SYM_FUNC_END(__efi64_thunk)
> SYM_FUNC_START(efi32_stub_entry)
> call 1f
> 1: popl %ecx
> + leal (efi32_boot_args - 1b)(%ecx), %ebx
>
> /* Clear BSS */
> xorl %eax, %eax
> @@ -169,6 +172,7 @@ SYM_FUNC_START(efi32_stub_entry)
> popl %ecx
> popl %edx
> popl %esi
> + movl %esi, 8(%ebx)
> jmp efi32_entry
> SYM_FUNC_END(efi32_stub_entry)
> #endif
> @@ -245,8 +249,6 @@ SYM_FUNC_END(efi_enter32)
> *
> * Arguments: %ecx image handle
> * %edx EFI system table pointer
> - * %esi struct bootparams pointer (or NULL when not using
> - * the EFI handover protocol)
> *
> * Since this is the point of no return for ordinary execution, no registers
> * are considered live except for the function parameters. [Note that the EFI
> @@ -272,9 +274,18 @@ SYM_FUNC_START_LOCAL(efi32_entry)
> leal (efi32_boot_args - 1b)(%ebx), %ebx
> movl %ecx, 0(%ebx)
> movl %edx, 4(%ebx)
> - movl %esi, 8(%ebx)
> movb $0x0, 12(%ebx) // efi_is64
>
> + /*
> + * Allocate some memory for a temporary struct boot_params, which only
> + * needs the minimal pieces that will get us through startup_32().
> + */
> + subl $PARAM_SIZE, %esp
> + movl %esp, %esi
> + movl $PAGE_SIZE, BP_kernel_alignment(%esi)
> + movl $_end - 1b, BP_init_size(%esi)
> + subl $startup_32 - 1b, BP_init_size(%esi)
> +
> /* Disable paging */
> movl %cr0, %eax
> btrl $X86_CR0_PG_BIT, %eax
> @@ -300,8 +311,7 @@ SYM_FUNC_START(efi32_pe_entry)
>
> movl 8(%ebp), %ecx // image_handle
> movl 12(%ebp), %edx // sys_table
> - xorl %esi, %esi
> - jmp efi32_entry // pass %ecx, %edx, %esi
> + jmp efi32_entry // pass %ecx, %edx
> // no other registers remain live
>
> 2: popl %edi // restore callee-save registers
