Hi,
Code like retpoline or RSB stuffing, which is used to mitigate some of
the speculative execution issues, is currently ignored by objtool with
the ANNOTATE_NOSPEC_ALTERNATIVE directive. This series adds support
for intra-function calls and trampoline return instructions to objtool
so that it can handle such a code. With these changes, we can remove
most ANNOTATE_NOSPEC_ALTERNATIVE directives.
Thanks,
alex.
--
Alexandre Chartre (7):
objtool: is_fentry_call() crashes if call has no destination
objtool: Allow branches within the same alternative.
objtool: Add support for intra-function calls
objtool: Add support for return trampoline call
x86/speculation: Annotate intra-function calls
x86/speculation: Annotate retpoline return instructions
x86/speculation: Remove most ANNOTATE_NOSPEC_ALTERNATIVE
arch/x86/include/asm/nospec-branch.h | 38 ++++--
tools/objtool/check.c | 172 +++++++++++++++++++++++++--
tools/objtool/check.h | 5 +-
3 files changed, 196 insertions(+), 19 deletions(-)
--
2.18.2
Some speculative execution mitigations (like retpoline) use intra-
function calls. Provide a macro to annotate such intra-function calls
so they can be properly handled by objtool, and use this macro to
annotate intra-function calls.
Signed-off-by: Alexandre Chartre <[email protected]>
---
arch/x86/include/asm/nospec-branch.h | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 5c24a7b35166..a2885f801e13 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -77,13 +77,27 @@
.popsection
.endm
+/*
+ * Intra-function call instruction. This should be used as a substitute
+ * for the call instruction when doing an intra-function call. It is
+ * similar to the call instruction but it tells objtool that this is
+ * an intra-function call.
+ */
+.macro INTRA_FUNCTION_CALL dst:req
+ .Lannotate_\@:
+ .pushsection .discard.intra_function_call
+ _ASM_PTR .Lannotate_\@
+ .popsection
+ call \dst
+.endm
+
/*
* These are the bare retpoline primitives for indirect jmp and call.
* Do not use these directly; they only exist to make the ALTERNATIVE
* invocation below less ugly.
*/
.macro RETPOLINE_JMP reg:req
- call .Ldo_rop_\@
+ INTRA_FUNCTION_CALL .Ldo_rop_\@
.Lspec_trap_\@:
pause
lfence
@@ -102,7 +116,7 @@
.Ldo_retpoline_jmp_\@:
RETPOLINE_JMP \reg
.Ldo_call_\@:
- call .Ldo_retpoline_jmp_\@
+ INTRA_FUNCTION_CALL .Ldo_retpoline_jmp_\@
.endm
/*
--
2.18.2
Now that intra-function calls and retpoline return instructions have
been annotated and that they are supported by objtool, almost all
ANNOTATE_NOSPEC_ALTERNATIVE can be removed.
ANNOTATE_NOSPEC_ALTERNATIVE is still needed when using the
__FILL_RETURN_BUFFER macro because it loops on intra-function calls
and objtool doesn't handle loops modifying the stack pointer.
Signed-off-by: Alexandre Chartre <[email protected]>
---
arch/x86/include/asm/nospec-branch.h | 4 ----
1 file changed, 4 deletions(-)
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 9ae6dde2f10b..70d4444120b1 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -140,7 +140,6 @@
*/
.macro JMP_NOSPEC reg:req
#ifdef CONFIG_RETPOLINE
- ANNOTATE_NOSPEC_ALTERNATIVE
ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *\reg), \
__stringify(RETPOLINE_JMP \reg), X86_FEATURE_RETPOLINE, \
__stringify(lfence; ANNOTATE_RETPOLINE_SAFE; jmp *\reg), X86_FEATURE_RETPOLINE_AMD
@@ -151,7 +150,6 @@
.macro CALL_NOSPEC reg:req
#ifdef CONFIG_RETPOLINE
- ANNOTATE_NOSPEC_ALTERNATIVE
ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; call *\reg), \
__stringify(RETPOLINE_CALL \reg), X86_FEATURE_RETPOLINE,\
__stringify(lfence; ANNOTATE_RETPOLINE_SAFE; call *\reg), X86_FEATURE_RETPOLINE_AMD
@@ -190,7 +188,6 @@
* which is ensured when CONFIG_RETPOLINE is defined.
*/
# define CALL_NOSPEC \
- ANNOTATE_NOSPEC_ALTERNATIVE \
ALTERNATIVE_2( \
ANNOTATE_RETPOLINE_SAFE \
"call *%[thunk_target]\n", \
@@ -209,7 +206,6 @@
* here, anyway.
*/
# define CALL_NOSPEC \
- ANNOTATE_NOSPEC_ALTERNATIVE \
ALTERNATIVE_2( \
ANNOTATE_RETPOLINE_SAFE \
"call *%[thunk_target]\n", \
--
2.18.2
On Thu, Apr 02, 2020 at 10:22:18AM +0200, Alexandre Chartre wrote:
> .macro RETPOLINE_JMP reg:req
> - call .Ldo_rop_\@
> + INTRA_FUNCTION_CALL .Ldo_rop_\@
> .Lspec_trap_\@:
> pause
> lfence
> @@ -102,7 +116,7 @@
> .Ldo_retpoline_jmp_\@:
> RETPOLINE_JMP \reg
> .Ldo_call_\@:
> - call .Ldo_retpoline_jmp_\@
> + INTRA_FUNCTION_CALL .Ldo_retpoline_jmp_\@
> .endm
There's a catch: this is part of an alternative. Which means if
X86_FEATURE_RETPOLINE isn't set at runtime, then the retpoline won't be
there and the ORC data will be wrong.
In fact objtool should probably be made smart enough to warn about this
situation, when an alternative changes the stack state.
The only way I can think of to fix this is to have ORC alternatives :-/
--
Josh
On Fri, Apr 03, 2020 at 11:05:38AM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 02, 2020 at 10:22:18AM +0200, Alexandre Chartre wrote:
> > .macro RETPOLINE_JMP reg:req
> > - call .Ldo_rop_\@
> > + INTRA_FUNCTION_CALL .Ldo_rop_\@
> > .Lspec_trap_\@:
> > pause
> > lfence
> > @@ -102,7 +116,7 @@
> > .Ldo_retpoline_jmp_\@:
> > RETPOLINE_JMP \reg
> > .Ldo_call_\@:
> > - call .Ldo_retpoline_jmp_\@
> > + INTRA_FUNCTION_CALL .Ldo_retpoline_jmp_\@
> > .endm
>
> There's a catch: this is part of an alternative. Which means if
> X86_FEATURE_RETPOLINE isn't set at runtime, then the retpoline won't be
> there and the ORC data will be wrong.
>
> In fact objtool should probably be made smart enough to warn about this
> situation, when an alternative changes the stack state.
>
> The only way I can think of to fix this is to have ORC alternatives :-/
Or they could be converted to use static branches instead of
alternatives.
--
Josh
On 4/3/20 6:16 PM, Josh Poimboeuf wrote:
> On Fri, Apr 03, 2020 at 11:05:38AM -0500, Josh Poimboeuf wrote:
>> On Thu, Apr 02, 2020 at 10:22:18AM +0200, Alexandre Chartre wrote:
>>> .macro RETPOLINE_JMP reg:req
>>> - call .Ldo_rop_\@
>>> + INTRA_FUNCTION_CALL .Ldo_rop_\@
>>> .Lspec_trap_\@:
>>> pause
>>> lfence
>>> @@ -102,7 +116,7 @@
>>> .Ldo_retpoline_jmp_\@:
>>> RETPOLINE_JMP \reg
>>> .Ldo_call_\@:
>>> - call .Ldo_retpoline_jmp_\@
>>> + INTRA_FUNCTION_CALL .Ldo_retpoline_jmp_\@
>>> .endm
>>
>> There's a catch: this is part of an alternative. Which means if
>> X86_FEATURE_RETPOLINE isn't set at runtime, then the retpoline won't be
>> there and the ORC data will be wrong.
>>
>> In fact objtool should probably be made smart enough to warn about this
>> situation, when an alternative changes the stack state.
>>
>> The only way I can think of to fix this is to have ORC alternatives :-/
So that means that any alternative that does a stack manipulation isn't
currently supported?
alex.
> Or they could be converted to use static branches instead of
> alternatives.
>
On Fri, Apr 03, 2020 at 10:14:49AM -0700, Alexandre Chartre wrote:
> So that means that any alternative that does a stack manipulation isn't
> currently supported?
It's fundamentally impossible to correctly unwind through.
Instructions before and after the alternative need to have the same
stack layout irrespective of the alternative chosen.
What we need in this case though is only a different stack layout inside
the alternative, and that is doable.
On Fri, Apr 03, 2020 at 07:18:36PM +0200, Peter Zijlstra wrote:
> What we need in this case though is only a different stack layout inside
> the alternative, and that is doable.
I'm not sure what you mean... any stack changes within the alternative
have to match exactly the stack changes at the same offsets of the
original code because ORC doesn't know the difference between the two.
--
Josh
On Fri, Apr 03, 2020 at 12:24:08PM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 03, 2020 at 07:18:36PM +0200, Peter Zijlstra wrote:
> > What we need in this case though is only a different stack layout inside
> > the alternative, and that is doable.
>
> I'm not sure what you mean... any stack changes within the alternative
> have to match exactly the stack changes at the same offsets of the
> original code because ORC doesn't know the difference between the two.
I mean that the ORC entries on either side of the alternative have to be
invariant wrt the specific alternative chosen, but that -- provded you
whip up that ORC alternative stuff -- stack layout inside of an
alternative could possibly be different.