Re: [PATCH 1/2] arm64: efi: Execute runtime services from a dedicated stack

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > >
> > > > On Mon, 05 Dec 2022, Ard Biesheuvel wrote:
> > > >
> > > > > With the introduction of PRMT in the ACPI subsystem, the EFI rts
> > > > > workqueue is no longer the only caller of efi_call_virt_pointer() in the
> > > > > kernel. This means the EFI runtime services lock is no longer sufficient
> > > > > to manage concurrent calls into firmware, but also that firmware calls
> > > > > may occur that are not marshalled via the workqueue mechanism, but
> > > > > originate directly from the caller context.
> > > > >
> > > > > For added robustness, and to ensure that the runtime services have 8 KiB
> > > > > of stack space available as per the EFI spec, introduce a spinlock
> > > > > protected EFI runtime stack of 8 KiB, where the spinlock also ensures
> > > > > serialization between the EFI rts workqueue (which itself serializes EFI
> > > > > runtime calls) and other callers of efi_call_virt_pointer().
> > > > >
> > > > > While at it, use the stack pivot to avoid reloading the shadow call
> > > > > stack pointer from the ordinary stack, as doing so could produce a
> > > > > gadget to defeat it.
> > > > >
> > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > ---
> > > > >  arch/arm64/include/asm/efi.h       |  3 +++
> > > > >  arch/arm64/kernel/efi-rt-wrapper.S | 13 +++++++++-
> > > > >  arch/arm64/kernel/efi.c            | 25 ++++++++++++++++++++
> > > > >  3 files changed, 40 insertions(+), 1 deletion(-)
> > > >
> > > > Could we have this in Stable please?
> > > >
> > > > Upstream commit: ff7a167961d1b ("arm64: efi: Execute runtime services from a dedicated stack")
> > > >
> > > > Ard, do we need Patch 2 as well, or can this be applied on its own?
> > > >
> > >
> > > Thanks for the reminder.
> > >
> > > Only patch #1 is needed. It should be applied to v5.10 and later.
> >
> > Hold on, why did this go into mainline when I had an outstanding comment w.r.t.
> > the stack unwinder?
> >
> > From your last reply to me there I was expecting a respin with that fixed.
> >
> 
> Apologies for the confusion.
> 
> I have a patch for this queued up, but AIUI, that cannot be merged all
> the way back to v5.10, so these need to remain separate changes in any
> case.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c2530a04a73e6b75ed71ed14d09d7b42d6300013

Ah, ok, thanks for the pointer!

I'm a little uneasy here, still.

By backporting this we're also backporting the new breakage of the stack
unwinder, and the minimal change for backports would be to add the lock and not
the new stack (which was added for additinoal robustness, not to fix the bug
the lock fixes).

I do appreciate that the additional stack is likely more useful than the
occasional diagnostic output from the kernel, but it does seem like this has
traded off one bug for another, and I'm just a little annoyed because I pointed
that out before the first pull request was made.

I do know that this isn't malicious, and I'm not trying to start a fight, but
now we have to consider whether we want/need to backport a stack unwinder fix
to account for this, and we hadn't had that discussion before.

Thanks,
Mark.