Re: [PATCH v10 19/20] x86/asm/efi: Create a stack frame in efi_call()

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Fri, Aug 14, 2015 at 10:11:59AM +0100, Matt Fleming wrote:
> On Thu, 13 Aug, at 10:10:40PM, Josh Poimboeuf wrote:
> > efi_call() is a callable non-leaf function which doesn't honor
> > CONFIG_FRAME_POINTER, which can result in bad stack traces.
> > 
> > Create a stack frame for it when CONFIG_FRAME_POINTER is enabled.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Matt Fleming <matt.fleming@intel.com>
> > ---
> >  arch/x86/platform/efi/efi_stub_64.S | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/x86/platform/efi/efi_stub_64.S b/arch/x86/platform/efi/efi_stub_64.S
> > index 86d0f9e..0df2dcc 100644
> > --- a/arch/x86/platform/efi/efi_stub_64.S
> > +++ b/arch/x86/platform/efi/efi_stub_64.S
> > @@ -11,6 +11,7 @@
> >  #include <asm/msr.h>
> >  #include <asm/processor-flags.h>
> >  #include <asm/page_types.h>
> > +#include <asm/frame.h>
> >  
> >  #define SAVE_XMM			\
> >  	mov %rsp, %rax;			\
> > @@ -74,6 +75,7 @@
> >  	.endm
> >  
> >  ENTRY(efi_call)
> > +	FRAME_BEGIN
> >  	SAVE_XMM
> >  	mov (%rsp), %rax
> >  	mov 8(%rax), %rax
> > @@ -88,6 +90,7 @@ ENTRY(efi_call)
> >  	RESTORE_PGT
> >  	addq $48, %rsp
> >  	RESTORE_XMM
> > +	FRAME_END
> >  	ret
> >  ENDPROC(efi_call)
> 
> You mention that stackvalidate will recursively validate the frame
> pointers in all code paths. Since we're calling into firmware code from
> efi_call(), we don't need to do anything special here right?
>
> I'm guessing stackvalidate would just stop since it has no way of
> knowing the target address of the %call instruction, but I just wanted
> to check (especially since the firmware ABI is different).

It recursively follows all code paths *inside* each function, including
jumps.  It doesn't try to follow calls across functions, which can't be
done reliably because of function pointers.  Instead it just verifies
that each function follows the calling conventions.

The result of creating a stack frame is that the *caller* of the
function shows up in the stack trace.  So this patch ensures that
efi_call()'s caller would show up if, for example, the stack was dumped
from an interrupt which occurred in efi_call() between FRAME_BEGIN and
FRAME_END.

As to whether it helps in case the stack is dumped from firmware code
(or from an interrupt to firmware), it really depends on what the
firmware does:

- If it follows frame pointer convention, great (but I'm guessing this
  is unlikely...)

- If it doesn't follow frame pointer convention, but still leaves rbp
  alone, then efi_call() would be skipped in the stack trace but
  efi_call()'s caller and the rest of the stack would still show up.

- If it trashes rbp, then we're out of luck and there's no stack trace.

But regardless of what firmware does, this patch still helps in the case
where the stack is dumped starting from efi_call().

> Reviewed-by: Matt Fleming <matt.fleming@intel.com>

Thanks!

-- 
Josh