Re: [PATCH] i386/relocator: Align stack in grub_relocator64_efi relocator

[Vladimir 'phcoder' Serbinenko]

On Thu, 2 Feb 2017, 15:43 Andrei Borzenkov <address@hidden> wrote:

On Thu, Feb 2, 2017 at 5:27 PM, Daniel Kiper <address@hidden> wrote:
> Unified Extensible Firmware Interface Specification, Version 2.6,
> section 2.3.4, x64 Platforms, boot services, says among others:
> The stack must be 16-byte aligned. So, do it. Otherwise OS may
> boot only by chance as it happens right now.
>
> Signed-off-by: Daniel Kiper <address@hidden>
> ---
>  grub-core/lib/i386/relocator64.S |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/lib/i386/relocator64.S b/grub-core/lib/i386/relocator64.S
> index 75725cf..148f38a 100644
> --- a/grub-core/lib/i386/relocator64.S
> +++ b/grub-core/lib/i386/relocator64.S
> @@ -73,14 +73,22 @@ VARIABLE(grub_relocator64_rsp)
>
>         movq    %rax, %rsp
>
> +#ifdef GRUB_MACHINE_EFI
> +       jmp     LOCAL(skip_efi_stack_align)
> +
>         /*
> -        * Here is grub_relocator64_efi_start() entry point.
> -        * Following code is shared between grub_relocator64_efi_start()
> +        * Here is grub_relocator64_efi_start() entry point. Most of the
> +        * code below is shared between grub_relocator64_efi_start()
>          * and grub_relocator64_start().
>          *
> -        * Think twice before changing anything below!!!
> +        * Think twice before changing anything there!!!
>          */
>  VARIABLE(grub_relocator64_efi_start)
> +       /* Align the stack as UEFI spec requires. */
> +       andq    $~15, %rsp
> +

It sounds like it should be fixed on caller side instead. I mean,
caller allocated some memory, we cannot just arbitrary adjust it now.
It sounds wrong.

Unlike normal relocator, EFI relocator didn't use .RSP field but instead uses EFI stack, so there is no other place to fix this.

> +LOCAL(skip_efi_stack_align):
> +#endif
>         /* mov imm64, %rax */
>         .byte   0x48
>         .byte   0xb8
> @@ -128,8 +136,10 @@ LOCAL(jump_addr):
>  VARIABLE(grub_relocator64_rip)
>         .quad   0
>
> +#ifdef GRUB_MACHINE_EFI
>         /* Here grub_relocator64_efi_start() ends. Ufff... */
>  VARIABLE(grub_relocator64_efi_end)
> +#endif
>
>  #ifndef __x86_64__
>         .p2align        4
> --
> 1.7.10.4
>