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

From: Arvind Sankar <nivedita@alum.mit.edu>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Arvind Sankar <nivedita@alum.mit.edu>,
	linux-efi <linux-efi@vger.kernel.org>
Subject: Re: [PATCH 1/1] efi/x86: Use firmware stack for mixed-mode EFI stub
Date: Mon, 15 Jun 2020 11:56:05 -0400	[thread overview]
Message-ID: <20200615155605.GA672028@rani.riverdale.lan> (raw)
In-Reply-To: <CAMj1kXFGubFssfb1K_KUGXHhwF1X-vsL+ENYHU4EtbEXCTsQXw@mail.gmail.com>

On Mon, Jun 15, 2020 at 11:58:43AM +0200, Ard Biesheuvel wrote:
> 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.
> 

This was based on the runtime-relocation removing patch series (see
cover letter).
https://lore.kernel.org/linux-efi/20200526170226.2371024-1-nivedita@alum.mit.edu/

I can rework it to apply on mainline if we decide this patch could be
useful.

> 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?)
> 

The EFI stub code itself doesn't use much stack. The largest frame is
720 bytes and the rest are below 300, so it probably doesn't even reach
4k. The risk is really that inside the firmware it uses stack space more
liberally given it can assume it has 128KiB available. A safer
alternative would be to switch to the firmware stack only when actually
calling the firmware, inside the mixed-mode thunk.

Also, this patch fixed up one other small issue, which is that when we
enter via the compat 32-bit entry, we will call efi_pe_entry with a
misaligned stack (0 mod 16 instead of 8 mod 16). It gets correctly
aligned once efi_pe_entry finishes and calls efi_stub_entry though, so
most of the stub will still execute with proper alignment.