[PATCH 0/1] efi/x86: Use firmware stack for mixed-mode EFI stub

[PATCH 0/1] efi/x86: Use firmware stack for mixed-mode EFI stub

[Arvind Sankar]

This patch is on top of the series removing runtime relocs [1].

[1] https://lore.kernel.org/lkml/20200525225918.1624470-1-nivedita@alum.mit.edu/

Arvind Sankar (1):
  efi/x86: Use firmware stack for mixed-mode EFI stub

 arch/x86/boot/compressed/head_64.S | 46 ++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 15 deletions(-)

-- 
2.26.2

[PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub

[Arvind Sankar]

The UEFI specification requires a 128KiB stack during boot services. On
a native mode boot, the EFI stub executes on the firmware stack.
However, on a mixed-mode boot, startup_32 switches to the kernel's boot
stack, which is only 16KiB, and the EFI stub is executed with this
stack.

To avoid any potential problems with running out of stack space, save
and restore the UEFI stack pointer in the mixed-mode entry, so that the
EFI stub can use the firmware stack in this case as well.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_64.S | 46 ++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 4b7ad1dfbea6..923e5c381804 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -237,21 +237,7 @@ SYM_FUNC_START(startup_32)
 	 * used to perform that far jump.
 	 */
 	pushl	$__KERNEL_CS
-	leal	rva(startup_64)(%ebp), %eax
-#ifdef CONFIG_EFI_MIXED
-	movl	rva(efi32_boot_args)(%ebp), %edi
-	cmp	$0, %edi
-	jz	1f
-	leal	rva(efi64_stub_entry)(%ebp), %eax
-	movl	rva(efi32_boot_args+4)(%ebp), %esi
-	movl	rva(efi32_boot_args+8)(%ebp), %edx	// saved bootparams pointer
-	cmpl	$0, %edx
-	jnz	1f
-	leal	rva(efi_pe_entry)(%ebp), %eax
-	movl	%edi, %ecx			// MS calling convention
-	movl	%esi, %edx
-1:
-#endif
+	leal	rva(.L64bit)(%ebp), %eax
 	pushl	%eax
 
 	/* Enter paged protected Mode, activating Long Mode */
@@ -260,6 +246,31 @@ SYM_FUNC_START(startup_32)
 
 	/* Jump from 32bit compatibility mode into 64bit mode. */
 	lret
+
+	.code64
+SYM_INNER_LABEL(.L64bit, SYM_L_LOCAL)
+#ifndef CONFIG_EFI_MIXED
+	jmp	startup_64
+#else
+	movl	efi32_boot_args(%rip), %edi
+	testl	%edi, %edi
+	jz	startup_64
+
+	/* Restore firmware stack pointer */
+	movl	efi32_boot_sp(%rip), %esp
+	/* and align it to 8 mod 16 */
+	andl	$~0xf, %esp
+	subl	$8, %esp
+
+	movl	efi32_boot_args+4(%rip), %esi
+	movl	efi32_boot_args+8(%rip), %edx	// saved bootparams pointer
+	testl	%edx, %edx
+	jnz	efi64_stub_entry
+	movl	%edi, %ecx			// MS calling convention
+	movl	%esi, %edx
+	jmp	efi_pe_entry
+#endif
+	.code32
 SYM_FUNC_END(startup_32)
 
 #ifdef CONFIG_EFI_MIXED
@@ -285,6 +296,9 @@ SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL)
 	movw	%cs, rva(efi32_boot_cs)(%ebp)
 	movw	%ds, rva(efi32_boot_ds)(%ebp)
 
+	/* Save firmware stack pointer */
+	movl	%esp, rva(efi32_boot_sp)(%ebp)
+
 	/* Disable paging */
 	movl	%cr0, %eax
 	btrl	$X86_CR0_PG_BIT, %eax
@@ -648,6 +662,7 @@ SYM_DATA(image_offset, .long 0)
 
 #ifdef CONFIG_EFI_MIXED
 SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0, 0)
+SYM_DATA_LOCAL(efi32_boot_sp, .long 0)
 SYM_DATA(efi_is64, .byte 1)
 
 #define ST32_boottime		60 // offsetof(efi_system_table_32_t, boottime)
@@ -710,6 +725,7 @@ SYM_FUNC_START(efi32_pe_entry)
 	movl	12(%ebp), %edx			// sys_table
 	movl	-4(%ebp), %esi			// loaded_image
 	movl	LI32_image_base(%esi), %esi	// loaded_image->image_base
+	leave					// clear stack frame
 	movl	%ebx, %ebp			// startup_32 for efi32_pe_stub_entry
 	/*
 	 * We need to set the image_offset variable here since startup_32() will
-- 
2.26.2

Re: [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub

[Ard Biesheuvel]

On Tue, 26 May 2020 at 19:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> The UEFI specification requires a 128KiB stack during boot services. On
> a native mode boot, the EFI stub executes on the firmware stack.
> However, on a mixed-mode boot, startup_32 switches to the kernel's boot
> stack, which is only 16KiB, and the EFI stub is executed with this
> stack.
>
> To avoid any potential problems with running out of stack space, save
> and restore the UEFI stack pointer in the mixed-mode entry, so that the
> EFI stub can use the firmware stack in this case as well.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

This does not apply onto v5.8-rc1, and I was going to take it as a fix.

However, are we sure this is safe? Do we have a ballpark figure of how
much stack we use in the stub?

This is one of those things I am reluctant to change, given that we
are not sure that firmware implementations conform to this, and IA32
firmware was not designed to boot a 64-bit image (which might use more
stack space?)


> ---
>  arch/x86/boot/compressed/head_64.S | 46 ++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index 4b7ad1dfbea6..923e5c381804 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -237,21 +237,7 @@ SYM_FUNC_START(startup_32)
>          * used to perform that far jump.
>          */
>         pushl   $__KERNEL_CS
> -       leal    rva(startup_64)(%ebp), %eax
> -#ifdef CONFIG_EFI_MIXED
> -       movl    rva(efi32_boot_args)(%ebp), %edi
> -       cmp     $0, %edi
> -       jz      1f
> -       leal    rva(efi64_stub_entry)(%ebp), %eax
> -       movl    rva(efi32_boot_args+4)(%ebp), %esi
> -       movl    rva(efi32_boot_args+8)(%ebp), %edx      // saved bootparams pointer
> -       cmpl    $0, %edx
> -       jnz     1f
> -       leal    rva(efi_pe_entry)(%ebp), %eax
> -       movl    %edi, %ecx                      // MS calling convention
> -       movl    %esi, %edx
> -1:
> -#endif
> +       leal    rva(.L64bit)(%ebp), %eax
>         pushl   %eax
>
>         /* Enter paged protected Mode, activating Long Mode */
> @@ -260,6 +246,31 @@ SYM_FUNC_START(startup_32)
>
>         /* Jump from 32bit compatibility mode into 64bit mode. */
>         lret
> +
> +       .code64
> +SYM_INNER_LABEL(.L64bit, SYM_L_LOCAL)
> +#ifndef CONFIG_EFI_MIXED
> +       jmp     startup_64
> +#else
> +       movl    efi32_boot_args(%rip), %edi
> +       testl   %edi, %edi
> +       jz      startup_64
> +
> +       /* Restore firmware stack pointer */
> +       movl    efi32_boot_sp(%rip), %esp
> +       /* and align it to 8 mod 16 */
> +       andl    $~0xf, %esp
> +       subl    $8, %esp
> +
> +       movl    efi32_boot_args+4(%rip), %esi
> +       movl    efi32_boot_args+8(%rip), %edx   // saved bootparams pointer
> +       testl   %edx, %edx
> +       jnz     efi64_stub_entry
> +       movl    %edi, %ecx                      // MS calling convention
> +       movl    %esi, %edx
> +       jmp     efi_pe_entry
> +#endif
> +       .code32
>  SYM_FUNC_END(startup_32)
>
>  #ifdef CONFIG_EFI_MIXED
> @@ -285,6 +296,9 @@ SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL)
>         movw    %cs, rva(efi32_boot_cs)(%ebp)
>         movw    %ds, rva(efi32_boot_ds)(%ebp)
>
> +       /* Save firmware stack pointer */
> +       movl    %esp, rva(efi32_boot_sp)(%ebp)
> +
>         /* Disable paging */
>         movl    %cr0, %eax
>         btrl    $X86_CR0_PG_BIT, %eax
> @@ -648,6 +662,7 @@ SYM_DATA(image_offset, .long 0)
>
>  #ifdef CONFIG_EFI_MIXED
>  SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0, 0)
> +SYM_DATA_LOCAL(efi32_boot_sp, .long 0)
>  SYM_DATA(efi_is64, .byte 1)
>
>  #define ST32_boottime          60 // offsetof(efi_system_table_32_t, boottime)
> @@ -710,6 +725,7 @@ SYM_FUNC_START(efi32_pe_entry)
>         movl    12(%ebp), %edx                  // sys_table
>         movl    -4(%ebp), %esi                  // loaded_image
>         movl    LI32_image_base(%esi), %esi     // loaded_image->image_base
> +       leave                                   // clear stack frame
>         movl    %ebx, %ebp                      // startup_32 for efi32_pe_stub_entry
>         /*
>          * We need to set the image_offset variable here since startup_32() will
> --
> 2.26.2
>

Re: [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub

[Arvind Sankar]

On Mon, Jun 15, 2020 at 11:58:43AM +0200, Ard Biesheuvel wrote:
> On Tue, 26 May 2020 at 19:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > The UEFI specification requires a 128KiB stack during boot services. On
> > a native mode boot, the EFI stub executes on the firmware stack.
> > However, on a mixed-mode boot, startup_32 switches to the kernel's boot
> > stack, which is only 16KiB, and the EFI stub is executed with this
> > stack.
> >
> > To avoid any potential problems with running out of stack space, save
> > and restore the UEFI stack pointer in the mixed-mode entry, so that the
> > EFI stub can use the firmware stack in this case as well.
> >
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> 
> This does not apply onto v5.8-rc1, and I was going to take it as a fix.
> 

This was based on the runtime-relocation removing patch series (see
cover letter).
https://lore.kernel.org/linux-efi/20200526170226.2371024-1-nivedita@alum.mit.edu/

I can rework it to apply on mainline if we decide this patch could be
useful.

> However, are we sure this is safe? Do we have a ballpark figure of how
> much stack we use in the stub?
> 
> This is one of those things I am reluctant to change, given that we
> are not sure that firmware implementations conform to this, and IA32
> firmware was not designed to boot a 64-bit image (which might use more
> stack space?)
> 

The EFI stub code itself doesn't use much stack. The largest frame is
720 bytes and the rest are below 300, so it probably doesn't even reach
4k. The risk is really that inside the firmware it uses stack space more
liberally given it can assume it has 128KiB available. A safer
alternative would be to switch to the firmware stack only when actually
calling the firmware, inside the mixed-mode thunk.

Also, this patch fixed up one other small issue, which is that when we
enter via the compat 32-bit entry, we will call efi_pe_entry with a
misaligned stack (0 mod 16 instead of 8 mod 16). It gets correctly
aligned once efi_pe_entry finishes and calls efi_stub_entry though, so
most of the stub will still execute with proper alignment.

Re: [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub

[Arvind Sankar]

On Mon, Jun 15, 2020 at 11:56:05AM -0400, Arvind Sankar wrote:
> On Mon, Jun 15, 2020 at 11:58:43AM +0200, Ard Biesheuvel wrote:
> > On Tue, 26 May 2020 at 19:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > The UEFI specification requires a 128KiB stack during boot services. On
> > > a native mode boot, the EFI stub executes on the firmware stack.
> > > However, on a mixed-mode boot, startup_32 switches to the kernel's boot
> > > stack, which is only 16KiB, and the EFI stub is executed with this
> > > stack.
> > >
> > > To avoid any potential problems with running out of stack space, save
> > > and restore the UEFI stack pointer in the mixed-mode entry, so that the
> > > EFI stub can use the firmware stack in this case as well.
> > >
> > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > 
> > This does not apply onto v5.8-rc1, and I was going to take it as a fix.
> > 
> 
> This was based on the runtime-relocation removing patch series (see
> cover letter).
> https://lore.kernel.org/linux-efi/20200526170226.2371024-1-nivedita@alum.mit.edu/
> 
> I can rework it to apply on mainline if we decide this patch could be
> useful.
> 
> > However, are we sure this is safe? Do we have a ballpark figure of how
> > much stack we use in the stub?
> > 
> > This is one of those things I am reluctant to change, given that we
> > are not sure that firmware implementations conform to this, and IA32
> > firmware was not designed to boot a 64-bit image (which might use more
> > stack space?)
> > 
> 
> The EFI stub code itself doesn't use much stack. The largest frame is
> 720 bytes and the rest are below 300, so it probably doesn't even reach
> 4k. The risk is really that inside the firmware it uses stack space more
> liberally given it can assume it has 128KiB available. A safer
> alternative would be to switch to the firmware stack only when actually
> calling the firmware, inside the mixed-mode thunk.

So one thing that mostly mitigates this is that the boot heap, which at
this time would be unused, is right below the stack and is at least
64KiB in size. Taking that into account we really have 80KiB of stack
available, so this might be fragile wrt future changes but right now
it should be safe to run on the boot stack.

> 
> Also, this patch fixed up one other small issue, which is that when we
> enter via the compat 32-bit entry, we will call efi_pe_entry with a
> misaligned stack (0 mod 16 instead of 8 mod 16). It gets correctly
> aligned once efi_pe_entry finishes and calls efi_stub_entry though, so
> most of the stub will still execute with proper alignment.

Should I do a patch just for the alignment thing then?

Re: [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub

[Ard Biesheuvel]

On Tue, 16 Jun 2020 at 20:48, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Jun 15, 2020 at 11:56:05AM -0400, Arvind Sankar wrote:
> > On Mon, Jun 15, 2020 at 11:58:43AM +0200, Ard Biesheuvel wrote:
> > > On Tue, 26 May 2020 at 19:02, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > > >
> > > > The UEFI specification requires a 128KiB stack during boot services. On
> > > > a native mode boot, the EFI stub executes on the firmware stack.
> > > > However, on a mixed-mode boot, startup_32 switches to the kernel's boot
> > > > stack, which is only 16KiB, and the EFI stub is executed with this
> > > > stack.
> > > >
> > > > To avoid any potential problems with running out of stack space, save
> > > > and restore the UEFI stack pointer in the mixed-mode entry, so that the
> > > > EFI stub can use the firmware stack in this case as well.
> > > >
> > > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > >
> > > This does not apply onto v5.8-rc1, and I was going to take it as a fix.
> > >
> >
> > This was based on the runtime-relocation removing patch series (see
> > cover letter).
> > https://lore.kernel.org/linux-efi/20200526170226.2371024-1-nivedita@alum.mit.edu/
> >
> > I can rework it to apply on mainline if we decide this patch could be
> > useful.
> >
> > > However, are we sure this is safe? Do we have a ballpark figure of how
> > > much stack we use in the stub?
> > >
> > > This is one of those things I am reluctant to change, given that we
> > > are not sure that firmware implementations conform to this, and IA32
> > > firmware was not designed to boot a 64-bit image (which might use more
> > > stack space?)
> > >
> >
> > The EFI stub code itself doesn't use much stack. The largest frame is
> > 720 bytes and the rest are below 300, so it probably doesn't even reach
> > 4k. The risk is really that inside the firmware it uses stack space more
> > liberally given it can assume it has 128KiB available. A safer
> > alternative would be to switch to the firmware stack only when actually
> > calling the firmware, inside the mixed-mode thunk.
>
> So one thing that mostly mitigates this is that the boot heap, which at
> this time would be unused, is right below the stack and is at least
> 64KiB in size. Taking that into account we really have 80KiB of stack
> available, so this might be fragile wrt future changes but right now
> it should be safe to run on the boot stack.
>
> >
> > Also, this patch fixed up one other small issue, which is that when we
> > enter via the compat 32-bit entry, we will call efi_pe_entry with a
> > misaligned stack (0 mod 16 instead of 8 mod 16). It gets correctly
> > aligned once efi_pe_entry finishes and calls efi_stub_entry though, so
> > most of the stub will still execute with proper alignment.
>
> Should I do a patch just for the alignment thing then?

Yes please. I am about to send some fixes to -tip so I'd like to
include it. The other issue can be dealt with for the next merge
window, and we can do some light testing first.

[PATCH] efi/x86: Setup stack correctly for efi_pe_entry

[Arvind Sankar]

Commit
  17054f492dfd ("efi/x86: Implement mixed mode boot without the handover protocol")
introduced a new entry point for the EFI stub to be booted in mixed mode
on 32-bit firmware.

When entered via efi32_pe_entry, control is first transferred to
startup_32 to setup for the switch to long mode, and then the EFI stub
proper is entered via efi_pe_entry. efi_pe_entry is an MS ABI function,
and the ABI requires 32 bytes of shadow stack space to be allocated by
the caller, as well as the stack being aligned to 8 mod 16 on entry.

Allocate 40 bytes on the stack before switching to 64-bit mode when
calling efi_pe_entry to account for this.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_64.S | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index e821a7d7d5c4..d073e3c919dd 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -213,7 +213,6 @@ SYM_FUNC_START(startup_32)
 	 * We place all of the values on our mini stack so lret can
 	 * used to perform that far jump.
 	 */
-	pushl	$__KERNEL_CS
 	leal	startup_64(%ebp), %eax
 #ifdef CONFIG_EFI_MIXED
 	movl	efi32_boot_args(%ebp), %edi
@@ -224,11 +223,20 @@ SYM_FUNC_START(startup_32)
 	movl	efi32_boot_args+8(%ebp), %edx	// saved bootparams pointer
 	cmpl	$0, %edx
 	jnz	1f
+	/*
+	 * efi_pe_entry uses MS calling convention, which requires 32 bytes of
+	 * shadow space on the stack even if all arguments are passed in
+	 * registers. We also need an additional 8 bytes for the space that
+	 * would be occupied by the return address, and this also results in
+	 * the correct stack alignment for entry.
+	 */
+	subl	$40, %esp
 	leal	efi_pe_entry(%ebp), %eax
 	movl	%edi, %ecx			// MS calling convention
 	movl	%esi, %edx
 1:
 #endif
+	pushl	$__KERNEL_CS
 	pushl	%eax
 
 	/* Enter paged protected Mode, activating Long Mode */
-- 
2.26.2

Re: [PATCH] efi/x86: Setup stack correctly for efi_pe_entry

[Ard Biesheuvel]

On Tue, 16 Jun 2020 at 21:48, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Commit
>   17054f492dfd ("efi/x86: Implement mixed mode boot without the handover protocol")
> introduced a new entry point for the EFI stub to be booted in mixed mode
> on 32-bit firmware.
>
> When entered via efi32_pe_entry, control is first transferred to
> startup_32 to setup for the switch to long mode, and then the EFI stub
> proper is entered via efi_pe_entry. efi_pe_entry is an MS ABI function,
> and the ABI requires 32 bytes of shadow stack space to be allocated by
> the caller, as well as the stack being aligned to 8 mod 16 on entry.
>
> Allocate 40 bytes on the stack before switching to 64-bit mode when
> calling efi_pe_entry to account for this.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Queued as a fix, thanks.


> ---
>  arch/x86/boot/compressed/head_64.S | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index e821a7d7d5c4..d073e3c919dd 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -213,7 +213,6 @@ SYM_FUNC_START(startup_32)
>          * We place all of the values on our mini stack so lret can
>          * used to perform that far jump.
>          */
> -       pushl   $__KERNEL_CS
>         leal    startup_64(%ebp), %eax
>  #ifdef CONFIG_EFI_MIXED
>         movl    efi32_boot_args(%ebp), %edi
> @@ -224,11 +223,20 @@ SYM_FUNC_START(startup_32)
>         movl    efi32_boot_args+8(%ebp), %edx   // saved bootparams pointer
>         cmpl    $0, %edx
>         jnz     1f
> +       /*
> +        * efi_pe_entry uses MS calling convention, which requires 32 bytes of
> +        * shadow space on the stack even if all arguments are passed in
> +        * registers. We also need an additional 8 bytes for the space that
> +        * would be occupied by the return address, and this also results in
> +        * the correct stack alignment for entry.
> +        */
> +       subl    $40, %esp
>         leal    efi_pe_entry(%ebp), %eax
>         movl    %edi, %ecx                      // MS calling convention
>         movl    %esi, %edx
>  1:
>  #endif
> +       pushl   $__KERNEL_CS
>         pushl   %eax
>
>         /* Enter paged protected Mode, activating Long Mode */
> --
> 2.26.2
>

Re: [PATCH] efi/x86: Setup stack correctly for efi_pe_entry

[Ard Biesheuvel]

On Wed, 17 Jun 2020 at 00:06, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 16 Jun 2020 at 21:48, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > Commit
> >   17054f492dfd ("efi/x86: Implement mixed mode boot without the handover protocol")
> > introduced a new entry point for the EFI stub to be booted in mixed mode
> > on 32-bit firmware.
> >
> > When entered via efi32_pe_entry, control is first transferred to
> > startup_32 to setup for the switch to long mode, and then the EFI stub
> > proper is entered via efi_pe_entry. efi_pe_entry is an MS ABI function,
> > and the ABI requires 32 bytes of shadow stack space to be allocated by
> > the caller, as well as the stack being aligned to 8 mod 16 on entry.
> >
> > Allocate 40 bytes on the stack before switching to 64-bit mode when
> > calling efi_pe_entry to account for this.
> >
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
>
> Queued as a fix, thanks.
>

Shouldn't boot_stack_end be 16 byte aligned for this to work reliably?
This seems to work out in practice, as .bss is cacheline aligned, and
the heap and stack happen to be emitted first. but it would be better
to make this explicit.


>
> > ---
> >  arch/x86/boot/compressed/head_64.S | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> > index e821a7d7d5c4..d073e3c919dd 100644
> > --- a/arch/x86/boot/compressed/head_64.S
> > +++ b/arch/x86/boot/compressed/head_64.S
> > @@ -213,7 +213,6 @@ SYM_FUNC_START(startup_32)
> >          * We place all of the values on our mini stack so lret can
> >          * used to perform that far jump.
> >          */
> > -       pushl   $__KERNEL_CS
> >         leal    startup_64(%ebp), %eax
> >  #ifdef CONFIG_EFI_MIXED
> >         movl    efi32_boot_args(%ebp), %edi
> > @@ -224,11 +223,20 @@ SYM_FUNC_START(startup_32)
> >         movl    efi32_boot_args+8(%ebp), %edx   // saved bootparams pointer
> >         cmpl    $0, %edx
> >         jnz     1f
> > +       /*
> > +        * efi_pe_entry uses MS calling convention, which requires 32 bytes of
> > +        * shadow space on the stack even if all arguments are passed in
> > +        * registers. We also need an additional 8 bytes for the space that
> > +        * would be occupied by the return address, and this also results in
> > +        * the correct stack alignment for entry.
> > +        */
> > +       subl    $40, %esp
> >         leal    efi_pe_entry(%ebp), %eax
> >         movl    %edi, %ecx                      // MS calling convention
> >         movl    %esi, %edx
> >  1:
> >  #endif
> > +       pushl   $__KERNEL_CS
> >         pushl   %eax
> >
> >         /* Enter paged protected Mode, activating Long Mode */
> > --
> > 2.26.2
> >

[PATCH v2] efi/x86: Setup stack correctly for efi_pe_entry

[Arvind Sankar]

Commit
  17054f492dfd ("efi/x86: Implement mixed mode boot without the handover protocol")
introduced a new entry point for the EFI stub to be booted in mixed mode
on 32-bit firmware.

When entered via efi32_pe_entry, control is first transferred to
startup_32 to setup for the switch to long mode, and then the EFI stub
proper is entered via efi_pe_entry. efi_pe_entry is an MS ABI function,
and the ABI requires 32 bytes of shadow stack space to be allocated by
the caller, as well as the stack being aligned to 8 mod 16 on entry.

Allocate 40 bytes on the stack before switching to 64-bit mode when
calling efi_pe_entry to account for this.

For robustness, explicitly align boot_stack_end to 16 bytes. It is
currently implicitly aligned since .bss is cacheline-size aligned,
head_64.o is the first object file with a .bss section, and the heap and
boot sizes are aligned.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_64.S | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index e821a7d7d5c4..97d37f0a34f5 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -213,7 +213,6 @@ SYM_FUNC_START(startup_32)
 	 * We place all of the values on our mini stack so lret can
 	 * used to perform that far jump.
 	 */
-	pushl	$__KERNEL_CS
 	leal	startup_64(%ebp), %eax
 #ifdef CONFIG_EFI_MIXED
 	movl	efi32_boot_args(%ebp), %edi
@@ -224,11 +223,20 @@ SYM_FUNC_START(startup_32)
 	movl	efi32_boot_args+8(%ebp), %edx	// saved bootparams pointer
 	cmpl	$0, %edx
 	jnz	1f
+	/*
+	 * efi_pe_entry uses MS calling convention, which requires 32 bytes of
+	 * shadow space on the stack even if all arguments are passed in
+	 * registers. We also need an additional 8 bytes for the space that
+	 * would be occupied by the return address, and this also results in
+	 * the correct stack alignment for entry.
+	 */
+	subl	$40, %esp
 	leal	efi_pe_entry(%ebp), %eax
 	movl	%edi, %ecx			// MS calling convention
 	movl	%esi, %edx
 1:
 #endif
+	pushl	$__KERNEL_CS
 	pushl	%eax
 
 	/* Enter paged protected Mode, activating Long Mode */
@@ -784,6 +792,7 @@ SYM_DATA_LOCAL(boot_heap,	.fill BOOT_HEAP_SIZE, 1, 0)
 
 SYM_DATA_START_LOCAL(boot_stack)
 	.fill BOOT_STACK_SIZE, 1, 0
+	.balign 16
 SYM_DATA_END_LABEL(boot_stack, SYM_L_LOCAL, boot_stack_end)
 
 /*
-- 
2.26.2

Re: [PATCH v2] efi/x86: Setup stack correctly for efi_pe_entry

[Ard Biesheuvel]

On Wed, 17 Jun 2020 at 15:20, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> Commit
>   17054f492dfd ("efi/x86: Implement mixed mode boot without the handover protocol")
> introduced a new entry point for the EFI stub to be booted in mixed mode
> on 32-bit firmware.
>
> When entered via efi32_pe_entry, control is first transferred to
> startup_32 to setup for the switch to long mode, and then the EFI stub
> proper is entered via efi_pe_entry. efi_pe_entry is an MS ABI function,
> and the ABI requires 32 bytes of shadow stack space to be allocated by
> the caller, as well as the stack being aligned to 8 mod 16 on entry.
>
> Allocate 40 bytes on the stack before switching to 64-bit mode when
> calling efi_pe_entry to account for this.
>
> For robustness, explicitly align boot_stack_end to 16 bytes. It is
> currently implicitly aligned since .bss is cacheline-size aligned,
> head_64.o is the first object file with a .bss section, and the heap and
> boot sizes are aligned.
>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Cheers

> ---
>  arch/x86/boot/compressed/head_64.S | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index e821a7d7d5c4..97d37f0a34f5 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -213,7 +213,6 @@ SYM_FUNC_START(startup_32)
>          * We place all of the values on our mini stack so lret can
>          * used to perform that far jump.
>          */
> -       pushl   $__KERNEL_CS
>         leal    startup_64(%ebp), %eax
>  #ifdef CONFIG_EFI_MIXED
>         movl    efi32_boot_args(%ebp), %edi
> @@ -224,11 +223,20 @@ SYM_FUNC_START(startup_32)
>         movl    efi32_boot_args+8(%ebp), %edx   // saved bootparams pointer
>         cmpl    $0, %edx
>         jnz     1f
> +       /*
> +        * efi_pe_entry uses MS calling convention, which requires 32 bytes of
> +        * shadow space on the stack even if all arguments are passed in
> +        * registers. We also need an additional 8 bytes for the space that
> +        * would be occupied by the return address, and this also results in
> +        * the correct stack alignment for entry.
> +        */
> +       subl    $40, %esp
>         leal    efi_pe_entry(%ebp), %eax
>         movl    %edi, %ecx                      // MS calling convention
>         movl    %esi, %edx
>  1:
>  #endif
> +       pushl   $__KERNEL_CS
>         pushl   %eax
>
>         /* Enter paged protected Mode, activating Long Mode */
> @@ -784,6 +792,7 @@ SYM_DATA_LOCAL(boot_heap,   .fill BOOT_HEAP_SIZE, 1, 0)
>
>  SYM_DATA_START_LOCAL(boot_stack)
>         .fill BOOT_STACK_SIZE, 1, 0
> +       .balign 16
>  SYM_DATA_END_LABEL(boot_stack, SYM_L_LOCAL, boot_stack_end)
>
>  /*
> --
> 2.26.2
>

[tip: efi/urgent] efi/x86: Setup stack correctly for efi_pe_entry

[tip-bot2 for Arvind Sankar]

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     41d90b0c1108d1e46c48cf79964636c553844f4c
Gitweb:        https://git.kernel.org/tip/41d90b0c1108d1e46c48cf79964636c553844f4c
Author:        Arvind Sankar <nivedita@alum.mit.edu>
AuthorDate:    Wed, 17 Jun 2020 09:19:57 -04:00
Committer:     Ard Biesheuvel <ardb@kernel.org>
CommitterDate: Wed, 17 Jun 2020 15:28:58 +02:00

efi/x86: Setup stack correctly for efi_pe_entry

Commit

  17054f492dfd ("efi/x86: Implement mixed mode boot without the handover protocol")

introduced a new entry point for the EFI stub to be booted in mixed mode
on 32-bit firmware.

When entered via efi32_pe_entry, control is first transferred to
startup_32 to setup for the switch to long mode, and then the EFI stub
proper is entered via efi_pe_entry. efi_pe_entry is an MS ABI function,
and the ABI requires 32 bytes of shadow stack space to be allocated by
the caller, as well as the stack being aligned to 8 mod 16 on entry.

Allocate 40 bytes on the stack before switching to 64-bit mode when
calling efi_pe_entry to account for this.

For robustness, explicitly align boot_stack_end to 16 bytes. It is
currently implicitly aligned since .bss is cacheline-size aligned,
head_64.o is the first object file with a .bss section, and the heap and
boot sizes are aligned.

Fixes: 17054f492dfd ("efi/x86: Implement mixed mode boot without the handover protocol")
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Link: https://lore.kernel.org/r/20200617131957.2507632-1-nivedita@alum.mit.edu
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/head_64.S | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index e821a7d..97d37f0 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -213,7 +213,6 @@ SYM_FUNC_START(startup_32)
 	 * We place all of the values on our mini stack so lret can
 	 * used to perform that far jump.
 	 */
-	pushl	$__KERNEL_CS
 	leal	startup_64(%ebp), %eax
 #ifdef CONFIG_EFI_MIXED
 	movl	efi32_boot_args(%ebp), %edi
@@ -224,11 +223,20 @@ SYM_FUNC_START(startup_32)
 	movl	efi32_boot_args+8(%ebp), %edx	// saved bootparams pointer
 	cmpl	$0, %edx
 	jnz	1f
+	/*
+	 * efi_pe_entry uses MS calling convention, which requires 32 bytes of
+	 * shadow space on the stack even if all arguments are passed in
+	 * registers. We also need an additional 8 bytes for the space that
+	 * would be occupied by the return address, and this also results in
+	 * the correct stack alignment for entry.
+	 */
+	subl	$40, %esp
 	leal	efi_pe_entry(%ebp), %eax
 	movl	%edi, %ecx			// MS calling convention
 	movl	%esi, %edx
 1:
 #endif
+	pushl	$__KERNEL_CS
 	pushl	%eax
 
 	/* Enter paged protected Mode, activating Long Mode */
@@ -784,6 +792,7 @@ SYM_DATA_LOCAL(boot_heap,	.fill BOOT_HEAP_SIZE, 1, 0)
 
 SYM_DATA_START_LOCAL(boot_stack)
 	.fill BOOT_STACK_SIZE, 1, 0
+	.balign 16
 SYM_DATA_END_LABEL(boot_stack, SYM_L_LOCAL, boot_stack_end)
 
 /*