[PATCH 0/4] objtool validation of static branches

[PATCH 0/4] objtool validation of static branches

[Peter Zijlstra]

Because dwmw2 is a wee bit paranoid and GCC has a history of being slightly
retarded at times; here are a few patches that validate it will in fact
generate sensible code for static branches.

With this I hope to take away dwmw2's concerns for hard relying on asm-goto.

With that, we can stop sprinkling LFENCE all over the place with the IBRS/IBPB
patches.

[PATCH 1/4] objtool: Implement base jump_assert support

[Peter Zijlstra]

[-- Attachment #1: peter_zijlstra-q-objtool_vs_jump_label.patch --]
[-- Type: text/plain, Size: 3919 bytes --]

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 br_static=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 br_static 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: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++--
 tools/objtool/check.h |    1 
 2 files changed, 69 insertions(+), 2 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -687,8 +687,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->br_static = true;
+		(*new_insn)->br_static = true;
 		return 0;
+	}
 
 	if (orig_insn->type != INSN_JUMP_UNCONDITIONAL) {
 		WARN_FUNC("unsupported instruction at jump label",
@@ -696,7 +705,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->br_static = true;
+	next_insn->br_static = true;
+
+	*new_insn = next_insn;
+
 	return 0;
 }
 
@@ -1067,6 +1085,50 @@ static int read_unwind_hints(struct objt
 	return 0;
 }
 
+static int read_jump_assertions(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->br_static) {
+			WARN_FUNC("static assert FAIL", insn->sec, insn->offset);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 static int decode_sections(struct objtool_file *file)
 {
 	int ret;
@@ -1105,6 +1167,10 @@ static int decode_sections(struct objtoo
 	if (ret)
 		return ret;
 
+	ret = read_jump_assertions(file);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -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 br_static;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct list_head alts;

[PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Peter Zijlstra]

[-- Attachment #1: peterz-objtool-static_cpu_has.patch --]
[-- Type: text/plain, Size: 3398 bytes --]

Unlike the jump_label bits, static_cpu_has is implemented with
alternatives. Sadly it doesn't readily distinguish itself from any
other alternatives.

Use a heuristic to guess at it :/

But like jump_labels, make static_cpu_has set br_static on the
instructions after the static branch such that we can assert on it.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c   |   21 +++++++++++++++++++++
 tools/objtool/special.c |   27 ++++++++++++++++++++++++++-
 tools/objtool/special.h |    1 +
 3 files changed, 48 insertions(+), 1 deletion(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -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_feat)
+			fake_jump->jump_dest->br_static = 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_feat) {
+			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->br_static = true;
+		}
 	}
 
 	if (!last_new_insn) {
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -41,6 +41,7 @@
 #define ALT_ORIG_LEN_OFFSET	10
 #define ALT_NEW_LEN_OFFSET	11
 
+#define X86_FEATURE_ALWAYS (3*32+21)
 #define X86_FEATURE_POPCNT (4*32+23)
 
 struct special_entry {
@@ -110,6 +111,9 @@ static int get_alt_entry(struct elf *elf
 		 */
 		if (feature == X86_FEATURE_POPCNT)
 			alt->skip_orig = true;
+
+		if (feature == X86_FEATURE_ALWAYS)
+			alt->static_feat = true;
 	}
 
 	orig_rela = find_rela_by_dest(sec, offset + entry->orig);
@@ -155,7 +159,7 @@ int special_get_alts(struct elf *elf, st
 	struct special_entry *entry;
 	struct section *sec;
 	unsigned int nr_entries;
-	struct special_alt *alt;
+	struct special_alt *alt, *last = NULL;
 	int idx, ret;
 
 	INIT_LIST_HEAD(alts);
@@ -186,6 +190,27 @@ int special_get_alts(struct elf *elf, st
 				return ret;
 
 			list_add_tail(&alt->list, alts);
+
+			if (last) {
+				/*
+				 * If we have two consecutive alternatives for
+				 * the same location, for which the first has
+				 * FEATURE_ALWAYS set and the second has no
+				 * replacement, then assume this is
+				 * _static_cpu_has() and will have to set
+				 * br_static to make jump_assert work.
+				 */
+				if (alt->orig_sec == last->orig_sec &&
+				    alt->orig_off == last->orig_off &&
+				    alt->orig_len == last->orig_len &&
+				    !alt->new_len && !alt->new_sec && !alt->new_off &&
+				    last->static_feat)
+					alt->static_feat = true;
+				else
+					last->static_feat = false;
+			}
+
+			last = alt;
 		}
 	}
 
--- a/tools/objtool/special.h
+++ b/tools/objtool/special.h
@@ -27,6 +27,7 @@ struct special_alt {
 	bool group;
 	bool skip_orig;
 	bool jump_or_nop;
+	bool static_feat;
 
 	struct section *orig_sec;
 	unsigned long orig_off;

[PATCH 3/4] x86/jump_label: Implement arch_static_assert()

[Peter Zijlstra]

[-- Attachment #1: peterz-x86-asm-goto-assert.patch --]
[-- Type: text/plain, Size: 1088 bytes --]

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: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/jump_label.h |    9 +++++++++
 1 file changed, 9 insertions(+)

--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -62,6 +62,15 @@ static __always_inline bool arch_static_
 	return true;
 }
 
+static __always_inline void arch_static_assert(void)
+{
+	asm volatile ("1:\n\t"
+		      ".pushsection .discard.jump_assert, \"aw\" \n\t"
+		      _ASM_ALIGN  "\n\t"
+		      _ASM_PTR "1b \n\t"
+		      ".popsection \n\t");
+}
+
 #ifdef CONFIG_X86_64
 typedef u64 jump_label_t;
 #else

[PATCH 4/4] x86: Reindent _static_cpu_has

[Peter Zijlstra]

[-- Attachment #1: peterz-fix-static_cpu_has.patch --]
[-- Type: text/plain, Size: 3013 bytes --]

Because its daft..

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/cpufeature.h |   78 +++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 39 deletions(-)

--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -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)					\

Re: [PATCH 4/4] x86: Reindent _static_cpu_has

[Josh Poimboeuf]

On Mon, Jan 15, 2018 at 05:44:32PM +0100, Peter Zijlstra wrote:
> Because its daft..
> 
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/cpufeature.h |   78 +++++++++++++++++++-------------------
>  1 file changed, 39 insertions(+), 39 deletions(-)
> 
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -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;

While you're at it, might as well convert the feature bit constraints to
have names like %P[name] instead of %P0 and %P1?

-- 
Josh

Re: [PATCH 3/4] x86/jump_label: Implement arch_static_assert()

[Josh Poimboeuf]

On Mon, Jan 15, 2018 at 05:44:31PM +0100, Peter Zijlstra wrote:
> 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: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/jump_label.h |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> --- a/arch/x86/include/asm/jump_label.h
> +++ b/arch/x86/include/asm/jump_label.h
> @@ -62,6 +62,15 @@ static __always_inline bool arch_static_
>  	return true;
>  }
>  
> +static __always_inline void arch_static_assert(void)
> +{
> +	asm volatile ("1:\n\t"
> +		      ".pushsection .discard.jump_assert, \"aw\" \n\t"
> +		      _ASM_ALIGN  "\n\t"
> +		      _ASM_PTR "1b \n\t"
> +		      ".popsection \n\t");
> +}
> +

This needs a nice comment about what exactly it asserts.

And also, people without objtool enabled (i.e., no ORC or livepatch)
won't see the assertion.  Do we care about those people? :-)

-- 
Josh

Re: [PATCH 1/4] objtool: Implement base jump_assert support

[Josh Poimboeuf]

Big props to you for braving the bowels of the objtool code.

On Mon, Jan 15, 2018 at 05:44:29PM +0100, Peter Zijlstra wrote:
> +static int read_jump_assertions(struct objtool_file *file)

This does more than just _read_ the assertions.  Can you call it
something like assert_static_jumps() or do_static_jump_assertions() and
then call it from the main check() function?

> +{
> +	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->br_static) {
> +			WARN_FUNC("static assert FAIL", insn->sec, insn->offset);
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int decode_sections(struct objtool_file *file)
>  {
>  	int ret;
> @@ -1105,6 +1167,10 @@ static int decode_sections(struct objtoo
>  	if (ret)
>  		return ret;
>  
> +	ret = read_jump_assertions(file);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -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 br_static;

s/br_static/static_jump_dest/?

Also, fellow objtool expert, you forgot the patch to the MAINTAINERS
file ;-)

-- 
Josh

Re: [PATCH 3/4] x86/jump_label: Implement arch_static_assert()

[David Woodhouse]

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

On Mon, 2018-01-15 at 11:22 -0600, Josh Poimboeuf wrote:
> And also, people without objtool enabled (i.e., no ORC or livepatch)
> won't see the assertion.  Do we care about those people? :-)

I think that's OK. Peter is right that this *would* be a GCC
regression. Someone who *does* have objtool enabled would see it, and
we'd be aware of it. That's enough to assuage my paranoia.

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

Re: [PATCH 3/4] x86/jump_label: Implement arch_static_assert()

[Peter Zijlstra]

On Mon, Jan 15, 2018 at 11:22:12AM -0600, Josh Poimboeuf wrote:
> On Mon, Jan 15, 2018 at 05:44:31PM +0100, Peter Zijlstra wrote:


> > +static __always_inline void arch_static_assert(void)
> > +{
> > +	asm volatile ("1:\n\t"
> > +		      ".pushsection .discard.jump_assert, \"aw\" \n\t"
> > +		      _ASM_ALIGN  "\n\t"
> > +		      _ASM_PTR "1b \n\t"
> > +		      ".popsection \n\t");
> > +}
> > +
> 
> This needs a nice comment about what exactly it asserts.

Right, I'll go write one, if you don't see anything horrendous in the
objtool code ofcourse :-)

> And also, people without objtool enabled (i.e., no ORC or livepatch)
> won't see the assertion.  Do we care about those people? :-)

Nah, we should get plenty of build coverage with the others I think.

Re: [PATCH 1/4] objtool: Implement base jump_assert support

[Peter Zijlstra]

On Mon, Jan 15, 2018 at 11:39:28AM -0600, Josh Poimboeuf wrote:
> Big props to you for braving the bowels of the objtool code.
> 
> On Mon, Jan 15, 2018 at 05:44:29PM +0100, Peter Zijlstra wrote:
> > +static int read_jump_assertions(struct objtool_file *file)
> 
> This does more than just _read_ the assertions.  Can you call it
> something like assert_static_jumps() or do_static_jump_assertions() and
> then call it from the main check() function?

Sure.

> > --- a/tools/objtool/check.h
> > +++ b/tools/objtool/check.h
> > @@ -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 br_static;
> 
> s/br_static/static_jump_dest/?

fair enough.

> Also, fellow objtool expert, you forgot the patch to the MAINTAINERS
> file ;-)

Well, uhm.. I'll think about it ;-)

Re: [PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Josh Poimboeuf]

On Mon, Jan 15, 2018 at 05:44:30PM +0100, Peter Zijlstra wrote:
> Unlike the jump_label bits, static_cpu_has is implemented with
> alternatives. Sadly it doesn't readily distinguish itself from any
> other alternatives.
> 
> Use a heuristic to guess at it :/
> 
> But like jump_labels, make static_cpu_has set br_static on the
> instructions after the static branch such that we can assert on it.

This seems a bit heavy handed and fragile, though maybe it is the best
way.  Still I wonder if there's a better way to do it.

Some quick ideas:

a) Somehow use __jump_table in the _static_cpu_has() macro?

b) Add another special annotation to tell objtool where
_static_cpu_has() locations are?

May need to ruminate on this one a bit...

-- 
Josh

Re: [PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Peter Zijlstra]

On Mon, Jan 15, 2018 at 12:04:05PM -0600, Josh Poimboeuf wrote:
> On Mon, Jan 15, 2018 at 05:44:30PM +0100, Peter Zijlstra wrote:
> > Unlike the jump_label bits, static_cpu_has is implemented with
> > alternatives. Sadly it doesn't readily distinguish itself from any
> > other alternatives.
> > 
> > Use a heuristic to guess at it :/
> > 
> > But like jump_labels, make static_cpu_has set br_static on the
> > instructions after the static branch such that we can assert on it.
> 
> This seems a bit heavy handed and fragile, though maybe it is the best
> way.  Still I wonder if there's a better way to do it.
> 
> Some quick ideas:
> 
> a) Somehow use __jump_table in the _static_cpu_has() macro?

Can do, but adds permanent overhead for the fake table entries, also the
alternative in _static_cpu_has is slightly more complex, but it would
work I think.

> b) Add another special annotation to tell objtool where
> _static_cpu_has() locations are?

Almost did that, but I figured I'd give this a try first. But yes I
agree it is somewhat ugly.

Re: [PATCH 3/4] x86/jump_label: Implement arch_static_assert()

[Josh Poimboeuf]

On Mon, Jan 15, 2018 at 05:44:31PM +0100, Peter Zijlstra wrote:
> 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: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/jump_label.h |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> --- a/arch/x86/include/asm/jump_label.h
> +++ b/arch/x86/include/asm/jump_label.h
> @@ -62,6 +62,15 @@ static __always_inline bool arch_static_
>  	return true;
>  }
>  
> +static __always_inline void arch_static_assert(void)
> +{
> +	asm volatile ("1:\n\t"
> +		      ".pushsection .discard.jump_assert, \"aw\" \n\t"

The "aw" flags aren't needed, the section is neither allocatable nor
writable.

-- 
Josh

Re: [PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Josh Poimboeuf]

On Mon, Jan 15, 2018 at 07:12:30PM +0100, Peter Zijlstra wrote:
> > b) Add another special annotation to tell objtool where
> > _static_cpu_has() locations are?
> 
> Almost did that, but I figured I'd give this a try first. But yes I
> agree it is somewhat ugly.

I'm thinking this is the way to go.  Create an ANNOTATE_STATIC_JUMP
macro, which is used in both _static_cpu_has() and
arch_static_branch[_jump]().  Then it's very generalized and all the
special-casing for both alternatives and jump tables will go away.

-- 
Josh

Re: [PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Borislav Petkov]

Right,

I've been putting away extending struct alt_instr for a long time now,
trying to be conservative about it but I guess this might be the right
time to change that. How about:

struct alt_instr {
        s32 instr_offset;       /* original instruction */
        s32 repl_offset;        /* offset to replacement instruction */
        u16 cpuid;              /* cpuid bit set for replacement */
        u8  instrlen;           /* length of original instruction */
        u8  replacementlen;     /* length of new instruction */
        u8  padlen;             /* length of build-time padding */
	u64 flags;		/* alternative flags, see <some enum> */
} __packed;

This way we have 64 settings. So we could do:

...
	.flags 	= ALT_FLAGS_STATIC_CPU_HAS,

or something like that and then we can do additional processing/matching
for the alternatives.

Or, we can do

struct alt_instr {
        s32 instr_offset;       /* original instruction */
        s32 repl_offset;        /* offset to replacement instruction */
        u16 cpuid;              /* cpuid bit set for replacement */
        u8  instrlen;           /* length of original instruction */
        u8  replacementlen;     /* length of new instruction */
        u8  padlen;             /* length of build-time padding */
	u8 type;		/* types */
} __packed;

and have 256 types but that would be limiting as we won't be able to set
more than one.

Hmmm?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Josh Poimboeuf]

On Mon, Jan 15, 2018 at 07:59:37PM +0100, Borislav Petkov wrote:
> Right,
> 
> I've been putting away extending struct alt_instr for a long time now,
> trying to be conservative about it but I guess this might be the right
> time to change that. How about:
> 
> struct alt_instr {
>         s32 instr_offset;       /* original instruction */
>         s32 repl_offset;        /* offset to replacement instruction */
>         u16 cpuid;              /* cpuid bit set for replacement */
>         u8  instrlen;           /* length of original instruction */
>         u8  replacementlen;     /* length of new instruction */
>         u8  padlen;             /* length of build-time padding */
> 	u64 flags;		/* alternative flags, see <some enum> */
> } __packed;
> 
> This way we have 64 settings. So we could do:
> 
> ...
> 	.flags 	= ALT_FLAGS_STATIC_CPU_HAS,
> 
> or something like that and then we can do additional processing/matching
> for the alternatives.
> 
> Or, we can do
> 
> struct alt_instr {
>         s32 instr_offset;       /* original instruction */
>         s32 repl_offset;        /* offset to replacement instruction */
>         u16 cpuid;              /* cpuid bit set for replacement */
>         u8  instrlen;           /* length of original instruction */
>         u8  replacementlen;     /* length of new instruction */
>         u8  padlen;             /* length of build-time padding */
> 	u8 type;		/* types */
> } __packed;
> 
> and have 256 types but that would be limiting as we won't be able to set
> more than one.
> 
> Hmmm?

That might be a good idea, but here we also need to annotate jump
labels.  So unless you want to make alternatives broad enough to
encompass jump labels, I don't think it solves this particular problem.

-- 
Josh

Re: [PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Josh Poimboeuf]

On Mon, Jan 15, 2018 at 01:08:19PM -0600, Josh Poimboeuf wrote:
> On Mon, Jan 15, 2018 at 07:59:37PM +0100, Borislav Petkov wrote:
> > Right,
> > 
> > I've been putting away extending struct alt_instr for a long time now,
> > trying to be conservative about it but I guess this might be the right
> > time to change that. How about:
> > 
> > struct alt_instr {
> >         s32 instr_offset;       /* original instruction */
> >         s32 repl_offset;        /* offset to replacement instruction */
> >         u16 cpuid;              /* cpuid bit set for replacement */
> >         u8  instrlen;           /* length of original instruction */
> >         u8  replacementlen;     /* length of new instruction */
> >         u8  padlen;             /* length of build-time padding */
> > 	u64 flags;		/* alternative flags, see <some enum> */
> > } __packed;
> > 
> > This way we have 64 settings. So we could do:
> > 
> > ...
> > 	.flags 	= ALT_FLAGS_STATIC_CPU_HAS,
> > 
> > or something like that and then we can do additional processing/matching
> > for the alternatives.
> > 
> > Or, we can do
> > 
> > struct alt_instr {
> >         s32 instr_offset;       /* original instruction */
> >         s32 repl_offset;        /* offset to replacement instruction */
> >         u16 cpuid;              /* cpuid bit set for replacement */
> >         u8  instrlen;           /* length of original instruction */
> >         u8  replacementlen;     /* length of new instruction */
> >         u8  padlen;             /* length of build-time padding */
> > 	u8 type;		/* types */
> > } __packed;
> > 
> > and have 256 types but that would be limiting as we won't be able to set
> > more than one.
> > 
> > Hmmm?
> 
> That might be a good idea, but here we also need to annotate jump
> labels.  So unless you want to make alternatives broad enough to
> encompass jump labels, I don't think it solves this particular problem.

Well, to clarify, it would solve _some_ of the problem.  Maybe even most
of the problem.  We'd still need to special-case jump labels in objtool
(like in 1/4), but that's probably not a big deal.

So, contradicting my previous answer here... yes, it would help.

-- 
Josh

Re: [PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Borislav Petkov]

On Mon, Jan 15, 2018 at 01:11:51PM -0600, Josh Poimboeuf wrote:
> Well, to clarify, it would solve _some_ of the problem.  Maybe even most
> of the problem.  We'd still need to special-case jump labels in objtool
> (like in 1/4), but that's probably not a big deal.
> 
> So, contradicting my previous answer here... yes, it would help.

Yeah, so PeterZ was asking on IRC today how to detect static_cpu_has()
and even if we do some magic matching, nothing beats an explicit flag.

And I'm proposing this because the flags thing has come up in the past
already. So it's not like it won't find more users. We can do all kinds
of fancy alternatives processing when we tag the call sites properly.
:-)

Now lemme look at the rest of the patchset.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Peter Zijlstra]

On Mon, Jan 15, 2018 at 01:11:51PM -0600, Josh Poimboeuf wrote:
> Well, to clarify, it would solve _some_ of the problem.  Maybe even most
> of the problem.  We'd still need to special-case jump labels in objtool
> (like in 1/4), but that's probably not a big deal.
> 
> So, contradicting my previous answer here... yes, it would help.

OK, saves me from having to argue otherwise. My argument would've been
that jump labels are fully described and don't need further annotation.

Yes, Boris' proposal would certainly help with the alternative stuff.

Re: [PATCH 2/4] objtool: Implement jump_assert for _static_cpu_has()

[Josh Poimboeuf]

On Mon, Jan 15, 2018 at 09:15:45PM +0100, Peter Zijlstra wrote:
> On Mon, Jan 15, 2018 at 01:11:51PM -0600, Josh Poimboeuf wrote:
> > Well, to clarify, it would solve _some_ of the problem.  Maybe even most
> > of the problem.  We'd still need to special-case jump labels in objtool
> > (like in 1/4), but that's probably not a big deal.
> > 
> > So, contradicting my previous answer here... yes, it would help.
> 
> OK, saves me from having to argue otherwise. My argument would've been
> that jump labels are fully described and don't need further annotation.
> 
> Yes, Boris' proposal would certainly help with the alternative stuff.

Fair enough.  So either annotating _static_cpu_has() or adding an
alt_instr flag works for me.

-- 
Josh