[PATCH 0/7] objtool changes to remove most ANNOTATE_NOSPEC_ALTERNATIVE

[PATCH 0/7] objtool changes to remove most ANNOTATE_NOSPEC_ALTERNATIVE

[Alexandre Chartre]

Hi,

Code like retpoline or RSB stuffing, which is used to mitigate some of
the speculative execution issues, is currently ignored by objtool with
the ANNOTATE_NOSPEC_ALTERNATIVE directive. This series adds support
for intra-function calls and trampoline return instructions to objtool
so that it can handle such a code. With these changes, we can remove
most ANNOTATE_NOSPEC_ALTERNATIVE directives.

Thanks,

alex.

--

Alexandre Chartre (7):
  objtool: is_fentry_call() crashes if call has no destination
  objtool: Allow branches within the same alternative.
  objtool: Add support for intra-function calls
  objtool: Add support for return trampoline call
  x86/speculation: Annotate intra-function calls
  x86/speculation: Annotate retpoline return instructions
  x86/speculation: Remove most ANNOTATE_NOSPEC_ALTERNATIVE

 arch/x86/include/asm/nospec-branch.h |  38 ++++--
 tools/objtool/check.c                | 172 +++++++++++++++++++++++++--
 tools/objtool/check.h                |   5 +-
 3 files changed, 196 insertions(+), 19 deletions(-)

-- 
2.18.2

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

[Alexandre Chartre]

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>
---
 tools/objtool/check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4768d91c6d68..708562fb89e1 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1363,7 +1363,7 @@ static int decode_sections(struct objtool_file *file)
 
 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;
-- 
2.18.2

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

[Alexandre Chartre]

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>
---
 tools/objtool/check.c | 19 +++++++++++++------
 tools/objtool/check.h |  3 ++-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 708562fb89e1..214809ac2776 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -712,7 +712,9 @@ static int handle_group_alt(struct objtool_file *file,
 			    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;
@@ -721,7 +723,7 @@ static int handle_group_alt(struct objtool_file *file,
 		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;
 	}
 
@@ -1942,6 +1944,7 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
  * tools/objtool/Documentation/stack-validation.txt.
  */
 static int validate_branch(struct objtool_file *file, struct symbol *func,
+			   struct instruction *from,
 			   struct instruction *first, struct insn_state state)
 {
 	struct alternative *alt;
@@ -1953,7 +1956,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 	insn = first;
 	sec = insn->sec;
 
-	if (insn->alt_group && list_empty(&insn->alts)) {
+	if (insn->alt_group &&
+	    (!from || from->alt_group != insn->alt_group) &&
+	    list_empty(&insn->alts)) {
 		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
 			  sec, insn->offset);
 		return 1;
@@ -2035,7 +2040,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				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);
@@ -2105,7 +2111,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 					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)
@@ -2236,7 +2242,8 @@ static int validate_unwind_hints(struct objtool_file *file)
 
 	for_each_insn(file, insn) {
 		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;
@@ -2377,7 +2384,7 @@ static int validate_functions(struct objtool_file *file)
 
 			state.uaccess = func->uaccess_safe;
 
-			ret = validate_branch(file, func, insn, state);
+			ret = validate_branch(file, func, NULL, insn, state);
 			if (ret && backtrace)
 				BT_FUNC("<=== (func)", insn);
 			warnings += ret;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 6d875ca6fce0..cffb23d81782 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,7 +33,8 @@ struct instruction {
 	unsigned int len;
 	enum insn_type type;
 	unsigned long immediate;
-	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+	unsigned int alt_group;
+	bool dead_end, ignore, hint, save, restore, ignore_alts;
 	bool retpoline_safe;
 	u8 visited;
 	struct symbol *call_dest;
-- 
2.18.2

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

[Alexandre Chartre]

Change objtool to support intra-function calls. 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>
---
 tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
 tools/objtool/check.h |  1 +
 2 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 214809ac2776..0cec91291d46 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -657,6 +657,18 @@ static int add_call_destinations(struct objtool_file *file)
 		if (insn->type != INSN_CALL)
 			continue;
 
+		if (insn->intra_function_call) {
+			dest_off = insn->offset + insn->len + insn->immediate;
+			insn->jump_dest = find_insn(file, insn->sec, dest_off);
+			if (insn->jump_dest)
+				continue;
+
+			WARN_FUNC("can't find call dest at %s+0x%lx",
+				  insn->sec, insn->offset,
+				  insn->sec->name, dest_off);
+			return -1;
+		}
+
 		rela = find_rela_by_dest_range(insn->sec, insn->offset,
 					       insn->len);
 		if (!rela) {
@@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct objtool_file *file)
 	return 0;
 }
 
+static int read_intra_function_call(struct objtool_file *file)
+{
+	struct section *sec;
+	struct instruction *insn;
+	struct rela *rela;
+
+	sec = find_section_by_name(file->elf,
+				   ".rela.discard.intra_function_call");
+	if (!sec)
+		return 0;
+
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		if (rela->sym->type != STT_SECTION) {
+			WARN("unexpected relocation symbol type in %s",
+			     sec->name);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("bad .discard.intra_function_call entry");
+			return -1;
+		}
+
+		if (insn->type != INSN_CALL) {
+			WARN_FUNC("intra_function_call not a call",
+				  insn->sec, insn->offset);
+			return -1;
+		}
+
+		insn->intra_function_call = true;
+		/*
+		 * For the impact on the stack, make an intra-function
+		 * call behaves like a push of an immediate value (the
+		 * return address).
+		 */
+		insn->stack_op.src.type = OP_SRC_CONST;
+		insn->stack_op.dest.type = OP_DEST_PUSH;
+	}
+
+	return 0;
+}
+
 static void mark_rodata(struct objtool_file *file)
 {
 	struct section *sec;
@@ -1344,6 +1399,10 @@ static int decode_sections(struct objtool_file *file)
 	if (ret)
 		return ret;
 
+	ret = read_intra_function_call(file);
+	if (ret)
+		return ret;
+
 	ret = add_call_destinations(file);
 	if (ret)
 		return ret;
@@ -2092,7 +2151,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				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;
@@ -2101,6 +2161,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			if (dead_end_function(file, insn->call_dest))
 				return 0;
 
+			if (insn->intra_function_call) {
+				update_insn_state(insn, &state);
+				ret = validate_branch(file, func, insn,
+						      insn->jump_dest, state);
+				if (ret) {
+					if (backtrace)
+						BT_FUNC("(intra-call)", insn);
+					return ret;
+				}
+			}
+
 			break;
 
 		case INSN_JUMP_CONDITIONAL:
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index cffb23d81782..2bd6d2f46baa 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -35,6 +35,7 @@ struct instruction {
 	unsigned long immediate;
 	unsigned int alt_group;
 	bool dead_end, ignore, hint, save, restore, ignore_alts;
+	bool intra_function_call;
 	bool retpoline_safe;
 	u8 visited;
 	struct symbol *call_dest;
-- 
2.18.2

[PATCH 4/7] objtool: Add support for return trampoline call

[Alexandre Chartre]

With retpoline, the return instruction is used to branch to an address
stored on the stack. So, unlike a regular return instruction, when a
retpoline return instruction is reached the stack has been modified
compared to what we have when the function was entered.

Provide the mechanism to explicitly call-out such return instruction
so that objtool can correctly handle them.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 tools/objtool/check.c | 78 +++++++++++++++++++++++++++++++++++++++++--
 tools/objtool/check.h |  1 +
 2 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0cec91291d46..ed8e3ea1d8da 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1344,6 +1344,48 @@ static int read_intra_function_call(struct objtool_file *file)
 	return 0;
 }
 
+static int read_retpoline_ret(struct objtool_file *file)
+{
+	struct section *sec;
+	struct instruction *insn;
+	struct rela *rela;
+
+	sec = find_section_by_name(file->elf, ".rela.discard.retpoline_ret");
+	if (!sec)
+		return 0;
+
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		if (rela->sym->type != STT_SECTION) {
+			WARN("unexpected relocation symbol type in %s",
+			     sec->name);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("bad .discard.retpoline_ret entry");
+			return -1;
+		}
+
+		if (insn->type != INSN_RETURN) {
+			WARN_FUNC("retpoline_ret not a return",
+				  insn->sec, insn->offset);
+			return -1;
+		}
+
+		insn->retpoline_ret = true;
+		/*
+		 * For the impact on the stack, make a return trampoline
+		 * behaves like a pop of the return address.
+		 */
+		insn->stack_op.src.type = OP_SRC_POP;
+		insn->stack_op.dest.type = OP_DEST_REG;
+		insn->stack_op.dest.reg = CFI_RA;
+	}
+
+	return 0;
+}
+
 static void mark_rodata(struct objtool_file *file)
 {
 	struct section *sec;
@@ -1403,6 +1445,10 @@ static int decode_sections(struct objtool_file *file)
 	if (ret)
 		return ret;
 
+	ret = read_retpoline_ret(file);
+	if (ret)
+		return ret;
+
 	ret = add_call_destinations(file);
 	if (ret)
 		return ret;
@@ -1432,7 +1478,8 @@ static bool is_fentry_call(struct instruction *insn)
 	return false;
 }
 
-static bool has_modified_stack_frame(struct insn_state *state)
+static bool has_modified_stack_frame(struct insn_state *state,
+				     bool check_registers)
 {
 	int i;
 
@@ -1442,6 +1489,9 @@ static bool has_modified_stack_frame(struct insn_state *state)
 	    state->drap)
 		return true;
 
+	if (!check_registers)
+		return false;
+
 	for (i = 0; i < CFI_NUM_REGS; i++)
 		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
 		    state->regs[i].offset != initial_func_cfi.regs[i].offset)
@@ -1987,7 +2037,7 @@ static int validate_call(struct instruction *insn, struct insn_state *state)
 
 static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
 {
-	if (has_modified_stack_frame(state)) {
+	if (has_modified_stack_frame(state, true)) {
 		WARN_FUNC("sibling call from callable instruction with modified stack frame",
 				insn->sec, insn->offset);
 		return 1;
@@ -2009,6 +2059,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 	struct alternative *alt;
 	struct instruction *insn, *next_insn;
 	struct section *sec;
+	bool check_registers;
 	u8 visited;
 	int ret;
 
@@ -2130,7 +2181,28 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				return 1;
 			}
 
-			if (func && has_modified_stack_frame(&state)) {
+			/*
+			 * With retpoline, the return instruction is used
+			 * to branch to an address stored on the stack.
+			 * So when we reach the ret instruction, the stack
+			 * frame has been modified with the address to
+			 * branch to and we need update the stack state.
+			 *
+			 * The retpoline address to branch to is typically
+			 * pushed on the stack from a register, but this
+			 * confuses the logic which checks callee saved
+			 * registers. So we don't check if registers have
+			 * been modified if we have a return trampoline.
+			 */
+			if (insn->retpoline_ret) {
+				update_insn_state(insn, &state);
+				check_registers = false;
+			} else {
+				check_registers = true;
+			}
+
+			if (func && has_modified_stack_frame(&state,
+							     check_registers)) {
 				WARN_FUNC("return with modified stack frame",
 					  sec, insn->offset);
 				return 1;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 2bd6d2f46baa..5ecd16ad71a8 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -37,6 +37,7 @@ struct instruction {
 	bool dead_end, ignore, hint, save, restore, ignore_alts;
 	bool intra_function_call;
 	bool retpoline_safe;
+	bool retpoline_ret;
 	u8 visited;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
-- 
2.18.2

[PATCH 5/7] x86/speculation: Annotate intra-function calls

[Alexandre Chartre]

Some speculative execution mitigations (like retpoline) use intra-
function calls. Provide a macro to annotate such intra-function calls
so they can be properly handled by objtool, and use this macro to
annotate intra-function calls.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 arch/x86/include/asm/nospec-branch.h | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 5c24a7b35166..a2885f801e13 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -77,13 +77,27 @@
 	.popsection
 .endm
 
+/*
+ * Intra-function call instruction. This should be used as a substitute
+ * for the call instruction when doing an intra-function call. It is
+ * similar to the call instruction but it tells objtool that this is
+ * an intra-function call.
+ */
+.macro INTRA_FUNCTION_CALL dst:req
+	.Lannotate_\@:
+	.pushsection .discard.intra_function_call
+	_ASM_PTR .Lannotate_\@
+	.popsection
+	call \dst
+.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_\@
+	INTRA_FUNCTION_CALL .Ldo_rop_\@
 .Lspec_trap_\@:
 	pause
 	lfence
@@ -102,7 +116,7 @@
 .Ldo_retpoline_jmp_\@:
 	RETPOLINE_JMP \reg
 .Ldo_call_\@:
-	call	.Ldo_retpoline_jmp_\@
+	INTRA_FUNCTION_CALL .Ldo_retpoline_jmp_\@
 .endm
 
 /*
-- 
2.18.2

[PATCH 6/7] x86/speculation: Annotate retpoline return instructions

[Alexandre Chartre]

With retpoline, the return instruction (ret) is used to branch to an
address stored on the stack. Provide a macro to annotate such trampoline
returns so they can be properly handled by objtool, and use this macro
to annotate retpoline return instructions.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 arch/x86/include/asm/nospec-branch.h | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index a2885f801e13..9ae6dde2f10b 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -91,6 +91,20 @@
 	call \dst
 .endm
 
+/*
+ * Retpoline return instruction. This should be used as a substitute
+ * for the ret instruction when doing a trampoline return. It is
+ * similar to the ret instruction but it tells objtool this is a
+ * trampoline return.
+ */
+.macro RETPOLINE_RET
+	.Lannotate_\@:
+	.pushsection .discard.retpoline_ret
+	_ASM_PTR .Lannotate_\@
+	.popsection
+	ret
+.endm
+
 /*
  * These are the bare retpoline primitives for indirect jmp and call.
  * Do not use these directly; they only exist to make the ALTERNATIVE
@@ -104,7 +118,7 @@
 	jmp	.Lspec_trap_\@
 .Ldo_rop_\@:
 	mov	\reg, (%_ASM_SP)
-	ret
+	RETPOLINE_RET
 .endm
 
 /*
-- 
2.18.2

[PATCH 7/7] x86/speculation: Remove most ANNOTATE_NOSPEC_ALTERNATIVE

[Alexandre Chartre]

Now that intra-function calls and retpoline return instructions have
been annotated and that they are supported by objtool, almost all
ANNOTATE_NOSPEC_ALTERNATIVE can be removed.

ANNOTATE_NOSPEC_ALTERNATIVE is still needed when using the
__FILL_RETURN_BUFFER macro because it loops on intra-function calls
and objtool doesn't handle loops modifying the stack pointer.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 arch/x86/include/asm/nospec-branch.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 9ae6dde2f10b..70d4444120b1 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -140,7 +140,6 @@
  */
 .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
@@ -151,7 +150,6 @@
 
 .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
@@ -190,7 +188,6 @@
  * which is ensured when CONFIG_RETPOLINE is defined.
  */
 # define CALL_NOSPEC						\
-	ANNOTATE_NOSPEC_ALTERNATIVE				\
 	ALTERNATIVE_2(						\
 	ANNOTATE_RETPOLINE_SAFE					\
 	"call *%[thunk_target]\n",				\
@@ -209,7 +206,6 @@
  * here, anyway.
  */
 # define CALL_NOSPEC						\
-	ANNOTATE_NOSPEC_ALTERNATIVE				\
 	ALTERNATIVE_2(						\
 	ANNOTATE_RETPOLINE_SAFE					\
 	"call *%[thunk_target]\n",				\
-- 
2.18.2

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

[Julien Thierry]

Hi Alexandre,

I ran into the same issue for the arm64 work:
https://lkml.org/lkml/2020/1/9/656

Your solution seems nicer however.

On 4/2/20 9:22 AM, Alexandre Chartre wrote:
> 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>
> ---
>   tools/objtool/check.c | 19 +++++++++++++------
>   tools/objtool/check.h |  3 ++-
>   2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 708562fb89e1..214809ac2776 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -712,7 +712,9 @@ static int handle_group_alt(struct objtool_file *file,
>   			    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;
> @@ -721,7 +723,7 @@ static int handle_group_alt(struct objtool_file *file,
>   		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;
>   	}
>   
> @@ -1942,6 +1944,7 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
>    * tools/objtool/Documentation/stack-validation.txt.
>    */
>   static int validate_branch(struct objtool_file *file, struct symbol *func,
> +			   struct instruction *from,

Maybe instead of passing a new instruction pointer, just the alt_group 
could be passed? 0 Meaning it was not in an alt group.

>   			   struct instruction *first, struct insn_state state)
>   {
>   	struct alternative *alt;
> @@ -1953,7 +1956,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   	insn = first;
>   	sec = insn->sec;
>   
> -	if (insn->alt_group && list_empty(&insn->alts)) {
> +	if (insn->alt_group &&
> +	    (!from || from->alt_group != insn->alt_group) &&
> +	    list_empty(&insn->alts)) {

This would become

	if (insn->alt_group != alt_group && list_empty(&insn->alts))

And the recursive validate_branch() calls would just take 
insn->alt_group as parameter (and the calls in validate_functions() and 
validate_unwind_hints() would take 0).

Any opinions on that?

>   		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
>   			  sec, insn->offset);
>   		return 1;
> @@ -2035,7 +2040,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   				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);
> @@ -2105,7 +2111,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   					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)
> @@ -2236,7 +2242,8 @@ static int validate_unwind_hints(struct objtool_file *file)
>   
>   	for_each_insn(file, insn) {
>   		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;
> @@ -2377,7 +2384,7 @@ static int validate_functions(struct objtool_file *file)
>   
>   			state.uaccess = func->uaccess_safe;
>   
> -			ret = validate_branch(file, func, insn, state);
> +			ret = validate_branch(file, func, NULL, insn, state);
>   			if (ret && backtrace)
>   				BT_FUNC("<=== (func)", insn);
>   			warnings += ret;
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index 6d875ca6fce0..cffb23d81782 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -33,7 +33,8 @@ struct instruction {
>   	unsigned int len;
>   	enum insn_type type;
>   	unsigned long immediate;
> -	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> +	unsigned int alt_group;
> +	bool dead_end, ignore, hint, save, restore, ignore_alts;
>   	bool retpoline_safe;
>   	u8 visited;
>   	struct symbol *call_dest;
> 

Cheers,

-- 
Julien Thierry

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

[Alexandre Chartre]

On 4/2/20 2:03 PM, Julien Thierry wrote:
> Hi Alexandre,
> 
> I ran into the same issue for the arm64 work:
> https://lkml.org/lkml/2020/1/9/656

Thanks for the reference, I didn't notice that change, but I saw a more
recent one where you were just removing the branch to alternative check
(https://lkml.org/lkml/2020/3/25/151).

> Your solution seems nicer however.
> 
> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>> 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>
>> ---
>>   tools/objtool/check.c | 19 +++++++++++++------
>>   tools/objtool/check.h |  3 ++-
>>   2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 708562fb89e1..214809ac2776 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -712,7 +712,9 @@ static int handle_group_alt(struct objtool_file *file,
>>                   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;
>> @@ -721,7 +723,7 @@ static int handle_group_alt(struct objtool_file *file,
>>           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;
>>       }
>> @@ -1942,6 +1944,7 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
>>    * tools/objtool/Documentation/stack-validation.txt.
>>    */
>>   static int validate_branch(struct objtool_file *file, struct symbol *func,
>> +               struct instruction *from,
> 
> Maybe instead of passing a new instruction pointer, just the
> alt_group could be passed? 0 Meaning it was not in an alt group> 
>>                  struct instruction *first, struct insn_state state)
>>   {
>>       struct alternative *alt;
>> @@ -1953,7 +1956,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>>       insn = first;
>>       sec = insn->sec;
>> -    if (insn->alt_group && list_empty(&insn->alts)) {
>> +    if (insn->alt_group &&
>> +        (!from || from->alt_group != insn->alt_group) &&
>> +        list_empty(&insn->alts)) {
> 
> This would become
> 
>      if (insn->alt_group != alt_group && list_empty(&insn->alts))
> 
> And the recursive validate_branch() calls would just take
> insn->alt_group as parameter (and the calls in validate_functions()
> and validate_unwind_hints() would take 0).
> 
> Any opinions on that?

Yes, that would work too. I choose to pass the instruction pointer because
I was thinking that having the "from" instruction might be useful in the
future if there's a need to do additional check about the origin of the
branch.

alex.


>>           WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
>>                 sec, insn->offset);
>>           return 1;
>> @@ -2035,7 +2040,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>>                   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);
>> @@ -2105,7 +2111,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>>                       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)
>> @@ -2236,7 +2242,8 @@ static int validate_unwind_hints(struct objtool_file *file)
>>       for_each_insn(file, insn) {
>>           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;
>> @@ -2377,7 +2384,7 @@ static int validate_functions(struct objtool_file *file)
>>               state.uaccess = func->uaccess_safe;
>> -            ret = validate_branch(file, func, insn, state);
>> +            ret = validate_branch(file, func, NULL, insn, state);
>>               if (ret && backtrace)
>>                   BT_FUNC("<=== (func)", insn);
>>               warnings += ret;
>> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
>> index 6d875ca6fce0..cffb23d81782 100644
>> --- a/tools/objtool/check.h
>> +++ b/tools/objtool/check.h
>> @@ -33,7 +33,8 @@ struct instruction {
>>       unsigned int len;
>>       enum insn_type type;
>>       unsigned long immediate;
>> -    bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
>> +    unsigned int alt_group;
>> +    bool dead_end, ignore, hint, save, restore, ignore_alts;
>>       bool retpoline_safe;
>>       u8 visited;
>>       struct symbol *call_dest;
>>
> 
> Cheers,
> 

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

[Julien Thierry]

Hi Alexandre,

I ran into the limitation of intra-function call for the arm64 support 
but didn't take the time to make a clean patch to support them properly.

Nice to see you've gone through that work :) .

On 4/2/20 9:22 AM, Alexandre Chartre wrote:
> Change objtool to support intra-function calls. An intra-function call
> is represented in objtool as a push onto the stack (of the return

I have to disagree a bit with that. The push onto the stack is true on 
x86, but other architectures might not have that (arm/arm64 have a link 
register that gets set by "bl" instructions and do not modify the stack).

> 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>
> ---
>   tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
>   tools/objtool/check.h |  1 +
>   2 files changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 214809ac2776..0cec91291d46 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -657,6 +657,18 @@ static int add_call_destinations(struct objtool_file *file)
>   		if (insn->type != INSN_CALL)
>   			continue;
>   
> +		if (insn->intra_function_call) {
> +			dest_off = insn->offset + insn->len + insn->immediate;
> +			insn->jump_dest = find_insn(file, insn->sec, dest_off);
> +			if (insn->jump_dest)
> +				continue;
> +
> +			WARN_FUNC("can't find call dest at %s+0x%lx",
> +				  insn->sec, insn->offset,
> +				  insn->sec->name, dest_off);
> +			return -1;
> +		}
> +
>   		rela = find_rela_by_dest_range(insn->sec, insn->offset,
>   					       insn->len);
>   		if (!rela) {
> @@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct objtool_file *file)
>   	return 0;
>   }
>   
> +static int read_intra_function_call(struct objtool_file *file)
> +{
> +	struct section *sec;
> +	struct instruction *insn;
> +	struct rela *rela;
> +
> +	sec = find_section_by_name(file->elf,
> +				   ".rela.discard.intra_function_call");

I'm wondering, do we really need to annotate the intra_function_call and 
group the in a section?

Would it be a problem to consider all (static) call instructions with a 
destination that is not the start offset of a symbol to be an 
intra-function call (and set insn->intra_function_call and 
insn->jump_dest accordingly)?

Other than that the logic would stay the same.

> +	if (!sec)
> +		return 0;
> +
> +	list_for_each_entry(rela, &sec->rela_list, list) {
> +		if (rela->sym->type != STT_SECTION) {
> +			WARN("unexpected relocation symbol type in %s",
> +			     sec->name);
> +			return -1;
> +		}
> +
> +		insn = find_insn(file, rela->sym->sec, rela->addend);
> +		if (!insn) {
> +			WARN("bad .discard.intra_function_call entry");
> +			return -1;
> +		}
> +
> +		if (insn->type != INSN_CALL) {
> +			WARN_FUNC("intra_function_call not a call",
> +				  insn->sec, insn->offset);

Nit: This could be slightly confusing with INSN_CALL_DYNAMIC. Maybe just:
	"unsupported instruction for intra-function call " ?

> +			return -1;
> +		}
> +
> +		insn->intra_function_call = true;
> +		/*
> +		 * For the impact on the stack, make an intra-function
> +		 * call behaves like a push of an immediate value (the
> +		 * return address).
> +		 */
> +		insn->stack_op.src.type = OP_SRC_CONST;
> +		insn->stack_op.dest.type = OP_DEST_PUSH;

As commented above, this should be arch dependent.

> +	}
> +
> +	return 0;
> +}
> +
>   static void mark_rodata(struct objtool_file *file)
>   {
>   	struct section *sec;
> @@ -1344,6 +1399,10 @@ static int decode_sections(struct objtool_file *file)
>   	if (ret)
>   		return ret;
>   
> +	ret = read_intra_function_call(file);
> +	if (ret)
> +		return ret;
> +
>   	ret = add_call_destinations(file);
>   	if (ret)
>   		return ret;
> @@ -2092,7 +2151,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   				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;
> @@ -2101,6 +2161,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   			if (dead_end_function(file, insn->call_dest))
>   				return 0;
>   
> +			if (insn->intra_function_call) {
> +				update_insn_state(insn, &state);
> +				ret = validate_branch(file, func, insn,
> +						      insn->jump_dest, state);
> +				if (ret) {
> +					if (backtrace)
> +						BT_FUNC("(intra-call)", insn);
> +					return ret;
> +				}
> +			}
> +
>   			break;
>   
>   		case INSN_JUMP_CONDITIONAL:
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index cffb23d81782..2bd6d2f46baa 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -35,6 +35,7 @@ struct instruction {
>   	unsigned long immediate;
>   	unsigned int alt_group;
>   	bool dead_end, ignore, hint, save, restore, ignore_alts;
> +	bool intra_function_call;
>   	bool retpoline_safe;
>   	u8 visited;
>   	struct symbol *call_dest;
> 

Thanks,

-- 
Julien Thierry

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

[Alexandre Chartre]

On 4/2/20 2:53 PM, Julien Thierry wrote:
> Hi Alexandre,
> 
> I ran into the limitation of intra-function call for the arm64
> support but didn't take the time to make a clean patch to support
> them properly.
> 
> Nice to see you've gone through that work :) .
> 
> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>> Change objtool to support intra-function calls. An intra-function call
>> is represented in objtool as a push onto the stack (of the return
> 
> I have to disagree a bit with that. The push onto the stack is true
> on x86, but other architectures might not have that (arm/arm64 have a
> link register that gets set by "bl" instructions and do not modify
> the stack).

Correct, this is x86 specific.

> 
>> 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>
>> ---
>>   tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
>>   tools/objtool/check.h |  1 +
>>   2 files changed, 73 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 214809ac2776..0cec91291d46 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -657,6 +657,18 @@ static int add_call_destinations(struct objtool_file *file)
>>           if (insn->type != INSN_CALL)
>>               continue;
>> +        if (insn->intra_function_call) {
>> +            dest_off = insn->offset + insn->len + insn->immediate;
>> +            insn->jump_dest = find_insn(file, insn->sec, dest_off);
>> +            if (insn->jump_dest)
>> +                continue;
>> +
>> +            WARN_FUNC("can't find call dest at %s+0x%lx",
>> +                  insn->sec, insn->offset,
>> +                  insn->sec->name, dest_off);
>> +            return -1;
>> +        }
>> +
>>           rela = find_rela_by_dest_range(insn->sec, insn->offset,
>>                              insn->len);
>>           if (!rela) {
>> @@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct objtool_file *file)
>>       return 0;
>>   }
>> +static int read_intra_function_call(struct objtool_file *file)
>> +{
>> +    struct section *sec;
>> +    struct instruction *insn;
>> +    struct rela *rela;
>> +
>> +    sec = find_section_by_name(file->elf,
>> +                   ".rela.discard.intra_function_call");
> 
> I'm wondering, do we really need to annotate the intra_function_call
> and group the in a section?
> 
> Would it be a problem to consider all (static) call instructions with
> a destination that is not the start offset of a symbol to be an
> intra-function call (and set insn->intra_function_call and
> insn->jump_dest accordingly)?

Correct, we could automatically detect intra-function calls instead of
having to annotate them. However, I choose to annotate them because I don't
think that's not an expected construct in a "normal" code flow (at least
on x86). So objtool would still issue a warning on intra-function calls
by default, and you can annotate them to indicate if they are expected.

If intra-function calls are frequent on arm then I can add an option to
objtool so it automatically detects them. This way, we won't use the option
on x86 and we have to annotate intra-function call on x86, and you can
use it on arm to automatically detect intra-function calls.


> Other than that the logic would stay the same.
> 
>> +    if (!sec)
>> +        return 0;
>> +
>> +    list_for_each_entry(rela, &sec->rela_list, list) {
>> +        if (rela->sym->type != STT_SECTION) {
>> +            WARN("unexpected relocation symbol type in %s",
>> +                 sec->name);
>> +            return -1;
>> +        }
>> +
>> +        insn = find_insn(file, rela->sym->sec, rela->addend);
>> +        if (!insn) {
>> +            WARN("bad .discard.intra_function_call entry");
>> +            return -1;
>> +        }
>> +
>> +        if (insn->type != INSN_CALL) {
>> +            WARN_FUNC("intra_function_call not a call",
>> +                  insn->sec, insn->offset);
> 
> Nit: This could be slightly confusing with INSN_CALL_DYNAMIC. Maybe just:
>      "unsupported instruction for intra-function call " ?

Right, I will change that: "intra_function_call not a direct call"

>> +            return -1;
>> +        }
>> +
>> +        insn->intra_function_call = true;
>> +        /*
>> +         * For the impact on the stack, make an intra-function
>> +         * call behaves like a push of an immediate value (the
>> +         * return address).
>> +         */
>> +        insn->stack_op.src.type = OP_SRC_CONST;
>> +        insn->stack_op.dest.type = OP_DEST_PUSH;
> 
> As commented above, this should be arch dependent.

I will add a arch dependent call. I will also do that for the return
trampoline call case (patch 4).

Thanks,

alex.

>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static void mark_rodata(struct objtool_file *file)
>>   {
>>       struct section *sec;
>> @@ -1344,6 +1399,10 @@ static int decode_sections(struct objtool_file *file)
>>       if (ret)
>>           return ret;
>> +    ret = read_intra_function_call(file);
>> +    if (ret)
>> +        return ret;
>> +
>>       ret = add_call_destinations(file);
>>       if (ret)
>>           return ret;
>> @@ -2092,7 +2151,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>>                   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;
>> @@ -2101,6 +2161,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>>               if (dead_end_function(file, insn->call_dest))
>>                   return 0;
>> +            if (insn->intra_function_call) {
>> +                update_insn_state(insn, &state);
>> +                ret = validate_branch(file, func, insn,
>> +                              insn->jump_dest, state);
>> +                if (ret) {
>> +                    if (backtrace)
>> +                        BT_FUNC("(intra-call)", insn);
>> +                    return ret;
>> +                }
>> +            }
>> +
>>               break;
>>           case INSN_JUMP_CONDITIONAL:
>> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
>> index cffb23d81782..2bd6d2f46baa 100644
>> --- a/tools/objtool/check.h
>> +++ b/tools/objtool/check.h
>> @@ -35,6 +35,7 @@ struct instruction {
>>       unsigned long immediate;
>>       unsigned int alt_group;
>>       bool dead_end, ignore, hint, save, restore, ignore_alts;
>> +    bool intra_function_call;
>>       bool retpoline_safe;
>>       u8 visited;
>>       struct symbol *call_dest;
>>
> 
> Thanks,
> 

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Julien Thierry]

Hi Alexandre,

On 4/2/20 9:22 AM, Alexandre Chartre wrote:
> With retpoline, the return instruction is used to branch to an address
> stored on the stack. So, unlike a regular return instruction, when a
> retpoline return instruction is reached the stack has been modified
> compared to what we have when the function was entered.
> 
> Provide the mechanism to explicitly call-out such return instruction
> so that objtool can correctly handle them.
> 
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> ---
>   tools/objtool/check.c | 78 +++++++++++++++++++++++++++++++++++++++++--
>   tools/objtool/check.h |  1 +
>   2 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 0cec91291d46..ed8e3ea1d8da 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1344,6 +1344,48 @@ static int read_intra_function_call(struct objtool_file *file)
>   	return 0;
>   }
>   
> +static int read_retpoline_ret(struct objtool_file *file)
> +{
> +	struct section *sec;
> +	struct instruction *insn;
> +	struct rela *rela;
> +
> +	sec = find_section_by_name(file->elf, ".rela.discard.retpoline_ret");
> +	if (!sec)
> +		return 0;
> +
> +	list_for_each_entry(rela, &sec->rela_list, list) {
> +		if (rela->sym->type != STT_SECTION) {
> +			WARN("unexpected relocation symbol type in %s",
> +			     sec->name);
> +			return -1;
> +		}
> +
> +		insn = find_insn(file, rela->sym->sec, rela->addend);
> +		if (!insn) {
> +			WARN("bad .discard.retpoline_ret entry");
> +			return -1;
> +		}
> +
> +		if (insn->type != INSN_RETURN) {
> +			WARN_FUNC("retpoline_ret not a return",
> +				  insn->sec, insn->offset);
> +			return -1;
> +		}
> +
> +		insn->retpoline_ret = true;
> +		/*
> +		 * For the impact on the stack, make a return trampoline
> +		 * behaves like a pop of the return address.
> +		 */
> +		insn->stack_op.src.type = OP_SRC_POP;
> +		insn->stack_op.dest.type = OP_DEST_REG;
> +		insn->stack_op.dest.reg = CFI_RA;
> +	}
> +
> +	return 0;
> +}
> +
>   static void mark_rodata(struct objtool_file *file)
>   {
>   	struct section *sec;
> @@ -1403,6 +1445,10 @@ static int decode_sections(struct objtool_file *file)
>   	if (ret)
>   		return ret;
>   
> +	ret = read_retpoline_ret(file);
> +	if (ret)
> +		return ret;
> +
>   	ret = add_call_destinations(file);
>   	if (ret)
>   		return ret;
> @@ -1432,7 +1478,8 @@ static bool is_fentry_call(struct instruction *insn)
>   	return false;
>   }
>   
> -static bool has_modified_stack_frame(struct insn_state *state)
> +static bool has_modified_stack_frame(struct insn_state *state,
> +				     bool check_registers)
>   {
>   	int i;
>   
> @@ -1442,6 +1489,9 @@ static bool has_modified_stack_frame(struct insn_state *state)
>   	    state->drap)
>   		return true;
>   
> +	if (!check_registers)
> +		return false;
> +
>   	for (i = 0; i < CFI_NUM_REGS; i++)
>   		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>   		    state->regs[i].offset != initial_func_cfi.regs[i].offset)
> @@ -1987,7 +2037,7 @@ static int validate_call(struct instruction *insn, struct insn_state *state)
>   
>   static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
>   {
> -	if (has_modified_stack_frame(state)) {
> +	if (has_modified_stack_frame(state, true)) {
>   		WARN_FUNC("sibling call from callable instruction with modified stack frame",
>   				insn->sec, insn->offset);
>   		return 1;
> @@ -2009,6 +2059,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   	struct alternative *alt;
>   	struct instruction *insn, *next_insn;
>   	struct section *sec;
> +	bool check_registers;
>   	u8 visited;
>   	int ret;
>   
> @@ -2130,7 +2181,28 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   				return 1;
>   			}
>   
> -			if (func && has_modified_stack_frame(&state)) {
> +			/*
> +			 * With retpoline, the return instruction is used
> +			 * to branch to an address stored on the stack.
> +			 * So when we reach the ret instruction, the stack
> +			 * frame has been modified with the address to
> +			 * branch to and we need update the stack state.
> +			 *
> +			 * The retpoline address to branch to is typically
> +			 * pushed on the stack from a register, but this
> +			 * confuses the logic which checks callee saved
> +			 * registers. So we don't check if registers have
> +			 * been modified if we have a return trampoline.

I think there are two different things to consider here.

First, the update of the stack frame which I believe should be done when 
returning from intra_function_calls, as it undoes what the call 
instruction did (push more stuff on the stack in the case of x86).

This might mean that intra_function_call should be part of the state (as 
intra_function_calls pass a modified state to validate_branch() ).

Second is supporting retpoline_ret which is just accepting that the 
return address in the stack frame has changed.

> +			 */
> +			if (insn->retpoline_ret) {
> +				update_insn_state(insn, &state);
> +				check_registers = false;
> +			} else {
> +				check_registers = true;
> +			}
> +
> +			if (func && has_modified_stack_frame(&state,
> +							     check_registers)) {
>   				WARN_FUNC("return with modified stack frame",
>   					  sec, insn->offset);
>   				return 1;
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index 2bd6d2f46baa..5ecd16ad71a8 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -37,6 +37,7 @@ struct instruction {
>   	bool dead_end, ignore, hint, save, restore, ignore_alts;
>   	bool intra_function_call;
>   	bool retpoline_safe;
> +	bool retpoline_ret;
>   	u8 visited;
>   	struct symbol *call_dest;
>   	struct instruction *jump_dest;
> 

Cheers,

-- 
Julien Thierry

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

[Julien Thierry]

On 4/2/20 2:24 PM, Alexandre Chartre wrote:
> 
> 
> On 4/2/20 2:53 PM, Julien Thierry wrote:
>> Hi Alexandre,
>>
>> I ran into the limitation of intra-function call for the arm64
>> support but didn't take the time to make a clean patch to support
>> them properly.
>>
>> Nice to see you've gone through that work :) .
>>
>> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>>> Change objtool to support intra-function calls. An intra-function call
>>> is represented in objtool as a push onto the stack (of the return
>>
>> I have to disagree a bit with that. The push onto the stack is true
>> on x86, but other architectures might not have that (arm/arm64 have a
>> link register that gets set by "bl" instructions and do not modify
>> the stack).
> 
> Correct, this is x86 specific.
> 
>>
>>> 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>
>>> ---
>>>   tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
>>>   tools/objtool/check.h |  1 +
>>>   2 files changed, 73 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>>> index 214809ac2776..0cec91291d46 100644
>>> --- a/tools/objtool/check.c
>>> +++ b/tools/objtool/check.c
>>> @@ -657,6 +657,18 @@ static int add_call_destinations(struct 
>>> objtool_file *file)
>>>           if (insn->type != INSN_CALL)
>>>               continue;
>>> +        if (insn->intra_function_call) {
>>> +            dest_off = insn->offset + insn->len + insn->immediate;
>>> +            insn->jump_dest = find_insn(file, insn->sec, dest_off);
>>> +            if (insn->jump_dest)
>>> +                continue;
>>> +
>>> +            WARN_FUNC("can't find call dest at %s+0x%lx",
>>> +                  insn->sec, insn->offset,
>>> +                  insn->sec->name, dest_off);
>>> +            return -1;
>>> +        }
>>> +
>>>           rela = find_rela_by_dest_range(insn->sec, insn->offset,
>>>                              insn->len);
>>>           if (!rela) {
>>> @@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct 
>>> objtool_file *file)
>>>       return 0;
>>>   }
>>> +static int read_intra_function_call(struct objtool_file *file)
>>> +{
>>> +    struct section *sec;
>>> +    struct instruction *insn;
>>> +    struct rela *rela;
>>> +
>>> +    sec = find_section_by_name(file->elf,
>>> +                   ".rela.discard.intra_function_call");
>>
>> I'm wondering, do we really need to annotate the intra_function_call
>> and group the in a section?
>>
>> Would it be a problem to consider all (static) call instructions with
>> a destination that is not the start offset of a symbol to be an
>> intra-function call (and set insn->intra_function_call and
>> insn->jump_dest accordingly)?
> 
> Correct, we could automatically detect intra-function calls instead of
> having to annotate them. However, I choose to annotate them because I don't
> think that's not an expected construct in a "normal" code flow (at least
> on x86). So objtool would still issue a warning on intra-function calls
> by default, and you can annotate them to indicate if they are expected.
> 
> If intra-function calls are frequent on arm then I can add an option to
> objtool so it automatically detects them. This way, we won't use the option
> on x86 and we have to annotate intra-function call on x86, and you can
> use it on arm to automatically detect intra-function calls.
> 

That makes sense. Maybe we can just allow them in !file->c_file, I don't 
think gcc generates such call on arm64, so I think we'd only have that 
in assembly.

If people prefer to keep the annotation, would you mind having a 
"ANNOTATE_INTRA_FUNCTION_CALL" macro in include/linux/frame.h to add the 
label and the reference to the right section?
This way it could be reused for other archs.

> 
>> Other than that the logic would stay the same.
>>
>>> +    if (!sec)
>>> +        return 0;
>>> +
>>> +    list_for_each_entry(rela, &sec->rela_list, list) {
>>> +        if (rela->sym->type != STT_SECTION) {
>>> +            WARN("unexpected relocation symbol type in %s",
>>> +                 sec->name);
>>> +            return -1;
>>> +        }
>>> +
>>> +        insn = find_insn(file, rela->sym->sec, rela->addend);
>>> +        if (!insn) {
>>> +            WARN("bad .discard.intra_function_call entry");
>>> +            return -1;
>>> +        }
>>> +
>>> +        if (insn->type != INSN_CALL) {
>>> +            WARN_FUNC("intra_function_call not a call",
>>> +                  insn->sec, insn->offset);
>>
>> Nit: This could be slightly confusing with INSN_CALL_DYNAMIC. Maybe just:
>>      "unsupported instruction for intra-function call " ?
> 
> Right, I will change that: "intra_function_call not a direct call"
> 
>>> +            return -1;
>>> +        }
>>> +
>>> +        insn->intra_function_call = true;
>>> +        /*
>>> +         * For the impact on the stack, make an intra-function
>>> +         * call behaves like a push of an immediate value (the
>>> +         * return address).
>>> +         */
>>> +        insn->stack_op.src.type = OP_SRC_CONST;
>>> +        insn->stack_op.dest.type = OP_DEST_PUSH;
>>
>> As commented above, this should be arch dependent.
> 
> I will add a arch dependent call. I will also do that for the return
> trampoline call case (patch 4).
> 

Thank you!

-- 
Julien Thierry

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Alexandre Chartre]

On 4/2/20 3:26 PM, Julien Thierry wrote:
> Hi Alexandre,
> 
> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>> With retpoline, the return instruction is used to branch to an address
>> stored on the stack. So, unlike a regular return instruction, when a
>> retpoline return instruction is reached the stack has been modified
>> compared to what we have when the function was entered.
>>
>> Provide the mechanism to explicitly call-out such return instruction
>> so that objtool can correctly handle them.
>>
>> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
>> ---
>>   tools/objtool/check.c | 78 +++++++++++++++++++++++++++++++++++++++++--
>>   tools/objtool/check.h |  1 +
>>   2 files changed, 76 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 0cec91291d46..ed8e3ea1d8da 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -1344,6 +1344,48 @@ static int read_intra_function_call(struct objtool_file *file)
>>       return 0;
>>   }
>> +static int read_retpoline_ret(struct objtool_file *file)
>> +{
>> +    struct section *sec;
>> +    struct instruction *insn;
>> +    struct rela *rela;
>> +
>> +    sec = find_section_by_name(file->elf, ".rela.discard.retpoline_ret");
>> +    if (!sec)
>> +        return 0;
>> +
>> +    list_for_each_entry(rela, &sec->rela_list, list) {
>> +        if (rela->sym->type != STT_SECTION) {
>> +            WARN("unexpected relocation symbol type in %s",
>> +                 sec->name);
>> +            return -1;
>> +        }
>> +
>> +        insn = find_insn(file, rela->sym->sec, rela->addend);
>> +        if (!insn) {
>> +            WARN("bad .discard.retpoline_ret entry");
>> +            return -1;
>> +        }
>> +
>> +        if (insn->type != INSN_RETURN) {
>> +            WARN_FUNC("retpoline_ret not a return",
>> +                  insn->sec, insn->offset);
>> +            return -1;
>> +        }
>> +
>> +        insn->retpoline_ret = true;
>> +        /*
>> +         * For the impact on the stack, make a return trampoline
>> +         * behaves like a pop of the return address.
>> +         */
>> +        insn->stack_op.src.type = OP_SRC_POP;
>> +        insn->stack_op.dest.type = OP_DEST_REG;
>> +        insn->stack_op.dest.reg = CFI_RA;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static void mark_rodata(struct objtool_file *file)
>>   {
>>       struct section *sec;
>> @@ -1403,6 +1445,10 @@ static int decode_sections(struct objtool_file *file)
>>       if (ret)
>>           return ret;
>> +    ret = read_retpoline_ret(file);
>> +    if (ret)
>> +        return ret;
>> +
>>       ret = add_call_destinations(file);
>>       if (ret)
>>           return ret;
>> @@ -1432,7 +1478,8 @@ static bool is_fentry_call(struct instruction *insn)
>>       return false;
>>   }
>> -static bool has_modified_stack_frame(struct insn_state *state)
>> +static bool has_modified_stack_frame(struct insn_state *state,
>> +                     bool check_registers)
>>   {
>>       int i;
>> @@ -1442,6 +1489,9 @@ static bool has_modified_stack_frame(struct insn_state *state)
>>           state->drap)
>>           return true;
>> +    if (!check_registers)
>> +        return false;
>> +
>>       for (i = 0; i < CFI_NUM_REGS; i++)
>>           if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>>               state->regs[i].offset != initial_func_cfi.regs[i].offset)
>> @@ -1987,7 +2037,7 @@ static int validate_call(struct instruction *insn, struct insn_state *state)
>>   static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
>>   {
>> -    if (has_modified_stack_frame(state)) {
>> +    if (has_modified_stack_frame(state, true)) {
>>           WARN_FUNC("sibling call from callable instruction with modified stack frame",
>>                   insn->sec, insn->offset);
>>           return 1;
>> @@ -2009,6 +2059,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>>       struct alternative *alt;
>>       struct instruction *insn, *next_insn;
>>       struct section *sec;
>> +    bool check_registers;
>>       u8 visited;
>>       int ret;
>> @@ -2130,7 +2181,28 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>>                   return 1;
>>               }
>> -            if (func && has_modified_stack_frame(&state)) {
>> +            /*
>> +             * With retpoline, the return instruction is used
>> +             * to branch to an address stored on the stack.
>> +             * So when we reach the ret instruction, the stack
>> +             * frame has been modified with the address to
>> +             * branch to and we need update the stack state.
>> +             *
>> +             * The retpoline address to branch to is typically
>> +             * pushed on the stack from a register, but this
>> +             * confuses the logic which checks callee saved
>> +             * registers. So we don't check if registers have
>> +             * been modified if we have a return trampoline.
> 
> I think there are two different things to consider here.
> 
> First, the update of the stack frame which I believe should be done
> when returning from intra_function_calls, as it undoes what the call
> instruction did (push more stuff on the stack in the case of x86).

The problem is that an intra-function call is not necessarily going
to return. With retpoline (or RSB stuffing) intra-function calls are
basically fake calls only present to fill the RSB buffer. Such calls
won't return, the stack pointer is just adjusted to cancel the impact
of these calls on the stack.

> This might mean that intra_function_call should be part of the state
> (as intra_function_calls pass a modified state to validate_branch()).
> 
> Second is supporting retpoline_ret which is just accepting that the
> return address in the stack frame has changed.
With retpoline_ret, the stack just has an extra address we are going to
jump to (this will be like an indirect jump). If we remove that extra
address from the stack, we should have the regular stack we have at
the end of a function. This is precisely what the code is doing here.

alex.

>> +             */
>> +            if (insn->retpoline_ret) {
>> +                update_insn_state(insn, &state);
>> +                check_registers = false;
>> +            } else {
>> +                check_registers = true;
>> +            }
>> +
>> +            if (func && has_modified_stack_frame(&state,
>> +                                 check_registers)) {
>>                   WARN_FUNC("return with modified stack frame",
>>                         sec, insn->offset);
>>                   return 1;
>> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
>> index 2bd6d2f46baa..5ecd16ad71a8 100644
>> --- a/tools/objtool/check.h
>> +++ b/tools/objtool/check.h
>> @@ -37,6 +37,7 @@ struct instruction {
>>       bool dead_end, ignore, hint, save, restore, ignore_alts;
>>       bool intra_function_call;
>>       bool retpoline_safe;
>> +    bool retpoline_ret;
>>       u8 visited;
>>       struct symbol *call_dest;
>>       struct instruction *jump_dest;
>>
> 
> Cheers,
> 

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

[Alexandre Chartre]

On 4/2/20 3:38 PM, Julien Thierry wrote:
> 
> 
> On 4/2/20 2:24 PM, Alexandre Chartre wrote:
>>
>>
>> On 4/2/20 2:53 PM, Julien Thierry wrote:
>>> Hi Alexandre,
>>>
>>> I ran into the limitation of intra-function call for the arm64
>>> support but didn't take the time to make a clean patch to support
>>> them properly.
>>>
>>> Nice to see you've gone through that work :) .
>>>
>>> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>>>> Change objtool to support intra-function calls. An intra-function call
>>>> is represented in objtool as a push onto the stack (of the return
>>>
>>> I have to disagree a bit with that. The push onto the stack is true
>>> on x86, but other architectures might not have that (arm/arm64 have a
>>> link register that gets set by "bl" instructions and do not modify
>>> the stack).
>>
>> Correct, this is x86 specific.
>>
>>>
>>>> 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>
>>>> ---
>>>>   tools/objtool/check.c | 73 ++++++++++++++++++++++++++++++++++++++++++-
>>>>   tools/objtool/check.h |  1 +
>>>>   2 files changed, 73 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>>>> index 214809ac2776..0cec91291d46 100644
>>>> --- a/tools/objtool/check.c
>>>> +++ b/tools/objtool/check.c
>>>> @@ -657,6 +657,18 @@ static int add_call_destinations(struct objtool_file *file)
>>>>           if (insn->type != INSN_CALL)
>>>>               continue;
>>>> +        if (insn->intra_function_call) {
>>>> +            dest_off = insn->offset + insn->len + insn->immediate;
>>>> +            insn->jump_dest = find_insn(file, insn->sec, dest_off);
>>>> +            if (insn->jump_dest)
>>>> +                continue;
>>>> +
>>>> +            WARN_FUNC("can't find call dest at %s+0x%lx",
>>>> +                  insn->sec, insn->offset,
>>>> +                  insn->sec->name, dest_off);
>>>> +            return -1;
>>>> +        }
>>>> +
>>>>           rela = find_rela_by_dest_range(insn->sec, insn->offset,
>>>>                              insn->len);
>>>>           if (!rela) {
>>>> @@ -1289,6 +1301,49 @@ static int read_retpoline_hints(struct objtool_file *file)
>>>>       return 0;
>>>>   }
>>>> +static int read_intra_function_call(struct objtool_file *file)
>>>> +{
>>>> +    struct section *sec;
>>>> +    struct instruction *insn;
>>>> +    struct rela *rela;
>>>> +
>>>> +    sec = find_section_by_name(file->elf,
>>>> +                   ".rela.discard.intra_function_call");
>>>
>>> I'm wondering, do we really need to annotate the intra_function_call
>>> and group the in a section?
>>>
>>> Would it be a problem to consider all (static) call instructions with
>>> a destination that is not the start offset of a symbol to be an
>>> intra-function call (and set insn->intra_function_call and
>>> insn->jump_dest accordingly)?
>>
>> Correct, we could automatically detect intra-function calls instead of
>> having to annotate them. However, I choose to annotate them because I don't
>> think that's not an expected construct in a "normal" code flow (at least
>> on x86). So objtool would still issue a warning on intra-function calls
>> by default, and you can annotate them to indicate if they are expected.
>>
>> If intra-function calls are frequent on arm then I can add an option to
>> objtool so it automatically detects them. This way, we won't use the option
>> on x86 and we have to annotate intra-function call on x86, and you can
>> use it on arm to automatically detect intra-function calls.
>>
> 
> That makes sense. Maybe we can just allow them in !file->c_file, I
> don't think gcc generates such call on arm64, so I think we'd only
> have that in assembly.

We can have also intra-function call in C file with the asm directive, for
example with retpoline. Actually I think I forgot to check that as this is
only on 32bit on x86.

> If people prefer to keep the annotation, would you mind having a
> "ANNOTATE_INTRA_FUNCTION_CALL" macro in include/linux/frame.h to add
> the label and the reference to the right section?
>
> This way it could be reused for other archs.

Sure, I will do that.

alex.
  
>>
>>> Other than that the logic would stay the same.
>>>
>>>> +    if (!sec)
>>>> +        return 0;
>>>> +
>>>> +    list_for_each_entry(rela, &sec->rela_list, list) {
>>>> +        if (rela->sym->type != STT_SECTION) {
>>>> +            WARN("unexpected relocation symbol type in %s",
>>>> +                 sec->name);
>>>> +            return -1;
>>>> +        }
>>>> +
>>>> +        insn = find_insn(file, rela->sym->sec, rela->addend);
>>>> +        if (!insn) {
>>>> +            WARN("bad .discard.intra_function_call entry");
>>>> +            return -1;
>>>> +        }
>>>> +
>>>> +        if (insn->type != INSN_CALL) {
>>>> +            WARN_FUNC("intra_function_call not a call",
>>>> +                  insn->sec, insn->offset);
>>>
>>> Nit: This could be slightly confusing with INSN_CALL_DYNAMIC. Maybe just:
>>>      "unsupported instruction for intra-function call " ?
>>
>> Right, I will change that: "intra_function_call not a direct call"
>>
>>>> +            return -1;
>>>> +        }
>>>> +
>>>> +        insn->intra_function_call = true;
>>>> +        /*
>>>> +         * For the impact on the stack, make an intra-function
>>>> +         * call behaves like a push of an immediate value (the
>>>> +         * return address).
>>>> +         */
>>>> +        insn->stack_op.src.type = OP_SRC_CONST;
>>>> +        insn->stack_op.dest.type = OP_DEST_PUSH;
>>>
>>> As commented above, this should be arch dependent.
>>
>> I will add a arch dependent call. I will also do that for the return
>> trampoline call case (patch 4).
>>
> 
> Thank you!
> 

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

[Peter Zijlstra]

On Thu, Apr 02, 2020 at 03:24:45PM +0200, Alexandre Chartre wrote:
> On 4/2/20 2:53 PM, Julien Thierry wrote:
> > On 4/2/20 9:22 AM, Alexandre Chartre wrote:

> > > +    sec = find_section_by_name(file->elf,
> > > +                   ".rela.discard.intra_function_call");
> > 
> > I'm wondering, do we really need to annotate the intra_function_call
> > and group the in a section?
> > 
> > Would it be a problem to consider all (static) call instructions with
> > a destination that is not the start offset of a symbol to be an
> > intra-function call (and set insn->intra_function_call and
> > insn->jump_dest accordingly)?
> 
> Correct, we could automatically detect intra-function calls instead of
> having to annotate them. However, I choose to annotate them because I don't
> think that's not an expected construct in a "normal" code flow (at least
> on x86). So objtool would still issue a warning on intra-function calls
> by default, and you can annotate them to indicate if they are expected.

I wondered the same thing when reading the patch. I'm confliected on
this. On the one hand auto-detecting this seems like an excellent idea.

If/when the compiler generates them, they had better be okay too.

Josh?

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Peter Zijlstra]

On Thu, Apr 02, 2020 at 10:22:17AM +0200, Alexandre Chartre wrote:
> With retpoline, the return instruction is used to branch to an address
> stored on the stack. So, unlike a regular return instruction, when a
> retpoline return instruction is reached the stack has been modified
> compared to what we have when the function was entered.
> 
> Provide the mechanism to explicitly call-out such return instruction
> so that objtool can correctly handle them.

https://lkml.kernel.org/r/20200331222703.GH2452@worktop.programming.kicks-ass.net

And also, the split out version:

  https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=core/objtool&id=ec9d9549901dfd2ff411676dfc624e50219e4d5a

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Julien Thierry]

On 4/2/20 3:46 PM, Alexandre Chartre wrote:
> 
> On 4/2/20 3:26 PM, Julien Thierry wrote:
>> Hi Alexandre,
>>
>> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>>> With retpoline, the return instruction is used to branch to an address
>>> stored on the stack. So, unlike a regular return instruction, when a
>>> retpoline return instruction is reached the stack has been modified
>>> compared to what we have when the function was entered.
>>>
>>> Provide the mechanism to explicitly call-out such return instruction
>>> so that objtool can correctly handle them.
>>>
>>> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
>>> ---
>>>   tools/objtool/check.c | 78 +++++++++++++++++++++++++++++++++++++++++--
>>>   tools/objtool/check.h |  1 +
>>>   2 files changed, 76 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>>> index 0cec91291d46..ed8e3ea1d8da 100644
>>> --- a/tools/objtool/check.c
>>> +++ b/tools/objtool/check.c
>>> @@ -1344,6 +1344,48 @@ static int read_intra_function_call(struct 
>>> objtool_file *file)
>>>       return 0;
>>>   }
>>> +static int read_retpoline_ret(struct objtool_file *file)
>>> +{
>>> +    struct section *sec;
>>> +    struct instruction *insn;
>>> +    struct rela *rela;
>>> +
>>> +    sec = find_section_by_name(file->elf, 
>>> ".rela.discard.retpoline_ret");
>>> +    if (!sec)
>>> +        return 0;
>>> +
>>> +    list_for_each_entry(rela, &sec->rela_list, list) {
>>> +        if (rela->sym->type != STT_SECTION) {
>>> +            WARN("unexpected relocation symbol type in %s",
>>> +                 sec->name);
>>> +            return -1;
>>> +        }
>>> +
>>> +        insn = find_insn(file, rela->sym->sec, rela->addend);
>>> +        if (!insn) {
>>> +            WARN("bad .discard.retpoline_ret entry");
>>> +            return -1;
>>> +        }
>>> +
>>> +        if (insn->type != INSN_RETURN) {
>>> +            WARN_FUNC("retpoline_ret not a return",
>>> +                  insn->sec, insn->offset);
>>> +            return -1;
>>> +        }
>>> +
>>> +        insn->retpoline_ret = true;
>>> +        /*
>>> +         * For the impact on the stack, make a return trampoline
>>> +         * behaves like a pop of the return address.
>>> +         */
>>> +        insn->stack_op.src.type = OP_SRC_POP;
>>> +        insn->stack_op.dest.type = OP_DEST_REG;
>>> +        insn->stack_op.dest.reg = CFI_RA;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static void mark_rodata(struct objtool_file *file)
>>>   {
>>>       struct section *sec;
>>> @@ -1403,6 +1445,10 @@ static int decode_sections(struct objtool_file 
>>> *file)
>>>       if (ret)
>>>           return ret;
>>> +    ret = read_retpoline_ret(file);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>>       ret = add_call_destinations(file);
>>>       if (ret)
>>>           return ret;
>>> @@ -1432,7 +1478,8 @@ static bool is_fentry_call(struct instruction 
>>> *insn)
>>>       return false;
>>>   }
>>> -static bool has_modified_stack_frame(struct insn_state *state)
>>> +static bool has_modified_stack_frame(struct insn_state *state,
>>> +                     bool check_registers)
>>>   {
>>>       int i;
>>> @@ -1442,6 +1489,9 @@ static bool has_modified_stack_frame(struct 
>>> insn_state *state)
>>>           state->drap)
>>>           return true;
>>> +    if (!check_registers)
>>> +        return false;
>>> +
>>>       for (i = 0; i < CFI_NUM_REGS; i++)
>>>           if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>>>               state->regs[i].offset != initial_func_cfi.regs[i].offset)
>>> @@ -1987,7 +2037,7 @@ static int validate_call(struct instruction 
>>> *insn, struct insn_state *state)
>>>   static int validate_sibling_call(struct instruction *insn, struct 
>>> insn_state *state)
>>>   {
>>> -    if (has_modified_stack_frame(state)) {
>>> +    if (has_modified_stack_frame(state, true)) {
>>>           WARN_FUNC("sibling call from callable instruction with 
>>> modified stack frame",
>>>                   insn->sec, insn->offset);
>>>           return 1;
>>> @@ -2009,6 +2059,7 @@ static int validate_branch(struct objtool_file 
>>> *file, struct symbol *func,
>>>       struct alternative *alt;
>>>       struct instruction *insn, *next_insn;
>>>       struct section *sec;
>>> +    bool check_registers;
>>>       u8 visited;
>>>       int ret;
>>> @@ -2130,7 +2181,28 @@ static int validate_branch(struct objtool_file 
>>> *file, struct symbol *func,
>>>                   return 1;
>>>               }
>>> -            if (func && has_modified_stack_frame(&state)) {
>>> +            /*
>>> +             * With retpoline, the return instruction is used
>>> +             * to branch to an address stored on the stack.
>>> +             * So when we reach the ret instruction, the stack
>>> +             * frame has been modified with the address to
>>> +             * branch to and we need update the stack state.
>>> +             *
>>> +             * The retpoline address to branch to is typically
>>> +             * pushed on the stack from a register, but this
>>> +             * confuses the logic which checks callee saved
>>> +             * registers. So we don't check if registers have
>>> +             * been modified if we have a return trampoline.
>>
>> I think there are two different things to consider here.
>>
>> First, the update of the stack frame which I believe should be done
>> when returning from intra_function_calls, as it undoes what the call
>> instruction did (push more stuff on the stack in the case of x86).
> 
> The problem is that an intra-function call is not necessarily going
> to return. With retpoline (or RSB stuffing) intra-function calls are
> basically fake calls only present to fill the RSB buffer. Such calls
> won't return, the stack pointer is just adjusted to cancel the impact
> of these calls on the stack.
> 

Right, but running into an intra-function call will start a validate 
branch with a modified stack frame.

So, starting from an intra-function call, if we run into a return, I 
guess objtool will complain about a return with modified stack frame.

My understanding is that once you find an intra-function call, either 
you hit a return, ending the branch, so the return should undo the 
modification the intra-function call did (whether is it a retpoline 
return or not). Otherwise, the intra-function call branch will need to 
reach an end in some way (e.g. hitting a CONTEXT_SWITCH instruction, 
calling a dead_end_function).

Am I missing something?

-- 
Julien Thierry

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Peter Zijlstra]

On Thu, Apr 02, 2020 at 04:31:05PM +0100, Julien Thierry wrote:
> My understanding is that once you find an intra-function call, either you
> hit a return, ending the branch, so the return should undo the modification
> the intra-function call did (whether is it a retpoline return or not).
> Otherwise, the intra-function call branch will need to reach an end in some
> way (e.g. hitting a CONTEXT_SWITCH instruction, calling a
> dead_end_function).
> 
> Am I missing something?

The thing is basically doing:

	mov  $n, cx
1:	call 2f
2:	dec  cx
	jnz  1b
	add  8*n, sp

So it does N calls to self, then subtracts N words from the stack.

The reason being that the CPU has a return-stack-buffer for predicting
returns, and call/ret being naturally paired, that works. The above
is a software flush of the RSB.

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

[Josh Poimboeuf]

On Thu, Apr 02, 2020 at 01:53:49PM +0100, Julien Thierry wrote:
> Hi Alexandre,
> 
> I ran into the limitation of intra-function call for the arm64 support but
> didn't take the time to make a clean patch to support them properly.

Can you give an example of where arm64 uses intra-function calls?  It
sounds sketchy to me :-)  Is it really needed/useful?

-- 
Josh

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

[Josh Poimboeuf]

On Thu, Apr 02, 2020 at 05:04:07PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 03:24:45PM +0200, Alexandre Chartre wrote:
> > On 4/2/20 2:53 PM, Julien Thierry wrote:
> > > On 4/2/20 9:22 AM, Alexandre Chartre wrote:
> 
> > > > +    sec = find_section_by_name(file->elf,
> > > > +                   ".rela.discard.intra_function_call");
> > > 
> > > I'm wondering, do we really need to annotate the intra_function_call
> > > and group the in a section?
> > > 
> > > Would it be a problem to consider all (static) call instructions with
> > > a destination that is not the start offset of a symbol to be an
> > > intra-function call (and set insn->intra_function_call and
> > > insn->jump_dest accordingly)?
> > 
> > Correct, we could automatically detect intra-function calls instead of
> > having to annotate them. However, I choose to annotate them because I don't
> > think that's not an expected construct in a "normal" code flow (at least
> > on x86). So objtool would still issue a warning on intra-function calls
> > by default, and you can annotate them to indicate if they are expected.
> 
> I wondered the same thing when reading the patch. I'm confliected on
> this. On the one hand auto-detecting this seems like an excellent idea.
> 
> If/when the compiler generates them, they had better be okay too.
> 
> Josh?

In general I prefer to keep it simple, and keep the annotations to a
minimum.  And I don't think this warning has ever found anything useful.
So I'd be inclined to say just allow them and automatically detect them.

However the fact that arm64 asm actually uses them worries me a bit.

So for me it kind of hinges on whether arm64 has a legitimate use case
for them, or if the warning actually points to smelly code.

-- 
Josh

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

[Josh Poimboeuf]

On Thu, Apr 02, 2020 at 10:49:19AM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 02, 2020 at 01:53:49PM +0100, Julien Thierry wrote:
> > Hi Alexandre,
> > 
> > I ran into the limitation of intra-function call for the arm64 support but
> > didn't take the time to make a clean patch to support them properly.
> 
> Can you give an example of where arm64 uses intra-function calls?  It
> sounds sketchy to me :-)  Is it really needed/useful?

BTW, I have a vague memory of discussing this before, so I apologize if
I'm repeating myself ;-)

-- 
Josh

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

[Alexandre Chartre]

On 4/2/20 5:54 PM, Josh Poimboeuf wrote:
> On Thu, Apr 02, 2020 at 05:04:07PM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 02, 2020 at 03:24:45PM +0200, Alexandre Chartre wrote:
>>> On 4/2/20 2:53 PM, Julien Thierry wrote:
>>>> On 4/2/20 9:22 AM, Alexandre Chartre wrote:
>>
>>>>> +    sec = find_section_by_name(file->elf,
>>>>> +                   ".rela.discard.intra_function_call");
>>>>
>>>> I'm wondering, do we really need to annotate the intra_function_call
>>>> and group the in a section?
>>>>
>>>> Would it be a problem to consider all (static) call instructions with
>>>> a destination that is not the start offset of a symbol to be an
>>>> intra-function call (and set insn->intra_function_call and
>>>> insn->jump_dest accordingly)?
>>>
>>> Correct, we could automatically detect intra-function calls instead of
>>> having to annotate them. However, I choose to annotate them because I don't
>>> think that's not an expected construct in a "normal" code flow (at least
>>> on x86). So objtool would still issue a warning on intra-function calls
>>> by default, and you can annotate them to indicate if they are expected.
>>
>> I wondered the same thing when reading the patch. I'm confliected on
>> this. On the one hand auto-detecting this seems like an excellent idea.
>>
>> If/when the compiler generates them, they had better be okay too.
>>
>> Josh?
> 
> In general I prefer to keep it simple, and keep the annotations to a
> minimum.  And I don't think this warning has ever found anything useful.
> So I'd be inclined to say just allow them and automatically detect them.
> 
> However the fact that arm64 asm actually uses them worries me a bit.
> 
> So for me it kind of hinges on whether arm64 has a legitimate use case
> for them, or if the warning actually points to smelly code.
> 

Then what I can do is:
- by default, automatically detect and validate intra-function calls
- remove the INTRA_FUNCTION_CALL annotation
- add an objtool option to print a warning about intra-function calls
   (but still validate such calls).

This way by default intra-function calls are automatically detected and
validated. But you still have the option to print them out if you want
to check if there are any intra-function calls.

alex.

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Alexandre Chartre]

On 4/2/20 5:27 PM, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 10:22:17AM +0200, Alexandre Chartre wrote:
>> With retpoline, the return instruction is used to branch to an address
>> stored on the stack. So, unlike a regular return instruction, when a
>> retpoline return instruction is reached the stack has been modified
>> compared to what we have when the function was entered.
>>
>> Provide the mechanism to explicitly call-out such return instruction
>> so that objtool can correctly handle them.
> 
> https://lkml.kernel.org/r/20200331222703.GH2452@worktop.programming.kicks-ass.net
> 
> And also, the split out version:
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=core/objtool&id=ec9d9549901dfd2ff411676dfc624e50219e4d5a
> 

Okay, so I can get rid of my retpoline_ret annotation and use UNWIND_HINT_RET_OFFSET
instead (which is more generic). Basically I can change my RETPOLINE_RET macro to:

.macro RETPOLINE_RET
	UNWIND_HINT_RET_OFFSET
	ret
.endm

Thanks,

alex.

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

[Julien Thierry]

On 4/2/20 4:49 PM, Josh Poimboeuf wrote:
> On Thu, Apr 02, 2020 at 01:53:49PM +0100, Julien Thierry wrote:
>> Hi Alexandre,
>>
>> I ran into the limitation of intra-function call for the arm64 support but
>> didn't take the time to make a clean patch to support them properly.
> 
> Can you give an example of where arm64 uses intra-function calls?  It
> sounds sketchy to me :-)  Is it really needed/useful?
> 

So the most notable/necessary one(s) is the one in tramp_ventry [1]. 
This macro is used as the begining of exception handlers for exceptions 
coming from userland. It was added as part of the mitigations of spectre 
(v1???).

To give some context, x30 is the register that "ret" instruction will 
use as return address, "bl" is the equivalent of x86 "call" and sets x30 
before jumping to the target address. (However, it doesn't have a 
special semantic for exception returns)

Note: I believe the comment about the return "stack" is about processor 
internal state (speculative thingies and all) rather than the actual 
stack, since the stack is untouched by that code. But I don't know the 
actual details.


There are also some in arch/arm64/crypto/crct10dif-ce-core.o , which is 
probably full of fast, smart and optimized code I don't understand :) . 
So I wouldn't feel confident commenting on whether those intra-function 
calls are needed or not.


Last I found is in qcom_link_stack_sanitization() [2], but that's just a 
workaround for a very specific hardware. In my local tree I just put the 
function as STACK_FRAME_NON_STANDARD. But the code just saves the return 
address, has 16 call instructions that just call the instruction after 
them, restores the return address and lets the C-function return 
normally (and it somehow fixes something for that hardware).


Those are the ones I stumbled on. So yes, it a bit sketchy, corner case 
code, but it's there and unlikely to go away.


[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/entry.S?h=v5.6#n803

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/cpu_errata.c?h=v5.6#n195

Cheers,

-- 
Julien Thierry

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Julien Thierry]

On 4/2/20 4:40 PM, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 04:31:05PM +0100, Julien Thierry wrote:
>> My understanding is that once you find an intra-function call, either you
>> hit a return, ending the branch, so the return should undo the modification
>> the intra-function call did (whether is it a retpoline return or not).
>> Otherwise, the intra-function call branch will need to reach an end in some
>> way (e.g. hitting a CONTEXT_SWITCH instruction, calling a
>> dead_end_function).
>>
>> Am I missing something?
> 
> The thing is basically doing:
> 
> 	mov  $n, cx
> 1:	call 2f
> 2:	dec  cx
> 	jnz  1b
> 	add  8*n, sp
> 
> So it does N calls to self, then subtracts N words from the stack.
> 
> The reason being that the CPU has a return-stack-buffer for predicting
> returns, and call/ret being naturally paired, that works. The above
> is a software flush of the RSB.
> 

Ah, lovely... Maybe that's where SAVE/RESTORE unwind hints could be nice 
;) .

Otherwise, I don't really have a good suggestion for this...

-- 
Julien Thierry

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

[Peter Zijlstra]

On Fri, Apr 03, 2020 at 09:01:38AM +0100, Julien Thierry wrote:
> 
> Last I found is in qcom_link_stack_sanitization() [2], but that's just a
> workaround for a very specific hardware. In my local tree I just put the
> function as STACK_FRAME_NON_STANDARD. But the code just saves the return
> address, has 16 call instructions that just call the instruction after them,
> restores the return address and lets the C-function return normally (and it
> somehow fixes something for that hardware).
> 
That sounds very much like the RSB flushing we do.

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

[Julien Thierry]

On 4/3/20 1:41 PM, Peter Zijlstra wrote:
> On Fri, Apr 03, 2020 at 09:01:38AM +0100, Julien Thierry wrote:
>>
>> Last I found is in qcom_link_stack_sanitization() [2], but that's just a
>> workaround for a very specific hardware. In my local tree I just put the
>> function as STACK_FRAME_NON_STANDARD. But the code just saves the return
>> address, has 16 call instructions that just call the instruction after them,
>> restores the return address and lets the C-function return normally (and it
>> somehow fixes something for that hardware).
>>
> That sounds very much like the RSB flushing we do.
> 

Yes, the piece of code you posted reminded me of this. The difference is 
that the RSB part uses a loop and counter while the qcom thing has a 
fixed amount of call instructions (which can make things easier for 
static analysis, if we'd really want to go down that road).

-- 
Julien Thierry

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

[Peter Zijlstra]

On Fri, Apr 03, 2020 at 01:49:24PM +0100, Julien Thierry wrote:
> 
> 
> On 4/3/20 1:41 PM, Peter Zijlstra wrote:
> > On Fri, Apr 03, 2020 at 09:01:38AM +0100, Julien Thierry wrote:
> > > 
> > > Last I found is in qcom_link_stack_sanitization() [2], but that's just a
> > > workaround for a very specific hardware. In my local tree I just put the
> > > function as STACK_FRAME_NON_STANDARD. But the code just saves the return
> > > address, has 16 call instructions that just call the instruction after them,
> > > restores the return address and lets the C-function return normally (and it
> > > somehow fixes something for that hardware).
> > > 
> > That sounds very much like the RSB flushing we do.
> > 
> 
> Yes, the piece of code you posted reminded me of this. The difference is
> that the RSB part uses a loop and counter while the qcom thing has a fixed
> amount of call instructions (which can make things easier for static
> analysis, if we'd really want to go down that road).

We have different depth RSBs for the various uarchs which is what
necessitates the counter. That is, we could always do the max size (32
IIRC) but then, it's expensive and people already complain etc.. etc..

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

[Josh Poimboeuf]

On Fri, Apr 03, 2020 at 09:01:38AM +0100, Julien Thierry wrote:
> 
> 
> On 4/2/20 4:49 PM, Josh Poimboeuf wrote:
> > On Thu, Apr 02, 2020 at 01:53:49PM +0100, Julien Thierry wrote:
> > > Hi Alexandre,
> > > 
> > > I ran into the limitation of intra-function call for the arm64 support but
> > > didn't take the time to make a clean patch to support them properly.
> > 
> > Can you give an example of where arm64 uses intra-function calls?  It
> > sounds sketchy to me :-)  Is it really needed/useful?
> > 
> 
> So the most notable/necessary one(s) is the one in tramp_ventry [1]. This
> macro is used as the begining of exception handlers for exceptions coming
> from userland. It was added as part of the mitigations of spectre (v1???).
> 
> To give some context, x30 is the register that "ret" instruction will use as
> return address, "bl" is the equivalent of x86 "call" and sets x30 before
> jumping to the target address. (However, it doesn't have a special semantic
> for exception returns)
> 
> Note: I believe the comment about the return "stack" is about processor
> internal state (speculative thingies and all) rather than the actual stack,
> since the stack is untouched by that code. But I don't know the actual
> details.

Ok.  So another Spectre special case.

> There are also some in arch/arm64/crypto/crct10dif-ce-core.o , which is
> probably full of fast, smart and optimized code I don't understand :) . So I
> wouldn't feel confident commenting on whether those intra-function calls are
> needed or not.

Glancing at that code, there's a macro which has bl to
.L__pmull_p8_core, which, because it has a local label prefix, doesn't
have an ELF symbol associated with it.  I bet changing that branch to
"bl __pmull_p8_core" (and removing the unnecessary .L__pmull_p8_core
label) would fix the warnings.

So IIUC, this is actually a case where the warning found a cleanup,
albeit a trivial one.

> Last I found is in qcom_link_stack_sanitization() [2], but that's just a
> workaround for a very specific hardware. In my local tree I just put the
> function as STACK_FRAME_NON_STANDARD. But the code just saves the return
> address, has 16 call instructions that just call the instruction after them,
> restores the return address and lets the C-function return normally (and it
> somehow fixes something for that hardware).

Yeah, like Peter said this sounds like x86 RSB stuffing.  More Spectre
nastiness.

So it sounds like the only valid use case for intra-function calls is
Spectre BS...

So, at the risk of possibly contradicting the past version of myself, my
current feeling is that we should annotate such cases, and then warn on
non-annotated cases like the crypto example above.  There's nothing
normal about these Spectre mitigations, so it's ok to add annotations to
such craziness.

-- 
Josh

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Josh Poimboeuf]

On Fri, Apr 03, 2020 at 09:11:55AM +0100, Julien Thierry wrote:
> 
> 
> On 4/2/20 4:40 PM, Peter Zijlstra wrote:
> > On Thu, Apr 02, 2020 at 04:31:05PM +0100, Julien Thierry wrote:
> > > My understanding is that once you find an intra-function call, either you
> > > hit a return, ending the branch, so the return should undo the modification
> > > the intra-function call did (whether is it a retpoline return or not).
> > > Otherwise, the intra-function call branch will need to reach an end in some
> > > way (e.g. hitting a CONTEXT_SWITCH instruction, calling a
> > > dead_end_function).
> > > 
> > > Am I missing something?
> > 
> > The thing is basically doing:
> > 
> > 	mov  $n, cx
> > 1:	call 2f
> > 2:	dec  cx
> > 	jnz  1b
> > 	add  8*n, sp
> > 
> > So it does N calls to self, then subtracts N words from the stack.
> > 
> > The reason being that the CPU has a return-stack-buffer for predicting
> > returns, and call/ret being naturally paired, that works. The above
> > is a software flush of the RSB.
> > 
> 
> Ah, lovely... Maybe that's where SAVE/RESTORE unwind hints could be nice ;)
> .
> 
> Otherwise, I don't really have a good suggestion for this...

Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
work here?

-- 
Josh

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Josh Poimboeuf]

On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 03, 2020 at 09:11:55AM +0100, Julien Thierry wrote:
> > 
> > 
> > On 4/2/20 4:40 PM, Peter Zijlstra wrote:
> > > On Thu, Apr 02, 2020 at 04:31:05PM +0100, Julien Thierry wrote:
> > > > My understanding is that once you find an intra-function call, either you
> > > > hit a return, ending the branch, so the return should undo the modification
> > > > the intra-function call did (whether is it a retpoline return or not).
> > > > Otherwise, the intra-function call branch will need to reach an end in some
> > > > way (e.g. hitting a CONTEXT_SWITCH instruction, calling a
> > > > dead_end_function).
> > > > 
> > > > Am I missing something?
> > > 
> > > The thing is basically doing:
> > > 
> > > 	mov  $n, cx
> > > 1:	call 2f
> > > 2:	dec  cx
> > > 	jnz  1b
> > > 	add  8*n, sp
> > > 
> > > So it does N calls to self, then subtracts N words from the stack.
> > > 
> > > The reason being that the CPU has a return-stack-buffer for predicting
> > > returns, and call/ret being naturally paired, that works. The above
> > > is a software flush of the RSB.
> > > 
> > 
> > Ah, lovely... Maybe that's where SAVE/RESTORE unwind hints could be nice ;)
> > .
> > 
> > Otherwise, I don't really have a good suggestion for this...
> 
> Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
> work here?

And if we're going to need that hint anyway, maybe we could get rid of
the nasty arch_exception_frame_size for the IRET thing and just use a
hint there after all ;-)

-- 
Josh

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Josh Poimboeuf]

On Fri, Apr 03, 2020 at 10:22:14AM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
> > On Fri, Apr 03, 2020 at 09:11:55AM +0100, Julien Thierry wrote:
> > > 
> > > 
> > > On 4/2/20 4:40 PM, Peter Zijlstra wrote:
> > > > On Thu, Apr 02, 2020 at 04:31:05PM +0100, Julien Thierry wrote:
> > > > > My understanding is that once you find an intra-function call, either you
> > > > > hit a return, ending the branch, so the return should undo the modification
> > > > > the intra-function call did (whether is it a retpoline return or not).
> > > > > Otherwise, the intra-function call branch will need to reach an end in some
> > > > > way (e.g. hitting a CONTEXT_SWITCH instruction, calling a
> > > > > dead_end_function).
> > > > > 
> > > > > Am I missing something?
> > > > 
> > > > The thing is basically doing:
> > > > 
> > > > 	mov  $n, cx
> > > > 1:	call 2f
> > > > 2:	dec  cx
> > > > 	jnz  1b
> > > > 	add  8*n, sp
> > > > 
> > > > So it does N calls to self, then subtracts N words from the stack.
> > > > 
> > > > The reason being that the CPU has a return-stack-buffer for predicting
> > > > returns, and call/ret being naturally paired, that works. The above
> > > > is a software flush of the RSB.
> > > > 
> > > 
> > > Ah, lovely... Maybe that's where SAVE/RESTORE unwind hints could be nice ;)
> > > .
> > > 
> > > Otherwise, I don't really have a good suggestion for this...
> > 
> > Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
> > work here?
> 
> And if we're going to need that hint anyway, maybe we could get rid of
> the nasty arch_exception_frame_size for the IRET thing and just use a
> hint there after all ;-)

Actually, never mind -- I guess it wouldn't work because of inconsistent
stack states and all that...

-- 
Josh

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Peter Zijlstra]

On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
> Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
> work here?

Yes, it would.

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Josh Poimboeuf]

On Fri, Apr 03, 2020 at 05:46:20PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
> > Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
> > work here?
> 
> Yes, it would.

Only thing is we'd need to unroll the RSB loop so each instruction has
a deterministic stack size.

-- 
Josh

Re: [PATCH 5/7] x86/speculation: Annotate intra-function calls

[Josh Poimboeuf]

On Thu, Apr 02, 2020 at 10:22:18AM +0200, Alexandre Chartre wrote:
>  .macro RETPOLINE_JMP reg:req
> -	call	.Ldo_rop_\@
> +	INTRA_FUNCTION_CALL .Ldo_rop_\@
>  .Lspec_trap_\@:
>  	pause
>  	lfence
> @@ -102,7 +116,7 @@
>  .Ldo_retpoline_jmp_\@:
>  	RETPOLINE_JMP \reg
>  .Ldo_call_\@:
> -	call	.Ldo_retpoline_jmp_\@
> +	INTRA_FUNCTION_CALL .Ldo_retpoline_jmp_\@
>  .endm

There's a catch: this is part of an alternative.  Which means if
X86_FEATURE_RETPOLINE isn't set at runtime, then the retpoline won't be
there and the ORC data will be wrong.

In fact objtool should probably be made smart enough to warn about this
situation, when an alternative changes the stack state.

The only way I can think of to fix this is to have ORC alternatives :-/

-- 
Josh

Re: [PATCH 5/7] x86/speculation: Annotate intra-function calls

[Josh Poimboeuf]

On Fri, Apr 03, 2020 at 11:05:38AM -0500, Josh Poimboeuf wrote:
> On Thu, Apr 02, 2020 at 10:22:18AM +0200, Alexandre Chartre wrote:
> >  .macro RETPOLINE_JMP reg:req
> > -	call	.Ldo_rop_\@
> > +	INTRA_FUNCTION_CALL .Ldo_rop_\@
> >  .Lspec_trap_\@:
> >  	pause
> >  	lfence
> > @@ -102,7 +116,7 @@
> >  .Ldo_retpoline_jmp_\@:
> >  	RETPOLINE_JMP \reg
> >  .Ldo_call_\@:
> > -	call	.Ldo_retpoline_jmp_\@
> > +	INTRA_FUNCTION_CALL .Ldo_retpoline_jmp_\@
> >  .endm
> 
> There's a catch: this is part of an alternative.  Which means if
> X86_FEATURE_RETPOLINE isn't set at runtime, then the retpoline won't be
> there and the ORC data will be wrong.
> 
> In fact objtool should probably be made smart enough to warn about this
> situation, when an alternative changes the stack state.
> 
> The only way I can think of to fix this is to have ORC alternatives :-/

Or they could be converted to use static branches instead of
alternatives.

-- 
Josh

Re: [PATCH 5/7] x86/speculation: Annotate intra-function calls

[Alexandre Chartre]

On 4/3/20 6:16 PM, Josh Poimboeuf wrote:
> On Fri, Apr 03, 2020 at 11:05:38AM -0500, Josh Poimboeuf wrote:
>> On Thu, Apr 02, 2020 at 10:22:18AM +0200, Alexandre Chartre wrote:
>>>   .macro RETPOLINE_JMP reg:req
>>> -	call	.Ldo_rop_\@
>>> +	INTRA_FUNCTION_CALL .Ldo_rop_\@
>>>   .Lspec_trap_\@:
>>>   	pause
>>>   	lfence
>>> @@ -102,7 +116,7 @@
>>>   .Ldo_retpoline_jmp_\@:
>>>   	RETPOLINE_JMP \reg
>>>   .Ldo_call_\@:
>>> -	call	.Ldo_retpoline_jmp_\@
>>> +	INTRA_FUNCTION_CALL .Ldo_retpoline_jmp_\@
>>>   .endm
>>
>> There's a catch: this is part of an alternative.  Which means if
>> X86_FEATURE_RETPOLINE isn't set at runtime, then the retpoline won't be
>> there and the ORC data will be wrong.
>>
>> In fact objtool should probably be made smart enough to warn about this
>> situation, when an alternative changes the stack state.
>>
>> The only way I can think of to fix this is to have ORC alternatives :-/

So that means that any alternative that does a stack manipulation isn't
currently supported?

alex.

> Or they could be converted to use static branches instead of
> alternatives.
> 

Re: [PATCH 5/7] x86/speculation: Annotate intra-function calls

[Peter Zijlstra]

On Fri, Apr 03, 2020 at 10:14:49AM -0700, Alexandre Chartre wrote:

> So that means that any alternative that does a stack manipulation isn't
> currently supported?

It's fundamentally impossible to correctly unwind through.

Instructions before and after the alternative need to have the same
stack layout irrespective of the alternative chosen.

What we need in this case though is only a different stack layout inside
the alternative, and that is doable.

Re: [PATCH 5/7] x86/speculation: Annotate intra-function calls

[Josh Poimboeuf]

On Fri, Apr 03, 2020 at 07:18:36PM +0200, Peter Zijlstra wrote:
> What we need in this case though is only a different stack layout inside
> the alternative, and that is doable.

I'm not sure what you mean... any stack changes within the alternative
have to match exactly the stack changes at the same offsets of the
original code because ORC doesn't know the difference between the two.

-- 
Josh

Re: [PATCH 5/7] x86/speculation: Annotate intra-function calls

[Peter Zijlstra]

On Fri, Apr 03, 2020 at 12:24:08PM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 03, 2020 at 07:18:36PM +0200, Peter Zijlstra wrote:
> > What we need in this case though is only a different stack layout inside
> > the alternative, and that is doable.
> 
> I'm not sure what you mean... any stack changes within the alternative
> have to match exactly the stack changes at the same offsets of the
> original code because ORC doesn't know the difference between the two.

I mean that the ORC entries on either side of the alternative have to be
invariant wrt the specific alternative chosen, but that -- provded you
whip up that ORC alternative stuff -- stack layout inside of an
alternative could possibly be different.

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Peter Zijlstra]

On Fri, Apr 03, 2020 at 05:46:20PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
> > Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
> > work here?
> 
> Yes, it would.

Sorry, I have reconsidered. While it will shut up objtool, it will not
'work'. That is, the ORC data generated will not correctly unwind.

I'll try and write a longer email tonight.

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Josh Poimboeuf]

On Sat, Apr 04, 2020 at 03:32:18PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 03, 2020 at 05:46:20PM +0200, Peter Zijlstra wrote:
> > On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
> > > Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
> > > work here?
> > 
> > Yes, it would.
> 
> Sorry, I have reconsidered. While it will shut up objtool, it will not
> 'work'. That is, the ORC data generated will not correctly unwind.
> 
> I'll try and write a longer email tonight.

Right, that's what I've been trying to say.  The ORC data will be
non-deterministic unless we unroll the loop.  Or did you mean something
else?

-- 
Josh

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Peter Zijlstra]

On Sat, Apr 04, 2020 at 09:22:32AM -0500, Josh Poimboeuf wrote:
> On Sat, Apr 04, 2020 at 03:32:18PM +0200, Peter Zijlstra wrote:
> > On Fri, Apr 03, 2020 at 05:46:20PM +0200, Peter Zijlstra wrote:
> > > On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
> > > > Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
> > > > work here?
> > > 
> > > Yes, it would.
> > 
> > Sorry, I have reconsidered. While it will shut up objtool, it will not
> > 'work'. That is, the ORC data generated will not correctly unwind.
> > 
> > I'll try and write a longer email tonight.
> 
> Right, that's what I've been trying to say.  The ORC data will be
> non-deterministic unless we unroll the loop.  Or did you mean something
> else?

The below should result in deterministic code.

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 07e95dcb40ad..109ee65f4a11 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -59,8 +59,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: [PATCH 4/7] objtool: Add support for return trampoline call

[Alexandre Chartre]

On 4/4/20 5:51 PM, Peter Zijlstra wrote:
> On Sat, Apr 04, 2020 at 09:22:32AM -0500, Josh Poimboeuf wrote:
>> On Sat, Apr 04, 2020 at 03:32:18PM +0200, Peter Zijlstra wrote:
>>> On Fri, Apr 03, 2020 at 05:46:20PM +0200, Peter Zijlstra wrote:
>>>> On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
>>>>> Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
>>>>> work here?
>>>>
>>>> Yes, it would.
>>>
>>> Sorry, I have reconsidered. While it will shut up objtool, it will not
>>> 'work'. That is, the ORC data generated will not correctly unwind.
>>>
>>> I'll try and write a longer email tonight.
>>
>> Right, that's what I've been trying to say.  The ORC data will be
>> non-deterministic unless we unroll the loop.  Or did you mean something
>> else?
> 
> The below should result in deterministic code.
> 
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index 07e95dcb40ad..109ee65f4a11 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -59,8 +59,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__

Nice. This works fine and allows to remove ANNOTATE_NOSPEC_ALTERNATIVE when
using __FILL_RETURN_BUFFER. However this is probably less performant because
we now have nr/2 add instructions instead of just 1.

Here is a variant where I unroll half of the loop. This way we have 2
add+dec+jnz instruction instructions instead of nr/2 dec+jnz and 1 add
instruction.

#define __FILL_RETURN_BUFFER(reg, nr, sp)	\
	mov	$1, reg;			\
771:						\
	.rept (nr/2);				\
	call	772f;				\
773:	/* speculation trap */			\
	pause;					\
	lfence;					\
	jmp	773b;				\
772:	;					\
	.endr;					\
	add	$(BITS_PER_LONG/8) * (nr/2), sp; \
	dec	reg;				\
	jnz	771b;


Note that we can't unroll the entire loop: it won't work with nr=32 because
the code is then too large to fit into an alternative (the alternative size
is encoded on only one byte so this allows a maximum size of 255, while with
nr=32 the size is around 390).

alex.

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Peter Zijlstra]

On Mon, Apr 06, 2020 at 10:19:56AM +0200, Alexandre Chartre wrote:
> 
> On 4/4/20 5:51 PM, Peter Zijlstra wrote:
> > On Sat, Apr 04, 2020 at 09:22:32AM -0500, Josh Poimboeuf wrote:
> > > On Sat, Apr 04, 2020 at 03:32:18PM +0200, Peter Zijlstra wrote:
> > > > On Fri, Apr 03, 2020 at 05:46:20PM +0200, Peter Zijlstra wrote:
> > > > > On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
> > > > > > Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
> > > > > > work here?
> > > > > 
> > > > > Yes, it would.
> > > > 
> > > > Sorry, I have reconsidered. While it will shut up objtool, it will not
> > > > 'work'. That is, the ORC data generated will not correctly unwind.
> > > > 
> > > > I'll try and write a longer email tonight.
> > > 
> > > Right, that's what I've been trying to say.  The ORC data will be
> > > non-deterministic unless we unroll the loop.  Or did you mean something
> > > else?
> > 
> > The below should result in deterministic code.
> > 
> > diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> > index 07e95dcb40ad..109ee65f4a11 100644
> > --- a/arch/x86/include/asm/nospec-branch.h
> > +++ b/arch/x86/include/asm/nospec-branch.h
> > @@ -59,8 +59,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__
> 
> Nice. This works fine and allows to remove ANNOTATE_NOSPEC_ALTERNATIVE when
> using __FILL_RETURN_BUFFER. However this is probably less performant because
> we now have nr/2 add instructions instead of just 1.

Does it actually matter though? That is, can you measure the difference?

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Alexandre Chartre]

On 4/6/20 11:31 AM, Peter Zijlstra wrote:
> On Mon, Apr 06, 2020 at 10:19:56AM +0200, Alexandre Chartre wrote:
>>
>> On 4/4/20 5:51 PM, Peter Zijlstra wrote:
>>> On Sat, Apr 04, 2020 at 09:22:32AM -0500, Josh Poimboeuf wrote:
>>>> On Sat, Apr 04, 2020 at 03:32:18PM +0200, Peter Zijlstra wrote:
>>>>> On Fri, Apr 03, 2020 at 05:46:20PM +0200, Peter Zijlstra wrote:
>>>>>> On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
>>>>>>> Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
>>>>>>> work here?
>>>>>>
>>>>>> Yes, it would.
>>>>>
>>>>> Sorry, I have reconsidered. While it will shut up objtool, it will not
>>>>> 'work'. That is, the ORC data generated will not correctly unwind.
>>>>>
>>>>> I'll try and write a longer email tonight.
>>>>
>>>> Right, that's what I've been trying to say.  The ORC data will be
>>>> non-deterministic unless we unroll the loop.  Or did you mean something
>>>> else?
>>>
>>> The below should result in deterministic code.
>>>
>>> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
>>> index 07e95dcb40ad..109ee65f4a11 100644
>>> --- a/arch/x86/include/asm/nospec-branch.h
>>> +++ b/arch/x86/include/asm/nospec-branch.h
>>> @@ -59,8 +59,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__
>>
>> Nice. This works fine and allows to remove ANNOTATE_NOSPEC_ALTERNATIVE when
>> using __FILL_RETURN_BUFFER. However this is probably less performant because
>> we now have nr/2 add instructions instead of just 1.
> 
> Does it actually matter though? That is, can you measure the difference?
> 

I didn't do any measurement, I am just anticipating concerns others might have
as this code is used during context and vmexit. But I agree this might not be
significant.

alex.

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Josh Poimboeuf]

On Sat, Apr 04, 2020 at 05:51:26PM +0200, Peter Zijlstra wrote:
> On Sat, Apr 04, 2020 at 09:22:32AM -0500, Josh Poimboeuf wrote:
> > On Sat, Apr 04, 2020 at 03:32:18PM +0200, Peter Zijlstra wrote:
> > > On Fri, Apr 03, 2020 at 05:46:20PM +0200, Peter Zijlstra wrote:
> > > > On Fri, Apr 03, 2020 at 10:17:57AM -0500, Josh Poimboeuf wrote:
> > > > > Peter, I think my previous idea for UNWIND_HINT_ADJUST stack_add=8 would
> > > > > work here?
> > > > 
> > > > Yes, it would.
> > > 
> > > Sorry, I have reconsidered. While it will shut up objtool, it will not
> > > 'work'. That is, the ORC data generated will not correctly unwind.
> > > 
> > > I'll try and write a longer email tonight.
> > 
> > Right, that's what I've been trying to say.  The ORC data will be
> > non-deterministic unless we unroll the loop.  Or did you mean something
> > else?
> 
> The below should result in deterministic code.
> 
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index 07e95dcb40ad..109ee65f4a11 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -59,8 +59,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__

That should work, though we should make sure it doesn't break the RSB
clearing -- I'm pretty sure it wouldn't, but you never know...

-- 
Josh

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Alexandre Chartre]

On 4/2/20 5:27 PM, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 10:22:17AM +0200, Alexandre Chartre wrote:
>> With retpoline, the return instruction is used to branch to an address
>> stored on the stack. So, unlike a regular return instruction, when a
>> retpoline return instruction is reached the stack has been modified
>> compared to what we have when the function was entered.
>>
>> Provide the mechanism to explicitly call-out such return instruction
>> so that objtool can correctly handle them.
> 
> https://lkml.kernel.org/r/20200331222703.GH2452@worktop.programming.kicks-ass.net
> 
> And also, the split out version:
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=core/objtool&id=ec9d9549901dfd2ff411676dfc624e50219e4d5a
> 

HINT_RET_OFFSET works fine when an immediate value is pushed on the
stack. However if the value is pushed from a callee-saved register
(%rbp, %rbx, %r12-%r15) then we still have a "return with modified
stack frame" warning. That's because objtool checks callee-saved
registers pushed/popped on the stack, and we have retpoline functions
built for each register (see arch/x86/lib/retpoline.S)

So that's why I also added a bool to has_modified_stack_frame() to
no check registers:

@@ -1432,7 +1478,8 @@ static bool is_fentry_call(struct instruction *insn)
  	return false;
  }
  
-static bool has_modified_stack_frame(struct insn_state *state)
+static bool has_modified_stack_frame(struct insn_state *state,
+				     bool check_registers)
  {
  	int i;
  
@@ -1442,6 +1489,9 @@ static bool has_modified_stack_frame(struct insn_state *state)
  	    state->drap)
  		return true;
  
+	if (!check_registers)
+		return false;
+
  	for (i = 0; i < CFI_NUM_REGS; i++)
  		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
  		    state->regs[i].offset != initial_func_cfi.regs[i].offset)


alex.

Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Alexandre Chartre]

On 4/6/20 4:34 PM, Alexandre Chartre wrote:
> 
> On 4/2/20 5:27 PM, Peter Zijlstra wrote:
>> On Thu, Apr 02, 2020 at 10:22:17AM +0200, Alexandre Chartre wrote:
>>> With retpoline, the return instruction is used to branch to an address
>>> stored on the stack. So, unlike a regular return instruction, when a
>>> retpoline return instruction is reached the stack has been modified
>>> compared to what we have when the function was entered.
>>>
>>> Provide the mechanism to explicitly call-out such return instruction
>>> so that objtool can correctly handle them.
>>
>> https://lkml.kernel.org/r/20200331222703.GH2452@worktop.programming.kicks-ass.net
>>
>> And also, the split out version:
>>
>>    https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=core/objtool&id=ec9d9549901dfd2ff411676dfc624e50219e4d5a
>>
> 
> HINT_RET_OFFSET works fine when an immediate value is pushed on the
> stack. However if the value is pushed from a callee-saved register
> (%rbp, %rbx, %r12-%r15) then we still have a "return with modified
> stack frame" warning. That's because objtool checks callee-saved
> registers pushed/popped on the stack, and we have retpoline functions
> built for each register (see arch/x86/lib/retpoline.S)
> 
> So that's why I also added a bool to has_modified_stack_frame() to
> no check registers:
> 
> @@ -1432,7 +1478,8 @@ static bool is_fentry_call(struct instruction *insn)
>       return false;
>   }
> 
> -static bool has_modified_stack_frame(struct insn_state *state)
> +static bool has_modified_stack_frame(struct insn_state *state,
> +                     bool check_registers)
>   {
>       int i;
> 
> @@ -1442,6 +1489,9 @@ static bool has_modified_stack_frame(struct insn_state *state)
>           state->drap)
>           return true;
> 
> +    if (!check_registers)
> +        return false;
> +
>       for (i = 0; i < CFI_NUM_REGS; i++)
>           if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>               state->regs[i].offset != initial_func_cfi.regs[i].offset)
> 
> 

Here is a simple change on top of the UNWIND_HINT_RET_OFFSET patch to prevent
this problem:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index dbb2b2187037..97db8f49e06f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1390,6 +1390,14 @@ static bool has_modified_stack_frame(struct instruction *insn,
         if (state->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 (state->regs[i].base != initial_func_cfi.regs[i].base ||
                     state->regs[i].offset != initial_func_cfi.regs[i].offset)


alex.