diff mbox series

x86/efistub: Add missing boot_params for mixed mode compat entry

Message ID 20240325083905.13163-2-ardb+git@google.com
State New
Headers show
Series x86/efistub: Add missing boot_params for mixed mode compat entry | expand

Commit Message

Ard Biesheuvel March 25, 2024, 8:39 a.m. UTC 
From: Ard Biesheuvel <ardb@kernel.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@redhat.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@craftyguy.net>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/efi_mixed.S | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Hans de Goede March 25, 2024, 1:18 p.m. UTC | #1 
Hi,

On 3/25/24 9:39 AM, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.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@redhat.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@craftyguy.net>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.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@redhat.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
Ard Biesheuvel March 25, 2024, 1:22 p.m. UTC | #2 
On Mon, 25 Mar 2024 at 15:18, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 3/25/24 9:39 AM, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.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@redhat.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@craftyguy.net>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.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@redhat.com>
>

Many thanks Hans.
Clayton Craft March 25, 2024, 5:39 p.m. UTC | #3 
On Mon, 25 Mar 2024 14:18:01 +0100 Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
> 
> On 3/25/24 9:39 AM, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.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@redhat.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@craftyguy.net>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.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:

I did the same test (with 6.9-rc1) on my Bay Trail tablet & NUC that failed
previously, and this fixes booting with EFI mixed mode on them.

Tested-by: Clayton Craft <clayton@craftyguy.net>

-Clayton
Ard Biesheuvel March 25, 2024, 5:41 p.m. UTC | #4 
On Mon, 25 Mar 2024 at 19:39, Clayton Craft <clayton@craftyguy.net> wrote:
>
> On Mon, 25 Mar 2024 14:18:01 +0100 Hans de Goede <hdegoede@redhat.com> wrote:
> > Hi,
> >
> > On 3/25/24 9:39 AM, Ard Biesheuvel wrote:
> > > From: Ard Biesheuvel <ardb@kernel.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@redhat.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@craftyguy.net>
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.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:
>
> I did the same test (with 6.9-rc1) on my Bay Trail tablet & NUC that failed
> previously, and this fixes booting with EFI mixed mode on them.
>
> Tested-by: Clayton Craft <clayton@craftyguy.net>
>

Thanks for testing again.

I'll get this to Linus in the next week or so, and the fix should make
its way back through the stable trees in the following weeks.
diff mbox series

Patch

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