linux-kernel - Re: [PATCH v10 19/20] x86/asm/efi: Create a stack frame in efi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 14 Aug 2015 09:07:12 -0500
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Matt Fleming <matt@...eblueprint.co.uk>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, Michal Marek <mmarek@...e.cz>,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Lutomirski <luto@...nel.org>,
	Borislav Petkov <bp@...en8.de>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	Pedro Alves <palves@...hat.com>,
	Namhyung Kim <namhyung@...il.com>,
	Bernd Petrovitsch <bernd@...rovitsch.priv.at>,
	Chris J Arges <chris.j.arges@...onical.com>,
	live-patching@...r.kernel.org,
	Matt Fleming <matt.fleming@...el.com>
Subject: Re: [PATCH v10 19/20] x86/asm/efi: Create a stack frame in efi_call()

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@...hat.com>
> > Cc: Matt Fleming <matt.fleming@...el.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@...el.com>

Thanks!

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/