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@xxxxxxxxxx> > > Cc: Matt Fleming <matt.fleming@xxxxxxxxx> > > --- > > 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@xxxxxxxxx> Thanks! -- Josh -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
