* [PATCH 0/1] efi/x86: Use firmware stack for mixed-mode EFI stub
@ 2020-05-26 17:02 Arvind Sankar
2020-05-26 17:02 ` [PATCH 1/1] " Arvind Sankar
0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-05-26 17:02 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub
2020-05-26 17:02 [PATCH 0/1] efi/x86: Use firmware stack for mixed-mode EFI stub Arvind Sankar
@ 2020-05-26 17:02 ` Arvind Sankar
2020-06-15 9:58 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-05-26 17:02 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub
2020-05-26 17:02 ` [PATCH 1/1] " Arvind Sankar
@ 2020-06-15 9:58 ` Ard Biesheuvel
2020-06-15 15:56 ` Arvind Sankar
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-06-15 9:58 UTC (permalink / raw)
To: Arvind Sankar; +Cc: linux-efi
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub
2020-06-15 9:58 ` Ard Biesheuvel
@ 2020-06-15 15:56 ` Arvind Sankar
2020-06-16 18:48 ` Arvind Sankar
0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-06-15 15:56 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Arvind Sankar, linux-efi
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub
2020-06-15 15:56 ` Arvind Sankar
@ 2020-06-16 18:48 ` Arvind Sankar
2020-06-16 18:50 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-06-16 18:48 UTC (permalink / raw)
To: Arvind Sankar; +Cc: Ard Biesheuvel, linux-efi
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub
2020-06-16 18:48 ` Arvind Sankar
@ 2020-06-16 18:50 ` Ard Biesheuvel
2020-06-16 19:48 ` [PATCH] efi/x86: Setup stack correctly for efi_pe_entry Arvind Sankar
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-06-16 18:50 UTC (permalink / raw)
To: Arvind Sankar; +Cc: linux-efi
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] efi/x86: Setup stack correctly for efi_pe_entry
2020-06-16 18:50 ` Ard Biesheuvel
@ 2020-06-16 19:48 ` Arvind Sankar
2020-06-16 22:06 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-06-16 19:48 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, x86
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] efi/x86: Setup stack correctly for efi_pe_entry
2020-06-16 19:48 ` [PATCH] efi/x86: Setup stack correctly for efi_pe_entry Arvind Sankar
@ 2020-06-16 22:06 ` Ard Biesheuvel
2020-06-17 10:33 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-06-16 22:06 UTC (permalink / raw)
To: Arvind Sankar; +Cc: linux-efi, X86 ML
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] efi/x86: Setup stack correctly for efi_pe_entry
2020-06-16 22:06 ` Ard Biesheuvel
@ 2020-06-17 10:33 ` Ard Biesheuvel
2020-06-17 13:19 ` [PATCH v2] " Arvind Sankar
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-06-17 10:33 UTC (permalink / raw)
To: Arvind Sankar; +Cc: linux-efi, X86 ML
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
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] efi/x86: Setup stack correctly for efi_pe_entry
2020-06-17 10:33 ` Ard Biesheuvel
@ 2020-06-17 13:19 ` Arvind Sankar
2020-06-17 13:26 ` Ard Biesheuvel
2020-06-19 16:45 ` [tip: efi/urgent] " tip-bot2 for Arvind Sankar
0 siblings, 2 replies; 12+ messages in thread
From: Arvind Sankar @ 2020-06-17 13:19 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, X86 ML
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] efi/x86: Setup stack correctly for efi_pe_entry
2020-06-17 13:19 ` [PATCH v2] " Arvind Sankar
@ 2020-06-17 13:26 ` Ard Biesheuvel
2020-06-19 16:45 ` [tip: efi/urgent] " tip-bot2 for Arvind Sankar
1 sibling, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-06-17 13:26 UTC (permalink / raw)
To: Arvind Sankar; +Cc: linux-efi, X86 ML
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tip: efi/urgent] efi/x86: Setup stack correctly for efi_pe_entry
2020-06-17 13:19 ` [PATCH v2] " Arvind Sankar
2020-06-17 13:26 ` Ard Biesheuvel
@ 2020-06-19 16:45 ` tip-bot2 for Arvind Sankar
1 sibling, 0 replies; 12+ messages in thread
From: tip-bot2 for Arvind Sankar @ 2020-06-19 16:45 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Arvind Sankar, Ard Biesheuvel, x86, LKML
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)
/*
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-06-19 16:46 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-26 17:02 [PATCH 0/1] efi/x86: Use firmware stack for mixed-mode EFI stub Arvind Sankar
2020-05-26 17:02 ` [PATCH 1/1] " Arvind Sankar
2020-06-15 9:58 ` Ard Biesheuvel
2020-06-15 15:56 ` Arvind Sankar
2020-06-16 18:48 ` Arvind Sankar
2020-06-16 18:50 ` Ard Biesheuvel
2020-06-16 19:48 ` [PATCH] efi/x86: Setup stack correctly for efi_pe_entry Arvind Sankar
2020-06-16 22:06 ` Ard Biesheuvel
2020-06-17 10:33 ` Ard Biesheuvel
2020-06-17 13:19 ` [PATCH v2] " Arvind Sankar
2020-06-17 13:26 ` Ard Biesheuvel
2020-06-19 16:45 ` [tip: efi/urgent] " tip-bot2 for Arvind Sankar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.