[PATCH] arm64: efi: Account for the EFI runtime stack in stack unwinder

[Ard Biesheuvel]

On Fri, 9 Dec 2022 at 16:00, Mark Rutland <mark.rutland at arm.com> wrote:
>
> On Fri, Dec 09, 2022 at 03:46:48PM +0100, Ard Biesheuvel wrote:
> > On Fri, 9 Dec 2022 at 15:37, Mark Rutland <mark.rutland at arm.com> wrote:
> > >
> > > On Fri, Dec 09, 2022 at 02:34:14PM +0100, Ard Biesheuvel wrote:
> > > > The EFI runtime services run from a dedicated stack now, and so the
> > > > stack unwinder needs to be informed about this.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> > > > ---
> > > >
> > > > I realised while looking into this that comparing current_work() against
> > > > efi_rts_work.work is not sufficient to decide whether current is running
> > > > EFI code, given that the ACPI subsystem will call efi_call_virt_pointer()
> > > > directly.
> > > >
> > > > So instead, we can check whether the stashed thread stack pointer value
> > > > matches current's thread stack if the EFI runtime stack is currently in
> > > > use:
> > > >
> > > > #define current_in_efi()                                               \
> > > >        (!preemptible() && spin_is_locked(&efi_rt_lock) &&              \
> > > >         on_task_stack(current, efi_rt_stack_top[-1], 1))
> > >
> > > Unless you're overwriting task_struct::stack (which seems scary to me), that
> > > doesn't look right; on_task_stack() checks whether a given base + size is on
> > > the stack allocated for the task (i.e. task_struct::stack + THREAD_SIZE), not
> > > the stack the task is currently using.
> > >
> >
> > Note the [-1].
> >
> > efi_rt_stack_top[-1] contains the value the stack pointer had before
> > switching to the EFI runtime stack. If that value is an address
> > covered by current's thread stack, current must be the task that has a
> > live call frame inside the EFI code at the time the call stack is
> > captured.
>
> Ah, I had missed that subtlety.
>
> Would you mind if we add that first sentence as a comment for that code, i.e.
>
> | /*
> |  * efi_rt_stack_top[-1] contains the value the stack pointer had before
> |  * switching to the EFI runtime stack.
> |  */
> |  #define current_in_efi()                                               \
> |         (!preemptible() && spin_is_locked(&efi_rt_lock) &&              \
> |          on_task_stack(current, efi_rt_stack_top[-1], 1))
>
> ... that way when I look at this in 3 to 6 months time I won't fall into the
> same trap. :)
>

Will do.

> I assume that the EFI trampoline code clobbers the value on the way out so it
> doesn't spruriously match later.
>

Not currently, no. But that's easily added.


> > > I would expect this to be something like:
> > >
> > > #define current_in_efi()                                                \
> > >         (!preemptible() && spin_is_locked(&efi_rt_lock) &&              \
> > >          stackinfo_on_stack(stackinfo_get_efi(), current_stack_pointer, 1))
> > >
> > > ... or an inline function given this is sufficiently painful as a macro.
> >
> > current_stack_pointer is the actual value of SP at the time this code
> > is called. So if we are unwinding from a sync exception taken while
> > handling an IRQ that arrived while running the EFI code, that SP value
> > has nothing to do with the EFI stack.
>
> Yes, good point.
>
> > > ... unless I've confused myself?
> > >
> >
> > I think you might have ... :-)
>
> :)
>
> Mark.