Re: [PATCH v2 10/21] efi/libstub/x86: avoid thunking for native firmware calls

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Arvind Sankar <nivedita@alum.mit.edu>
Cc: Andy Lutomirski <luto@amacapital.net>,
	Hans de Goede <hdegoede@redhat.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	linux-efi <linux-efi@vger.kernel.org>,
	Matthew Garrett <matthewgarrett@google.com>,
	Ingo Molnar <mingo@kernel.org>, Andy Lutomirski <luto@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v2 10/21] efi/libstub/x86: avoid thunking for native firmware calls
Date: Sun, 22 Dec 2019 22:25:13 +0100	[thread overview]
Message-ID: <CAKv+Gu-J7npYr7dRfvf8_--eMdpZaa09HD2SkXUyQfv8UyY3Mw@mail.gmail.com> (raw)
In-Reply-To: <20191222211257.GA23363@rani.riverdale.lan>

On Sun, 22 Dec 2019 at 22:13, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Sun, Dec 22, 2019 at 04:29:48PM +0100, Ard Biesheuvel wrote:
> > On Sun, 22 Dec 2019 at 13:46, Andy Lutomirski <luto@amacapital.net> wrote:
> > >
> > > Hmm. Most of the kernel is compiled with the stack alignment set to 8, and there a lot of asm that makes no effort to preserve alignment beyond 8 bytes.  So if EFI calls need 16 byte alignment, you may need to do something special.
> > >
> > > On new enough gcc (the versions that actually support the flags to set the alignment to 8), maybe you can use function attributes, or maybe you can stick a 16-byte-aligned local variable in functions that call EFI functions?  The latter would be rather fragile.
> >
> > This patch replaces open coded SysV to MS calling convention
> > translation to GCC generated code (using __attribute__((ms_abi)) which
> > we have been using for a long time in EDK2), because the former relies
> > on a wrapper function
> >
> > efi_call(fn, ...)
> >
> > which is type unsafe and relies on a lot of nasty casting, especially
> > combined with the mixed mode support. efi_call() is implemented as
> > below, and as Hans reports, omitting this sequence causes a boot
> > regression on one of the platforms he has tested this on.
> >
> > So the question is which of the pieces below this UEFI implementation
> > is actually relying on, and the stack pointer alignment is my first
> > guess, but it could be any of the other things as well. Once we
> > identify what it is we are missing, I can simply stick it back in, but
> > without reverting to using the efi_call() thunk.
> >
> > Note that the decompressor/stub are built with the default stack
> > alignment of 16 afaict, but if GRUB enters the decompressor with a
> > misaligned stack, we probably wouldn't notice until we hit something
> > like a movaps, right?
> >
> > Thanks,
> > Ard.
> >
>
> Won't the entry code misalign the stack when efi_main is called,
> assuming the stack was properly aligned at efi_stub_entry? There needs
> to be a sub $8, %rsp in there, no?
>
> arch/x86/boot/compressed/head_64.S:
>
> #ifdef CONFIG_EFI_STUB
>         .org 0x390
> SYM_FUNC_START(efi64_stub_entry)
> SYM_FUNC_START_ALIAS(efi_stub_entry)
>         movq    $1, %rcx
> handover_entry:
>         call    efi_main        <--- this will enter efi_main with a misaligned stack?
>         movq    %rax,%rsi
>         movl    BP_code32_start(%esi), %eax
>         leaq    startup_64(%rax), %rax
>         jmp     *%rax
> SYM_FUNC_END(efi64_stub_entry)
> SYM_FUNC_END_ALIAS(efi_stub_entry)
> #endif
>

Indeed, well spotted. Note that the above is the version from the top
of that branch, but the version that Hans tested isn't any different.