[PATCH 0/3] objtool: retpoline compatibility

[PATCH 0/3] objtool: retpoline compatibility

[Josh Poimboeuf]

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

[PATCH 1/3] objtool: Detect jumps to retpoline thunks

[Josh Poimboeuf]

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

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 9b341584eb1b..de053fb7049b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -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

[PATCH 2/3] objtool: Ignore retpoline alternatives

[Josh Poimboeuf]

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.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 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(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 7d70ea977fbe..9c4e1d985e40 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -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
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index de053fb7049b..f40d46e24bcc 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -427,6 +427,40 @@ static void add_ignores(struct objtool_file *file)
 	}
 }
 
+/*
+ * FIXME: For now, just ignore any alternatives which add retpolines.  This is
+ * a temporary hack, as it doesn't allow ORC to unwind from inside a retpoline.
+ * But it at least allows objtool to understand the control flow *around* the
+ * retpoline.
+ */
+static int add_nospec_ignores(struct objtool_file *file)
+{
+	struct section *sec;
+	struct rela *rela;
+	struct instruction *insn;
+
+	sec = find_section_by_name(file->elf, ".rela.discard.nospec");
+	if (!sec)
+		return 0;
+
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		if (rela->sym->type != STT_SECTION) {
+			WARN("unexpected relocation symbol type in %s", sec->name);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("bad .discard.nospec entry");
+			return -1;
+		}
+
+		insn->ignore_alts = true;
+	}
+
+	return 0;
+}
+
 /*
  * Find the destination instructions for all jumps.
  */
@@ -509,11 +543,18 @@ static int add_call_destinations(struct objtool_file *file)
 			dest_off = insn->offset + insn->len + insn->immediate;
 			insn->call_dest = find_symbol_by_offset(insn->sec,
 								dest_off);
+			/*
+			 * FIXME: Thanks to retpolines, it's now considered
+			 * normal for a function to call within itself.  So
+			 * disable this warning for now.
+			 */
+#if 0
 			if (!insn->call_dest) {
 				WARN_FUNC("can't find call dest symbol at offset 0x%lx",
 					  insn->sec, insn->offset, dest_off);
 				return -1;
 			}
+#endif
 		} else if (rela->sym->type == STT_SECTION) {
 			insn->call_dest = find_symbol_by_offset(rela->sym->sec,
 								rela->addend+4);
@@ -678,12 +719,6 @@ static int add_special_section_alts(struct objtool_file *file)
 		return ret;
 
 	list_for_each_entry_safe(special_alt, tmp, &special_alts, list) {
-		alt = malloc(sizeof(*alt));
-		if (!alt) {
-			WARN("malloc failed");
-			ret = -1;
-			goto out;
-		}
 
 		orig_insn = find_insn(file, special_alt->orig_sec,
 				      special_alt->orig_off);
@@ -694,6 +729,10 @@ static int add_special_section_alts(struct objtool_file *file)
 			goto out;
 		}
 
+		/* Ignore retpoline alternatives. */
+		if (orig_insn->ignore_alts)
+			continue;
+
 		new_insn = NULL;
 		if (!special_alt->group || special_alt->new_len) {
 			new_insn = find_insn(file, special_alt->new_sec,
@@ -719,6 +758,13 @@ static int add_special_section_alts(struct objtool_file *file)
 				goto out;
 		}
 
+		alt = malloc(sizeof(*alt));
+		if (!alt) {
+			WARN("malloc failed");
+			ret = -1;
+			goto out;
+		}
+
 		alt->insn = new_insn;
 		list_add_tail(&alt->list, &orig_insn->alts);
 
@@ -1035,6 +1081,10 @@ static int decode_sections(struct objtool_file *file)
 
 	add_ignores(file);
 
+	ret = add_nospec_ignores(file);
+	if (ret)
+		return ret;
+
 	ret = add_jump_destinations(file);
 	if (ret)
 		return ret;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 47d9ea70a83d..dbadb304a410 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -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

[PATCH 3/3] Revert "x86/retpoline: Temporarily disable objtool when CONFIG_RETPOLINE=y"

[Josh Poimboeuf]

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.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/Kconfig       | 4 ++--
 arch/x86/Kconfig.debug | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index abeac4b80b74..d1819161cc6c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -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
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 9f3928d744bc..6293a8768a91 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -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

Re: [PATCH 0/3] objtool: retpoline compatibility

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On Wed, 2018-01-10 at 19:48 -0600, Josh Poimboeuf wrote:
> 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(-)

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"

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

On Wed, 2018-01-10 at 19:48 -0600, Josh Poimboeuf wrote:
> 
> +#define ANNOTATE_NOSPEC_ALTERNATIVE                            \
> +       "999:\n\t"                                              \
> +       ".pushsection .discard.nospec\n\t"                      \
> +       ".long 999b - .\n\t"                                    \
> +       ".popsection\n\t"
> +

<mode name="peterz">
  Ick, numbers. Use .Lfoo_%= instead.
</mode>

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Josh Poimboeuf]

On Thu, Jan 11, 2018 at 04:27:38PM +0000, David Woodhouse wrote:
> On Wed, 2018-01-10 at 19:48 -0600, Josh Poimboeuf wrote:
> > 
> > +#define ANNOTATE_NOSPEC_ALTERNATIVE                            \
> > +       "999:\n\t"                                              \
> > +       ".pushsection .discard.nospec\n\t"                      \
> > +       ".long 999b - .\n\t"                                    \
> > +       ".popsection\n\t"
> > +
> 
> <mode name="peterz">
>   Ick, numbers. Use .Lfoo_%= instead.
> </mode>

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

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Thu, 2018-01-11 at 10:33 -0600, Josh Poimboeuf wrote:
> On Thu, Jan 11, 2018 at 04:27:38PM +0000, David Woodhouse wrote:
> > 
> > On Wed, 2018-01-10 at 19:48 -0600, Josh Poimboeuf wrote:
> > > 
> > > 
> > > +#define ANNOTATE_NOSPEC_ALTERNATIVE                            \
> > > +       "999:\n\t"                                              \
> > > +       ".pushsection .discard.nospec\n\t"                      \
> > > +       ".long 999b - .\n\t"                                    \
> > > +       ".popsection\n\t"
> > > +
> > 
> >   Ick, numbers. Use .Lfoo_%= instead.
> > 
> 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.

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)

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Peter Zijlstra]

On Thu, Jan 11, 2018 at 10:33:26AM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 11, 2018 at 04:27:38PM +0000, David Woodhouse wrote:
> > On Wed, 2018-01-10 at 19:48 -0600, Josh Poimboeuf wrote:
> > > 
> > > +#define ANNOTATE_NOSPEC_ALTERNATIVE                            \
> > > +       "999:\n\t"                                              \
> > > +       ".pushsection .discard.nospec\n\t"                      \
> > > +       ".long 999b - .\n\t"                                    \
> > > +       ".popsection\n\t"
> > > +
> > 
> > <mode name="peterz">
> >   Ick, numbers. Use .Lfoo_%= instead.
> > </mode>
> 
> 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.

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

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Josh Poimboeuf]

On Thu, Jan 11, 2018 at 04:39:38PM +0000, David Woodhouse wrote:
> On Thu, 2018-01-11 at 10:33 -0600, Josh Poimboeuf wrote:
> > On Thu, Jan 11, 2018 at 04:27:38PM +0000, David Woodhouse wrote:
> > > 
> > > On Wed, 2018-01-10 at 19:48 -0600, Josh Poimboeuf wrote:
> > > > 
> > > > 
> > > > +#define ANNOTATE_NOSPEC_ALTERNATIVE                            \
> > > > +       "999:\n\t"                                              \
> > > > +       ".pushsection .discard.nospec\n\t"                      \
> > > > +       ".long 999b - .\n\t"                                    \
> > > > +       ".popsection\n\t"
> > > > +
> > > 
> > >   Ick, numbers. Use .Lfoo_%= instead.
> > > 
> > 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.
> 
> 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()")

> A workaround for this issue is to ensure that each instance of the
> inline asm statement uses a different label, so that GCC sees the
> statements are unique and leaves them alone.  The inline asm ‘%=’ token
> could be used for that, but unfortunately older versions of GCC don't
> support it.  So I implemented a poor man's version of it with the
> __LINE__ macro.

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

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

On Thu, 2018-01-11 at 10:48 -0600, Josh Poimboeuf wrote:
> 
> 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.

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

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

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Peter Zijlstra]

On Thu, Jan 11, 2018 at 10:48:26AM -0600, Josh Poimboeuf wrote:
> 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.

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);
}

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Jiri Kosina]

On Thu, 11 Jan 2018, Josh Poimboeuf wrote:

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

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

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Jiri Kosina]

On Thu, 11 Jan 2018, Peter Zijlstra wrote:

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

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Peter Zijlstra]

On Thu, Jan 11, 2018 at 06:01:23PM +0100, Jiri Kosina wrote:
> On Thu, 11 Jan 2018, Josh Poimboeuf wrote:
> 
> > 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.
> 
> 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 :)

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.

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

On Thu, 2018-01-11 at 18:05 +0100, Peter Zijlstra wrote:
> On Thu, Jan 11, 2018 at 06:01:23PM +0100, Jiri Kosina wrote:
> > On Thu, 11 Jan 2018, Josh Poimboeuf wrote:
> > 
> > > 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.
> > 
> > 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 :)
> 
> 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?

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Peter Zijlstra]

On Thu, Jan 11, 2018 at 05:19:23PM +0000, David Woodhouse wrote:
> On Thu, 2018-01-11 at 18:05 +0100, Peter Zijlstra wrote:
> > On Thu, Jan 11, 2018 at 06:01:23PM +0100, Jiri Kosina wrote:
> > > On Thu, 11 Jan 2018, Josh Poimboeuf wrote:
> > > 
> > > > 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.
> > > 
> > > 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 :)
> > 
> > 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.

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

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Josh Poimboeuf]

On Thu, Jan 11, 2018 at 04:55:18PM +0000, David Woodhouse wrote:
> On Thu, 2018-01-11 at 10:48 -0600, Josh Poimboeuf wrote:
> > 
> > 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.
> 
> 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.

#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						\
...

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

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

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

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.

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

Ew :-)

-- 
Josh

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Linus Torvalds]

On Thu, Jan 11, 2018 at 8:27 AM, David Woodhouse <dwmw2@infradead.org> wrote:
>
> <mode name="peterz">
>   Ick, numbers. Use .Lfoo_%= instead.
> </mode>

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

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Josh Poimboeuf]

On Thu, Jan 11, 2018 at 09:29:48AM -0800, Linus Torvalds wrote:
> 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.

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

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

On Thu, 2018-01-11 at 09:29 -0800, Linus Torvalds wrote:
> On Thu, Jan 11, 2018 at 8:27 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> >
> > 
> >   Ick, numbers. Use .Lfoo_%= instead.
> > 
> 
> 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.

OK. I'll change it back.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Peter Zijlstra]

On Thu, Jan 11, 2018 at 09:29:48AM -0800, Linus Torvalds wrote:

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

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.

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

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.

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

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.

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

On Thu, 2018-01-11 at 09:29 -0800, Linus Torvalds wrote:
> 
> 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".

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

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH 2/3] objtool: Ignore retpoline alternatives

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> > But if you're an actual human, the "small numbers as labels" is fine.
> 
> 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:

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

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