[RFC][PATCH 0/7] objtool vs retpoline

[RFC][PATCH 0/7] objtool vs retpoline

[Peter Zijlstra]

Hi!

Based on Alexandre's patches, here's a few patches that apply on top of my
other series:

  https://lkml.kernel.org/r/20200416114706.625340212@infradead.org

which seems to build a clean defconfig without ANNOTATE_NOSPEC_ALTERNATIVE on.

I've not tried to boot the thing, I'll leave that to the robots.

[RFC][PATCH 1/7] objtool: is_fentry_call() crashes if call has no destination

[Peter Zijlstra]

From: Alexandre Chartre <alexandre.chartre@oracle.com>

Fix is_fentry_call() so that it works if a call has no destination
set (call_dest). This needs to be done in order to support intra-
function calls.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200414103618.12657-2-alexandre.chartre@oracle.com
---
 tools/objtool/check.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1490,7 +1490,7 @@ static int decode_sections(struct objtoo
 
 static bool is_fentry_call(struct instruction *insn)
 {
-	if (insn->type == INSN_CALL &&
+	if (insn->type == INSN_CALL && insn->call_dest &&
 	    insn->call_dest->type == STT_NOTYPE &&
 	    !strcmp(insn->call_dest->name, "__fentry__"))
 		return true;

[RFC][PATCH 2/7] objtool: UNWIND_HINT_RET_OFFSET should not check registers

[Peter Zijlstra]

From: Alexandre Chartre <alexandre.chartre@oracle.com>

UNWIND_HINT_RET_OFFSET will adjust a modified stack. However if a
callee-saved register was pushed on the stack then the stack frame
will still appear modified. So stop checking registers when
UNWIND_HINT_RET_OFFSET is used.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200407073142.20659-3-alexandre.chartre@oracle.com
---
 tools/objtool/check.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1513,6 +1513,14 @@ static bool has_modified_stack_frame(str
 	if (cfi->stack_size != initial_func_cfi.cfa.offset + ret_offset)
 		return true;
 
+	/*
+	 * If there is a ret offset hint then don't check registers
+	 * because a callee-saved register might have been pushed on
+	 * the stack.
+	 */
+	if (ret_offset)
+		return false;
+
 	for (i = 0; i < CFI_NUM_REGS; i++) {
 		if (cfi->regs[i].base != initial_func_cfi.regs[i].base ||
 		    cfi->regs[i].offset != initial_func_cfi.regs[i].offset)

[RFC][PATCH 3/7] objtool: Allow branches within the same alternative.

[Peter Zijlstra]

From: Alexandre Chartre <alexandre.chartre@oracle.com>

Currently objtool prevents any branch to an alternative. While preventing
branching from the outside to the middle of an alternative makes perfect
sense, branching within the same alternative should be allowed. To do so,
identify each alternative and check that a branch to an alternative comes
from the same alternative.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200414103618.12657-3-alexandre.chartre@oracle.com
---
 tools/objtool/check.c |   27 +++++++++++++++++++++------
 tools/objtool/check.h |    3 ++-
 2 files changed, 23 insertions(+), 7 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -768,7 +768,9 @@ static int handle_group_alt(struct objto
 			    struct instruction *orig_insn,
 			    struct instruction **new_insn)
 {
+	static unsigned int alt_group_next_index = 1;
 	struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL;
+	unsigned int alt_group = alt_group_next_index++;
 	unsigned long dest_off;
 
 	last_orig_insn = NULL;
@@ -777,7 +779,7 @@ static int handle_group_alt(struct objto
 		if (insn->offset >= special_alt->orig_off + special_alt->orig_len)
 			break;
 
-		insn->alt_group = true;
+		insn->alt_group = alt_group;
 		last_orig_insn = insn;
 	}
 
@@ -811,6 +813,7 @@ static int handle_group_alt(struct objto
 	}
 
 	last_new_insn = NULL;
+	alt_group = alt_group_next_index++;
 	insn = *new_insn;
 	sec_for_each_insn_from(file, insn) {
 		if (insn->offset >= special_alt->new_off + special_alt->new_len)
@@ -820,6 +823,7 @@ static int handle_group_alt(struct objto
 
 		insn->ignore = orig_insn->ignore_alts;
 		insn->func = orig_insn->func;
+		insn->alt_group = alt_group;
 
 		/*
 		 * Since alternative replacement code is copy/pasted by the
@@ -2157,6 +2161,15 @@ static int validate_return(struct symbol
 	return 0;
 }
 
+static bool is_branch_to_alternative(struct instruction *from,
+				     struct instruction *to)
+{
+	if (!from || !to->alt_group || !list_empty(&to->alts))
+		return false;
+
+	return (from->alt_group != to->alt_group);
+}
+
 /*
  * Follow the branch starting at the given instruction, and recursively follow
  * any other branches (jumps).  Meanwhile, track the frame pointer state at
@@ -2164,6 +2177,7 @@ static int validate_return(struct symbol
  * tools/objtool/Documentation/stack-validation.txt.
  */
 static int validate_branch(struct objtool_file *file, struct symbol *func,
+			   struct instruction *from,
 			   struct instruction *insn, struct insn_state state)
 {
 	struct alternative *alt;
@@ -2174,7 +2188,7 @@ static int validate_branch(struct objtoo
 
 	sec = insn->sec;
 
-	if (insn->alt_group && list_empty(&insn->alts)) {
+	if (is_branch_to_alternative(from, insn)) {
 		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
 			  sec, insn->offset);
 		return 1;
@@ -2221,7 +2235,8 @@ static int validate_branch(struct objtoo
 				if (alt->skip_orig)
 					skip_orig = true;
 
-				ret = validate_branch(file, func, alt->insn, state);
+				ret = validate_branch(file, func,
+						      NULL, alt->insn, state);
 				if (ret) {
 					if (backtrace)
 						BT_FUNC("(alt)", insn);
@@ -2264,7 +2279,7 @@ static int validate_branch(struct objtoo
 					return ret;
 
 			} else if (insn->jump_dest) {
-				ret = validate_branch(file, func,
+				ret = validate_branch(file, func, insn,
 						      insn->jump_dest, state);
 				if (ret) {
 					if (backtrace)
@@ -2396,7 +2411,7 @@ static int validate_unwind_hints(struct
 
 	while (&insn->list != &file->insn_list && (!sec || insn->sec == sec)) {
 		if (insn->hint && !insn->visited) {
-			ret = validate_branch(file, insn->func, insn, state);
+			ret = validate_branch(file, insn->func, NULL, insn, state);
 			if (ret && backtrace)
 				BT_FUNC("<=== (hint)", insn);
 			warnings += ret;
@@ -2537,7 +2552,7 @@ static int validate_symbol(struct objtoo
 
 	state->uaccess = sym->uaccess_safe;
 
-	ret = validate_branch(file, insn->func, insn, *state);
+	ret = validate_branch(file, insn->func, NULL, insn, *state);
 	if (ret && backtrace)
 		BT_FUNC("<=== (sym)", insn);
 	return ret;
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -30,7 +30,8 @@ struct instruction {
 	unsigned int len;
 	enum insn_type type;
 	unsigned long immediate;
-	bool alt_group, dead_end, ignore, ignore_alts;
+	int alt_group;
+	bool dead_end, ignore, ignore_alts;
 	bool hint;
 	bool retpoline_safe;
 	s8 instr;

Re: [RFC][PATCH 3/7] objtool: Allow branches within the same alternative.

[Josh Poimboeuf]

On Thu, Apr 16, 2020 at 05:07:55PM +0200, Peter Zijlstra wrote:
> From: Alexandre Chartre <alexandre.chartre@oracle.com>
> 
> Currently objtool prevents any branch to an alternative. While preventing
> branching from the outside to the middle of an alternative makes perfect
> sense, branching within the same alternative should be allowed. To do so,
> identify each alternative and check that a branch to an alternative comes
> from the same alternative.
> 
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lkml.kernel.org/r/20200414103618.12657-3-alexandre.chartre@oracle.com

I I think I'd rather just drop this patch.  It adds some complexity and
I don't really see a benefit to having the warning in the first place.

Branching to the middle of an alternative group just doesn't seem like a
realistic scenario (can it even be done with the alternative macros?).

Even if it were possible, I'm not sure why somebody would attempt such a
feat.  And it seems like it would just wreck the system anyway.

-- 
Josh

Re: [RFC][PATCH 3/7] objtool: Allow branches within the same alternative.

[Peter Zijlstra]

On Sun, Apr 19, 2020 at 11:32:19AM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 16, 2020 at 05:07:55PM +0200, Peter Zijlstra wrote:
> > From: Alexandre Chartre <alexandre.chartre@oracle.com>
> > 
> > Currently objtool prevents any branch to an alternative. While preventing
> > branching from the outside to the middle of an alternative makes perfect
> > sense, branching within the same alternative should be allowed. To do so,
> > identify each alternative and check that a branch to an alternative comes
> > from the same alternative.
> > 
> > Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Link: https://lkml.kernel.org/r/20200414103618.12657-3-alexandre.chartre@oracle.com
> 
> I I think I'd rather just drop this patch.  It adds some complexity and
> I don't really see a benefit to having the warning in the first place.

Also; we totally don't need it, sorry for not realizing that earlier.
Consider it gone.

[RFC][PATCH 4/7] objtool: Add support for intra-function calls

[Peter Zijlstra]

From: Alexandre Chartre <alexandre.chartre@oracle.com>

Change objtool to support intra-function calls. On x86, an intra-function
call is represented in objtool as a push onto the stack (of the return
address), and a jump to the destination address. That way the stack
information is correctly updated and the call flow is still accurate.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200414103618.12657-4-alexandre.chartre@oracle.com
---
 include/linux/frame.h                            |   11 +
 tools/objtool/Documentation/stack-validation.txt |    8 +
 tools/objtool/arch/x86/decode.c                  |   17 ++-
 tools/objtool/check.c                            |  129 ++++++++++++++++++-----
 tools/objtool/check.h                            |    1 
 5 files changed, 140 insertions(+), 26 deletions(-)

--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -15,9 +15,20 @@
 	static void __used __section(.discard.func_stack_frame_non_standard) \
 		*__func_stack_frame_non_standard_##func = func
 
+/*
+ * This macro indicates that the following intra-function call is valid.
+ * Any non-annotated intra-function call will cause objtool to issue warning.
+ */
+#define ANNOTATE_INTRA_FUNCTION_CALL				\
+	999:							\
+	.pushsection .discard.intra_function_calls;		\
+	.long 999b;						\
+	.popsection;
+
 #else /* !CONFIG_STACK_VALIDATION */
 
 #define STACK_FRAME_NON_STANDARD(func)
+#define ANNOTATE_INTRA_FUNCTION_CALL
 
 #endif /* CONFIG_STACK_VALIDATION */
 
--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/stack-validation.txt
@@ -290,6 +290,14 @@ they mean, and suggestions for how to fi
       https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
 
 
+9. file.o: warning: unsupported intra-function call
+
+   This warning means that a direct call is done to a destination which
+   is not at the beginning of a function. If this is a legit call, you
+   can remove this warning by putting the ANNOTATE_INTRA_FUNCTION_CALL
+   directive right before the call.
+
+
 If the error doesn't seem to make sense, it could be a bug in objtool.
 Feel free to ask the objtool maintainer for help.
 
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -453,6 +453,12 @@ int arch_decode_instruction(struct elf *
 
 	case 0xe8:
 		*type = INSN_CALL;
+		/*
+		 * For the impact on the stack, a call behaves like
+		 * a push of an immediate value (the return address).
+		 */
+		op->src.type = OP_SRC_CONST;
+		op->dest.type = OP_DEST_PUSH;
 		break;
 
 	case 0xfc:
@@ -493,10 +499,17 @@ int arch_decode_instruction(struct elf *
 
 	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
 
-	if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
+	switch (*type) {
+	case INSN_STACK:
+	case INSN_EXCEPTION_RETURN:
+	case INSN_CALL:
 		list_add_tail(&op->list, ops_list);
-	else
+		break;
+
+	default:
 		free(op);
+		break;
+	}
 
 	return 0;
 }
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -691,44 +691,51 @@ static int add_jump_destinations(struct
 	return 0;
 }
 
+static int setup_call_dest(struct objtool_file *file, struct instruction *insn)
+{
+	unsigned long dest_off;
+
+	dest_off = insn->offset + insn->len + insn->immediate;
+	insn->call_dest = find_func_by_offset(insn->sec, dest_off);
+	if (!insn->call_dest)
+		insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
+
+	if (!insn->call_dest) {
+		/* intra-function call */
+		if (insn->intra_function_call)
+			return 0;
+
+		WARN_FUNC("intra-function call", insn->sec, insn->offset);
+		return -1;
+	}
+
+	/* regular call */
+	if (insn->func && insn->call_dest->type != STT_FUNC) {
+		WARN_FUNC("unsupported call to non-function",
+			  insn->sec, insn->offset);
+		return -1;
+	}
+
+	return 0;
+}
+
 /*
  * Find the destination instructions for all calls.
  */
 static int add_call_destinations(struct objtool_file *file)
 {
 	struct instruction *insn;
-	unsigned long dest_off;
 	struct rela *rela;
 
 	for_each_insn(file, insn) {
-		if (insn->type != INSN_CALL)
+		if (insn->type != INSN_CALL || insn->ignore)
 			continue;
 
 		rela = find_rela_by_dest_range(file->elf, insn->sec,
 					       insn->offset, insn->len);
 		if (!rela) {
-			dest_off = insn->offset + insn->len + insn->immediate;
-			insn->call_dest = find_func_by_offset(insn->sec, dest_off);
-			if (!insn->call_dest)
-				insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
-
-			if (insn->ignore)
-				continue;
-
-			if (!insn->call_dest) {
-				WARN_FUNC("unsupported intra-function call",
-					  insn->sec, insn->offset);
-				if (retpoline)
-					WARN("If this is a retpoline, please patch it in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE.");
-				return -1;
-			}
-
-			if (insn->func && insn->call_dest->type != STT_FUNC) {
-				WARN_FUNC("unsupported call to non-function",
-					  insn->sec, insn->offset);
+			if (setup_call_dest(file, insn))
 				return -1;
-			}
-
 		} else if (rela->sym->type == STT_SECTION) {
 			insn->call_dest = find_func_by_offset(rela->sym->sec,
 							      rela->addend+4);
@@ -1414,6 +1421,52 @@ static int read_instr_hints(struct objto
 	return 0;
 }
 
+static int read_intra_function_calls(struct objtool_file *file)
+{
+	struct instruction *insn;
+	struct section *sec;
+	struct rela *rela;
+
+	sec = find_section_by_name(file->elf, ".rela.discard.intra_function_calls");
+	if (!sec)
+		return 0;
+
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		unsigned long dest_off;
+
+		if (rela->sym->type != STT_SECTION) {
+			WARN("unexpected relocation symbol type in %s",
+			     sec->name);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("bad .discard.intra_function_call entry");
+			return -1;
+		}
+
+		if (insn->type != INSN_CALL) {
+			WARN_FUNC("intra_function_call not a direct call",
+				  insn->sec, insn->offset);
+			return -1;
+		}
+
+		insn->intra_function_call = true;
+
+		dest_off = insn->offset + insn->len + insn->immediate;
+		insn->jump_dest = find_insn(file, insn->sec, dest_off);
+		if (!insn->jump_dest) {
+			WARN_FUNC("can't find call dest at %s+0x%lx",
+				  insn->sec, insn->offset,
+				  insn->sec->name, dest_off);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 static void mark_rodata(struct objtool_file *file)
 {
 	struct section *sec;
@@ -1469,6 +1522,10 @@ static int decode_sections(struct objtoo
 	if (ret)
 		return ret;
 
+	ret = read_intra_function_calls(file);
+	if (ret)
+		return ret;
+
 	ret = add_call_destinations(file);
 	if (ret)
 		return ret;
@@ -2260,7 +2317,8 @@ static int validate_branch(struct objtoo
 				return ret;
 
 			if (!no_fp && func && !is_fentry_call(insn) &&
-			    !has_valid_stack_frame(&state)) {
+			    !has_valid_stack_frame(&state) &&
+			    !insn->intra_function_call) {
 				WARN_FUNC("call without frame pointer save/setup",
 					  sec, insn->offset);
 				return 1;
@@ -2269,6 +2327,29 @@ static int validate_branch(struct objtoo
 			if (dead_end_function(file, insn->call_dest))
 				return 0;
 
+			if (insn->intra_function_call) {
+				/*
+				 * The call instruction can update the stack
+				 * state. Then make the intra-function call
+				 * behaves like and unconditional jump.
+				 */
+				ret = handle_insn_ops(insn, &state);
+				if (ret)
+					return ret;
+
+				ret = validate_branch(file, func, insn,
+						      insn->jump_dest, state);
+				if (ret) {
+					if (backtrace) {
+						BT_FUNC("(intra-function call)",
+							insn);
+					}
+					return ret;
+				}
+
+				return 0;
+			}
+
 			break;
 
 		case INSN_JUMP_CONDITIONAL:
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,6 +33,7 @@ struct instruction {
 	int alt_group;
 	bool dead_end, ignore, ignore_alts;
 	bool hint;
+	bool intra_function_call;
 	bool retpoline_safe;
 	s8 instr;
 	u8 visited;

Re: [RFC][PATCH 4/7] objtool: Add support for intra-function calls

[Josh Poimboeuf]

On Thu, Apr 16, 2020 at 05:07:56PM +0200, Peter Zijlstra wrote:
> From: Alexandre Chartre <alexandre.chartre@oracle.com>
> 
> Change objtool to support intra-function calls. On x86, an intra-function
> call is represented in objtool as a push onto the stack (of the return
> address), and a jump to the destination address. That way the stack
> information is correctly updated and the call flow is still accurate.
> 
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lkml.kernel.org/r/20200414103618.12657-4-alexandre.chartre@oracle.com
> ---
>  include/linux/frame.h                            |   11 +
>  tools/objtool/Documentation/stack-validation.txt |    8 +
>  tools/objtool/arch/x86/decode.c                  |   17 ++-
>  tools/objtool/check.c                            |  129 ++++++++++++++++++-----
>  tools/objtool/check.h                            |    1 
>  5 files changed, 140 insertions(+), 26 deletions(-)
> 
> --- a/include/linux/frame.h
> +++ b/include/linux/frame.h
> @@ -15,9 +15,20 @@
>  	static void __used __section(.discard.func_stack_frame_non_standard) \
>  		*__func_stack_frame_non_standard_##func = func
>  
> +/*
> + * This macro indicates that the following intra-function call is valid.
> + * Any non-annotated intra-function call will cause objtool to issue warning.

issue *a* warning

> +static int setup_call_dest(struct objtool_file *file, struct instruction *insn)
> +{
> +	unsigned long dest_off;
> +
> +	dest_off = insn->offset + insn->len + insn->immediate;
> +	insn->call_dest = find_func_by_offset(insn->sec, dest_off);
> +	if (!insn->call_dest)
> +		insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
> +
> +	if (!insn->call_dest) {
> +		/* intra-function call */
> +		if (insn->intra_function_call)
> +			return 0;
> +
> +		WARN_FUNC("intra-function call", insn->sec, insn->offset);
> +		return -1;
> +	}
> +
> +	/* regular call */
> +	if (insn->func && insn->call_dest->type != STT_FUNC) {
> +		WARN_FUNC("unsupported call to non-function",
> +			  insn->sec, insn->offset);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +

This function should first be added in a separate patch which doesn't
change functionality.

> @@ -2269,6 +2327,29 @@ static int validate_branch(struct objtoo
>  			if (dead_end_function(file, insn->call_dest))
>  				return 0;
>  
> +			if (insn->intra_function_call) {
> +				/*
> +				 * The call instruction can update the stack
> +				 * state. Then make the intra-function call
> +				 * behaves like and unconditional jump.

grammar fix: "behave like an unconditional jump".

> +				 */
> +				ret = handle_insn_ops(insn, &state);
> +				if (ret)
> +					return ret;

validate_branch's callers aren't currently able to handle a negative
return code.

> +
> +				ret = validate_branch(file, func, insn,
> +						      insn->jump_dest, state);
> +				if (ret) {
> +					if (backtrace) {
> +						BT_FUNC("(intra-function call)",
> +							insn);
> +					}
> +					return ret;
> +				}
> +
> +				return 0;
> +			}
> +

Could this be cleaner if the insn->type were just changed to
INSN_JUMP_UNCONDITIONAL?  Then it could share the normal jump logic.

Also, now that more instructions are getting stack ops, I wonder if the
ops can just be handled generically for every instruction, with a call
to handle_insn_ops() above, before the switch statement.

-- 
Josh

Re: [RFC][PATCH 4/7] objtool: Add support for intra-function calls

[Peter Zijlstra]

On Sun, Apr 19, 2020 at 11:41:50AM -0500, Josh Poimboeuf wrote:

> Could this be cleaner if the insn->type were just changed to
> INSN_JUMP_UNCONDITIONAL?  Then it could share the normal jump logic.

A little something like so, then?

---
Subject: objtool: Add support for intra-function calls
From: Alexandre Chartre <alexandre.chartre@oracle.com>
Date: Tue, 14 Apr 2020 12:36:12 +0200

From: Alexandre Chartre <alexandre.chartre@oracle.com>

Change objtool to support intra-function calls. On x86, an intra-function
call is represented in objtool as a push onto the stack (of the return
address), and a jump to the destination address. That way the stack
information is correctly updated and the call flow is still accurate.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200414103618.12657-4-alexandre.chartre@oracle.com
---
 include/linux/frame.h                            |   11 +++
 tools/objtool/Documentation/stack-validation.txt |    8 ++
 tools/objtool/arch/x86/decode.c                  |    8 ++
 tools/objtool/check.c                            |   83 +++++++++++++++++++++--
 4 files changed, 103 insertions(+), 7 deletions(-)

--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -15,9 +15,20 @@
 	static void __used __section(.discard.func_stack_frame_non_standard) \
 		*__func_stack_frame_non_standard_##func = func
 
+/*
+ * This macro indicates that the following intra-function call is valid.
+ * Any non-annotated intra-function call will cause objtool to issue a warning.
+ */
+#define ANNOTATE_INTRA_FUNCTION_CALL				\
+	999:							\
+	.pushsection .discard.intra_function_calls;		\
+	.long 999b;						\
+	.popsection;
+
 #else /* !CONFIG_STACK_VALIDATION */
 
 #define STACK_FRAME_NON_STANDARD(func)
+#define ANNOTATE_INTRA_FUNCTION_CALL
 
 #endif /* CONFIG_STACK_VALIDATION */
 
--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/stack-validation.txt
@@ -316,6 +316,14 @@ they mean, and suggestions for how to fi
       sources).
 
 
+10. file.o: warning: unsupported intra-function call
+
+   This warning means that a direct call is done to a destination which
+   is not at the beginning of a function. If this is a legit call, you
+   can remove this warning by putting the ANNOTATE_INTRA_FUNCTION_CALL
+   directive right before the call.
+
+
 If the error doesn't seem to make sense, it could be a bug in objtool.
 Feel free to ask the objtool maintainer for help.
 
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -516,6 +516,14 @@ int arch_decode_instruction(struct elf *
 
 	case 0xe8:
 		*type = INSN_CALL;
+		/*
+		 * For the impact on the stack, a CALL behaves like
+		 * a PUSH of an immediate value (the return address).
+		 */
+		ADD_OP(op) {
+			op->src.type = OP_SRC_CONST;
+			op->dest.type = OP_DEST_PUSH;
+		}
 		break;
 
 	case 0xfc:
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -690,6 +690,16 @@ static int add_jump_destinations(struct
 	return 0;
 }
 
+static void remove_insn_ops(struct instruction *insn)
+{
+	struct stack_op *op, *tmp;
+
+	list_for_each_entry_safe(op, tmp, &insn->stack_ops, list) {
+		list_del(&op->list);
+		free(op);
+	}
+}
+
 static bool setup_call_dest(struct objtool_file *file, struct instruction *insn)
 {
 	unsigned long dest_off;
@@ -700,19 +710,23 @@ static bool setup_call_dest(struct objto
 		insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
 
 	if (!insn->call_dest) {
-		WARN_FUNC("unsupported intra-function call",
-			  insn->sec, insn->offset);
-		if (retpoline)
-			WARN("If this is a retpoline, please patch it in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE.");
+		WARN_FUNC("intra-function call", insn->sec, insn->offset);
 		return false;
 	}
 
+	/* regular call */
 	if (insn->func && insn->call_dest->type != STT_FUNC) {
 		WARN_FUNC("unsupported call to non-function",
 			  insn->sec, insn->offset);
 		return false;
 	}
 
+	/*
+	 * Whatever stack impact regular CALLs have, should be
+	 * undone by the RETURN of the called function.
+	 */
+	remove_insn_ops(insn);
+
 	return true;
 }
 
@@ -1423,6 +1437,57 @@ static int read_instr_hints(struct objto
 	return 0;
 }
 
+static int read_intra_function_calls(struct objtool_file *file)
+{
+	struct instruction *insn;
+	struct section *sec;
+	struct rela *rela;
+
+	sec = find_section_by_name(file->elf, ".rela.discard.intra_function_calls");
+	if (!sec)
+		return 0;
+
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		unsigned long dest_off;
+
+		if (rela->sym->type != STT_SECTION) {
+			WARN("unexpected relocation symbol type in %s",
+			     sec->name);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("bad .discard.intra_function_call entry");
+			return -1;
+		}
+
+		if (insn->type != INSN_CALL) {
+			WARN_FUNC("intra_function_call not a direct call",
+				  insn->sec, insn->offset);
+			return -1;
+		}
+
+		/*
+		 * Treat intra-function CALLs as JMPs, but with a stack_op.
+		 * Also see how setup_call_dest() strips stack_ops from normal
+		 * CALLs.
+		 */
+		insn->type = INSN_JUMP_UNCONDITIONAL;
+
+		dest_off = insn->offset + insn->len + insn->immediate;
+		insn->jump_dest = find_insn(file, insn->sec, dest_off);
+		if (!insn->jump_dest) {
+			WARN_FUNC("can't find call dest at %s+0x%lx",
+				  insn->sec, insn->offset,
+				  insn->sec->name, dest_off);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 static void mark_rodata(struct objtool_file *file)
 {
 	struct section *sec;
@@ -1478,6 +1543,10 @@ static int decode_sections(struct objtoo
 	if (ret)
 		return ret;
 
+	ret = read_intra_function_calls(file);
+	if (ret)
+		return ret;
+
 	ret = add_call_destinations(file);
 	if (ret)
 		return ret;
@@ -2252,6 +2321,9 @@ static int validate_branch(struct objtoo
 				return 0;
 		}
 
+		if (handle_insn_ops(insn, &state))
+			return 1;
+
 		switch (insn->type) {
 
 		case INSN_RETURN:
@@ -2311,9 +2383,6 @@ static int validate_branch(struct objtoo
 			break;
 
 		case INSN_EXCEPTION_RETURN:
-			if (handle_insn_ops(insn, &state))
-				return 1;
-
 			/*
 			 * This handles x86's sync_core() case, where we use an
 			 * IRET to self. All 'normal' IRET instructions are in

[RFC][PATCH 5/7] x86/speculation: Change __FILL_RETURN_BUFFER to work with objtool

[Peter Zijlstra]

From: Alexandre Chartre <alexandre.chartre@oracle.com>

Change __FILL_RETURN_BUFFER so that the stack state is deterministically
defined for each iteration and that objtool can have an accurate view
of the stack.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200414103618.12657-8-alexandre.chartre@oracle.com
---
 arch/x86/include/asm/nospec-branch.h |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -4,6 +4,7 @@
 #define _ASM_X86_NOSPEC_BRANCH_H_
 
 #include <linux/static_key.h>
+#include <linux/frame.h>
 
 #include <asm/alternative.h>
 #include <asm/alternative-asm.h>
@@ -46,12 +47,14 @@
 #define __FILL_RETURN_BUFFER(reg, nr, sp)	\
 	mov	$(nr/2), reg;			\
 771:						\
+	ANNOTATE_INTRA_FUNCTION_CALL		\
 	call	772f;				\
 773:	/* speculation trap */			\
 	pause;					\
 	lfence;					\
 	jmp	773b;				\
 772:						\
+	ANNOTATE_INTRA_FUNCTION_CALL		\
 	call	774f;				\
 775:	/* speculation trap */			\
 	pause;					\
@@ -59,8 +62,8 @@
 	jmp	775b;				\
 774:						\
 	dec	reg;				\
-	jnz	771b;				\
-	add	$(BITS_PER_LONG/8) * nr, sp;
+	add	$(BITS_PER_LONG/8) * 2, sp;	\
+	jnz	771b;
 
 #ifdef __ASSEMBLY__
 

Re: [RFC][PATCH 5/7] x86/speculation: Change __FILL_RETURN_BUFFER to work with objtool

[Josh Poimboeuf]

On Thu, Apr 16, 2020 at 05:07:57PM +0200, Peter Zijlstra wrote:
> From: Alexandre Chartre <alexandre.chartre@oracle.com>
> 
> Change __FILL_RETURN_BUFFER so that the stack state is deterministically
> defined for each iteration and that objtool can have an accurate view
> of the stack.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lkml.kernel.org/r/20200414103618.12657-8-alexandre.chartre@oracle.com
> ---
>  arch/x86/include/asm/nospec-branch.h |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -4,6 +4,7 @@
>  #define _ASM_X86_NOSPEC_BRANCH_H_
>  
>  #include <linux/static_key.h>
> +#include <linux/frame.h>
>  
>  #include <asm/alternative.h>
>  #include <asm/alternative-asm.h>
> @@ -46,12 +47,14 @@
>  #define __FILL_RETURN_BUFFER(reg, nr, sp)	\
>  	mov	$(nr/2), reg;			\
>  771:						\
> +	ANNOTATE_INTRA_FUNCTION_CALL		\
>  	call	772f;				\
>  773:	/* speculation trap */			\
>  	pause;					\
>  	lfence;					\
>  	jmp	773b;				\
>  772:						\
> +	ANNOTATE_INTRA_FUNCTION_CALL		\
>  	call	774f;				\
>  775:	/* speculation trap */			\
>  	pause;					\
> @@ -59,8 +62,8 @@
>  	jmp	775b;				\
>  774:						\
>  	dec	reg;				\
> -	jnz	771b;				\
> -	add	$(BITS_PER_LONG/8) * nr, sp;
> +	add	$(BITS_PER_LONG/8) * 2, sp;	\
> +	jnz	771b;
>  
>  #ifdef __ASSEMBLY__

Are we still planning to warn about stack changes inside an alternative?
If so then this would still fail...

In this case I think it should be safe, but I'm not sure how we can
ensure that will always be the case for other alternatives.

And do the ORC entries actually work for this?  As far as I can tell,
they would be associated with the .altinstructions section and not
.text, so it wouldn't work.

Also, does objtool not warn about the unreachable speculation traps?
I'm guessing it doesn't notice unreachable alternatives... which I guess
is probably fine.

-- 
Josh

Re: [RFC][PATCH 5/7] x86/speculation: Change __FILL_RETURN_BUFFER to work with objtool

[Josh Poimboeuf]

On Sun, Apr 19, 2020 at 11:52:00AM -0500, Josh Poimboeuf wrote:
> Are we still planning to warn about stack changes inside an alternative?
> If so then this would still fail...
> 
> In this case I think it should be safe, but I'm not sure how we can
> ensure that will always be the case for other alternatives.
> 
> And do the ORC entries actually work for this?  As far as I can tell,
> they would be associated with the .altinstructions section and not
> .text, so it wouldn't work.

My preference would be to move RSB stuffing out-of-line too, like you
did the retpolines.  Or use static branches.  Then we could add an
objtool warning to prevent stack changes in alternatives.

-- 
Josh

Re: [RFC][PATCH 5/7] x86/speculation: Change __FILL_RETURN_BUFFER to work with objtool

[Peter Zijlstra]

On Sun, Apr 19, 2020 at 11:55:19AM -0500, Josh Poimboeuf wrote:
> On Sun, Apr 19, 2020 at 11:52:00AM -0500, Josh Poimboeuf wrote:
> > Are we still planning to warn about stack changes inside an alternative?
> > If so then this would still fail...
> > 
> > In this case I think it should be safe, but I'm not sure how we can
> > ensure that will always be the case for other alternatives.
> > 
> > And do the ORC entries actually work for this?  As far as I can tell,
> > they would be associated with the .altinstructions section and not
> > .text, so it wouldn't work.
> 
> My preference would be to move RSB stuffing out-of-line too, like you
> did the retpolines.  Or use static branches.  Then we could add an
> objtool warning to prevent stack changes in alternatives.

I effectively did the static_branch thing, but with an alternative, it's
in the last patch, due to me being a moron and not refreshing the stack
before sending it out.

[RFC][PATCH 6/7] x86/retpoline: Out-of-line retpoline

[Peter Zijlstra]

Since GCC generated code already uses out-of-line retpolines and objtool
has trouble with retpolines in alternatives, out-of-line them entirely.

This will enable objtool (once it's been taught a few more tricks) to
generate valid ORC data for the out-of-line copies, which means we can
correctly and reliably unwind through a retpoline.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/crypto/aesni-intel_asm.S            |    4 -
 arch/x86/crypto/camellia-aesni-avx-asm_64.S  |    2 
 arch/x86/crypto/camellia-aesni-avx2-asm_64.S |    2 
 arch/x86/crypto/crc32c-pcl-intel-asm_64.S    |   26 +++++-----
 arch/x86/entry/entry_32.S                    |    6 +-
 arch/x86/entry/entry_64.S                    |    2 
 arch/x86/include/asm/asm-prototypes.h        |    8 ++-
 arch/x86/include/asm/nospec-branch.h         |   68 ++++++++-------------------
 arch/x86/kernel/ftrace_32.S                  |    2 
 arch/x86/kernel/ftrace_64.S                  |    4 -
 arch/x86/lib/checksum_32.S                   |    4 -
 arch/x86/lib/retpoline.S                     |   25 ++++++++-
 arch/x86/platform/efi/efi_stub_64.S          |    2 
 13 files changed, 75 insertions(+), 80 deletions(-)

--- a/arch/x86/crypto/aesni-intel_asm.S
+++ b/arch/x86/crypto/aesni-intel_asm.S
@@ -2758,7 +2758,7 @@ SYM_FUNC_START(aesni_xts_crypt8)
 	pxor INC, STATE4
 	movdqu IV, 0x30(OUTP)
 
-	CALL_NOSPEC %r11
+	CALL_NOSPEC r11
 
 	movdqu 0x00(OUTP), INC
 	pxor INC, STATE1
@@ -2803,7 +2803,7 @@ SYM_FUNC_START(aesni_xts_crypt8)
 	_aesni_gf128mul_x_ble()
 	movups IV, (IVP)
 
-	CALL_NOSPEC %r11
+	CALL_NOSPEC r11
 
 	movdqu 0x40(OUTP), INC
 	pxor INC, STATE1
--- a/arch/x86/crypto/camellia-aesni-avx-asm_64.S
+++ b/arch/x86/crypto/camellia-aesni-avx-asm_64.S
@@ -1228,7 +1228,7 @@ SYM_FUNC_START_LOCAL(camellia_xts_crypt_
 	vpxor 14 * 16(%rax), %xmm15, %xmm14;
 	vpxor 15 * 16(%rax), %xmm15, %xmm15;
 
-	CALL_NOSPEC %r9;
+	CALL_NOSPEC r9;
 
 	addq $(16 * 16), %rsp;
 
--- a/arch/x86/crypto/camellia-aesni-avx2-asm_64.S
+++ b/arch/x86/crypto/camellia-aesni-avx2-asm_64.S
@@ -1339,7 +1339,7 @@ SYM_FUNC_START_LOCAL(camellia_xts_crypt_
 	vpxor 14 * 32(%rax), %ymm15, %ymm14;
 	vpxor 15 * 32(%rax), %ymm15, %ymm15;
 
-	CALL_NOSPEC %r9;
+	CALL_NOSPEC r9;
 
 	addq $(16 * 32), %rsp;
 
--- a/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
+++ b/arch/x86/crypto/crc32c-pcl-intel-asm_64.S
@@ -75,7 +75,7 @@
 
 .text
 SYM_FUNC_START(crc_pcl)
-#define    bufp		%rdi
+#define    bufp		rdi
 #define    bufp_dw	%edi
 #define    bufp_w	%di
 #define    bufp_b	%dil
@@ -105,9 +105,9 @@ SYM_FUNC_START(crc_pcl)
 	## 1) ALIGN:
 	################################################################
 
-	mov     bufp, bufptmp		# rdi = *buf
-	neg     bufp
-	and     $7, bufp		# calculate the unalignment amount of
+	mov     %bufp, bufptmp		# rdi = *buf
+	neg     %bufp
+	and     $7, %bufp		# calculate the unalignment amount of
 					# the address
 	je      proc_block		# Skip if aligned
 
@@ -123,13 +123,13 @@ SYM_FUNC_START(crc_pcl)
 do_align:
 	#### Calculate CRC of unaligned bytes of the buffer (if any)
 	movq    (bufptmp), tmp		# load a quadward from the buffer
-	add     bufp, bufptmp		# align buffer pointer for quadword
+	add     %bufp, bufptmp		# align buffer pointer for quadword
 					# processing
-	sub     bufp, len		# update buffer length
+	sub     %bufp, len		# update buffer length
 align_loop:
 	crc32b  %bl, crc_init_dw 	# compute crc32 of 1-byte
 	shr     $8, tmp			# get next byte
-	dec     bufp
+	dec     %bufp
 	jne     align_loop
 
 proc_block:
@@ -169,10 +169,10 @@ SYM_FUNC_START(crc_pcl)
 	xor     crc2, crc2
 
 	## branch into array
-	lea	jump_table(%rip), bufp
-	movzxw  (bufp, %rax, 2), len
-	lea	crc_array(%rip), bufp
-	lea     (bufp, len, 1), bufp
+	lea	jump_table(%rip), %bufp
+	movzxw  (%bufp, %rax, 2), len
+	lea	crc_array(%rip), %bufp
+	lea     (%bufp, len, 1), %bufp
 	JMP_NOSPEC bufp
 
 	################################################################
@@ -218,9 +218,9 @@ LABEL crc_ %i
 	## 4) Combine three results:
 	################################################################
 
-	lea	(K_table-8)(%rip), bufp		# first entry is for idx 1
+	lea	(K_table-8)(%rip), %bufp		# first entry is for idx 1
 	shlq    $3, %rax			# rax *= 8
-	pmovzxdq (bufp,%rax), %xmm0		# 2 consts: K1:K2
+	pmovzxdq (%bufp,%rax), %xmm0		# 2 consts: K1:K2
 	leal	(%eax,%eax,2), %eax		# rax *= 3 (total *24)
 	subq    %rax, tmp			# tmp -= rax*24
 
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -816,7 +816,7 @@ SYM_CODE_START(ret_from_fork)
 
 	/* kernel thread */
 1:	movl	%edi, %eax
-	CALL_NOSPEC %ebx
+	CALL_NOSPEC ebx
 	/*
 	 * A kernel thread is allowed to return here after successfully
 	 * calling do_execve().  Exit to userspace to complete the execve()
@@ -1501,7 +1501,7 @@ SYM_CODE_START_LOCAL_NOALIGN(common_exce
 
 	TRACE_IRQS_OFF
 	movl	%esp, %eax			# pt_regs pointer
-	CALL_NOSPEC %edi
+	CALL_NOSPEC edi
 	jmp	ret_from_exception
 SYM_CODE_END(common_exception_read_cr2)
 
@@ -1522,7 +1522,7 @@ SYM_CODE_START_LOCAL_NOALIGN(common_exce
 
 	TRACE_IRQS_OFF
 	movl	%esp, %eax			# pt_regs pointer
-	CALL_NOSPEC %edi
+	CALL_NOSPEC edi
 	jmp	ret_from_exception
 SYM_CODE_END(common_exception)
 
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -349,7 +349,7 @@ SYM_CODE_START(ret_from_fork)
 	/* kernel thread */
 	UNWIND_HINT_EMPTY
 	movq	%r12, %rdi
-	CALL_NOSPEC %rbx
+	CALL_NOSPEC rbx
 	/*
 	 * A kernel thread is allowed to return here after successfully
 	 * calling do_execve().  Exit to userspace to complete the execve()
--- a/arch/x86/include/asm/asm-prototypes.h
+++ b/arch/x86/include/asm/asm-prototypes.h
@@ -18,9 +18,13 @@ extern void cmpxchg8b_emu(void);
 
 #ifdef CONFIG_RETPOLINE
 #ifdef CONFIG_X86_32
-#define INDIRECT_THUNK(reg) extern asmlinkage void __x86_indirect_thunk_e ## reg(void);
+#define INDIRECT_THUNK(reg) \
+	extern asmlinkage void __x86_retpoline_e ## reg(void); \
+	extern asmlinkage void __x86_indirect_thunk_e ## reg(void);
 #else
-#define INDIRECT_THUNK(reg) extern asmlinkage void __x86_indirect_thunk_r ## reg(void);
+#define INDIRECT_THUNK(reg) \
+	extern asmlinkage void __x86_retpoline_r ## reg(void); \
+	extern asmlinkage void __x86_indirect_thunk_r ## reg(void);
 INDIRECT_THUNK(8)
 INDIRECT_THUNK(9)
 INDIRECT_THUNK(10)
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -80,57 +80,34 @@
 .endm
 
 /*
- * These are the bare retpoline primitives for indirect jmp and call.
- * Do not use these directly; they only exist to make the ALTERNATIVE
- * invocation below less ugly.
- */
-.macro RETPOLINE_JMP reg:req
-	call	.Ldo_rop_\@
-.Lspec_trap_\@:
-	pause
-	lfence
-	jmp	.Lspec_trap_\@
-.Ldo_rop_\@:
-	mov	\reg, (%_ASM_SP)
-	ret
-.endm
-
-/*
- * This is a wrapper around RETPOLINE_JMP so the called function in reg
- * returns to the instruction after the macro.
- */
-.macro RETPOLINE_CALL reg:req
-	jmp	.Ldo_call_\@
-.Ldo_retpoline_jmp_\@:
-	RETPOLINE_JMP \reg
-.Ldo_call_\@:
-	call	.Ldo_retpoline_jmp_\@
-.endm
-
-/*
  * JMP_NOSPEC and CALL_NOSPEC macros can be used instead of a simple
  * indirect jmp/call which may be susceptible to the Spectre variant 2
  * attack.
  */
 .macro JMP_NOSPEC reg:req
 #ifdef CONFIG_RETPOLINE
-	ANNOTATE_NOSPEC_ALTERNATIVE
-	ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *\reg),	\
-		__stringify(RETPOLINE_JMP \reg), X86_FEATURE_RETPOLINE,	\
-		__stringify(lfence; ANNOTATE_RETPOLINE_SAFE; jmp *\reg), X86_FEATURE_RETPOLINE_AMD
+	ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *%\reg), \
+		      __stringify(jmp __x86_retpoline_\()\reg), X86_FEATURE_RETPOLINE, \
+		      __stringify(lfence; ANNOTATE_RETPOLINE_SAFE; jmp *%\reg), X86_FEATURE_RETPOLINE_AMD
 #else
-	jmp	*\reg
+	jmp	*%\reg
 #endif
 .endm
 
 .macro CALL_NOSPEC reg:req
 #ifdef CONFIG_RETPOLINE
-	ANNOTATE_NOSPEC_ALTERNATIVE
-	ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; call *\reg),	\
-		__stringify(RETPOLINE_CALL \reg), X86_FEATURE_RETPOLINE,\
-		__stringify(lfence; ANNOTATE_RETPOLINE_SAFE; call *\reg), X86_FEATURE_RETPOLINE_AMD
+	/*
+	 * This cannot be ALTERNATIVE_2 like with JMP_NOSPEC, because ORC
+	 * unwind data is alternative invariant and needs stack modifying
+	 * instructions to be in the same place for all alternatives.
+	 *
+	 * IOW the CALL instruction must be at the same offset for all cases.
+	 */
+	ALTERNATIVE "", "lfence", X86_FEATURE_RETPOLINE_AMD
+	ALTERNATIVE __stringify(ANNOTATE_RETPOLINE_SAFE; call *%\reg), \
+		    __stringify(call __x86_retpoline_\reg), X86_FEATURE_RETPOLINE
 #else
-	call	*\reg
+	call	*%\reg
 #endif
 .endm
 
@@ -165,15 +142,12 @@
  */
 # define CALL_NOSPEC						\
 	ANNOTATE_NOSPEC_ALTERNATIVE				\
-	ALTERNATIVE_2(						\
-	ANNOTATE_RETPOLINE_SAFE					\
-	"call *%[thunk_target]\n",				\
-	"call __x86_indirect_thunk_%V[thunk_target]\n",		\
-	X86_FEATURE_RETPOLINE,					\
-	"lfence;\n"						\
-	ANNOTATE_RETPOLINE_SAFE					\
-	"call *%[thunk_target]\n",				\
-	X86_FEATURE_RETPOLINE_AMD)
+	ALTERNATIVE("", "lfence", X86_FEATURE_RETPOLINE_AMD)	\
+	ALTERNATIVE(ANNOTATE_RETPOLINE_SAFE			\
+		    "call *%[thunk_target]\n",			\
+		    "call __x86_indirect_thunk_%V[thunk_target]\n", \
+		    X86_FEATURE_RETPOLINE)
+
 # define THUNK_TARGET(addr) [thunk_target] "r" (addr)
 
 #else /* CONFIG_X86_32 */
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -189,5 +189,5 @@ SYM_CODE_END(ftrace_graph_caller)
 	movl	%eax, %ecx
 	popl	%edx
 	popl	%eax
-	JMP_NOSPEC %ecx
+	JMP_NOSPEC ecx
 #endif
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -301,7 +301,7 @@ SYM_INNER_LABEL(ftrace_stub, SYM_L_GLOBA
 	 * function tracing is enabled.
 	 */
 	movq ftrace_trace_function, %r8
-	CALL_NOSPEC %r8
+	CALL_NOSPEC r8
 	restore_mcount_regs
 
 	jmp fgraph_trace
@@ -338,6 +338,6 @@ SYM_CODE_START(return_to_handler)
 	movq 8(%rsp), %rdx
 	movq (%rsp), %rax
 	addq $24, %rsp
-	JMP_NOSPEC %rdi
+	JMP_NOSPEC rdi
 SYM_CODE_END(return_to_handler)
 #endif
--- a/arch/x86/lib/checksum_32.S
+++ b/arch/x86/lib/checksum_32.S
@@ -153,7 +153,7 @@ SYM_FUNC_START(csum_partial)
 	negl %ebx
 	lea 45f(%ebx,%ebx,2), %ebx
 	testl %esi, %esi
-	JMP_NOSPEC %ebx
+	JMP_NOSPEC ebx
 
 	# Handle 2-byte-aligned regions
 20:	addw (%esi), %ax
@@ -436,7 +436,7 @@ SYM_FUNC_START(csum_partial_copy_generic
 	andl $-32,%edx
 	lea 3f(%ebx,%ebx), %ebx
 	testl %esi, %esi 
-	JMP_NOSPEC %ebx
+	JMP_NOSPEC ebx
 1:	addl $64,%esi
 	addl $64,%edi 
 	SRC(movb -32(%edx),%bl)	; SRC(movb (%edx),%bl)
--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -7,15 +7,30 @@
 #include <asm/alternative-asm.h>
 #include <asm/export.h>
 #include <asm/nospec-branch.h>
+#include <asm/unwind_hints.h>
+#include <asm/frame.h>
 
 .macro THUNK reg
 	.section .text.__x86.indirect_thunk
 
+	.align 32
 SYM_FUNC_START(__x86_indirect_thunk_\reg)
-	CFI_STARTPROC
-	JMP_NOSPEC %\reg
-	CFI_ENDPROC
+	JMP_NOSPEC \reg
 SYM_FUNC_END(__x86_indirect_thunk_\reg)
+
+SYM_FUNC_START_NOALIGN(__x86_retpoline_\reg)
+	ANNOTATE_INTRA_FUNCTION_CALL
+	call	.Ldo_rop_\@
+.Lspec_trap_\@:
+	pause
+	lfence
+	jmp	.Lspec_trap_\@
+.Ldo_rop_\@:
+	mov	%\reg, (%_ASM_SP)
+	UNWIND_HINT_RET_OFFSET
+	ret
+SYM_FUNC_END(__x86_retpoline_\reg)
+
 .endm
 
 /*
@@ -26,7 +41,9 @@ SYM_FUNC_END(__x86_indirect_thunk_\reg)
  * the simple and nasty way...
  */
 #define __EXPORT_THUNK(sym) _ASM_NOKPROBE(sym); EXPORT_SYMBOL(sym)
-#define EXPORT_THUNK(reg) __EXPORT_THUNK(__x86_indirect_thunk_ ## reg)
+#define EXPORT_THUNK(reg)				\
+	__EXPORT_THUNK(__x86_retpoline_ ## reg);	\
+	__EXPORT_THUNK(__x86_indirect_thunk_ ## reg)
 #define GENERATE_THUNK(reg) THUNK reg ; EXPORT_THUNK(reg)
 
 GENERATE_THUNK(_ASM_AX)
--- a/arch/x86/platform/efi/efi_stub_64.S
+++ b/arch/x86/platform/efi/efi_stub_64.S
@@ -21,7 +21,7 @@ SYM_FUNC_START(__efi_call)
 	mov %r8, %r9
 	mov %rcx, %r8
 	mov %rsi, %rcx
-	CALL_NOSPEC %rdi
+	CALL_NOSPEC rdi
 	leave
 	ret
 SYM_FUNC_END(__efi_call)

Re: [RFC][PATCH 6/7] x86/retpoline: Out-of-line retpoline

[Peter Zijlstra]

Bah, chunks went missing, I'll make sure to push out a working version.

On Thu, Apr 16, 2020 at 05:07:58PM +0200, Peter Zijlstra wrote:
> --- a/arch/x86/lib/retpoline.S
> +++ b/arch/x86/lib/retpoline.S
> @@ -7,15 +7,30 @@
>  #include <asm/alternative-asm.h>
>  #include <asm/export.h>
>  #include <asm/nospec-branch.h>
> +#include <asm/unwind_hints.h>
> +#include <asm/frame.h>
>  
>  .macro THUNK reg
>  	.section .text.__x86.indirect_thunk
>  
> +	.align 32
>  SYM_FUNC_START(__x86_indirect_thunk_\reg)
> -	CFI_STARTPROC
> -	JMP_NOSPEC %\reg
> -	CFI_ENDPROC
> +	JMP_NOSPEC \reg
>  SYM_FUNC_END(__x86_indirect_thunk_\reg)
> +
> +SYM_FUNC_START_NOALIGN(__x86_retpoline_\reg)
> +	ANNOTATE_INTRA_FUNCTION_CALL
> +	call	.Ldo_rop_\@
> +.Lspec_trap_\@:
	UNWIND_HINT_EMPTY
> +	pause
> +	lfence
> +	jmp	.Lspec_trap_\@
> +.Ldo_rop_\@:
> +	mov	%\reg, (%_ASM_SP)
> +	UNWIND_HINT_RET_OFFSET
> +	ret
> +SYM_FUNC_END(__x86_retpoline_\reg)
> +
>  .endm
>  
>  /*

[RFC][PATCH 7/7] x86/nospec: Remove ANNOTATE_NOSPEC_ALTERNATIVE

[Peter Zijlstra]

Now that objtool is smart enough; remove ANNOTATE_NOSPEC_ALTERNATIVE
so that we might generate correct ORC unwind information.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/nospec-branch.h |   26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -10,15 +10,7 @@
 #include <asm/alternative-asm.h>
 #include <asm/cpufeatures.h>
 #include <asm/msr-index.h>
-
-/*
- * This should be used immediately before a retpoline alternative. It tells
- * objtool where the retpolines are so that it can make sense of the control
- * flow by just reading the original instruction(s) and ignoring the
- * alternatives.
- */
-#define ANNOTATE_NOSPEC_ALTERNATIVE \
-	ANNOTATE_IGNORE_ALTERNATIVE
+#include <asm/unwind_hints.h>
 
 /*
  * Fill the CPU return stack buffer.
@@ -50,6 +42,7 @@
 	ANNOTATE_INTRA_FUNCTION_CALL		\
 	call	772f;				\
 773:	/* speculation trap */			\
+	UNWIND_HINT_EMPTY;			\
 	pause;					\
 	lfence;					\
 	jmp	773b;				\
@@ -57,6 +50,7 @@
 	ANNOTATE_INTRA_FUNCTION_CALL		\
 	call	774f;				\
 775:	/* speculation trap */			\
+	UNWIND_HINT_EMPTY;			\
 	pause;					\
 	lfence;					\
 	jmp	775b;				\
@@ -117,10 +111,8 @@
   */
 .macro FILL_RETURN_BUFFER reg:req nr:req ftr:req
 #ifdef CONFIG_RETPOLINE
-	ANNOTATE_NOSPEC_ALTERNATIVE
-	ALTERNATIVE "jmp .Lskip_rsb_\@",				\
-		__stringify(__FILL_RETURN_BUFFER(\reg,\nr,%_ASM_SP))	\
-		\ftr
+	ALTERNATIVE "jmp .Lskip_rsb_\@", "", \ftr
+	__FILL_RETURN_BUFFER(\reg,\nr,%_ASM_SP)
 .Lskip_rsb_\@:
 #endif
 .endm
@@ -141,7 +133,6 @@
  * which is ensured when CONFIG_RETPOLINE is defined.
  */
 # define CALL_NOSPEC						\
-	ANNOTATE_NOSPEC_ALTERNATIVE				\
 	ALTERNATIVE("", "lfence", X86_FEATURE_RETPOLINE_AMD)	\
 	ALTERNATIVE(ANNOTATE_RETPOLINE_SAFE			\
 		    "call *%[thunk_target]\n",			\
@@ -157,7 +148,6 @@
  * here, anyway.
  */
 # define CALL_NOSPEC						\
-	ANNOTATE_NOSPEC_ALTERNATIVE				\
 	ALTERNATIVE_2(						\
 	ANNOTATE_RETPOLINE_SAFE					\
 	"call *%[thunk_target]\n",				\
@@ -225,10 +215,8 @@ static inline void vmexit_fill_RSB(void)
 #ifdef CONFIG_RETPOLINE
 	unsigned long loops;
 
-	asm volatile (ANNOTATE_NOSPEC_ALTERNATIVE
-		      ALTERNATIVE("jmp 910f",
-				  __stringify(__FILL_RETURN_BUFFER(%0, RSB_CLEAR_LOOPS, %1)),
-				  X86_FEATURE_RETPOLINE)
+	asm volatile (ALTERNATIVE("jmp 910f", "", X86_FEATURE_RETPOLINE)
+		      __stringify(__FILL_RETURN_BUFFER(%0, RSB_CLEAR_LOOPS, %1))
 		      "910:"
 		      : "=r" (loops), ASM_CALL_CONSTRAINT
 		      : : "memory" );

Re: [RFC][PATCH 0/7] objtool vs retpoline

[Peter Zijlstra]

On Thu, Apr 16, 2020 at 05:07:52PM +0200, Peter Zijlstra wrote:
> Hi!
> 
> Based on Alexandre's patches, here's a few patches that apply on top of my
> other series:
> 
>   https://lkml.kernel.org/r/20200416114706.625340212@infradead.org
> 
> which seems to build a clean defconfig without ANNOTATE_NOSPEC_ALTERNATIVE on.
> 
> I've not tried to boot the thing, I'll leave that to the robots.

Find a slightly cleaner, and more actually working, version here:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git core/objtool-retpoline