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

[Andrei Borzenkov]

 On Thu, Feb 2, 2017 at 5:48 PM, Vladimir 'phcoder' Serbinenko
<address@hidden> 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.
>>

OK, I see.