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

[Ard Biesheuvel <ardb@kernel.org>]

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
>