* [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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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 0 siblings, 1 reply; 11+ 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] 11+ 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 0 siblings, 0 replies; 11+ 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] 11+ messages in thread
end of thread, other threads:[~2020-06-17 13:26 UTC | newest] Thread overview: 11+ 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).