From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>,
Matt Fleming <matt@codeblueprint.co.uk>,
Alex Thorlton <athorlton@sgi.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
"H. Peter Anvin" <hpa@zytor.com>,
Andrew Morton <akpm@linux-foundation.org>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>,
Steven Rostedt <rostedt@goodmis.org>,
Denys Vlasenko <dvlasenk@redhat.com>
Subject: [PATCH v2] x86/asm/entry: fix stack return address retrieval in thunk
Date: Tue, 17 May 2016 13:06:06 -0500 [thread overview]
Message-ID: <20160517180606.v5o7wcgdni7443ol@treble> (raw)
In-Reply-To: <CA+55aFxFXdhZzYy_0TE2JNyknzS3rCDDQhCB+VqBg2roAbZOMA@mail.gmail.com>
On Tue, May 17, 2016 at 09:31:12AM -0700, Linus Torvalds wrote:
> On Tue, May 17, 2016 at 7:43 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > index 98df1fa..dae7ca0 100644
> > --- a/arch/x86/entry/thunk_64.S
> > +++ b/arch/x86/entry/thunk_64.S
> > @@ -15,9 +15,10 @@
> > .globl \name
> > .type \name, @function
> > \name:
> > + /* push 1 register if frame pointers are enabled */
> > FRAME_BEGIN
> >
> > - /* this one pushes 9 elems, the next one would be %rIP */
> > + /* push 9 registers */
>
> I don't hate this patch, but quite frankly, as with the other case,
> I'd just make the frame pointer be unconditional in this case.
>
> If we push nine other registers, the frame pointer setup code is *not*
> going to matter.
>
> The reason to avoid frame pointers in code generation is two-fold:
>
> 1) for small leaf functions, it often ends up dominating
>
> 2) it removes a register that is otherwise usable, which can be
> particularly bad on 32-bit x86 due to the much more limited number of
> registers (and was apparently really noticeable on the older on-order
> atom cores)
>
> and in this case neither of them is really an issue.
>
> So I would suggest that any case that actually depends on a frame
> access just make the frame pointer not just unconditional, but
> _explicit_.
>
> So not just avoiding the macro because it's conditional, but write out
> the sequence to actually set up the frame, and then use
>
> - movq 9*8(%rsp), %rdi
> + movq 8(%rbp), %rdi # return address
>
> to entirely avoid all kind of "how many registers have we pushed" math.
>
> Considering that we got this wrong in two places, it's clearly too
> subtle for our little brains as-is.
Makes sense, thanks. Here's v2:
---
From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH v2] x86/asm/entry: fix stack return address retrieval in thunk
With CONFIG_FRAME_POINTER enabled, a thunk can pass a bad return address
value to the called function. '9*8(%rsp)' actually gets the frame
pointer, not the return address.
The only users of the 'put_ret_addr_in_rdi' option are two functions
which trace the enabling and disabling of interrupts, so this bug can
result in bad debug or tracing information with CONFIG_IRQSOFF_TRACER or
CONFIG_PROVE_LOCKING.
Fixes: 058fb73274f9 ("x86/asm/entry: Create stack frames in thunk functions")
Reported-by: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/x86/entry/thunk_64.S | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S
index 98df1fa..027aec4 100644
--- a/arch/x86/entry/thunk_64.S
+++ b/arch/x86/entry/thunk_64.S
@@ -8,16 +8,15 @@
#include <linux/linkage.h>
#include "calling.h"
#include <asm/asm.h>
-#include <asm/frame.h>
/* rdi: arg1 ... normal C conventions. rax is saved/restored. */
.macro THUNK name, func, put_ret_addr_in_rdi=0
.globl \name
.type \name, @function
\name:
- FRAME_BEGIN
+ pushq %rbp
+ movq %rsp, %rbp
- /* this one pushes 9 elems, the next one would be %rIP */
pushq %rdi
pushq %rsi
pushq %rdx
@@ -29,8 +28,8 @@
pushq %r11
.if \put_ret_addr_in_rdi
- /* 9*8(%rsp) is return addr on stack */
- movq 9*8(%rsp), %rdi
+ /* 8(%rbp) is return addr on stack */
+ movq 8(%rbp), %rdi
.endif
call \func
@@ -65,7 +64,7 @@ restore:
popq %rdx
popq %rsi
popq %rdi
- FRAME_END
+ popq %rbp
ret
_ASM_NOKPROBE(restore)
#endif
--
2.4.11
next prev parent reply other threads:[~2016-05-17 18:06 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-16 14:46 [GIT PULL] EFI fix Ingo Molnar
2016-05-16 20:05 ` Linus Torvalds
2016-05-16 20:23 ` Alex Thorlton
2016-05-16 22:40 ` Alex Thorlton
2016-05-17 6:30 ` [tip:x86/urgent] x86/efi: Fix 7-parameter efi_call()s tip-bot for Linus Torvalds
2016-05-17 9:04 ` [GIT PULL] EFI fix Matt Fleming
2016-05-17 9:46 ` Matt Fleming
2016-05-17 10:20 ` Ingo Molnar
2016-05-17 14:43 ` [PATCH] x86/asm/entry: fix stack return address retrieval in thunk Josh Poimboeuf
2016-05-17 16:31 ` Linus Torvalds
2016-05-17 16:51 ` Steven Rostedt
2016-05-17 17:21 ` Linus Torvalds
2016-05-17 17:25 ` Josh Poimboeuf
2016-05-17 18:06 ` Josh Poimboeuf [this message]
2016-05-17 18:33 ` [PATCH v2] " Linus Torvalds
2016-05-19 9:12 ` [tip:x86/urgent] x86/entry/64: Fix " tip-bot for Josh Poimboeuf
2016-05-23 12:08 ` [GIT PULL] EFI fix Matt Fleming
2016-05-23 12:33 ` Josh Poimboeuf
2016-05-24 9:03 ` Ingo Molnar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160517180606.v5o7wcgdni7443ol@treble \
--to=jpoimboe@redhat.com \
--cc=a.p.zijlstra@chello.nl \
--cc=akpm@linux-foundation.org \
--cc=athorlton@sgi.com \
--cc=bp@alien8.de \
--cc=dvlasenk@redhat.com \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=matt@codeblueprint.co.uk \
--cc=mingo@kernel.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).