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

[Daniel Kiper]

On Thu, Feb 02, 2017 at 02:48:31PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> 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.

The other option is call instead of jmp below but it seems to me a less 
reliable thing.

Daniel