Message-ID: <5b32b8c8-0a1b-44cf-aedf-cca1d6c83bd8@redhat.com>
Date: Mon, 25 Mar 2024 14:18:01 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Ard Biesheuvel <ardb+git@...gle.com>, linux-efi@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Ard Biesheuvel <ardb@...nel.org>,
 Clayton Craft <clayton@...ftyguy.net>
Subject: Re: [PATCH] x86/efistub: Add missing boot_params for mixed mode
 compat entry

Hi,

On 3/25/24 9:39 AM, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@...nel.org>
> 
> 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@...hat.com>
> Fixes: e2ab9eab324c ("x86/boot/compressed: Move 32-bit entrypoint code into .text section")
> Closes: https://lore.kernel.org/all/20240321150510.GI8211@craftyguy.net/
> Reported-by: Clayton Craft <clayton@...ftyguy.net>
> Signed-off-by: Ard Biesheuvel <ardb@...nel.org>

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@...hat.com>

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

Powered by blists - more mailing lists