objtool: retpoline compatibility

Make objtool compatible with CONFIG_RETPOLINE and re-enable the objtool-dependent features.

Josh Poimboeuf (3):
objtool: Detect jumps to retpoline thunks
objtool: Ignore retpoline alternatives
Revert "x86/retpoline: Temporarily disable objtool when
CONFIG_RETPOLINE=y"

 arch/x86/Kconfig                     |  4 +--
 arch/x86/Kconfig.debug               |  6 ++--
 arch/x86/include/asm/nospec-branch.h | 27 +++++++++++++-
 tools/objtool/check.c                | 69 ++++++++++++++++++++++++++++++++----
 tools/objtool/check.h                |  2 +-
 5 files changed, 95 insertions(+), 13 deletions(-)

2.14.3

A direct jump to a retpoline thunk is really an indirect jump in disguise. Change the objtool instruction type accordingly.

Objtool needs to know where indirect branches are so it can detect switch statement jump tables.

This fixes a bunch of warnings with CONFIG_RETPOLINE like:

arch/x86/events/intel/uncore_nhmex.o: warning: objtool: nhmex_rbox_msr_enable_event()+0x44: sibling call from callable instruction with modified stack frame kernel/signal.o: warning: objtool: copy_siginfo_to_user()+0x91: sibling call from callable instruction with modified stack frame
...

---
 tools/objtool/check.c | 7 +++++++
 1 file changed, 7 insertions(+)

index 9b341584eb1b..de053fb7049b 100644
@@ -456,6 +456,13 @@ static int add_jump_destinations(struct objtool_file *file)
 		} else if (rela->sym->sec->idx) {
 			dest_sec = rela->sym->sec;
 			dest_off = rela->sym->sym.st_value + rela->addend + 4;
+		} else if (strstr(rela->sym->name, "_indirect_thunk_")) {
+			/*
+			 * Retpoline jumps are really dynamic jumps in
+			 * disguise, so convert them accordingly.
+			 */
+			insn->type = INSN_JUMP_DYNAMIC;
+			continue;
 		} else {
 			/* sibling call */
 			insn->jump_dest = 0;

2.14.3

Getting objtool to understand retpolines is going to be a bit of a challenge. For now, take advantage of the fact that retpolines are patched in with alternatives. Just read the original (sane) non-alternative instruction, and ignore the patched-in retpoline.

This allows objtool to understand the control flow *around* the retpoline, even if it can't yet follow what's inside. This means the ORC unwinder will fail to unwind from inside a retpoline, but will work fine otherwise.

---
 arch/x86/include/asm/nospec-branch.h | 27 +++++++++++++++-
 tools/objtool/check.c                | 62 ++++++++++++++++++++++++++++++++----
 tools/objtool/check.h                |  2 +-
 3 files changed, 83 insertions(+), 8 deletions(-)

index 7d70ea977fbe..9c4e1d985e40 100644
@@ -9,6 +9,19 @@
 
 #ifdef __ASSEMBLY__
 
+/*
+ * This should be used immediately before a retpoline alternative.  It tells
+ * objtool where the retpolines are so that it can make sense of the control
+ * flow by just reading the original instruction(s) and ignoring the
+ * alternatives.
+ */
+.macro ANNOTATE_NOSPEC_ALTERNATIVE
+	.Lannotate_\@:
+	.pushsection .discard.nospec
+	.long .Lannotate_\@ - .
+	.popsection
+.endm
+
 /*
  * These are the bare retpoline primitives for indirect jmp and call.
  * Do not use these directly; they only exist to make the ALTERNATIVE

@@ -43,6 +56,7 @@
  */
 .macro JMP_NOSPEC reg:req
 #ifdef CONFIG_RETPOLINE
+	ANNOTATE_NOSPEC_ALTERNATIVE
 	ALTERNATIVE_2 __stringify(jmp *\reg),				\
 		__stringify(RETPOLINE_JMP \reg), X86_FEATURE_RETPOLINE,	\
 		__stringify(lfence; jmp *\reg), X86_FEATURE_RETPOLINE_AMD

@@ -53,6 +67,7 @@
 
 .macro CALL_NOSPEC reg:req
 #ifdef CONFIG_RETPOLINE
+	ANNOTATE_NOSPEC_ALTERNATIVE
 	ALTERNATIVE_2 __stringify(call *\reg),				\
 		__stringify(RETPOLINE_CALL \reg), X86_FEATURE_RETPOLINE,\
 		__stringify(lfence; call *\reg), X86_FEATURE_RETPOLINE_AMD

@@ -64,15 +79,25 @@
 #else /* __ASSEMBLY__ */
 
 #if defined(CONFIG_X86_64) && defined(RETPOLINE)
+
+#define ANNOTATE_NOSPEC_ALTERNATIVE				\
+	"999:\n\t"						\
+	".pushsection .discard.nospec\n\t"			\
+	".long 999b - .\n\t"					\
+	".popsection\n\t"
+
 /*
  * Since the inline asm uses the %V modifier which is only in newer GCC,
  * the 64-bit one is dependent on RETPOLINE not CONFIG_RETPOLINE.
  */
-# define CALL_NOSPEC ALTERNATIVE(				\
+# define CALL_NOSPEC						\
+	ANNOTATE_NOSPEC_ALTERNATIVE				\
+	ALTERNATIVE(						\
 	"call *%[thunk_target]\n",				\
 	"call __x86_indirect_thunk_%V[thunk_target]\n",		\
 	X86_FEATURE_RETPOLINE)
 # define THUNK_TARGET(addr) [thunk_target] "r" (addr)
+
 #elif defined(CONFIG_X86_32) && defined(CONFIG_RETPOLINE)
 /*
  * For i386 we use the original ret-equivalent retpoline, because

index 47d9ea70a83d..dbadb304a410 100644
@@ -44,7 +44,7 @@ struct instruction {
 	unsigned int len;
 	unsigned char type;
 	unsigned long immediate;
-	bool alt_group, visited, dead_end, ignore, hint, save, restore;
+	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct list_head alts;

2.14.3

I seem to recall that not working with inline asm, maybe old versions of GCC don't like it or something? I can try it and see if 0-day bot complains.

Josh

You just need %= (for inline asm) instead of \@ (for .macro).

I already fixed it up in
http://git.infradead.org/users/dwmw2/linux-retpoline.git/ and will get nagmail from 0day shortly if it doesn't work :)

(I love you Fengguang)

I found a description from an old commit of mine:

3d1e236022cc ("objtool: Prevent GCC from merging annotate_unreachable()")

The above macro is protected by '#ifdef RETPOLINE', and I seriously doubt 0-day is testing with an unreleased version of GCC. So you shouldn't see a 0-day warning.

I think I heard that retpolines won't be ported to anything older than GCC 4.9, so maybe it's safe to use '%='. I don't remember when it was introduced into GCC though.

Josh

It's actually #ifdef CONFIG_RETPOLINE isn't it?

If you enable CONFIG_RETPOLINE but don't have a new compiler, you still get all the asm thunks (which are the easy-to-attack targets). Only if you have a new compiler is RETPOLINE also set.

Also, the RSB stuffing we're looking at here is also needed for the IBRS-based mitigation, so won't even be under CONFIG_RETPOLINE by the time the IBRS patch set is beaten into shape on top. It'll probably be unconditional unless we get a CONFIG_IBRS_SUPPORT (which hasn't been suggested so far).

Hm. Peter? This is all your fault, right? Did you know you were making us ditch compatibility for older GCC?

Precisely when *did* %= get added to GCC?

Note that we can also just resort to using .macro even from inline asm. It just takes a rather icky asm(".include ..."). :)

#if defined(CONFIG_X86_64) && defined(RETPOLINE)

#define ANNOTATE_NOSPEC_ALTERNATIVE				\
	".Lannotate_%=:\n\t"					\
	".pushsection .discard.nospec\n\t"			\
	".long .Lannotate_%= - .\n\t"				\
	".popsection\n\t"

/*
* Since the inline asm uses the %V modifier which is only in newer GCC,
* the 64-bit one is dependent on RETPOLINE not CONFIG_RETPOLINE. */

# define CALL_NOSPEC						\
...

True. Maybe try changing the above to CONFIG_RETPOLINE and see if 0-day complains.

I'm still scratching my head about this one. From what I can tell, even 4.4 has it. I can't find any notes or old 0-day warnings which would tell me, but I remember running into the problem multiple times, so I'm pretty sure I'm not hallucinating.

Ew :-)

Josh

root@interlagos:~/tmp# gcc-4.8 -o test test.c
root@interlagos:~/tmp# ./test
11

--- test.c

#include <stdio.h>

void main(void)

{
        int val;

        asm ("mov $(%=),%0" : "=A" (val));

        printf("%d\n", val);
}

	# gcc --version
	gcc (SUSE Linux) 4.3.4 [gcc-4_3-branch revision 152973]
	# ./test 
	8

So this part is OK. The asm-goto hard dependency would be worse with that compiler though.

Jiri Kosina
SUSE Labs

I'm afraid we'll have to backport retpolines in some form to 4.3.x at least. I'd be surprised if we'd be the only ones on this planet :)

Jiri Kosina
SUSE Labs

So upstream code is going to require 4.5 at least, and 4.4 has %=. Backport effort will just have to cope or backport more GCC bits, that is, if you're backporting retpoline to 4.3 also backport asm-goto.

Again, the RSB thing is for more than just retpoline; it's needed for IBRS support too and that *doesn't* necessarily require a newer compiler at all.

But really, if %= is supported at least as far back as 4.4 (and maybe further?) then I'm not going to care about it unless someone really screams.

I think we're fairly much done, unless Andi you really want to make the RSB stuffing code out-of-line?

Thomas, do you want to leave Josh's patches on top with a revert, as they are at the moment in my tree, or rebase and fold them in as we go?

I think the asm-goto was exactly for IBRS, because without that you get to sprinkle LFENCE all over the place.

Note that we'll raise the GCC limit to 4.5 soon to mandate asm-goto, for x86 at least.

Actually, I think PeterZ is wrong on this one.

First off, we do *not* use %= in inline asms in the kernel yet, and we shouldn't start just because of these.

Secondly, we use lots of the the "small numbers for local labels" both in inline asm and in *.S files.

I think doing

jne 1f
...
1:

is a _hell_ of a lot more legible than

        jne .LPrefix_%
    ...

.LPrefix_%

unless you have some *major* reason to use an explicit label name.

Sure, if you grew up writing perl, and think that an illegible mess of random characters is a requirement for programming, then the ".L%" format looks natural.

But if you're an actual human, the "small numbers as labels" is fine.

The one requirement for the small numbers thing is that you really have to give the direction when you use them. Because re-using the same number *will* happen, and is normal.

That, btw, is also why it's pointless to make the small numbers "bigger". Using "1122" as a label is actively worse than just using "1".

You shouldn't try to fool yourself and think that your number is "unique". It doesn't need to be. A label needs to be unique within one use, and the use just tells _which_ of the non-unique numbers you use.

And yes, you could even re-use the number within one macro or inline asm. But that's just confusing. Make the number unique for the particular macro or inline use.

Linus

What if the macro is used by another macro which needs to jump across it? If the labels conflict then the outer macro will accidentally jump to the inner macro.

Josh

OK. I'll change it back.

Small number maybe; but the value at hand was a random 999 or so, which is not a small number.

But I find a descriptive label ever so much better than a random number.

I grew up on BASIC and have bad memories of random big number goto. I'll take those random trailing character any day if it includes human readable bits before.

I find descriptive labels much nicer than random numbers, I'll take some crazy characters if so required.

Consider the retpoline thing:

call .Lset_up_target
.Lcapture_spec:
pause
jmp .Lcapture_spec
.Lset_up_target:
mov %r11, (%rsp);
ret;

over:

call 2f
1:
pause
jmp 1b
2:
mov %r11, (%rsp)
ret

give me the first any day of the week.

Absolutely, in fact I detest what we have:

+#define __FILL_RETURN_BUFFER(reg, nr, sp)      \
+       mov     $(nr/2), reg;                   \
+771:                                           \
+       call    772f;                           \
+773:   /* speculation trap */                  \
+       pause;                                  \
+       jmp     773b;                           \
+772:                                           \
+       call    774f;                           \
+775:   /* speculation trap */                  \
+       pause;                                  \
+       jmp     775b;                           \
+774:                                           \
+       dec     reg;                            \
+       jnz     771b;                           \

+ add $(BITS_PER_LONG/8) * nr, sp;

I mean, WTF??

Also, note that this:

becomes even more readable with a bit more human-readability improvements:

call .L_set_up_target

.L_capture_spec:
pause
jmp .L_capture_spec

.L_set_up_target:

	mov %r11, (%rsp)
	ret 

I.e. the underscores in the label names, the tabs and the newlines create better vertical and horizontal separation between the syntactical elements and provide grouping of larger constructs.

Thanks,

Ingo

Actually in macros I don't think that's entirely true (depending on the assembler/preprocessor behaviour, which is often surprising).

You want to use labels in macros which are not going to conflict with what the human has typed into their .S file. If they have code along the lines of

  jnz 1f
INVOKE_MACRO
1:

... then you surely don't want to be using the label '1' in your macro.

I'm fairly sure that's true if you're using CPP macros (which we seem to do most of the time even in .S files). It might actually DTRT if you are using .macro; I'm not sure.

So I will go back to numeric labels, as I said. But not '1:'. :)

This reverts commit 59b6e22f92f9a86dbd0798db72adc97bdb831f86.

Now that objtool is retpoline-aware, we can re-enable objtool and all its friends, including ORC and the livepatch consistency model.

---
 arch/x86/Kconfig       | 4 ++--
 arch/x86/Kconfig.debug | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

index abeac4b80b74..d1819161cc6c 100644
@@ -172,8 +172,8 @@ config X86
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_RCU_TABLE_FREE
 	select HAVE_REGS_AND_STACK_ACCESS_API
-	select HAVE_RELIABLE_STACKTRACE		if X86_64 && UNWINDER_FRAME_POINTER && STACK_VALIDATION && !RETPOLINE
-	select HAVE_STACK_VALIDATION		if X86_64 && !RETPOLINE
+	select HAVE_RELIABLE_STACKTRACE		if X86_64 && UNWINDER_FRAME_POINTER && STACK_VALIDATION
+	select HAVE_STACK_VALIDATION		if X86_64
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select HAVE_USER_RETURN_NOTIFIER

index 9f3928d744bc..6293a8768a91 100644
@@ -359,8 +359,8 @@ config PUNIT_ATOM_DEBUG
 
 choice
 	prompt "Choose kernel unwinder"
-	default UNWINDER_ORC if X86_64 && !RETPOLINE
-	default UNWINDER_FRAME_POINTER if X86_32 || RETPOLINE
+	default UNWINDER_ORC if X86_64
+	default UNWINDER_FRAME_POINTER if X86_32
 	---help---
 	  This determines which method will be used for unwinding kernel stack
 	  traces for panics, oopses, bugs, warnings, perf, /proc/<pid>/stack,

@@ -368,7 +368,7 @@ choice
 
 config UNWINDER_ORC
 	bool "ORC unwinder"
-	depends on X86_64 && !RETPOLINE
+	depends on X86_64
 	select STACK_VALIDATION
 	---help---
 	  This option enables the ORC (Oops Rewind Capability) unwinder for

2.14.3

Brilliant, thank you. Can you take a look at the latest RSB stuffing code in http://git.infradead.org/users/dwmw2/linux-retpoline.git/ too please?

/bin/sh: line 1: 26242 Segmentation fault      (core dumped) ./tools/objtool/objtool orc generate --no-fp "arch/x86/kvm/vmx.o"