Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1207297 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v10 19/20] x86/asm/efi: Create a stack frame in efi_call() Josh Poimboeuf <jpoimboe@redhat.com> - 2015-08-14 05:20 +0200
    Re: [PATCH v10 19/20] x86/asm/efi: Create a stack frame in efi_call() Matt Fleming <matt@codeblueprint.co.uk> - 2015-08-14 11:20 +0200
      Re: [PATCH v10 19/20] x86/asm/efi: Create a stack frame in efi_call() Josh Poimboeuf <jpoimboe@redhat.com> - 2015-08-14 16:10 +0200

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

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)
 
-- 
2.4.3

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

[toc] | [next] | [standalone]

#1207434

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).

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

-- 
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1207555

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web