Skip to contentSkip to conversation

objtool: retpoline and asm-goto validation

PATCH In review Peter Zijlstra 7 participants 71 messages Jan 23, 2018 – Jan 31, 2018

Subsystems (11)
Documentation x86 Kbuild Scheduler +7 more subsystems
lore
Share & tools
lore

Peter Zijlstra

author maintainer
#0 lore raw

cover objtool: retpoline and asm-goto validation

This series adds two features to objtool:

- retpoline validation; it verifies no indirect jumps/calls are left in CONFIG_RETPOLINE=y builds.

includes fixups for kvm and others

- asm-goto validation; allows asserting that a block of code is not reachable through a dynamic branch.

After this it forces an asm-goto capable compiler to build arch/x86 and removes the fallback code for static_cpu_has(). After which we can use static_cpu_has() to guarantee a lack of dynamic branches, which is a requested feature for various spectre related things.

author maintainer
#1 lore raw

PATCH 1/24 objtool: Use existing global variables for options

Use the existing global variables instead of passing them around and creating duplicate global variables.

---
 tools/objtool/builtin-check.c |    2 +-
 tools/objtool/builtin-orc.c   |    6 +-----
 tools/objtool/builtin.h       |    5 +++++
 tools/objtool/check.c         |    5 ++---
 tools/objtool/check.h         |    2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)
tools/objtool/builtin-check.c +1 -1
@@ -53,5 +53,5 @@ int cmd_check(int argc, const char **arg
 
 	objname = argv[0];
 
-	return check(objname, no_fp, no_unreachable, false);
+	return check(objname, false);
 }
tools/objtool/builtin-orc.c +1 -5
@@ -25,7 +25,6 @@
  */
 
 #include <string.h>
-#include <subcmd/parse-options.h>
 #include "builtin.h"
 #include "check.h"
 
@@ -36,9 +35,6 @@ static const char *orc_usage[] = {
 	NULL,
 };
 
-extern const struct option check_options[];
-extern bool no_fp, no_unreachable;
-
 int cmd_orc(int argc, const char **argv)
 {
 	const char *objname;
@@ -51,7 +47,7 @@ int cmd_orc(int argc, const char **argv)
 
 		objname = argv[0];
 
-		return check(objname, no_fp, no_unreachable, true);
+		return check(objname, true);
 
 	}
 
tools/objtool/builtin.h +5 -0
@@ -17,6 +17,11 @@
 #ifndef _BUILTIN_H
 #define _BUILTIN_H
 
+#include <subcmd/parse-options.h>
+
+extern const struct option check_options[];
+extern bool no_fp, no_unreachable;
+
 extern int cmd_check(int argc, const char **argv);
 extern int cmd_orc(int argc, const char **argv);
 
tools/objtool/check.c +2 -3
@@ -18,6 +18,7 @@
 #include <string.h>
 #include <stdlib.h>
 
+#include "builtin.h"
 #include "check.h"
 #include "elf.h"
 #include "special.h"
@@ -33,7 +34,6 @@ struct alternative {
 };
 
 const char *objname;
-static bool no_fp;
 struct cfi_state initial_func_cfi;
 
 struct instruction *find_insn(struct objtool_file *file,
@@ -1973,13 +1973,12 @@ static void cleanup(struct objtool_file
 	elf_close(file->elf);
 }
 
-int check(const char *_objname, bool _no_fp, bool no_unreachable, bool orc)
+int check(const char *_objname, bool orc)
 {
 	struct objtool_file file;
 	int ret, warnings = 0;
 
 	objname = _objname;
-	no_fp = _no_fp;
 
 	file.elf = elf_open(objname, orc ? O_RDWR : O_RDONLY);
 	if (!file.elf)
tools/objtool/check.h +1 -1
@@ -62,7 +62,7 @@ struct objtool_file {
 	bool ignore_unreachables, c_file, hints;
 };
 
-int check(const char *objname, bool no_fp, bool no_unreachable, bool orc);
+int check(const char *objname, bool orc);
 
 struct instruction *find_insn(struct objtool_file *file,
 			      struct section *sec, unsigned long offset);
author maintainer
#2 lore raw

PATCH 2/24 objtool: Add retpoline validation

In reply to Peter Zijlstra

David requested a objtool validation pass for RETPOLINE enabled builds, where it validates no unannotated indirect jumps or calls are left.

Add an additional .discard.retpoline_safe section to allow annotating the few indirect sites that are required and safe.

---
 scripts/Makefile.build        |    4 +
 tools/objtool/builtin-check.c |    3 -
 tools/objtool/builtin.h       |    2 
 tools/objtool/check.c         |   87 ++++++++++++++++++++++++++++++++++++++++--
 tools/objtool/check.h         |    1 
 5 files changed, 92 insertions(+), 5 deletions(-)
scripts/Makefile.build +4 -0
@@ -269,6 +269,10 @@ objtool_args += --no-unreachable
 else
 objtool_args += $(call cc-ifversion, -lt, 0405, --no-unreachable)
 endif
+ifdef CONFIG_RETPOLINE
+  objtool_args += --retpoline
+endif
+
 
 ifdef CONFIG_MODVERSIONS
 objtool_o = $(@D)/.tmp_$(@F)
tools/objtool/builtin-check.c +2 -1
@@ -29,7 +29,7 @@
 #include "builtin.h"
 #include "check.h"
 
-bool no_fp, no_unreachable;
+bool no_fp, no_unreachable, retpoline;
 
 static const char * const check_usage[] = {
 	"objtool check [<options>] file.o",
@@ -39,6 +39,7 @@ static const char * const check_usage[]
 const struct option check_options[] = {
 	OPT_BOOLEAN('f', "no-fp", &no_fp, "Skip frame pointer validation"),
 	OPT_BOOLEAN('u', "no-unreachable", &no_unreachable, "Skip 'unreachable instruction' warnings"),
+	OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"),
 	OPT_END(),
 };
 
tools/objtool/builtin.h +1 -1
@@ -20,7 +20,7 @@
 #include <subcmd/parse-options.h>
 
 extern const struct option check_options[];
-extern bool no_fp, no_unreachable;
+extern bool no_fp, no_unreachable, retpoline;
 
 extern int cmd_check(int argc, const char **argv);
 extern int cmd_orc(int argc, const char **argv);
tools/objtool/check.c +84 -3

127 lines not on this page · raw message

tools/objtool/check.h +1 -0
@@ -45,6 +45,7 @@ struct instruction {
 	unsigned char type;
 	unsigned long immediate;
 	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
+	bool retpoline_safe;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct list_head alts;
maintainer
#3 lore raw

Re: [PATCH 02/24] objtool: Add retpoline validation

Quoted 9 lines · Peter Zijlstra

David requested a objtool validation pass for RETPOLINE enabled builds, where it validates no unannotated indirect jumps or calls are left.

Add an additional .discard.retpoline_safe section to allow annotating the few indirect sites that are required and safe.

Quoted 7 lines
---
 scripts/Makefile.build        |    4 +
 tools/objtool/builtin-check.c |    3 -
 tools/objtool/builtin.h       |    2 
 tools/objtool/check.c         |   87 ++++++++++++++++++++++++++++++++++++++++--
 tools/objtool/check.h         |    1 
 5 files changed, 92 insertions(+), 5 deletions(-)

Yap, very noisy:

$ grep indirect build.log | wc -l
12904

Let's see how many will remain at the end of the patchset. :)

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
Reviewed-by on v1 2/24
#4 lore raw

Re: [PATCH 02/24] objtool: Add retpoline validation

In reply to Peter Zijlstra
Quoted 6 lines · Peter Zijlstra

+               if (insn->type != INSN_JUMP_DYNAMIC &&
+                   insn->type != INSN_CALL_DYNAMIC) {
+                       WARN_FUNC("retpoline_safe hint not a indirect jump/call", +                                 insn->sec, insn->offset);
+                       return -1;

...

        case 0xff:
                if (modrm_reg == 2 || modrm_reg == 3)

                        *type = INSN_CALL_DYNAMIC;

                else if (modrm_reg == 4)

                        *type = INSN_JUMP_DYNAMIC;

                else if (modrm_reg == 5)

                        /* jmpf */
                        *type = INSN_CONTEXT_SWITCH;

I *think* your check includes far calls (FF/3), although not far jumps? It shouldn't, because I don't believe far calls are subject to the same speculation?

Other than that, which you can probably ignore if you didn't have to explicitly annotate [m]any safe far calls anyway,

Thanks for doing this.

author maintainer
#5 lore raw

PATCH 3/24 x86/paravirt: Annotate indirect calls

In reply to Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

---
 arch/x86/include/asm/paravirt.h       |   22 ++++++++++++++++++----
 arch/x86/include/asm/paravirt_types.h |    7 ++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)
arch/x86/include/asm/paravirt.h +18 -4
@@ -828,6 +828,12 @@ extern void default_banner(void);
 	 .short clobbers;			\
 	.popsection
 
+#define PARA_RETPOLINE_SAFE				\
+	773:;						\
+	.pushsection .discard.retpoline_safe;		\
+	_ASM_PTR 773b;					\
+	.popsection
+
 
 #define COND_PUSH(set, mask, reg)			\
 	.if ((~(set)) & mask); push %reg; .endif
@@ -879,23 +885,27 @@ extern void default_banner(void);
 
 #define INTERRUPT_RETURN						\
 	PARA_SITE(PARA_PATCH(pv_cpu_ops, PV_CPU_iret), CLBR_NONE,	\
-		  jmp PARA_INDIRECT(pv_cpu_ops+PV_CPU_iret))
+		  PARA_RETPOLINE_SAFE;					\
+		  jmp PARA_INDIRECT(pv_cpu_ops+PV_CPU_iret);)
 
 #define DISABLE_INTERRUPTS(clobbers)					\
 	PARA_SITE(PARA_PATCH(pv_irq_ops, PV_IRQ_irq_disable), clobbers, \
 		  PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);		\
+		  PARA_RETPOLINE_SAFE;					\
 		  call PARA_INDIRECT(pv_irq_ops+PV_IRQ_irq_disable);	\
 		  PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE);)
 
 #define ENABLE_INTERRUPTS(clobbers)					\
 	PARA_SITE(PARA_PATCH(pv_irq_ops, PV_IRQ_irq_enable), clobbers,	\
 		  PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);		\
+		  PARA_RETPOLINE_SAFE;					\
 		  call PARA_INDIRECT(pv_irq_ops+PV_IRQ_irq_enable);	\
 		  PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE);)
 
 #ifdef CONFIG_X86_32
 #define GET_CR0_INTO_EAX				\
 	push %ecx; push %edx;				\
+	PARA_RETPOLINE_SAFE;				\
 	call PARA_INDIRECT(pv_cpu_ops+PV_CPU_read_cr0);	\
 	pop %edx; pop %ecx
 #else	/* !CONFIG_X86_32 */
@@ -917,21 +927,25 @@ extern void default_banner(void);
  */
 #define SWAPGS								\
 	PARA_SITE(PARA_PATCH(pv_cpu_ops, PV_CPU_swapgs), CLBR_NONE,	\
-		  call PARA_INDIRECT(pv_cpu_ops+PV_CPU_swapgs)		\
+		  PARA_RETPOLINE_SAFE;					\
+		  call PARA_INDIRECT(pv_cpu_ops+PV_CPU_swapgs);		\
 		 )
 
 #define GET_CR2_INTO_RAX				\
-	call PARA_INDIRECT(pv_mmu_ops+PV_MMU_read_cr2)
+	PARA_RETPOLINE_SAFE;				\
+	call PARA_INDIRECT(pv_mmu_ops+PV_MMU_read_cr2);
 
 #define USERGS_SYSRET64							\
 	PARA_SITE(PARA_PATCH(pv_cpu_ops, PV_CPU_usergs_sysret64),	\
 		  CLBR_NONE,						\
-		  jmp PARA_INDIRECT(pv_cpu_ops+PV_CPU_usergs_sysret64))
+		  PARA_RETPOLINE_SAFE;					\
+		  jmp PARA_INDIRECT(pv_cpu_ops+PV_CPU_usergs_sysret64);)
 
 #ifdef CONFIG_DEBUG_ENTRY
 #define SAVE_FLAGS(clobbers)                                        \
 	PARA_SITE(PARA_PATCH(pv_irq_ops, PV_IRQ_save_fl), clobbers, \
 		  PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);        \
+		  PARA_RETPOLINE_SAFE;				    \
 		  call PARA_INDIRECT(pv_irq_ops+PV_IRQ_save_fl);    \
 		  PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE);)
 #endif
arch/x86/include/asm/paravirt_types.h +6 -1
@@ -392,7 +392,12 @@ int paravirt_disable_iospace(void);
  * offset into the paravirt_patch_template structure, and can therefore be
  * freely converted back into a structure offset.
  */
-#define PARAVIRT_CALL	"call *%c[paravirt_opptr];"
+#define PARAVIRT_CALL					\
+	"773:;\n"					\
+	".pushsection .discard.retpoline_safe\n"	\
+	_ASM_PTR " 773b\n"				\
+	".popsection\n"					\
+	"call *%c[paravirt_opptr];"
 
 /*
  * These macros are intended to wrap calls through one of the paravirt
#6 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

Quoted 3 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

I've seen this asserted repeatedly but I've never truly convinced myself of it. Is this absolutely unconditionally true in every case, even when we're running as a guest and there are *actual* calls to be made? We turn them into direct calls, never leave them indirect?

author maintainer
#7 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

Quoted 9 lines · David Woodhouse
Quoted 3 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

I've seen this asserted repeatedly but I've never truly convinced myself of it. Is this absolutely unconditionally true in every case, even when we're running as a guest and there are *actual* calls to be made? We turn them into direct calls, never leave them indirect?

That is my understanding; and when I worked on the paravirt spinlock code and disassembled live guest code this seemed to have happend.

But let me go read the paravirt code again to make a stronger argument in favour.

maintainer
#8 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · David Woodhouse
Quoted 3 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

I've seen this asserted repeatedly but I've never truly convinced myself of it. Is this absolutely unconditionally true in every case, even when we're running as a guest and there are *actual* calls to be made? We turn them into direct calls, never leave them indirect?

Quoted 6 lines

That is my understanding; and when I worked on the paravirt spinlock code and disassembled live guest code this seemed to have happend.

But let me go read the paravirt code again to make a stronger argument in favour.

paravirt_patch_default() is the function you want to look at: it either replaces the indirect call by some cutom code (which is never an indirect call) or by a call of the target function.

Juergen

#9 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

Quoted 16 lines · Juergen Gross
Quoted 15 lines · Peter Zijlstra
Quoted 10 lines · David Woodhouse
Quoted 4 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

I've seen this asserted repeatedly but I've never truly convinced myself of it. Is this absolutely unconditionally true in every case, even when we're running as a guest and there are *actual* calls to be made? We turn them into direct calls, never leave them indirect?

That is my understanding; and when I worked on the paravirt spinlock code and disassembled live guest code this seemed to have happend.

Quoted 6 lines
Quoted 3 lines

But let me go read the paravirt code again to make a stronger argument in favour.

paravirt_patch_default() is the function you want to look at: it either replaces the indirect call by some cutom code (which is never an indirect call) or by a call of the target function.

OK, my brain hurts a bit but I'm happy now. Thank you.

author maintainer
#10 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

Quoted 1 line · David Woodhouse

OK, my brain hurts a bit but I'm happy now. Thank you.

OK, I've updated the Changelog thusly. Is this satisfactory?

---
Subject: x86/paravirt: Annotate indirect calls
From: Peter Zijlstra <[email protected]>
Date: Wed Jan 17 16:58:11 CET 2018

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

This patching happens through alternative_instructions() -> apply_paravirt() -> pv_init_ops.patch() which will eventually end up in paravirt_patch_default(). This function _will_ write direct alternatives.

Reviewed-by
#11 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

Quoted 14 lines · Peter Zijlstra
Quoted 2 lines · David Woodhouse

OK, my brain hurts a bit but I'm happy now. Thank you.

OK, I've updated the Changelog thusly. Is this satisfactory?

---
Subject: x86/paravirt: Annotate indirect calls
From: Peter Zijlstra <[email protected]>
Date: Wed Jan 17 16:58:11 CET 2018

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

Quoted 6 lines

This patching happens through alternative_instructions() -> apply_paravirt() -> pv_init_ops.patch() which will eventually end up in paravirt_patch_default(). This function _will_ write direct alternatives.

I love you, Peter.

#12 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

In reply to Peter Zijlstra
Quoted 13 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

---
 arch/x86/include/asm/paravirt.h       |   22 ++++++++++++++++++----
 arch/x86/include/asm/paravirt_types.h |    7 ++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

arch/x86/include/asm/paravirt.h extern void default_banner(void); · line 835

Quoted 8 lines
arch/x86/include/asm/paravirt.h +5 -0
@@ -828,6 +828,12 @@ extern void default_banner(void);
 	 .short clobbers;			\
 	.popsection
 
+#define PARA_RETPOLINE_SAFE				\
+	773:;						\
+	.pushsection .discard.retpoline_safe;		\
+	_ASM_PTR 773b;					\
+	.popsection

Why does paravirt have its own version of this macro?

Signature · 1 line
Josh
#13 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

Quoted 14 lines · Josh Poimboeuf
Quoted 13 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline
checks, annotate it away because all these indirect calls will be patched out before we start userspace.

---
 arch/x86/include/asm/paravirt.h       |   22 ++++++++++++++++++--- -
 arch/x86/include/asm/paravirt_types.h |    7 ++++++-

 2 files changed, 24 insertions(+), 5 deletions(-)
Quoted 13 lines

arch/x86/include/asm/paravirt.h extern void default_banner(void); · line 835

Quoted 11 lines
arch/x86/include/asm/paravirt.h +0 -0
@@ -828,6 +828,12 @@ extern void default_banner(void);
  	 .short clobbers;			\

  .popsection

arch/x86/include/asm/paravirt.h +5 -0
+#define PARA_RETPOLINE_SAFE				\
+	773:;						\
+	.pushsection .discard.retpoline_safe;		\
+	_ASM_PTR 773b;					\
+	.popsection

Why does paravirt have its own version of this macro?

Because otherwise we'd only have two, and they might get lonely?

author maintainer
#14 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

In reply to Josh Poimboeuf
Quoted 17 lines · Josh Poimboeuf
Quoted 16 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

---
 arch/x86/include/asm/paravirt.h       |   22 ++++++++++++++++++----
 arch/x86/include/asm/paravirt_types.h |    7 ++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)
arch/x86/include/asm/paravirt.h +0 -0
@@ -828,6 +828,12 @@ extern void default_banner(void);
 	 .short clobbers;			\
 	.popsection
 
Quoted 7 lines

arch/x86/include/asm/paravirt.h extern void default_banner(void); · line 835

Quoted 5 lines
arch/x86/include/asm/paravirt.h +5 -0
+#define PARA_RETPOLINE_SAFE				\
+	773:;						\
+	.pushsection .discard.retpoline_safe;		\
+	_ASM_PTR 773b;					\
+	.popsection

Why does paravirt have its own version of this macro?

Every time I touch paravirt.h I land in header hell :/ Maybe it can be made to work, but I was too scared to try this time.

#15 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

In reply to Peter Zijlstra
Quoted 3 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

There's one condition where this isn't true: the 'noreplace-paravirt' cmdline option. I have no idea why that option exists but we should consider just removing it.

Signature · 1 line
Josh
author maintainer
#16 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

Quoted 8 lines · Josh Poimboeuf
Quoted 3 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

There's one condition where this isn't true: the 'noreplace-paravirt' cmdline option. I have no idea why that option exists but we should consider just removing it.

Smells like a debug remnant from back when all this patching crap was new. And yes, that sounds like something that should maybe go away. Esp. for retpoline builds.

#17 lore raw

Re: [PATCH 03/24] x86/paravirt: Annotate indirect calls

Quoted 6 lines · Peter Zijlstra
Quoted 5 lines · Josh Poimboeuf
Quoted 3 lines · Peter Zijlstra

Paravirt emits indirect calls which get flagged by objtool retpoline checks, annotate it away because all these indirect calls will be patched out before we start userspace.

Quoted 7 lines
Quoted 3 lines

There's one condition where this isn't true: the 'noreplace-paravirt' cmdline option. I have no idea why that option exists but we should consider just removing it.

Smells like a debug remnant from back when all this patching crap was new. And yes, that sounds like something that should maybe go away. Esp. for retpoline builds.

Ok, I'll whip up a patch to remove it and see if anybody complains.

Signature · 1 line
Josh
#18 lore raw

PATCH 1/1 x86/paravirt: Remove 'noreplace-paravirt' cmdline option

The 'noreplace-paravirt' option disables paravirt patching, leaving the original pv indirect calls in place.

That's highly incompatible with retpolines, unless we want to uglify paravirt even further and convert the paravirt calls to retpolines.

As far as I can tell, the option doesn't seem to be useful for much other than introducing surprising corner cases and making the kernel vulnerable to Spectre v2. It was probably a debug option from the early paravirt days. So just remove it.

---
 Documentation/admin-guide/kernel-parameters.txt |  2 --
 arch/x86/kernel/alternative.c                   | 14 --------------
 2 files changed, 16 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +0 -2
index a205567f537f..f6741a5d89e7 100644
@@ -2739,8 +2739,6 @@
 	norandmaps	Don't use address space randomization.  Equivalent to
 			echo 0 > /proc/sys/kernel/randomize_va_space
 
-	noreplace-paravirt	[X86,IA-64,PV_OPS] Don't patch paravirt_ops
-
 	noreplace-smp	[X86-32,SMP] Don't replace SMP instructions
 			with UP alternatives
 
arch/x86/kernel/alternative.c +0 -14
index 30571fdaaf6f..a481763a3776 100644
@@ -46,17 +46,6 @@ static int __init setup_noreplace_smp(char *str)
 }
 __setup("noreplace-smp", setup_noreplace_smp);
 
-#ifdef CONFIG_PARAVIRT
-static int __initdata_or_module noreplace_paravirt = 0;
-
-static int __init setup_noreplace_paravirt(char *str)
-{
-	noreplace_paravirt = 1;
-	return 1;
-}
-__setup("noreplace-paravirt", setup_noreplace_paravirt);
-#endif
-
 #define DPRINTK(fmt, args...)						\
 do {									\
 	if (debug_alternative)						\
@@ -599,9 +588,6 @@ void __init_or_module apply_paravirt(struct paravirt_patch_site *start,
 	struct paravirt_patch_site *p;
 	char insnbuf[MAX_PATCH_LEN];
 
-	if (noreplace_paravirt)
-		return;
-
 	for (p = start; p < end; p++) {
 		unsigned int used;
 
Signature · 1 line
2.14.3
maintainer Reviewed-by on v1 1/1
#19 lore raw

Re: [PATCH] x86/paravirt: Remove 'noreplace-paravirt' cmdline option

Quoted 6 lines · Josh Poimboeuf

The 'noreplace-paravirt' option disables paravirt patching, leaving the original pv indirect calls in place.

That's highly incompatible with retpolines, unless we want to uglify paravirt even further and convert the paravirt calls to retpolines.

Quoted 6 lines

As far as I can tell, the option doesn't seem to be useful for much other than introducing surprising corner cases and making the kernel vulnerable to Spectre v2. It was probably a debug option from the early paravirt days. So just remove it.

Juergen

#20 lore raw

[tip:x86/pti] x86/paravirt: Remove 'noreplace-paravirt' cmdline option

In reply to Josh Poimboeuf
x86/paravirt: Remove 'noreplace-paravirt' cmdline option
Commit-ID:  12c69f1e94c89d40696e83804dd2f0965b5250cd
Gitweb:     https://git.kernel.org/tip/12c69f1e94c89d40696e83804dd2f0965b5250cd
Author:     Josh Poimboeuf <[email protected]>
AuthorDate: Tue, 30 Jan 2018 22:13:33 -0600
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Wed, 31 Jan 2018 10:37:45 +0100

x86/paravirt: Remove 'noreplace-paravirt' cmdline option

The 'noreplace-paravirt' option disables paravirt patching, leaving the original pv indirect calls in place.

That's highly incompatible with retpolines, unless we want to uglify paravirt even further and convert the paravirt calls to retpolines.

As far as I can tell, the option doesn't seem to be useful for much other than introducing surprising corner cases and making the kernel vulnerable to Spectre v2. It was probably a debug option from the early paravirt days. So just remove it.

Cc Andrea Arcangeli <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Ashok Raj <[email protected]>
Cc Greg KH <[email protected]>
Cc Jun Nakajima <[email protected]>
Cc Tim Chen <[email protected]>
Cc Rusty Russell <[email protected]>
Cc Dave Hansen <[email protected]>
Cc Asit Mallick <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Jason Baron <[email protected]>
Cc Paolo Bonzini <[email protected]>
Cc Alok Kataria <[email protected]>
Cc Arjan Van De Ven <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Dan Williams <[email protected]>
---
 Documentation/admin-guide/kernel-parameters.txt |  2 --
 arch/x86/kernel/alternative.c                   | 14 --------------
 2 files changed, 16 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +0 -2
index 46b26bf..1e762c2 100644
@@ -2742,8 +2742,6 @@
 	norandmaps	Don't use address space randomization.  Equivalent to
 			echo 0 > /proc/sys/kernel/randomize_va_space
 
-	noreplace-paravirt	[X86,IA-64,PV_OPS] Don't patch paravirt_ops
-
 	noreplace-smp	[X86-32,SMP] Don't replace SMP instructions
 			with UP alternatives
 
arch/x86/kernel/alternative.c +0 -14
index 30571fd..a481763 100644
@@ -46,17 +46,6 @@ static int __init setup_noreplace_smp(char *str)
 }
 __setup("noreplace-smp", setup_noreplace_smp);
 
-#ifdef CONFIG_PARAVIRT
-static int __initdata_or_module noreplace_paravirt = 0;
-
-static int __init setup_noreplace_paravirt(char *str)
-{
-	noreplace_paravirt = 1;
-	return 1;
-}
-__setup("noreplace-paravirt", setup_noreplace_paravirt);
-#endif
-
 #define DPRINTK(fmt, args...)						\
 do {									\
 	if (debug_alternative)						\
@@ -599,9 +588,6 @@ void __init_or_module apply_paravirt(struct paravirt_patch_site *start,
 	struct paravirt_patch_site *p;
 	char insnbuf[MAX_PATCH_LEN];
 
-	if (noreplace_paravirt)
-		return;
-
 	for (p = start; p < end; p++) {
 		unsigned int used;
 
author maintainer
#21 lore raw

PATCH 4/24 x86,nospec: Annotate indirect calls/jumps

In reply to Peter Zijlstra

Annotate the indirect calls/jumps in the CALL_NOSPEC/JUMP_NOSPEC alternatives.

---
 arch/x86/include/asm/nospec-branch.h |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)
arch/x86/include/asm/nospec-branch.h +23 -4
@@ -67,6 +67,18 @@
 .endm
 
 /*
+ * This should be used immediately before an indirect jump/call. It tells
+ * objtool the subsequent indirect jump/call is vouched safe for retpoline
+ * builds.
+ */
+.macro ANNOTATE_RETPOLINE_SAFE
+	.Lannotate_\@:
+	.pushsection .discard.retpoline_safe
+	_ASM_PTR .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
  * invocation below less ugly.
@@ -102,9 +114,9 @@
 .macro JMP_NOSPEC reg:req
 #ifdef CONFIG_RETPOLINE
 	ANNOTATE_NOSPEC_ALTERNATIVE
-	ALTERNATIVE_2 __stringify(jmp *\reg),				\
+	ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *\reg),	\
 		__stringify(RETPOLINE_JMP \reg), X86_FEATURE_RETPOLINE,	\
-		__stringify(lfence; jmp *\reg), X86_FEATURE_RETPOLINE_AMD
+		__stringify(lfence; ANNOTATE_RETPOLINE_SAFE; jmp *\reg), X86_FEATURE_RETPOLINE_AMD
 #else
 	jmp	*\reg
 #endif
@@ -113,9 +125,9 @@
 .macro CALL_NOSPEC reg:req
 #ifdef CONFIG_RETPOLINE
 	ANNOTATE_NOSPEC_ALTERNATIVE
-	ALTERNATIVE_2 __stringify(call *\reg),				\
+	ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; call *\reg),	\
 		__stringify(RETPOLINE_CALL \reg), X86_FEATURE_RETPOLINE,\
-		__stringify(lfence; call *\reg), X86_FEATURE_RETPOLINE_AMD
+		__stringify(lfence; ANNOTATE_RETPOLINE_SAFE; call *\reg), X86_FEATURE_RETPOLINE_AMD
 #else
 	call	*\reg
 #endif
@@ -143,6 +155,12 @@
 	".long 999b - .\n\t"					\
 	".popsection\n\t"
 
+#define ANNOTATE_RETPOLINE_SAFE					\
+	"999:\n\t"						\
+	".pushsection .discard.retpoline_safe\n\t"		\
+	_ASM_PTR " 999b\n\t"					\
+	".popsection\n\t"
+
 #if defined(CONFIG_X86_64) && defined(RETPOLINE)
 
 /*
@@ -152,6 +170,7 @@
 # define CALL_NOSPEC						\
 	ANNOTATE_NOSPEC_ALTERNATIVE				\
 	ALTERNATIVE(						\
+	ANNOTATE_RETPOLINE_SAFE					\
 	"call *%[thunk_target]\n",				\
 	"call __x86_indirect_thunk_%V[thunk_target]\n",		\
 	X86_FEATURE_RETPOLINE)
Reviewed-by on v1 4/24
#22 lore raw

Re: [PATCH 04/24] x86,nospec: Annotate indirect calls/jumps

Quoted 5 lines · Peter Zijlstra

Annotate the indirect calls/jumps in the CALL_NOSPEC/JUMP_NOSPEC alternatives.

However...

arch/x86/include/asm/nospec-branch.h line 79

Quoted 11 lines

 /*
+ * This should be used immediately before an indirect jump/call. It tells
+ * objtool the subsequent indirect jump/call is vouched safe for retpoline
+ * builds.
+ */
+.macro ANNOTATE_RETPOLINE_SAFE

arch/x86/include/asm/nospec-branch.h +5 -0
+	.Lannotate_\@:
+	.pushsection .discard.retpoline_safe
+	_ASM_PTR .Lannotate_\@
+	.popsection
+.endm

Didn't I just see one of those in patch 3? So this makes two...

Quoted 11 lines
arch/x86/include/asm/nospec-branch.h +0 -0
@@ -143,6 +155,12 @@
  	".long 999b - .\n\t"					\

  ".popsection\n\t"

arch/x86/include/asm/nospec-branch.h +6 -0
+#define ANNOTATE_RETPOLINE_SAFE					\
+	"999:\n\t"						\
+	".pushsection .discard.retpoline_safe\n\t"		\
+	_ASM_PTR " 999b\n\t"					\
+	".popsection\n\t"
+

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

... three.

Now, I did briefly toy with the idea of using a .macro from both __ASSEMBLY__ and inline asm, making the latter work by means of asm(".include \"asm/nospec-branch.h\");

In the end I just ended up with the __FILL_RETURN_BUFFER CPP macro which is used from both by other tricks.

Can we look at doing something like that, please?

author maintainer
#23 lore raw

Re: [PATCH 04/24] x86,nospec: Annotate indirect calls/jumps

Quoted 42 lines · David Woodhouse
Quoted 5 lines · Peter Zijlstra

Annotate the indirect calls/jumps in the CALL_NOSPEC/JUMP_NOSPEC alternatives.

However...

arch/x86/include/asm/nospec-branch.h line 79

Quoted 11 lines

 /*
+ * This should be used immediately before an indirect jump/call. It tells
+ * objtool the subsequent indirect jump/call is vouched safe for retpoline
+ * builds.
+ */
+.macro ANNOTATE_RETPOLINE_SAFE

arch/x86/include/asm/nospec-branch.h +5 -0
+	.Lannotate_\@:
+	.pushsection .discard.retpoline_safe
+	_ASM_PTR .Lannotate_\@
+	.popsection
+.endm

Didn't I just see one of those in patch 3? So this makes two...

Quoted 11 lines
arch/x86/include/asm/nospec-branch.h +0 -0
@@ -143,6 +155,12 @@
  	".long 999b - .\n\t"					\

  ".popsection\n\t"

arch/x86/include/asm/nospec-branch.h +6 -0
+#define ANNOTATE_RETPOLINE_SAFE					\
+	"999:\n\t"						\
+	".pushsection .discard.retpoline_safe\n\t"		\
+	_ASM_PTR " 999b\n\t"					\
+	".popsection\n\t"
+

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

... three.

Quoted 8 lines

Now, I did briefly toy with the idea of using a .macro from both __ASSEMBLY__ and inline asm, making the latter work by means of asm(".include \"asm/nospec-branch.h\");

In the end I just ended up with the __FILL_RETURN_BUFFER CPP macro which is used from both by other tricks.

Can we look at doing something like that, please?

I'll try. The paravirt one might be tricky, I always end in header-hell with that thing.

author maintainer
#24 lore raw

PATCH 5/24 x86: Annotate indirect jump in head_64.S

In reply to Peter Zijlstra

The objtool retpoline validation found this indirect jump. Seeing how its on CPU bringup before we run userspace it should be safe, annotate it.

---
 arch/x86/kernel/head_64.S |    2 ++
 1 file changed, 2 insertions(+)
arch/x86/kernel/head_64.S +2 -0
@@ -23,6 +23,7 @@
 #include <asm/nops.h>
 #include "../entry/calling.h"
 #include <asm/export.h>
+#include <asm/nospec-branch.h>
 
 #ifdef CONFIG_PARAVIRT
 #include <asm/asm-offsets.h>
@@ -134,6 +135,7 @@ ENTRY(secondary_startup_64)
 
 	/* Ensure I am executing from virtual addresses */
 	movq	$1f, %rax
+	ANNOTATE_RETPOLINE_SAFE
 	jmp	*%rax
 1:
 	UNWIND_HINT_EMPTY
Reviewed-by on v1 5/24
#25 lore raw

Re: [PATCH 05/24] x86: Annotate indirect jump in head_64.S

Quoted 2 lines · Peter Zijlstra

The objtool retpoline validation found this indirect jump. Seeing how its on CPU bringup before we run userspace it should be safe, annotate

http://angryflower.com/itsits.gif

Quoted 3 lines

it.

Quoted 24 lines

---
 arch/x86/kernel/head_64.S |    2 ++

 1 file changed, 2 insertions(+)
arch/x86/kernel/head_64.S +1 -0
@@ -23,6 +23,7 @@
  #include 
  #include "../entry/calling.h"
  #include 
+#include 


 #ifdef CONFIG_PARAVIRT
 #include

arch/x86/kernel/head_64.S +0 -0
@@ -134,6 +135,7 @@ ENTRY(secondary_startup_64)


  /* Ensure I am executing from virtual addresses */
  movq $1f, %rax

arch/x86/kernel/head_64.S +1 -0
+	ANNOTATE_RETPOLINE_SAFE

  jmp *%rax
 1:
  UNWIND_HINT_EMPTY

author maintainer
#26 lore raw

PATCH 6/24 x86,kvm: Fix indirect calls in emulator

In reply to Peter Zijlstra

Replace the indirect calls with CALL_NOSPEC.

---
 arch/x86/kvm/emulate.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
arch/x86/kvm/emulate.c +5 -4
@@ -25,6 +25,7 @@
 #include <asm/kvm_emulate.h>
 #include <linux/stringify.h>
 #include <asm/debugreg.h>
+#include <asm/nospec-branch.h>
 
 #include "x86.h"
 #include "tss.h"
@@ -1021,8 +1022,8 @@ static __always_inline u8 test_cc(unsign
 	void (*fop)(void) = (void *)em_setcc + 4 * (condition & 0xf);
 
 	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
-	asm("push %[flags]; popf; call *%[fastop]"
-	    : "=a"(rc) : [fastop]"r"(fop), [flags]"r"(flags));
+	asm("push %[flags]; popf; " CALL_NOSPEC
+	    : "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags));
 	return rc;
 }
 
@@ -5305,9 +5306,9 @@ static int fastop(struct x86_emulate_ctx
 	if (!(ctxt->d & ByteOp))
 		fop += __ffs(ctxt->dst.bytes) * FASTOP_SIZE;
 
-	asm("push %[flags]; popf; call *%[fastop]; pushf; pop %[flags]\n"
+	asm("push %[flags]; popf; " CALL_NOSPEC " ; pushf; pop %[flags]\n"
 	    : "+a"(ctxt->dst.val), "+d"(ctxt->src.val), [flags]"+D"(flags),
-	      [fastop]"+S"(fop), ASM_CALL_CONSTRAINT
+	      [thunk_target]"+S"(fop), ASM_CALL_CONSTRAINT
 	    : "c"(ctxt->src2.val));
 
 	ctxt->eflags = (ctxt->eflags & ~EFLAGS_MASK) | (flags & EFLAGS_MASK);
maintainer
#27 lore raw

Re: [PATCH 06/24] x86,kvm: Fix indirect calls in emulator

Quoted 19 lines · Peter Zijlstra

Replace the indirect calls with CALL_NOSPEC.

---
 arch/x86/kvm/emulate.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
arch/x86/kvm/emulate.c +1 -0
@@ -25,6 +25,7 @@
 #include <asm/kvm_emulate.h>
 #include <linux/stringify.h>
 #include <asm/debugreg.h>
+#include <asm/nospec-branch.h>
 
 #include "x86.h"
 #include "tss.h"

arch/x86/kvm/emulate.c static __always_inline u8 test_cc(unsign · line 1026

Quoted 7 lines
arch/x86/kvm/emulate.c +2 -2
@@ -1021,8 +1022,8 @@ static __always_inline u8 test_cc(unsign
 	void (*fop)(void) = (void *)em_setcc + 4 * (condition & 0xf);
 
 	flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
-	asm("push %[flags]; popf; call *%[fastop]"
-	    : "=a"(rc) : [fastop]"r"(fop), [flags]"r"(flags));
+	asm("push %[flags]; popf; " CALL_NOSPEC
+	    : "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags));

Oh, "thunk_target" is magical.

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
#28 lore raw

Re: [PATCH 06/24] x86,kvm: Fix indirect calls in emulator

Quoted 8 lines · Borislav Petkov
Quoted 6 lines

       flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
-     asm("push %[flags]; popf; call *%[fastop]"
-         : "=a"(rc) : [fastop]"r"(fop), [flags]"r"(flags)); +     asm("push %[flags]; popf; " CALL_NOSPEC
+         : "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags));

Oh, "thunk_target" is magical.

You can use THUNK_TARGET(fop), which will be "rm" on 32-bit and avoids register starvation in some cases (I don't think the hyperv calls worked until I did that).

author maintainer
#29 lore raw

Re: [PATCH 06/24] x86,kvm: Fix indirect calls in emulator

Quoted 8 lines · David Woodhouse
Quoted 7 lines · Borislav Petkov
Quoted 6 lines

       flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
-     asm("push %[flags]; popf; call *%[fastop]"
-         : "=a"(rc) : [fastop]"r"(fop), [flags]"r"(flags)); +     asm("push %[flags]; popf; " CALL_NOSPEC
+         : "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags));

Quoted 5 lines
Quoted 1 line

Oh, "thunk_target" is magical.

You can use THUNK_TARGET(fop), which will be "rm" on 32-bit and avoids register starvation in some cases (I don't think the hyperv calls worked until I did that).

The reason I didn't use THUNK_TARGET() was exactly because it used "rm" and the current code did "r" only. I'm happy to change if people can agree on something ;-)

maintainer
#30 lore raw

Re: [PATCH 06/24] x86,kvm: Fix indirect calls in emulator

Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · David Woodhouse
Quoted 8 lines · Borislav Petkov
Quoted 6 lines

       flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
-     asm("push %[flags]; popf; call *%[fastop]"
-         : "=a"(rc) : [fastop]"r"(fop), [flags]"r"(flags)); +     asm("push %[flags]; popf; " CALL_NOSPEC
+         : "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags));

Oh, "thunk_target" is magical.

Quoted 7 lines
Quoted 3 lines

You can use THUNK_TARGET(fop), which will be "rm" on 32-bit and avoids register starvation in some cases (I don't think the hyperv calls worked until I did that).

The reason I didn't use THUNK_TARGET() was exactly because it used "rm" and the current code did "r" only. I'm happy to change if people can agree on something ;-)

In practice, "fastop" is going to be in a register because of how it's computed, but "rm" is okay.

Thanks,

Paolo

author maintainer
#31 lore raw

Re: [PATCH 06/24] x86,kvm: Fix indirect calls in emulator

Quoted 16 lines · Paolo Bonzini
Quoted 15 lines · Peter Zijlstra
Quoted 13 lines · David Woodhouse
Quoted 8 lines · Borislav Petkov
Quoted 6 lines

       flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
-     asm("push %[flags]; popf; call *%[fastop]"
-         : "=a"(rc) : [fastop]"r"(fop), [flags]"r"(flags)); +     asm("push %[flags]; popf; " CALL_NOSPEC
+         : "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags));

Oh, "thunk_target" is magical.

You can use THUNK_TARGET(fop), which will be "rm" on 32-bit and avoids register starvation in some cases (I don't think the hyperv calls worked until I did that).

Quoted 6 lines
Quoted 3 lines

The reason I didn't use THUNK_TARGET() was exactly because it used "rm" and the current code did "r" only. I'm happy to change if people can agree on something ;-)

In practice, "fastop" is going to be in a register because of how it's computed, but "rm" is okay.

OK, so the other occurence in that file uses "+S", which is the SI register. That cannot use THUNK_TARGET(), right?

So do you want one THUNK_TARGET and one open coded, or keep the patch as is (both open coded) ?

Reviewed-by
#32 lore raw

Re: [PATCH 06/24] x86,kvm: Fix indirect calls in emulator

Quoted 22 lines · Peter Zijlstra
Quoted 21 lines · Paolo Bonzini
Quoted 19 lines · Peter Zijlstra
Quoted 10 lines · David Woodhouse
Quoted 8 lines · Borislav Petkov
Quoted 8 lines

       flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
-     asm("push %[flags]; popf; call *%[fastop]"
-         : "=a"(rc) : [fastop]"r"(fop), [flags]"r"(flags)); +     asm("push %[flags]; popf; " CALL_NOSPEC
+         : "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags));

Quoted 4 lines
Quoted 1 line

Oh, "thunk_target" is magical.

You can use THUNK_TARGET(fop), which will be "rm" on 32-bit and avoids register starvation in some cases (I don't think the hyperv calls worked until I did that).

The reason I didn't use THUNK_TARGET() was exactly because it used "rm" and the current code did "r" only. I'm happy to change if people can agree on something ;-)

Quoted 7 lines
Quoted 2 lines

In practice, "fastop" is going to be in a register because of how it's computed, but "rm" is okay.

OK, so the other occurence in that file uses "+S", which is the SI register. That cannot use THUNK_TARGET(), right?

So do you want one THUNK_TARGET and one open coded, or keep the patch as is (both open coded) ?

As long as it builds for i386, you might as well keep them both open- coded. The "rm" was there mostly for the hyperv call, which ran out of registers completely when it was just "r".

This patch *really* wants to be going to Linus urgently as a retpoline fix, and not buried as patch 6/24 though, right? Likewise 7/24?

Both of them,

... unless you want me to send them on, but it makes most sense for Thomas to round them I suspect?

maintainer
#33 lore raw

Re: [PATCH 06/24] x86,kvm: Fix indirect calls in emulator

In reply to Peter Zijlstra
Quoted 21 lines · Peter Zijlstra
Quoted 20 lines · Paolo Bonzini
Quoted 18 lines · Peter Zijlstra
Quoted 8 lines · David Woodhouse
Quoted 7 lines · Borislav Petkov
Quoted 6 lines

       flags = (flags & EFLAGS_MASK) | X86_EFLAGS_IF;
-     asm("push %[flags]; popf; call *%[fastop]"
-         : "=a"(rc) : [fastop]"r"(fop), [flags]"r"(flags)); +     asm("push %[flags]; popf; " CALL_NOSPEC
+         : "=a"(rc) : [thunk_target]"r"(fop), [flags]"r"(flags));

Quoted 5 lines
Quoted 1 line

Oh, "thunk_target" is magical.

You can use THUNK_TARGET(fop), which will be "rm" on 32-bit and avoids register starvation in some cases (I don't think the hyperv calls worked until I did that).

The reason I didn't use THUNK_TARGET() was exactly because it used "rm" and the current code did "r" only. I'm happy to change if people can agree on something ;-)

Quoted 5 lines
Quoted 2 lines

In practice, "fastop" is going to be in a register because of how it's computed, but "rm" is okay.

OK, so the other occurence in that file uses "+S", which is the SI register. That cannot use THUNK_TARGET(), right?

Nope, it reads the output in %esi too.

Quoted 2 lines

So do you want one THUNK_TARGET and one open coded, or keep the patch as is (both open coded) ?

Open coded is okay.

Paolo

author maintainer
#34 lore raw

PATCH 7/24 x86,vmx: Fix indirect call

In reply to Peter Zijlstra

Replace indirect call with CALL_NOSPEC.

---
 arch/x86/kvm/vmx.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +2 -2
@@ -9064,14 +9064,14 @@ static void vmx_handle_external_intr(str
 #endif
 			"pushf\n\t"
 			__ASM_SIZE(push) " $%c[cs]\n\t"
-			"call *%[entry]\n\t"
+			CALL_NOSPEC
 			:
 #ifdef CONFIG_X86_64
 			[sp]"=&r"(tmp),
 #endif
 			ASM_CALL_CONSTRAINT
 			:
-			[entry]"r"(entry),
+			[thunk_target]"r"(entry),
 			[ss]"i"(__KERNEL_DS),
 			[cs]"i"(__KERNEL_CS)
 			);
author maintainer
#35 lore raw

Re: [PATCH 07/24] x86,vmx: Fix indirect call

Subject: x86,vmx: Fix indirect call
From: Peter Zijlstra <[email protected]>
Date: Tue Jan 23 14:10:19 CET 2018

Replace indirect call with CALL_NOSPEC.

---
- now with THUNK_TARGET()

 arch/x86/kvm/vmx.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +2 -2
@@ -9129,14 +9129,14 @@ static void vmx_handle_external_intr(str
 #endif
 			"pushf\n\t"
 			__ASM_SIZE(push) " $%c[cs]\n\t"
-			"call *%[entry]\n\t"
+			CALL_NOSPEC
 			:
 #ifdef CONFIG_X86_64
 			[sp]"=&r"(tmp),
 #endif
 			ASM_CALL_CONSTRAINT
 			:
-			[entry]"r"(entry),
+			THUNK_TARGET(entry),
 			[ss]"i"(__KERNEL_DS),
 			[cs]"i"(__KERNEL_CS)
 			);
author maintainer
#36 lore raw

PATCH 8/24 x86,sme: Annotate indirect call

In reply to Peter Zijlstra

This is boot code, we run this _way_ before userspace comes along to poison our branch predictor.

Cc Tom Lendacky <[email protected]>
Cc Borislav Petkov <[email protected]>
---
 arch/x86/mm/mem_encrypt_boot.S |    2 ++
 1 file changed, 2 insertions(+)
arch/x86/mm/mem_encrypt_boot.S +2 -0
@@ -15,6 +15,7 @@
 #include <asm/page.h>
 #include <asm/processor-flags.h>
 #include <asm/msr-index.h>
+#include <asm/nospec-branch.h>
 
 	.text
 	.code64
@@ -59,6 +60,7 @@ ENTRY(sme_encrypt_execute)
 	movq	%rax, %r8		/* Workarea encryption routine */
 	addq	$PAGE_SIZE, %r8		/* Workarea intermediate copy buffer */
 
+	ANNOTATE_RETPOLINE_SAFE
 	call	*%rax			/* Call the encryption routine */
 
 	pop	%r12
#37 lore raw

Re: [PATCH 08/24] x86,sme: Annotate indirect call

Quoted 2 lines · Peter Zijlstra

This is boot code, we run this _way_ before userspace comes along to poison our branch predictor.

Hm, objtool knows about sections, doesn't it? Why it is whining about indirect jumps in inittext anyway?

In fact, why are we even *doing* retpolines in inittext? Not that we are; since we flipped the ALTERNATIVE logic around, at that point we still have the 'oldinstr' which is a bare jmp anyway. We might as well do this:

include/linux/init.h +0 -0
@@ -37,10 +37,15 @@

  * as gcc otherwise puts the data into the bss section and not into the init
  * section.
  */
+#if defined(RETPOLINE) && !defined(MODULE)
+#define __noretpoline __attribute__((indirect_branch("keep")))
+#else
+#define __noretpoline
+#endif

 /* These are for everybody (although not all archs will actually
    discard it in modules) */
-#define __init         __section(.init.text) __cold __inittrace __latent_entropy +#define __init         __section(.init.text) __cold __inittrace __latent_entropy __noretpoline #define __initdata     __section(.init.data)
 #define __initconst    __section(.init.rodata)
 #define __exitdata     __section(.exit.data)

I had that once and dropped it because of concerns about VM guests being "vulnerable" at boot time. But really, do they even have any interesting data to purloin at that point? And shouldn't the hypervisor be protecting them with STIBP if they have nasty HT siblings?

(And if hypervisors do start doing that, it might be nice for a guest to have a way to say "you can stop now; I'm safe")

author maintainer
#38 lore raw

Re: [PATCH 08/24] x86,sme: Annotate indirect call

Quoted 26 lines · David Woodhouse
Quoted 2 lines · Peter Zijlstra

This is boot code, we run this _way_ before userspace comes along to poison our branch predictor.

Hm, objtool knows about sections, doesn't it? Why it is whining about indirect jumps in inittext anyway?

In fact, why are we even *doing* retpolines in inittext? Not that we are; since we flipped the ALTERNATIVE logic around, at that point we still have the 'oldinstr' which is a bare jmp anyway. We might as well do this:

include/linux/init.h +5 -0
@@ -37,10 +37,15 @@
   * as gcc otherwise puts the data into the bss section and not into the init
   * section.
   */
+#if defined(RETPOLINE) && !defined(MODULE)
+#define __noretpoline __attribute__((indirect_branch("keep")))
+#else
+#define __noretpoline
+#endif


 /* These are for everybody (although not all archs will actually discard it in modules) */

include/linux/init.h

Quoted 2 lines
include/linux/init.h +1 -1
-#define __init         __section(.init.text) __cold __inittrace __latent_entropy
+#define __init         __section(.init.text) __cold __inittrace __latent_entropy __noretpoline

We run module __init text concurrently with userspace.

author maintainer
#39 lore raw

Re: [PATCH 08/24] x86,sme: Annotate indirect call

Quoted 13 lines · Peter Zijlstra
Quoted 12 lines · David Woodhouse
Quoted 2 lines · Peter Zijlstra

This is boot code, we run this _way_ before userspace comes along to poison our branch predictor.

Hm, objtool knows about sections, doesn't it? Why it is whining about indirect jumps in inittext anyway?

In fact, why are we even *doing* retpolines in inittext? Not that we are; since we flipped the ALTERNATIVE logic around, at that point we still have the 'oldinstr' which is a bare jmp anyway. We might as well do this:

Quoted 11 lines

include/linux/init.h line 44

Quoted 11 lines
include/linux/init.h +5 -0
@@ -37,10 +37,15 @@
   * as gcc otherwise puts the data into the bss section and not into the init
   * section.
   */
+#if defined(RETPOLINE) && !defined(MODULE)
+#define __noretpoline __attribute__((indirect_branch("keep")))
+#else
+#define __noretpoline
+#endif

Clearly I cannot read...

Quoted 6 lines

include/linux/init.h

Quoted 4 lines
include/linux/init.h +0 -0
  /* These are for everybody (although not all archs will actually

    discard it in modules) */

include/linux/init.h +1 -1
-#define __init         __section(.init.text) __cold __inittrace __latent_entropy
+#define __init         __section(.init.text) __cold __inittrace __latent_entropy __noretpoline

We run module __init text concurrently with userspace.

author maintainer
#40 lore raw

Re: [PATCH 08/24] x86,sme: Annotate indirect call

In reply to David Woodhouse
Quoted 28 lines · David Woodhouse
Quoted 2 lines · Peter Zijlstra

This is boot code, we run this _way_ before userspace comes along to poison our branch predictor.

Hm, objtool knows about sections, doesn't it? Why it is whining about indirect jumps in inittext anyway?

In fact, why are we even *doing* retpolines in inittext? Not that we are; since we flipped the ALTERNATIVE logic around, at that point we still have the 'oldinstr' which is a bare jmp anyway. We might as well do this:

include/linux/init.h +5 -0
@@ -37,10 +37,15 @@
   * as gcc otherwise puts the data into the bss section and not into the init
   * section.
   */
+#if defined(RETPOLINE) && !defined(MODULE)
+#define __noretpoline __attribute__((indirect_branch("keep")))
+#else
+#define __noretpoline
+#endif


 /* These are for everybody (although not all archs will actually discard it in modules) */

include/linux/init.h +1 -1
-#define __init         __section(.init.text) __cold __inittrace __latent_entropy
+#define __init         __section(.init.text) __cold __inittrace __latent_entropy __noretpoline
Quoted 3 lines

 #define __initdata     __section(.init.data)
 #define __initconst    __section(.init.rodata)
 #define __exitdata     __section(.exit.data)

Something like so then?

---

Subject: objtool: Add module specific retpoline rules
From: Peter Zijlstra <[email protected]>
Date: Wed Jan 31 10:18:28 CET 2018

David wanted to not use retpolines in .init.text but that will trip up objtool retpoline validation, fix that.

---
 scripts/Makefile.build        |    2 ++
 tools/objtool/builtin-check.c |    3 ++-
 tools/objtool/builtin.h       |    2 +-
 tools/objtool/check.c         |    9 +++++++++
 4 files changed, 14 insertions(+), 2 deletions(-)
scripts/Makefile.build +2 -0
@@ -256,6 +256,8 @@ __objtool_obj := $(objtree)/tools/objtoo
 
 objtool_args = $(if $(CONFIG_UNWINDER_ORC),orc generate,check)
 
+objtool_args += $(if $(part-of-module), --module,)
+
 ifndef CONFIG_FRAME_POINTER
 objtool_args += --no-fp
 endif
tools/objtool/builtin-check.c +2 -1
@@ -29,7 +29,7 @@
 #include "builtin.h"
 #include "check.h"
 
-bool no_fp, no_unreachable, retpoline;
+bool no_fp, no_unreachable, retpoline, module;
 
 static const char * const check_usage[] = {
 	"objtool check [<options>] file.o",
@@ -40,6 +40,7 @@ const struct option check_options[] = {
 	OPT_BOOLEAN('f', "no-fp", &no_fp, "Skip frame pointer validation"),
 	OPT_BOOLEAN('u', "no-unreachable", &no_unreachable, "Skip 'unreachable instruction' warnings"),
 	OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"),
+	OPT_BOOLEAN('m', "module", &module, "Indicates the object will be part of a kernel module"),
 	OPT_END(),
 };
 
tools/objtool/builtin.h +1 -1
@@ -20,7 +20,7 @@
 #include <subcmd/parse-options.h>
 
 extern const struct option check_options[];
-extern bool no_fp, no_unreachable, retpoline;
+extern bool no_fp, no_unreachable, retpoline, module;
 
 extern int cmd_check(int argc, const char **argv);
 extern int cmd_orc(int argc, const char **argv);
tools/objtool/check.c +9 -0
@@ -1920,6 +1920,15 @@ static int validate_retpoline(struct obj
 		if (insn->retpoline_safe)
 			continue;
 
+		/*
+		 * .init.text code is ran before userspace and thus doesn't
+		 * strictly need retpolines, except for modules which are
+		 * loaded late, they very much do need retpoline in their
+		 * .init.text
+		 */
+		if (!strcmp(insn->sec->name, ".init.text") && !module)
+			continue;
+
 		WARN_FUNC("indirect %s found in RETPOLINE build",
 			  insn->sec, insn->offset,
 			  insn->type == INSN_JUMP_DYNAMIC ? "jump" : "call");
#41 lore raw

Re: [PATCH 08/24] x86,sme: Annotate indirect call

Quoted 5 lines · Peter Zijlstra
Quoted 4 lines · David Woodhouse
Quoted 2 lines · Peter Zijlstra

This is boot code, we run this _way_ before userspace comes along to poison our branch predictor.

Quoted 7 lines
Quoted 7 lines

Hm, objtool knows about sections, doesn't it? Why it is whining about indirect jumps in inittext anyway?

In fact, why are we even *doing* retpolines in inittext? Not that we are; since we flipped the ALTERNATIVE logic around, at that point we still have the 'oldinstr' which is a bare jmp anyway. We might as well do this:

Ont the other hand, is there any harm in doing retpolines in .init.text?

I also had a similar question about the ANNOTATE_RETPOLINE_SAFE thing.

If there's no harm, it would be simpler and more robust to just do retpolines everywhere and not worry about special cases.

(Forgetting paravirt for the moment, which is the eternal "special case".)

I was also thinking about adding a debug option for _runtime_ retpoline verification that decodes all kernel text and reports any indirect branches it finds (yes, kind of like an in-kernel objtool). That would be a lot more straightforward without special cases. Obviously .init.text wouldn't be a problem there, but the other annotated safe locations would.

Signature · 1 line
Josh
author maintainer
#42 lore raw

Re: [PATCH 08/24] x86,sme: Annotate indirect call

Quoted 6 lines · Josh Poimboeuf

If there's no harm, it would be simpler and more robust to just do retpolines everywhere and not worry about special cases.

(Forgetting paravirt for the moment, which is the eternal "special case".)

Quoted 6 lines

I was also thinking about adding a debug option for _runtime_ retpoline verification that decodes all kernel text and reports any indirect branches it finds (yes, kind of like an in-kernel objtool). That would be a lot more straightforward without special cases. Obviously .init.text wouldn't be a problem there, but the other annotated safe locations would.

Like said, even retpolines themselves need annotation. We could simply keep the section and not .discard. it.

author maintainer
#43 lore raw

PATCH 9/24 jump_label: Add branch hints to static_branch_{un,}likely()

In reply to Peter Zijlstra

For some reason these were missing, I've not observed this patch making a difference in the few code locations I checked, but this makes sense.

---
 include/linux/jump_label.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/jump_label.h +2 -2
@@ -388,7 +388,7 @@ extern bool ____wrong_branch_error(void)
 		branch = !arch_static_branch_jump(&(x)->key, true);		\
 	else									\
 		branch = ____wrong_branch_error();				\
-	branch;									\
+	likely(branch);								\
 })
 
 #define static_branch_unlikely(x)						\
@@ -400,7 +400,7 @@ extern bool ____wrong_branch_error(void)
 		branch = arch_static_branch(&(x)->key, false);			\
 	else									\
 		branch = ____wrong_branch_error();				\
-	branch;									\
+	unlikely(branch);							\
 })
 
 #else /* !HAVE_JUMP_LABEL */
maintainer Reviewed-by on v1 9/24
#44 lore raw

Re: [PATCH 09/24] jump_label: Add branch hints to static_branch_{un,}likely()

Quoted 22 lines · Peter Zijlstra

For some reason these were missing, I've not observed this patch making a difference in the few code locations I checked, but this makes sense.

---
 include/linux/jump_label.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
include/linux/jump_label.h +1 -1
@@ -388,7 +388,7 @@ extern bool ____wrong_branch_error(void)
 		branch = !arch_static_branch_jump(&(x)->key, true);		\
 	else									\
 		branch = ____wrong_branch_error();				\
-	branch;									\
+	likely(branch);								\
 })
 
 #define static_branch_unlikely(x)						\

include/linux/jump_label.h extern bool ____wrong_branch_error(void) · line 404

Quoted 6 lines
include/linux/jump_label.h +1 -1
@@ -400,7 +400,7 @@ extern bool ____wrong_branch_error(void)
 		branch = arch_static_branch(&(x)->key, false);			\
 	else									\
 		branch = ____wrong_branch_error();				\
-	branch;									\
+	unlikely(branch);							\
 })

LOL, it is practically in the name already. :-)

Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author maintainer
#45 lore raw

PATCH 10/24 sched: Optimize ttwu_stat()

In reply to Peter Zijlstra

The whole of ttwu_stat() is guarded by a single schedstat_enabled(), there is absolutely no point in then issuing another static_branch for every single schedstat_inc() in there.

---
 kernel/sched/core.c  |   16 ++++++++--------
 kernel/sched/stats.h |    2 ++
 2 files changed, 10 insertions(+), 8 deletions(-)
kernel/sched/core.c +8 -8
@@ -1630,16 +1630,16 @@ ttwu_stat(struct task_struct *p, int cpu
 
 #ifdef CONFIG_SMP
 	if (cpu == rq->cpu) {
-		schedstat_inc(rq->ttwu_local);
-		schedstat_inc(p->se.statistics.nr_wakeups_local);
+		__schedstat_inc(rq->ttwu_local);
+		__schedstat_inc(p->se.statistics.nr_wakeups_local);
 	} else {
 		struct sched_domain *sd;
 
-		schedstat_inc(p->se.statistics.nr_wakeups_remote);
+		__schedstat_inc(p->se.statistics.nr_wakeups_remote);
 		rcu_read_lock();
 		for_each_domain(rq->cpu, sd) {
 			if (cpumask_test_cpu(cpu, sched_domain_span(sd))) {
-				schedstat_inc(sd->ttwu_wake_remote);
+				__schedstat_inc(sd->ttwu_wake_remote);
 				break;
 			}
 		}
@@ -1647,14 +1647,14 @@ ttwu_stat(struct task_struct *p, int cpu
 	}
 
 	if (wake_flags & WF_MIGRATED)
-		schedstat_inc(p->se.statistics.nr_wakeups_migrate);
+		__schedstat_inc(p->se.statistics.nr_wakeups_migrate);
 #endif /* CONFIG_SMP */
 
-	schedstat_inc(rq->ttwu_count);
-	schedstat_inc(p->se.statistics.nr_wakeups);
+	__schedstat_inc(rq->ttwu_count);
+	__schedstat_inc(p->se.statistics.nr_wakeups);
 
 	if (wake_flags & WF_SYNC)
-		schedstat_inc(p->se.statistics.nr_wakeups_sync);
+		__schedstat_inc(p->se.statistics.nr_wakeups_sync);
 }
 
 static inline void ttwu_activate(struct rq *rq, struct task_struct *p, int en_flags)
kernel/sched/stats.h +2 -0
@@ -31,6 +31,7 @@ rq_sched_info_dequeued(struct rq *rq, un
 		rq->rq_sched_info.run_delay += delta;
 }
 #define schedstat_enabled()		static_branch_unlikely(&sched_schedstats)
+#define __schedstat_inc(var)		do { var++; } while (0)
 #define schedstat_inc(var)		do { if (schedstat_enabled()) { var++; } } while (0)
 #define schedstat_add(var, amt)		do { if (schedstat_enabled()) { var += (amt); } } while (0)
 #define schedstat_set(var, val)		do { if (schedstat_enabled()) { var = (val); } } while (0)
@@ -48,6 +49,7 @@ static inline void
 rq_sched_info_depart(struct rq *rq, unsigned long long delta)
 {}
 #define schedstat_enabled()		0
+#define __schedstat_inc(var)		do { } while (0)
 #define schedstat_inc(var)		do { } while (0)
 #define schedstat_add(var, amt)		do { } while (0)
 #define schedstat_set(var, val)		do { } while (0)
author maintainer
#46 lore raw

PATCH 11/24 x86: Reindent _static_cpu_has

In reply to Peter Zijlstra

Because its daft..

---
 arch/x86/include/asm/cpufeature.h |   78 +++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 39 deletions(-)
arch/x86/include/asm/cpufeature.h +39 -39
@@ -145,45 +145,45 @@ extern void clear_cpu_cap(struct cpuinfo
  */
 static __always_inline __pure bool _static_cpu_has(u16 bit)
 {
-		asm_volatile_goto("1: jmp 6f\n"
-			 "2:\n"
-			 ".skip -(((5f-4f) - (2b-1b)) > 0) * "
-			         "((5f-4f) - (2b-1b)),0x90\n"
-			 "3:\n"
-			 ".section .altinstructions,\"a\"\n"
-			 " .long 1b - .\n"		/* src offset */
-			 " .long 4f - .\n"		/* repl offset */
-			 " .word %P1\n"			/* always replace */
-			 " .byte 3b - 1b\n"		/* src len */
-			 " .byte 5f - 4f\n"		/* repl len */
-			 " .byte 3b - 2b\n"		/* pad len */
-			 ".previous\n"
-			 ".section .altinstr_replacement,\"ax\"\n"
-			 "4: jmp %l[t_no]\n"
-			 "5:\n"
-			 ".previous\n"
-			 ".section .altinstructions,\"a\"\n"
-			 " .long 1b - .\n"		/* src offset */
-			 " .long 0\n"			/* no replacement */
-			 " .word %P0\n"			/* feature bit */
-			 " .byte 3b - 1b\n"		/* src len */
-			 " .byte 0\n"			/* repl len */
-			 " .byte 0\n"			/* pad len */
-			 ".previous\n"
-			 ".section .altinstr_aux,\"ax\"\n"
-			 "6:\n"
-			 " testb %[bitnum],%[cap_byte]\n"
-			 " jnz %l[t_yes]\n"
-			 " jmp %l[t_no]\n"
-			 ".previous\n"
-			 : : "i" (bit), "i" (X86_FEATURE_ALWAYS),
-			     [bitnum] "i" (1 << (bit & 7)),
-			     [cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
-			 : : t_yes, t_no);
-	t_yes:
-		return true;
-	t_no:
-		return false;
+	asm_volatile_goto("1: jmp 6f\n"
+		 "2:\n"
+		 ".skip -(((5f-4f) - (2b-1b)) > 0) * "
+			 "((5f-4f) - (2b-1b)),0x90\n"
+		 "3:\n"
+		 ".section .altinstructions,\"a\"\n"
+		 " .long 1b - .\n"		/* src offset */
+		 " .long 4f - .\n"		/* repl offset */
+		 " .word %P1\n"			/* always replace */
+		 " .byte 3b - 1b\n"		/* src len */
+		 " .byte 5f - 4f\n"		/* repl len */
+		 " .byte 3b - 2b\n"		/* pad len */
+		 ".previous\n"
+		 ".section .altinstr_replacement,\"ax\"\n"
+		 "4: jmp %l[t_no]\n"
+		 "5:\n"
+		 ".previous\n"
+		 ".section .altinstructions,\"a\"\n"
+		 " .long 1b - .\n"		/* src offset */
+		 " .long 0\n"			/* no replacement */
+		 " .word %P0\n"			/* feature bit */
+		 " .byte 3b - 1b\n"		/* src len */
+		 " .byte 0\n"			/* repl len */
+		 " .byte 0\n"			/* pad len */
+		 ".previous\n"
+		 ".section .altinstr_aux,\"ax\"\n"
+		 "6:\n"
+		 " testb %[bitnum],%[cap_byte]\n"
+		 " jnz %l[t_yes]\n"
+		 " jmp %l[t_no]\n"
+		 ".previous\n"
+		 : : "i" (bit), "i" (X86_FEATURE_ALWAYS),
+		     [bitnum] "i" (1 << (bit & 7)),
+		     [cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
+		 : : t_yes, t_no);
+t_yes:
+	return true;
+t_no:
+	return false;
 }
 
 #define static_cpu_has(bit)					\
author maintainer
#47 lore raw

PATCH 12/24 x86: Update _static_cpu_has to use all named variables

In reply to Peter Zijlstra
---
 arch/x86/include/asm/cpufeature.h |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
arch/x86/include/asm/cpufeature.h +5 -4
@@ -153,7 +153,7 @@ static __always_inline __pure bool _stat
 		 ".section .altinstructions,\"a\"\n"
 		 " .long 1b - .\n"		/* src offset */
 		 " .long 4f - .\n"		/* repl offset */
-		 " .word %P1\n"			/* always replace */
+		 " .word %P[always]\n"		/* always replace */
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 5f - 4f\n"		/* repl len */
 		 " .byte 3b - 2b\n"		/* pad len */
@@ -165,7 +165,7 @@ static __always_inline __pure bool _stat
 		 ".section .altinstructions,\"a\"\n"
 		 " .long 1b - .\n"		/* src offset */
 		 " .long 0\n"			/* no replacement */
-		 " .word %P0\n"			/* feature bit */
+		 " .word %P[feature]\n"		/* feature bit */
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 0\n"			/* repl len */
 		 " .byte 0\n"			/* pad len */
@@ -176,8 +176,9 @@ static __always_inline __pure bool _stat
 		 " jnz %l[t_yes]\n"
 		 " jmp %l[t_no]\n"
 		 ".previous\n"
-		 : : "i" (bit), "i" (X86_FEATURE_ALWAYS),
-		     [bitnum] "i" (1 << (bit & 7)),
+		 : : [feature]  "i" (bit),
+		     [always]   "i" (X86_FEATURE_ALWAYS),
+		     [bitnum]   "i" (1 << (bit & 7)),
 		     [cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
 		 : : t_yes, t_no);
 t_yes:
maintainer Reviewed-by on v1 12/24
#48 lore raw

Re: [PATCH 12/24] x86: Update _static_cpu_has to use all named variables

Quoted 29 lines · Peter Zijlstra
---
 arch/x86/include/asm/cpufeature.h |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
arch/x86/include/asm/cpufeature.h +2 -2
@@ -153,7 +153,7 @@ static __always_inline __pure bool _stat
 		 ".section .altinstructions,\"a\"\n"
 		 " .long 1b - .\n"		/* src offset */
 		 " .long 4f - .\n"		/* repl offset */
-		 " .word %P1\n"			/* always replace */
+		 " .word %P[always]\n"		/* always replace */
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 5f - 4f\n"		/* repl len */
 		 " .byte 3b - 2b\n"		/* pad len */
@@ -165,7 +165,7 @@ static __always_inline __pure bool _stat
 		 ".section .altinstructions,\"a\"\n"
 		 " .long 1b - .\n"		/* src offset */
 		 " .long 0\n"			/* no replacement */
-		 " .word %P0\n"			/* feature bit */
+		 " .word %P[feature]\n"		/* feature bit */
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 0\n"			/* repl len */
 		 " .byte 0\n"			/* pad len */
@@ -176,8 +176,9 @@ static __always_inline __pure bool _stat
 		 " jnz %l[t_yes]\n"

arch/x86/include/asm/cpufeature.h static __always_inline __pure bool _stat · line 184

Quoted 10 lines
arch/x86/include/asm/cpufeature.h +3 -2
 		 " jmp %l[t_no]\n"
 		 ".previous\n"
-		 : : "i" (bit), "i" (X86_FEATURE_ALWAYS),
-		     [bitnum] "i" (1 << (bit & 7)),
+		 : : [feature]  "i" (bit),
+		     [always]   "i" (X86_FEATURE_ALWAYS),
+		     [bitnum]   "i" (1 << (bit & 7)),
 		     [cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
 		 : : t_yes, t_no);
 t_yes:
Signature · 4 lines
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
author maintainer
#49 lore raw

PATCH 13/24 objtool: Implement base jump_assert support

In reply to Peter Zijlstra

Implement a jump_label assertion that asserts that the code location is indeed only reachable through a static_branch. Because if GCC is absolutely retaded it could generate code like:

	xor rax,rax
	NOP/JMP 1f
	mov $1, rax
1:
	test rax,rax
	jz 2f
	<do-code>

2:

instead of the sensible:

	NOP/JMP 1f
	<do-code>

1:

This implements objtool infrastructure for ensuring the code ends up sane, since we'll rely on that for correctness and security.

We tag the instructions after the static branch with static_jump_dest=true; that is the instruction after the NOP and the instruction at the JMP+disp site.

Then, when we read the .discard.jump_assert section, we assert that each entry points to an instruction that has static_jump_dest set.

With this we can assert that the code emitted for the if statement ends up at the static jump location and nothing untowards happened.

Cc Thomas Gleixner <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Josh Poimboeuf <[email protected]>
---
 tools/objtool/check.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++--
 tools/objtool/check.h |    2 -
 2 files changed, 69 insertions(+), 3 deletions(-)
tools/objtool/check.c +68 -2
@@ -686,8 +686,17 @@ static int handle_jump_alt(struct objtoo
 			   struct instruction *orig_insn,
 			   struct instruction **new_insn)
 {
-	if (orig_insn->type == INSN_NOP)
+	struct instruction *next_insn = list_next_entry(orig_insn, list);
+
+	if (orig_insn->type == INSN_NOP) {
+		/*
+		 * If orig_insn is a NOP, then new_insn is the branch target
+		 * for when it would've been a JMP.
+		 */
+		next_insn->static_jump_dest = true;
+		(*new_insn)->static_jump_dest = true;
 		return 0;
+	}
 
 	if (orig_insn->type != INSN_JUMP_UNCONDITIONAL) {
 		WARN_FUNC("unsupported instruction at jump label",
@@ -695,7 +704,16 @@ static int handle_jump_alt(struct objtoo
 		return -1;
 	}
 
-	*new_insn = list_next_entry(orig_insn, list);
+	/*
+	 * Otherwise, orig_insn is a JMP and it will have orig_insn->jump_dest.
+	 * In this case we'll effectively NOP the alt by pointing new_insn at
+	 * next_insn.
+	 */
+	orig_insn->jump_dest->static_jump_dest = true;
+	next_insn->static_jump_dest = true;
+
+	*new_insn = next_insn;
+
 	return 0;
 }
 
@@ -1114,6 +1132,50 @@ static int read_retpoline_hints(struct o
 	return 0;
 }
 
+static int assert_static_jumps(struct objtool_file *file)
+{
+	struct section *sec, *relasec;
+	struct instruction *insn;
+	struct rela *rela;
+	int i;
+
+	sec = find_section_by_name(file->elf, ".discard.jump_assert");
+	if (!sec)
+		return 0;
+
+	relasec = sec->rela;
+	if (!relasec) {
+		WARN("missing .rela.discard.jump_assert section");
+		return -1;
+	}
+
+	if (sec->len % sizeof(unsigned long)) {
+		WARN("jump_assert size mismatch: %d %ld", sec->len, sizeof(unsigned long));
+		return -1;
+	}
+
+	for (i = 0; i < sec->len / sizeof(unsigned long); i++) {
+		rela = find_rela_by_dest(sec, i * sizeof(unsigned long));
+		if (!rela) {
+			WARN("can't find rela for jump_assert[%d]", i);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("can't find insn for jump_assert[%d]", i);
+			return -1;
+		}
+
+		if (!insn->static_jump_dest) {
+			WARN_FUNC("static assert FAIL", insn->sec, insn->offset);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 static int decode_sections(struct objtool_file *file)
 {
 	int ret;
@@ -2073,6 +2135,10 @@ int check(const char *_objname, bool orc
 		goto out;
 	warnings += ret;
 
+	ret = assert_static_jumps(&file);
+	if (ret < 0)
+		return ret;
+
 	if (list_empty(&file.insn_list))
 		goto out;
 
tools/objtool/check.h +1 -1
@@ -45,7 +45,7 @@ struct instruction {
 	unsigned char type;
 	unsigned long immediate;
 	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
-	bool retpoline_safe;
+	bool retpoline_safe, static_jump_dest;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct list_head alts;
Reviewed-by on v1 13/24
#50 lore raw

Re: [PATCH 13/24] objtool: Implement base jump_assert support

Quoted 29 lines · Peter Zijlstra

Implement a jump_label assertion that asserts that the code location is indeed only reachable through a static_branch. Because if GCC is absolutely retaded it could generate code like:

        xor rax,rax
        NOP/JMP 1f
        mov $1, rax
1:
        test rax,rax
        jz 2f

2:

instead of the sensible:

        NOP/JMP 1f

1:

This implements objtool infrastructure for ensuring the code ends up sane, since we'll rely on that for correctness and security.

We tag the instructions after the static branch with static_jump_dest=true; that is the instruction after the NOP and the instruction at the JMP+disp site.

Then, when we read the .discard.jump_assert section, we assert that each entry points to an instruction that has static_jump_dest set.

Quoted 8 lines

With this we can assert that the code emitted for the if statement ends up at the static jump location and nothing untowards happened.

Cc Thomas Gleixner <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Josh Poimboeuf <[email protected]>

Thank you for pandering to my paranoia. I suspect that misspelling the word 'retarded' isn't going to be sufficient to stop people from objecting to the use of that word, but other than that,

author maintainer
#51 lore raw

PATCH 14/24 x86: Add a type field to alt_instr

In reply to Peter Zijlstra

Add a type field to the alternative description. For now this will be used to annotate static_cpu_has() but possible future uses include using it to implement alternative alignment and special NOP handling.

---
 arch/x86/include/asm/alternative-asm.h |    3 ++-
 arch/x86/include/asm/alternative.h     |    6 +++++-
 arch/x86/include/asm/cpufeature.h      |    2 ++
 arch/x86/kernel/alternative.c          |    4 ++--
 tools/objtool/special.c                |    2 +-
 5 files changed, 12 insertions(+), 5 deletions(-)
arch/x86/include/asm/alternative-asm.h +2 -1
@@ -25,13 +25,14 @@
  * enough information for the alternatives patching code to patch an
  * instruction. See apply_alternatives().
  */
-.macro altinstruction_entry orig alt feature orig_len alt_len pad_len
+.macro altinstruction_entry orig alt feature orig_len alt_len pad_len type=0
 	.long \orig - .
 	.long \alt - .
 	.word \feature
 	.byte \orig_len
 	.byte \alt_len
 	.byte \pad_len
+	.byte \type
 .endm
 
 /*
arch/x86/include/asm/alternative.h +5 -1
@@ -45,6 +45,8 @@
 #define LOCK_PREFIX ""
 #endif
 
+#define ALT_TYPE_DEFAULT	0
+
 struct alt_instr {
 	s32 instr_offset;	/* original instruction */
 	s32 repl_offset;	/* offset to replacement instruction */
@@ -52,6 +54,7 @@ struct alt_instr {
 	u8  instrlen;		/* length of original instruction */
 	u8  replacementlen;	/* length of new instruction */
 	u8  padlen;		/* length of build-time padding */
+	u8  type;		/* type of alternative */
 } __packed;
 
 /*
@@ -127,7 +130,8 @@ static inline int alternatives_text_rese
 	" .word " __stringify(feature) "\n"		/* feature bit     */ \
 	" .byte " alt_total_slen "\n"			/* source len      */ \
 	" .byte " alt_rlen(num) "\n"			/* replacement len */ \
-	" .byte " alt_pad_len "\n"			/* pad len */
+	" .byte " alt_pad_len "\n"			/* pad len */	      \
+	" .byte 0 \n"					/* type */
 
 #define ALTINSTR_REPLACEMENT(newinstr, feature, num)	/* replacement */     \
 	b_replacement(num)":\n\t" newinstr "\n" e_replacement(num) ":\n\t"
arch/x86/include/asm/cpufeature.h +2 -0
@@ -157,6 +157,7 @@ static __always_inline __pure bool _stat
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 5f - 4f\n"		/* repl len */
 		 " .byte 3b - 2b\n"		/* pad len */
+		 " .byte 0\n"			/* type */
 		 ".previous\n"
 		 ".section .altinstr_replacement,\"ax\"\n"
 		 "4: jmp %l[t_no]\n"
@@ -169,6 +170,7 @@ static __always_inline __pure bool _stat
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 0\n"			/* repl len */
 		 " .byte 0\n"			/* pad len */
+		 " .byte 0\n"			/* type */
 		 ".previous\n"
 		 ".section .altinstr_aux,\"ax\"\n"
 		 "6:\n"
arch/x86/kernel/alternative.c +2 -2
@@ -400,11 +400,11 @@ void __init_or_module noinline apply_alt
 			continue;
 		}
 
-		DPRINTK("feat: %d*32+%d, old: (%p, len: %d), repl: (%p, len: %d), pad: %d",
+		DPRINTK("feat: %d*32+%d, old: (%p, len: %d), repl: (%p, len: %d), pad: %d, type: %d",
 			a->cpuid >> 5,
 			a->cpuid & 0x1f,
 			instr, a->instrlen,
-			replacement, a->replacementlen, a->padlen);
+			replacement, a->replacementlen, a->padlen, a->type);
 
 		DUMP_BYTES(instr, a->instrlen, "%p: old_insn: ", instr);
 		DUMP_BYTES(replacement, a->replacementlen, "%p: rpl_insn: ", replacement);
tools/objtool/special.c +1 -1
@@ -34,7 +34,7 @@
 #define JUMP_ORIG_OFFSET	0
 #define JUMP_NEW_OFFSET		8
 
-#define ALT_ENTRY_SIZE		13
+#define ALT_ENTRY_SIZE		14
 #define ALT_ORIG_OFFSET		0
 #define ALT_NEW_OFFSET		4
 #define ALT_FEATURE_OFFSET	8
author maintainer
#52 lore raw

PATCH 15/24 x86: Annotate static_cpu_has alternative

In reply to Peter Zijlstra

In order to recognise static_cpu_has() alternatives from any other alternative without dodgy heuristics, we need to explicitly mark them. Use the new type field for this.

---
 arch/x86/include/asm/alternative.h |    6 ++++++
 arch/x86/include/asm/cpufeature.h  |    5 +++--
 2 files changed, 9 insertions(+), 2 deletions(-)
arch/x86/include/asm/alternative.h +6 -0
@@ -47,6 +47,12 @@
 
 #define ALT_TYPE_DEFAULT	0
 
+/*
+ * This alt_instr descriptor is part of a static_cpu_has() construct. Use it to
+ * detect its type when processing with other tools, like objtool, for example.
+ */
+#define ALT_TYPE_STATIC_CPU_HAS	1
+
 struct alt_instr {
 	s32 instr_offset;	/* original instruction */
 	s32 repl_offset;	/* offset to replacement instruction */
arch/x86/include/asm/cpufeature.h +3 -2
@@ -157,7 +157,7 @@ static __always_inline __pure bool _stat
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 5f - 4f\n"		/* repl len */
 		 " .byte 3b - 2b\n"		/* pad len */
-		 " .byte 0\n"			/* type */
+		 " .byte %P[type]\n"		/* type */
 		 ".previous\n"
 		 ".section .altinstr_replacement,\"ax\"\n"
 		 "4: jmp %l[t_no]\n"
@@ -170,7 +170,7 @@ static __always_inline __pure bool _stat
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 0\n"			/* repl len */
 		 " .byte 0\n"			/* pad len */
-		 " .byte 0\n"			/* type */
+		 " .byte %P[type]\n"		/* type */
 		 ".previous\n"
 		 ".section .altinstr_aux,\"ax\"\n"
 		 "6:\n"
@@ -181,6 +181,7 @@ static __always_inline __pure bool _stat
 		 : : [feature]  "i" (bit),
 		     [always]   "i" (X86_FEATURE_ALWAYS),
 		     [bitnum]   "i" (1 << (bit & 7)),
+		     [type]	"i" (ALT_TYPE_STATIC_CPU_HAS),
 		     [cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
 		 : : t_yes, t_no);
 t_yes:
author maintainer
#53 lore raw

PATCH 16/24 objtool: Implement jump_assert for _static_cpu_has()

In reply to Peter Zijlstra

Unlike the jump_label bits, static_cpu_has is implemented with alternatives. We use the new type field to distinguish them from any other alternatives

Like jump_labels, make static_cpu_has set static_jump_dest on the instructions after the static branch such that we can assert on it.

Cc Thomas Gleixner <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Josh Poimboeuf <[email protected]>
---
 tools/objtool/check.c   |   21 +++++++++++++++++++++
 tools/objtool/special.c |   26 +++++++++++++++-----------
 tools/objtool/special.h |    1 +
 3 files changed, 37 insertions(+), 11 deletions(-)
tools/objtool/check.c +21 -0
@@ -636,6 +636,12 @@ static int handle_group_alt(struct objto
 	fake_jump->ignore = true;
 
 	if (!special_alt->new_len) {
+		/*
+		 * The NOP case for _static_cpu_has()
+		 */
+		if (special_alt->static_cpu_has)
+			fake_jump->jump_dest->static_jump_dest = true;
+
 		*new_insn = fake_jump;
 		return 0;
 	}
@@ -664,6 +670,21 @@ static int handle_group_alt(struct objto
 				  insn->sec, insn->offset);
 			return -1;
 		}
+
+		if (special_alt->static_cpu_has) {
+			if (insn->type != INSN_JUMP_UNCONDITIONAL) {
+				WARN_FUNC("not an unconditional jump in _static_cpu_has()",
+					  insn->sec, insn->offset);
+			}
+			if (insn->jump_dest == fake_jump) {
+				WARN_FUNC("jump inside alternative for _static_cpu_has()",
+					  insn->sec, insn->offset);
+			}
+			/*
+			 * The JMP+disp case for _static_cpu_has()
+			 */
+			insn->jump_dest->static_jump_dest = true;
+		}
 	}
 
 	if (!last_new_insn) {
tools/objtool/special.c +15 -11
@@ -40,6 +40,10 @@
 #define ALT_FEATURE_OFFSET	8
 #define ALT_ORIG_LEN_OFFSET	10
 #define ALT_NEW_LEN_OFFSET	11
+#define ALT_TYPE_OFFSET		13
+
+#define ALT_TYPE_DEFAULT	0
+#define ALT_TYPE_STATIC_CPU_HAS	1
 
 #define X86_FEATURE_POPCNT (4*32+23)
 
@@ -48,7 +52,6 @@ struct special_entry {
 	bool group, jump_or_nop;
 	unsigned char size, orig, new;
 	unsigned char orig_len, new_len; /* group only */
-	unsigned char feature; /* ALTERNATIVE macro CPU feature */
 };
 
 struct special_entry entries[] = {
@@ -60,7 +63,6 @@ struct special_entry entries[] = {
 		.orig_len = ALT_ORIG_LEN_OFFSET,
 		.new = ALT_NEW_OFFSET,
 		.new_len = ALT_NEW_LEN_OFFSET,
-		.feature = ALT_FEATURE_OFFSET,
 	},
 	{
 		.sec = "__jump_table",
@@ -84,24 +86,23 @@ static int get_alt_entry(struct elf *elf
 {
 	struct rela *orig_rela, *new_rela;
 	unsigned long offset;
+	void *data;
 
 	offset = idx * entry->size;
+	data = sec->data->d_buf + offset;
 
 	alt->group = entry->group;
 	alt->jump_or_nop = entry->jump_or_nop;
 
 	if (alt->group) {
-		alt->orig_len = *(unsigned char *)(sec->data->d_buf + offset +
-						   entry->orig_len);
-		alt->new_len = *(unsigned char *)(sec->data->d_buf + offset +
-						  entry->new_len);
-	}
-
-	if (entry->feature) {
 		unsigned short feature;
+		unsigned char type;
 
-		feature = *(unsigned short *)(sec->data->d_buf + offset +
-					      entry->feature);
+		alt->orig_len = *(unsigned char *)(data + entry->orig_len);
+		alt->new_len = *(unsigned char *)(data + entry->new_len);
+
+		feature = *(unsigned short *)(data + ALT_FEATURE_OFFSET);
+		type = *(unsigned char *)(data + ALT_TYPE_OFFSET);
 
 		/*
 		 * It has been requested that we don't validate the !POPCNT
@@ -110,6 +111,9 @@ static int get_alt_entry(struct elf *elf
 		 */
 		if (feature == X86_FEATURE_POPCNT)
 			alt->skip_orig = true;
+
+		if (type == ALT_TYPE_STATIC_CPU_HAS)
+			alt->static_cpu_has = true;
 	}
 
 	orig_rela = find_rela_by_dest(sec, offset + entry->orig);
tools/objtool/special.h +1 -0
@@ -27,6 +27,7 @@ struct special_alt {
 	bool group;
 	bool skip_orig;
 	bool jump_or_nop;
+	bool static_cpu_has;
 
 	struct section *orig_sec;
 	unsigned long orig_off;
author maintainer
#54 lore raw

PATCH 17/24 objtool: Introduce special_type

In reply to Peter Zijlstra

Use an enum for the special_alt entries instead of a collection of booleans.

---
 tools/objtool/check.c   |   14 +++++++++++---
 tools/objtool/special.c |   14 +++++++-------
 tools/objtool/special.h |   10 ++++++++--
 3 files changed, 26 insertions(+), 12 deletions(-)
tools/objtool/check.c +11 -3
@@ -773,7 +773,7 @@ static int add_special_section_alts(stru
 			continue;
 
 		new_insn = NULL;
-		if (!special_alt->group || special_alt->new_len) {
+		if (special_alt->type != alternative || special_alt->new_len) {
 			new_insn = find_insn(file, special_alt->new_sec,
 					     special_alt->new_off);
 			if (!new_insn) {
@@ -785,16 +785,24 @@ static int add_special_section_alts(stru
 			}
 		}
 
-		if (special_alt->group) {
+		switch (special_alt->type) {
+		case alternative:
 			ret = handle_group_alt(file, special_alt, orig_insn,
 					       &new_insn);
 			if (ret)
 				goto out;
-		} else if (special_alt->jump_or_nop) {
+			break;
+
+		case jump_label:
 			ret = handle_jump_alt(file, special_alt, orig_insn,
 					      &new_insn);
 			if (ret)
 				goto out;
+
+			break;
+
+		default:
+			break;
 		}
 
 		alt = malloc(sizeof(*alt));
tools/objtool/special.c +7 -7
@@ -49,7 +49,7 @@
 
 struct special_entry {
 	const char *sec;
-	bool group, jump_or_nop;
+	enum special_type type;
 	unsigned char size, orig, new;
 	unsigned char orig_len, new_len; /* group only */
 };
@@ -57,7 +57,7 @@ struct special_entry {
 struct special_entry entries[] = {
 	{
 		.sec = ".altinstructions",
-		.group = true,
+		.type = alternative,
 		.size = ALT_ENTRY_SIZE,
 		.orig = ALT_ORIG_OFFSET,
 		.orig_len = ALT_ORIG_LEN_OFFSET,
@@ -66,13 +66,14 @@ struct special_entry entries[] = {
 	},
 	{
 		.sec = "__jump_table",
-		.jump_or_nop = true,
+		.type = jump_label,
 		.size = JUMP_ENTRY_SIZE,
 		.orig = JUMP_ORIG_OFFSET,
 		.new = JUMP_NEW_OFFSET,
 	},
 	{
 		.sec = "__ex_table",
+		.type = exception,
 		.size = EX_ENTRY_SIZE,
 		.orig = EX_ORIG_OFFSET,
 		.new = EX_NEW_OFFSET,
@@ -91,10 +92,9 @@ static int get_alt_entry(struct elf *elf
 	offset = idx * entry->size;
 	data = sec->data->d_buf + offset;
 
-	alt->group = entry->group;
-	alt->jump_or_nop = entry->jump_or_nop;
+	alt->type = entry->type;
 
-	if (alt->group) {
+	if (alt->type == alternative) {
 		unsigned short feature;
 		unsigned char type;
 
@@ -130,7 +130,7 @@ static int get_alt_entry(struct elf *elf
 	alt->orig_sec = orig_rela->sym->sec;
 	alt->orig_off = orig_rela->addend;
 
-	if (!entry->group || alt->new_len) {
+	if (entry->type != alternative || alt->new_len) {
 		new_rela = find_rela_by_dest(sec, offset + entry->new);
 		if (!new_rela) {
 			WARN_FUNC("can't find new rela",
tools/objtool/special.h +8 -2
@@ -21,12 +21,18 @@
 #include <stdbool.h>
 #include "elf.h"
 
+enum special_type {
+	alternative,
+	jump_label,
+	exception,
+};
+
 struct special_alt {
 	struct list_head list;
 
-	bool group;
+	enum special_type type;
+
 	bool skip_orig;
-	bool jump_or_nop;
 	bool static_cpu_has;
 
 	struct section *orig_sec;
author maintainer
#55 lore raw

PATCH 18/24 objtool: More complex static jump implementation

In reply to Peter Zijlstra

When using something like:

  -#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x]))
  +#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x]) && \
  +			(arch_static_assert(), true))

we get an objtool assertion fail like:

kernel/sched/fair.o: warning: objtool: hrtick_update()+0xd: static assert FAIL

where:

0000000000001140 <hrtick_update>:

    1140:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
    1145:       c3                      retq
    1146:       48 8b b7 30 09 00 00    mov    0x930(%rdi),%rsi
    114d:       8b 87 d8 09 00 00       mov    0x9d8(%rdi),%eax
    1153:       48 0f a3 05 00 00 00    bt     %rax,0x0(%rip)        # 115b <hrtick_update+0x1b>
    115a:       00
                        1157: R_X86_64_PC32     __cpu_active_mask-0x4

and:

RELOCATION RECORDS FOR [__jump_table]:
0000000000000150 R_X86_64_64 .text+0x0000000000001140
0000000000000158 R_X86_64_64 .text+0x0000000000001146

RELOCATION RECORDS FOR [.discard.jump_assert]:
0000000000000028 R_X86_64_64 .text+0x000000000000114d

IOW, GCC managed to place the assertion 1 instruction _after_ the static jump target (it lifted a load over it).

So while the code generation is fine, the assertion gets placed wrong. We can 'fix' this by not only considering the immediate static jump locations but also all the unconditional code after it, terminating the basic block on any unconditional instruction or branch entry point.

---
 tools/objtool/check.c |   41 +++++++++++++++++++++++++++++++++++++++++
 tools/objtool/check.h |    1 +
 2 files changed, 42 insertions(+)
tools/objtool/check.c +41 -0
@@ -520,6 +520,8 @@ static int add_jump_destinations(struct
 				  dest_off);
 			return -1;
 		}
+
+		insn->jump_dest->branch_target++;
 	}
 
 	return 0;
@@ -1205,6 +1207,41 @@ static int assert_static_jumps(struct ob
 	return 0;
 }
 
+static int grow_static_blocks(struct objtool_file *file)
+{
+	struct instruction *insn;
+	bool static_block = false;
+
+	for_each_insn(file, insn) {
+		if (!static_block && !insn->static_jump_dest)
+			continue;
+
+		if (insn->static_jump_dest) {
+			static_block = true;
+			continue;
+		}
+
+		if (insn->branch_target) {
+			static_block = false;
+			continue;
+		} else switch (insn->type) {
+		case INSN_JUMP_CONDITIONAL:
+		case INSN_JUMP_UNCONDITIONAL:
+		case INSN_JUMP_DYNAMIC:
+		case INSN_CALL:
+		case INSN_CALL_DYNAMIC:
+		case INSN_RETURN:
+		case INSN_BUG:
+			static_block = false;
+			continue;
+		}
+
+		insn->static_jump_dest = static_block;
+	}
+
+	return 0;
+}
+
 static int decode_sections(struct objtool_file *file)
 {
 	int ret;
@@ -1247,6 +1284,10 @@ static int decode_sections(struct objtoo
 	if (ret)
 		return ret;
 
+	ret = grow_static_blocks(file);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
tools/objtool/check.h +1 -0
@@ -46,6 +46,7 @@ struct instruction {
 	unsigned long immediate;
 	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
 	bool retpoline_safe, static_jump_dest;
+	unsigned int branch_target;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct list_head alts;
author maintainer
#56 lore raw

PATCH 19/24 objtool: Even more complex static block checks

In reply to Peter Zijlstra

I've observed GCC transform:

f()

  {
	  if (!static_branch_unlikely())
		  return;

	  static_assert();
	  A;

}

g()

  {
	  f();

}

Into:

f()

  {
	  static_assert();
	  A;

}

g()

  {
	  if (static_branch_unlikely())
		  f();

}

Which results in the assertion landing at f+0. The transformation is valid and useful; it avoids a pointless CALL+RET sequence, so we'll have to teach objtool how to deal with this.

Do this by marking all CALL destinations with static_call when called from a static_block and non_static_call when called outside a static_block. This allows us to identify functions called exclusively from a static_block and start them with a static_block.

Limit to static functions and do not apply recursive.

---
 tools/objtool/check.c |   84 +++++++++++++++++++++++++++++++++++++-------------
 tools/objtool/elf.h   |    1 
 2 files changed, 64 insertions(+), 21 deletions(-)
tools/objtool/check.c +63 -21
@@ -1207,36 +1207,78 @@ static int assert_static_jumps(struct ob
 	return 0;
 }
 
+static bool __grow_static_block(struct objtool_file *file,
+				struct instruction *insn)
+{
+	/* if a !static jump can come in here, terminate */
+	if (insn->branch_target && !insn->static_jump_dest)
+		return false;
+
+	switch (insn->type) {
+	case INSN_JUMP_UNCONDITIONAL:
+		/* mark this instruction, terminate this section */
+		insn->static_jump_dest = true;
+		return false;
+
+	/* these disturb unconditional code flow, terminate */
+	case INSN_JUMP_CONDITIONAL:
+	case INSN_JUMP_DYNAMIC:
+	case INSN_RETURN:
+	case INSN_BUG:
+		return false;
+
+	/* these return right back and don't disturb the code flow */
+	case INSN_CALL:
+	case INSN_CALL_DYNAMIC:
+		break;
+	}
+
+	/* mark this insn, and continue the section */
+	insn->static_jump_dest = true;
+	return true;
+}
+
 static int grow_static_blocks(struct objtool_file *file)
 {
-	struct instruction *insn;
 	bool static_block = false;
+	struct symbol *func, *tmp;
+	struct instruction *insn;
+	struct section *sec;
 
 	for_each_insn(file, insn) {
-		if (!static_block && !insn->static_jump_dest)
-			continue;
+		if (static_block || insn->static_jump_dest)
+			static_block = __grow_static_block(file, insn);
 
-		if (insn->static_jump_dest) {
-			static_block = true;
-			continue;
+		if (insn->type == INSN_CALL) {
+			func = insn->call_dest;
+			if (!func)
+				continue;
+
+			if (static_block)
+				func->static_call = true;
+			else
+				func->non_static_call = true;
 		}
+	}
 
-		if (insn->branch_target) {
-			static_block = false;
-			continue;
-		} else switch (insn->type) {
-		case INSN_JUMP_CONDITIONAL:
-		case INSN_JUMP_UNCONDITIONAL:
-		case INSN_JUMP_DYNAMIC:
-		case INSN_CALL:
-		case INSN_CALL_DYNAMIC:
-		case INSN_RETURN:
-		case INSN_BUG:
-			static_block = false;
-			continue;
+	for_each_sec(file, sec) {
+		list_for_each_entry_safe(func, tmp, &sec->symbol_list, list) {
+			if (func->bind != STB_LOCAL)
+				continue;
+
+			if (!func->static_call)
+				continue;
+
+			if (func->non_static_call)
+				continue;
+
+			/* static && !non_static -- only static callers */
+
+			func_for_each_insn(file, func, insn) {
+				if (!__grow_static_block(file, insn))
+					break;
+			}
 		}
-
-		insn->static_jump_dest = static_block;
 	}
 
 	return 0;
tools/objtool/elf.h +1 -0
@@ -61,6 +61,7 @@ struct symbol {
 	unsigned char bind, type;
 	unsigned long offset;
 	unsigned int len;
+	bool static_call, non_static_call;
 };
 
 struct rela {
author maintainer
#57 lore raw

PATCH 20/24 objtool: Another static block fail

In reply to Peter Zijlstra

I've observed GCC generate:

sym:
NOP/JMP 1f (static_branch)
JMP 2f
1: /* crud */
JMP 3f
2: /* other crud */

3: RETQ

This means we need to follow unconditional jumps; be conservative and only follow if its a unique jump.

(I've not yet figured out which CONFIG option is responsible for this,
a normal defconfig build does not generate crap like this)

---
 tools/objtool/check.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++---
 tools/objtool/elf.h   |    2 -
 2 files changed, 51 insertions(+), 4 deletions(-)
tools/objtool/check.c +50 -3
@@ -1207,6 +1207,8 @@ static int assert_static_jumps(struct ob
 	return 0;
 }
 
+static void jmp_grow_static_block(struct objtool_file *file, struct instruction *insn);
+
 static bool __grow_static_block(struct objtool_file *file,
 				struct instruction *insn)
 {
@@ -1216,7 +1218,8 @@ static bool __grow_static_block(struct o
 
 	switch (insn->type) {
 	case INSN_JUMP_UNCONDITIONAL:
-		/* mark this instruction, terminate this section */
+		/* follow the jump, mark this instruction, terminate this section */
+		jmp_grow_static_block(file, insn);
 		insn->static_jump_dest = true;
 		return false;
 
@@ -1238,6 +1241,50 @@ static bool __grow_static_block(struct o
 	return true;
 }
 
+static void jmp_grow_static_block(struct objtool_file *file, struct instruction *insn)
+{
+	struct instruction *dest = insn->jump_dest;
+	bool old_sjd, static_block;
+
+	if (!dest)
+		return;
+
+	/* more than a single site jumps here, can't be certain */
+	if (dest->branch_target > 1)
+		return;
+
+	/* mark the destination, so we can continue there */
+	old_sjd = dest->static_jump_dest;
+	dest->static_jump_dest = true;
+
+	if (dest->offset > insn->offset) {
+		/* fwd jump, the main iteration will still get there. */
+		return;
+	}
+
+	/* backwards jump, we need to revisit the instructions */
+	do {
+		static_block = __grow_static_block(file, dest);
+
+		if (insn->type == INSN_CALL && !old_sjd && dest->static_jump_dest) {
+			struct symbol *func = insn->call_dest;
+			if (!func || func->bind != STB_LOCAL)
+				goto next;
+
+			/*
+			 * we flipped the call to static, update the stats.
+			 */
+			if (insn->static_jump_dest) {
+				func->non_static_call--;
+				func->static_call++;
+			}
+		}
+next:
+		dest = list_next_entry(dest, list);
+		old_sjd = dest->static_jump_dest;
+	} while (static_block && dest != insn);
+}
+
 static int grow_static_blocks(struct objtool_file *file)
 {
 	bool static_block = false;
@@ -1255,9 +1302,9 @@ static int grow_static_blocks(struct obj
 				continue;
 
 			if (static_block)
-				func->static_call = true;
+				func->static_call++;
 			else
-				func->non_static_call = true;
+				func->non_static_call++;
 		}
 	}
 
tools/objtool/elf.h +1 -1
@@ -61,7 +61,7 @@ struct symbol {
 	unsigned char bind, type;
 	unsigned long offset;
 	unsigned int len;
-	bool static_call, non_static_call;
+	unsigned int static_call, non_static_call;
 };
 
 struct rela {
#58 lore raw

Re: [PATCH 20/24] objtool: Another static block fail

Quoted 12 lines · Peter Zijlstra

I've observed GCC generate:

sym:
NOP/JMP 1f (static_branch)
JMP 2f
1: /* crud */
JMP 3f
2: /* other crud */

3: RETQ

Quoted 7 lines

This means we need to follow unconditional jumps; be conservative and only follow if its a unique jump.

(I've not yet figured out which CONFIG option is responsible for this,
a normal defconfig build does not generate crap like this)

Any chance we can just add a compiler barrier to the assertion macro and avoid all this grow_static_blocks() mess? It seems a bit... fragile.

Signature · 1 line
Josh
author maintainer
#59 lore raw

Re: [PATCH 20/24] objtool: Another static block fail

Quoted 16 lines · Josh Poimboeuf
Quoted 15 lines · Peter Zijlstra

I've observed GCC generate:

sym:
NOP/JMP 1f (static_branch)
JMP 2f
1: /* crud */
JMP 3f
2: /* other crud */

3: RETQ

This means we need to follow unconditional jumps; be conservative and only follow if its a unique jump.

Quoted 7 lines
Quoted 4 lines

(I've not yet figured out which CONFIG option is responsible for this,
a normal defconfig build does not generate crap like this)

Any chance we can just add a compiler barrier to the assertion macro and avoid all this grow_static_blocks() mess? It seems a bit... fragile.

It is all rather unfortunate yes.. :/ I've tried to keep the grow stuff as conservative as possible while still covering all the weirdness I found. And while it was great fun, I do agree it would be much better to not have to do this.

You're thinking of something like this?

static __always_inline void arch_static_assert(void)

 {
        asm volatile ("1:\n\t"
                      ".pushsection .discard.jump_assert \n\t"
                      _ASM_ALIGN  "\n\t"
                      _ASM_PTR "1b \n\t"

- ".popsection \n\t");
+ ".popsection \n\t" ::: "memory");
}

That doesn't seem to matter much; see here:

static void
ttwu_stat(struct task_struct *p, int cpu, int wake_flags)

{
        struct rq *rq;

        if (!schedstat_enabled())
                return;

        rq = this_rq();
$ objdump -dr build/kernel/sched/core.o

0000000000001910 <ttwu_stat>:

    1910:       e8 00 00 00 00          callq  1915 <ttwu_stat+0x5>
                        1911: R_X86_64_PC32     __fentry__-0x4
    1915:       41 57                   push   %r15
    1917:       41 56                   push   %r14
    1919:       41 55                   push   %r13
    191b:       41 54                   push   %r12
    191d:       55                      push   %rbp
    191e:       53                      push   %rbx
    191f:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
    1924:       eb 25                   jmp    194b <ttwu_stat+0x3b>
    1926:       41 89 d5                mov    %edx,%r13d
    1929:       41 89 f4                mov    %esi,%r12d
    192c:       48 89 fb                mov    %rdi,%rbx
    192f:       49 c7 c6 00 00 00 00    mov    $0x0,%r14
                        1932: R_X86_64_32S      runqueues
$ objdump -j __jump_table -sr build/kernel/sched.o
0000000000000048 R_X86_64_64       .text+0x000000000000191f
0000000000000050 R_X86_64_64       .text+0x0000000000001926
0000000000000058 R_X86_64_64       sched_schedstats
$ objdump -j .discard.jump_assert -dr build/kernel/sched.o

0000000000000000 R_X86_64_64 .text+0x000000000000192f

It still lifts random crud over that first initial statement (the rq load).

#60 lore raw

Re: [PATCH 20/24] objtool: Another static block fail

Quoted 22 lines · Peter Zijlstra
Quoted 21 lines · Josh Poimboeuf
Quoted 19 lines · Peter Zijlstra

I've observed GCC generate:

sym:
NOP/JMP 1f (static_branch)
JMP 2f
1: /* crud */
JMP 3f
2: /* other crud */

3: RETQ

This means we need to follow unconditional jumps; be conservative and only follow if its a unique jump.

(I've not yet figured out which CONFIG option is responsible for this,
a normal defconfig build does not generate crap like this)

Quoted 7 lines
Quoted 2 lines

Any chance we can just add a compiler barrier to the assertion macro and avoid all this grow_static_blocks() mess? It seems a bit... fragile.

It is all rather unfortunate yes.. :/ I've tried to keep the grow stuff as conservative as possible while still covering all the weirdness I found. And while it was great fun, I do agree it would be much better to not have to do this.

The more I look at this patch, and the previous one, the less sure I am about this idea. (This isn't a comment about the code, just some general uneasiness about the unpredictable GCC behavior we're trying to constrain, and whether it's worth the pain.)

Covering all the corner cases is complicated, and it might get even more complicated with future compiler optimizations.

What if we *only* use the assertions in places that really need them, like the IBRS checks? Would that allow us to simplify and drop these last two (or three) patches?

Or, maybe we should just forget the whole thing and just stick with the dynamic IBRS checks with lfence. Yes, it's less ideal for the kernel, but adding these acrobatics to objtool also has a cost.

Signature · 1 line
Josh
author maintainer
#61 lore raw

Re: [PATCH 20/24] objtool: Another static block fail

Quoted 4 lines · Josh Poimboeuf

Or, maybe we should just forget the whole thing and just stick with the dynamic IBRS checks with lfence. Yes, it's less ideal for the kernel, but adding these acrobatics to objtool also has a cost.

For now, IBRS seems off the table entirely. But no, I really don't want to have to unconditionally eat the LFENCE cost in all those sites.

#62 lore raw

Re: [PATCH 20/24] objtool: Another static block fail

Quoted 8 lines · Peter Zijlstra
Quoted 4 lines · Josh Poimboeuf


Or, maybe we should just forget the whole thing and just stick with the dynamic IBRS checks with lfence.  Yes, it's less ideal for the kernel, but adding these acrobatics to objtool also has a cost.

For now, IBRS seems off the table entirely. But no, I really don't want to have to unconditionally eat the LFENCE cost in all those sites.

There's also alternatives. And without the IBRS-on-kernel-entry bits there aren't that many call sites that really need this anyway and don't have *other* conditionals that really are runtime-only (like dumpable etc.).

author maintainer
#63 lore raw

Re: [PATCH 20/24] objtool: Another static block fail

Quoted 7 lines · David Woodhouse
Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · Josh Poimboeuf


Or, maybe we should just forget the whole thing and just stick with the dynamic IBRS checks with lfence.  Yes, it's less ideal for the kernel, but adding these acrobatics to objtool also has a cost.

Quoted 7 lines
Quoted 2 lines

For now, IBRS seems off the table entirely. But no, I really don't want to have to unconditionally eat the LFENCE cost in all those sites.

There's also alternatives. And without the IBRS-on-kernel-entry bits there aren't that many call sites that really need this anyway and don't have *other* conditionals that really are runtime-only (like dumpable etc.).

There is that.. So I think people wanted to use jump_labels for IBRS so that we could runtime enable it when people loaded dodgy modules or something. But even that we could all write in inline asm if needed I suppose, it'll be ugly, but it should work.

author maintainer
#64 lore raw

PATCH 21/24 objtool: Skip static assert when KCOV/KASAN

In reply to Peter Zijlstra

They make an absolutely horrid mess of things.

---
 scripts/Makefile.build        |    9 +++++++++
 tools/objtool/builtin-check.c |    3 ++-
 tools/objtool/builtin.h       |    2 +-
 tools/objtool/check.c         |    3 +++
 4 files changed, 15 insertions(+), 2 deletions(-)
scripts/Makefile.build +9 -0
@@ -272,6 +272,15 @@ endif
 ifdef CONFIG_RETPOLINE
   objtool_args += --retpoline
 endif
+ifdef CONFIG_KCOV
+  objtool_no_assert := 1
+endif
+ifdef CONFIG_KASAN
+  objtool_no_assert := 1
+endif
+ifdef objtool_no_assert
+  objtool_args += --no-static-assert
+endif
 
 
 ifdef CONFIG_MODVERSIONS
tools/objtool/builtin-check.c +2 -1
@@ -29,7 +29,7 @@
 #include "builtin.h"
 #include "check.h"
 
-bool no_fp, no_unreachable, retpoline;
+bool no_fp, no_unreachable, retpoline, no_assert;
 
 static const char * const check_usage[] = {
 	"objtool check [<options>] file.o",
@@ -40,6 +40,7 @@ const struct option check_options[] = {
 	OPT_BOOLEAN('f', "no-fp", &no_fp, "Skip frame pointer validation"),
 	OPT_BOOLEAN('u', "no-unreachable", &no_unreachable, "Skip 'unreachable instruction' warnings"),
 	OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"),
+	OPT_BOOLEAN('s', "no-static-assert", &no_assert, "Skip static branch validation"),
 	OPT_END(),
 };
 
tools/objtool/builtin.h +1 -1
@@ -20,7 +20,7 @@
 #include <subcmd/parse-options.h>
 
 extern const struct option check_options[];
-extern bool no_fp, no_unreachable, retpoline;
+extern bool no_fp, no_unreachable, retpoline, no_assert;
 
 extern int cmd_check(int argc, const char **argv);
 extern int cmd_orc(int argc, const char **argv);
tools/objtool/check.c +3 -0
@@ -1122,6 +1122,9 @@ static int assert_static_jumps(struct ob
 	struct rela *rela;
 	int i;
 
+	if (no_assert)
+		return 0;
+
 	sec = find_section_by_name(file->elf, ".discard.jump_assert");
 	if (!sec)
 		return 0;
author maintainer
#65 lore raw

PATCH 22/24 x86/jump_label: Implement arch_static_assert()

In reply to Peter Zijlstra

Implement the static (branch) assertion. It simply emits the address of the next instruction into a special section which objtool will read and validate against either __jump_table or .altinstructions.

Use like:

	if (static_branch_likely(_key)) {
		arch_static_assert();
		/* do stuff */
	}

Or

	if (static_cpu_has(_feat)) {
		arch_static_assert();
		/* do stuff */
	}
Cc Borislav Petkov <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Josh Poimboeuf <[email protected]>
---
 arch/x86/include/asm/jump_label.h |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
arch/x86/include/asm/jump_label.h +23 -0
@@ -62,6 +62,29 @@ static __always_inline bool arch_static_
 	return true;
 }
 
+/*
+ * Annotation for objtool; asserts that the previous instruction is the
+ * jump_label patch site. Or rather, that the next instruction is a static
+ * branch target.
+ *
+ * Use like:
+ *
+ *	if (static_branch_likely(key)) {
+ *		arch_static_assert();
+ *		do_code();
+ *	}
+ *
+ * Also works with static_cpu_has().
+ */
+static __always_inline void arch_static_assert(void)
+{
+	asm volatile ("1:\n\t"
+		      ".pushsection .discard.jump_assert \n\t"
+		      _ASM_ALIGN  "\n\t"
+		      _ASM_PTR "1b \n\t"
+		      ".popsection \n\t");
+}
+
 #ifdef CONFIG_X86_64
 typedef u64 jump_label_t;
 #else
author maintainer
#66 lore raw

PATCH 23/24 x86: Force asm-goto

In reply to Peter Zijlstra

Now that we have objtool to validate the correctness of asm-goto constructs we can start using it to guarantee the absence of dynamic branches (and thus speculation).

A primary prerequisite for this is of course that the compiler supports asm-goto. This effecively lifts the minimum GCC version to build an x86 kernel to gcc-4.5.

---
 Makefile          |   13 +++++++------
 arch/x86/Makefile |    4 ++++
 2 files changed, 11 insertions(+), 6 deletions(-)
Makefile +7 -6
@@ -513,6 +513,13 @@ ifneq ($(filter install,$(MAKECMDGOALS))
         endif
 endif
 
+# check for 'asm goto'
+ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
+  CC_HAVE_ASM_GOTO := 1
+  KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
+  KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
+endif
+
 ifeq ($(mixed-targets),1)
 # ===========================================================================
 # We're called with mixed targets (*config and build targets).
@@ -652,12 +659,6 @@ KBUILD_CFLAGS += $(call cc-ifversion, -l
 # Tell gcc to never replace conditional load with a non-conditional one
 KBUILD_CFLAGS	+= $(call cc-option,--param=allow-store-data-races=0)
 
-# check for 'asm goto'
-ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
-	KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
-	KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
-endif
-
 include scripts/Makefile.gcc-plugins
 
 ifdef CONFIG_READABLE_ASM
arch/x86/Makefile +4 -0
@@ -186,6 +186,10 @@ ifdef CONFIG_FUNCTION_GRAPH_TRACER
   endif
 endif
 
+ifndef CC_HAVE_ASM_GOTO
+  $(error Compiler lacks asm-goto support.)
+endif
+
 #
 # Jump labels need '-maccumulate-outgoing-args' for gcc < 4.5.2 to prevent a
 # GCC bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46226).  There's no way
author maintainer
#67 lore raw

PATCH 24/24 x86: Remove FAST_FEATURE_TESTS

In reply to Peter Zijlstra

Since we want to rely on static branches to avoid speculation, remove any possible fallback code for static_cpu_has.

---
 arch/x86/Kconfig                  |   11 -----------
 arch/x86/include/asm/cpufeature.h |    8 --------
 2 files changed, 19 deletions(-)
arch/x86/Kconfig +0 -11
@@ -389,17 +389,6 @@ config X86_FEATURE_NAMES
 
 	  If in doubt, say Y.
 
-config X86_FAST_FEATURE_TESTS
-	bool "Fast CPU feature tests" if EMBEDDED
-	default y
-	---help---
-	  Some fast-paths in the kernel depend on the capabilities of the CPU.
-	  Say Y here for the kernel to patch in the appropriate code at runtime
-	  based on the capabilities of the CPU. The infrastructure for patching
-	  code at runtime takes up some additional space; space-constrained
-	  embedded systems may wish to say N here to produce smaller, slightly
-	  slower code.
-
 config X86_X2APIC
 	bool "Support x2apic"
 	depends on X86_LOCAL_APIC && X86_64 && (IRQ_REMAP || HYPERVISOR_GUEST)
arch/x86/include/asm/cpufeature.h +0 -8
@@ -137,7 +137,6 @@ extern void clear_cpu_cap(struct cpuinfo
 
 #define setup_force_cpu_bug(bit) setup_force_cpu_cap(bit)
 
-#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_X86_FAST_FEATURE_TESTS)
 /*
  * Static testing of CPU features.  Used the same as boot_cpu_has().
  * These will statically patch the target code for additional
@@ -196,13 +195,6 @@ static __always_inline __pure bool _stat
 		boot_cpu_has(bit) :				\
 		_static_cpu_has(bit)				\
 )
-#else
-/*
- * Fall back to dynamic for gcc versions which don't support asm goto. Should be
- * a minority now anyway.
- */
-#define static_cpu_has(bit)		boot_cpu_has(bit)
-#endif
 
 #define cpu_has_bug(c, bit)		cpu_has(c, (bit))
 #define set_cpu_bug(c, bit)		set_cpu_cap(c, (bit))
author maintainer
#68 lore raw
In reply to Peter Zijlstra
Quoted 7 lines · Peter Zijlstra

This series adds two features to objtool:

- retpoline validation; it verifies no indirect jumps/calls are left in CONFIG_RETPOLINE=y builds.

includes fixups for kvm and others

Quoted 7 lines

- asm-goto validation; allows asserting that a block of code is not reachable through a dynamic branch.

After this it forces an asm-goto capable compiler to build arch/x86 and removes the fallback code for static_cpu_has(). After which we can use static_cpu_has() to guarantee a lack of dynamic branches, which is a requested feature for various spectre related things.

x86_64-allmodconfig -KCOV -KASAN +patch gives:

drivers/infiniband/hw/bnxt_re/.tmp_qplib_fp.o: warning: objtool: bnxt_qplib_poll_cq()+0xf8: sibling call from callable instruction with modified stack frame drivers/watchdog/.tmp_hpwdt.o: warning: objtool: .text+0x24: indirect call found in RETPOLINE build

---
Subject: jump_label: Add static assertion to every static_branch
From: Peter Zijlstra <[email protected]>
Date: Tue Jan 16 15:27:36 CET 2018

For testing only.. can't use in general because people do:

  static_branch() || more-cond

which confuses the living daylight out of objtool validation and really isn't fixable.

DO NOT MERGE

---
 arch/x86/include/asm/jump_label.h |    1 +
 include/linux/cgroup.h            |    2 +-
 include/linux/jump_label.h        |   15 +++++++++++++--
 3 files changed, 15 insertions(+), 3 deletions(-)
arch/x86/include/asm/jump_label.h +1 -0
@@ -76,6 +76,7 @@ static __always_inline bool arch_static_
  *
  * Also works with static_cpu_has().
  */
+#define arch_static_assert arch_static_assert
 static __always_inline void arch_static_assert(void)
 {
 	asm volatile ("1:\n\t"
include/linux/cgroup.h +1 -1
@@ -88,7 +88,7 @@ extern struct css_set init_css_set;
  * @ss: subsystem in question
  */
 #define cgroup_subsys_on_dfl(ss)						\
-	static_branch_likely(&ss ## _on_dfl_key)
+	__static_branch_likely(&ss ## _on_dfl_key)
 
 bool css_has_online_children(struct cgroup_subsys_state *css);
 struct cgroup_subsys_state *css_from_id(int id, struct cgroup_subsys *ss);
include/linux/jump_label.h +13 -2
@@ -323,6 +323,10 @@ extern bool ____wrong_branch_error(void)
 
 #ifdef HAVE_JUMP_LABEL
 
+#ifndef arch_static_assert
+#define arch_static_assert (void)
+#endif
+
 /*
  * Combine the right initial value (type) with the right branch order
  * to generate the desired result.
@@ -379,7 +383,7 @@ extern bool ____wrong_branch_error(void)
  * See jump_label_type() / jump_label_init_type().
  */
 
-#define static_branch_likely(x)							\
+#define __static_branch_likely(x)						\
 ({										\
 	bool branch;								\
 	if (__builtin_types_compatible_p(typeof(*x), struct static_key_true))	\
@@ -391,7 +395,9 @@ extern bool ____wrong_branch_error(void)
 	likely(branch);								\
 })
 
-#define static_branch_unlikely(x)						\
+#define static_branch_likely(x) (__static_branch_likely(x) && (arch_static_assert(), true))
+
+#define __static_branch_unlikely(x)						\
 ({										\
 	bool branch;								\
 	if (__builtin_types_compatible_p(typeof(*x), struct static_key_true))	\
@@ -403,11 +409,16 @@ extern bool ____wrong_branch_error(void)
 	unlikely(branch);							\
 })
 
+#define static_branch_unlikely(x) (__static_branch_unlikely(x) && (arch_static_assert(), true))
+
 #else /* !HAVE_JUMP_LABEL */
 
 #define static_branch_likely(x)		likely(static_key_enabled(&(x)->key))
 #define static_branch_unlikely(x)	unlikely(static_key_enabled(&(x)->key))
 
+#define __static_branch_likely(x)		likely(static_key_enabled(&(x)->key))
+#define __static_branch_unlikely(x)	unlikely(static_key_enabled(&(x)->key))
+
 #endif /* HAVE_JUMP_LABEL */
 
 /*
#69 lore raw
Quoted 1 line · Peter Zijlstra

drivers/watchdog/.tmp_hpwdt.o: warning: objtool: .text+0x24: indirect call found in RETPOLINE build

You're calling into BIOS there. Not a lot of point in protecting that one. This is one of the cases where we were saying you needed to turn on IBRS to have complete protection, even with retpoline.

author maintainer
#70 lore raw
Quoted 7 lines · David Woodhouse
Quoted 1 line · Peter Zijlstra

drivers/watchdog/.tmp_hpwdt.o: warning: objtool: .text+0x24: indirect call found in RETPOLINE build

You're calling into BIOS there. Not a lot of point in protecting that one. This is one of the cases where we were saying you needed to turn on IBRS to have complete protection, even with retpoline.

That code looked entirely dodgy, which is why I left it. I can easily add the ANNOTATE_RETPOLINE_SAFE thing to shut it up, but given it is an actual problem, maybe we should just keep it?