[PATCH V3 0/9] objtool changes to check retpoline code

[PATCH V3 0/9] objtool changes to check retpoline code

[Alexandre Chartre]

Hi,

This is version 3 of the patchset previously named "objtool changes to
remove all ANNOTATE_NOSPEC_ALTERNATIVE". This patchset proposes two
main changes:

 1. Add intra-function call support to objtool. This allows objtool
    to check code like retpoline and RSB stuffing. Such code is present
    in alternative and it is not currently checked because of
    ANNOTATE_NOSPEC_ALTERNATIVE directives.

 2. Add alternative code validation in objtool. For stack unwinding to
    work, Peter and Josh have clearly explained that alternative code
    should have the same stack change sequence as the original code.
    This patchset adds this capability to objtool.

After changes 1, we could remove the ANNOTATE_SPEC_ALTERNATIVE directives
from retpoline/RSB alternatives, and alternatives would be correclty checked
by objtool. But, because of changes 2, objtool would now report inconsistency
in alternatives, like this:

  AS      arch/x86/lib/retpoline.o
arch/x86/lib/retpoline.o: warning: objtool: __x86_indirect_thunk_rax()+0x0: error in alternative
arch/x86/lib/retpoline.o: warning: objtool: .altinstr_replacement+0x0: in alternative 1
arch/x86/lib/retpoline.o: warning: objtool: .altinstr_replacement+0x5: misaligned alternative state change
arch/x86/lib/retpoline.o: warning: objtool: .altinstr_replacement+0x11: in alternative 2
arch/x86/lib/retpoline.o: warning: objtool: .altinstr_replacement+0x14: misaligned alternative state change

So this pachset doesn't remove ANNOTATE_NOSPEC_ALTERNATIVE directives
(unlike v1 and v2). But it makes objtool able to detect inconsistent
alternatives. Then such alternative will need to be refactored to
have stack unwinding information compatible with the original code.
For example, here is Peter suggestion for retpoline code:
  https://lkml.org/lkml/2020/4/8/905


Changes:

v2->v3:
 - rebase on v5.7-rc1
 - add alternative code validation in objtool
 - add return address unwind hints
 - track return address to correctly handle ret with intra-function call
 - remove inclusion of PeterZ UNWIND_HINT_RET_OFFSET patch
 - move alt_group changes to appropriate patch
 - move stack changes for calls to INSN_CALL decode

v1->v2:
 - replace RETPOLINE_RET with PeterZ UNWIND_HINT_RET_OFFSET
 - make objtool intra-function call action architecture dependent
 - objtool now automatically detects and validates all intra-function
   calls but it issues a warning if the call was not explicitly tagged
 - change __FILL_RETURN_BUFFER to work with objtool
 - add generic ANNOTATE_INTRA_FUNCTION_CALL macro
 - remove all ANNOTATE_SPEC_ALTERNATIVE (even for __FILL_RETURN_BUFFER)

Thanks,

alex.

-----

Alexandre Chartre (9):
  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: Handle return instruction with intra-function call
  objtool: Add return address unwind hints
  objtool: Report inconsistent stack changes in alternative
  x86/speculation: Change __FILL_RETURN_BUFFER to work with objtool
  x86/speculation: Add return address unwind hints to retpoline and RSB
    stuffing
  x86/speculation: Annotate intra-function calls

 arch/x86/include/asm/nospec-branch.h          |  42 +-
 arch/x86/include/asm/orc_types.h              |   2 +
 arch/x86/include/asm/unwind_hints.h           |  23 +
 include/linux/frame.h                         |  11 +
 tools/arch/x86/include/asm/orc_types.h        |   2 +
 .../Documentation/stack-validation.txt        |   8 +
 tools/objtool/arch/x86/decode.c               |  13 +
 tools/objtool/check.c                         | 480 ++++++++++++++++--
 tools/objtool/check.h                         |   8 +-
 9 files changed, 545 insertions(+), 44 deletions(-)

-- 
2.18.2

[PATCH V3 1/9] 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 8dd01f986fbb..bd25c3ad651a 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1409,7 +1409,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 V3 2/9] 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 | 28 ++++++++++++++++++++++------
 tools/objtool/check.h |  4 +++-
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index bd25c3ad651a..8f0525d5d895 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -731,7 +731,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;
@@ -740,7 +742,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;
 	}
 
@@ -773,6 +775,7 @@ static int handle_group_alt(struct objtool_file *file,
 	}
 
 	last_new_insn = NULL;
+	alt_group = alt_group_next_index++;
 	insn = *new_insn;
 	sec_for_each_insn_from(file, insn) {
 		if (insn->offset >= special_alt->new_off + special_alt->new_len)
@@ -782,6 +785,7 @@ static int handle_group_alt(struct objtool_file *file,
 
 		insn->ignore = orig_insn->ignore_alts;
 		insn->func = orig_insn->func;
+		insn->alt_group = alt_group;
 
 		/*
 		 * Since alternative replacement code is copy/pasted by the
@@ -2016,6 +2020,15 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
 	return 0;
 }
 
+static bool is_branch_to_alternative(struct instruction *from,
+				     struct instruction *to)
+{
+	if (!from || !to->alt_group || !list_empty(&to->alts))
+		return false;
+
+	return (from->alt_group != to->alt_group);
+}
+
 /*
  * Follow the branch starting at the given instruction, and recursively follow
  * any other branches (jumps).  Meanwhile, track the frame pointer state at
@@ -2023,6 +2036,7 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
  * 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;
@@ -2034,7 +2048,7 @@ 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 (is_branch_to_alternative(from, insn)) {
 		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
 			  sec, insn->offset);
 		return 1;
@@ -2116,7 +2130,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);
@@ -2159,7 +2174,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)
@@ -2290,7 +2305,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;
@@ -2429,7 +2445,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
 
 		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 f0ce8ffe7135..953db38dfc35 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,7 +33,9 @@ struct instruction {
 	unsigned int len;
 	enum insn_type type;
 	unsigned long immediate;
-	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+	int alt_group;
+	bool dead_end, ignore, ignore_alts;
+	bool hint, save, restore;
 	bool retpoline_safe;
 	u8 visited;
 	struct symbol *call_dest;
-- 
2.18.2

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

[Alexandre Chartre]

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

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 include/linux/frame.h                         |  11 ++
 .../Documentation/stack-validation.txt        |   8 ++
 tools/objtool/arch/x86/decode.c               |   6 +
 tools/objtool/check.c                         | 126 ++++++++++++++----
 tools/objtool/check.h                         |   1 +
 5 files changed, 128 insertions(+), 24 deletions(-)

diff --git a/include/linux/frame.h b/include/linux/frame.h
index 02d3ca2d9598..c7178e6c9d48 100644
--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -15,9 +15,20 @@
 	static void __used __section(.discard.func_stack_frame_non_standard) \
 		*__func_stack_frame_non_standard_##func = func
 
+/*
+ * This macro indicates that the following intra-function call is valid.
+ * Any non-annotated intra-function call will cause objtool to issue warning.
+ */
+#define ANNOTATE_INTRA_FUNCTION_CALL				\
+	999:							\
+	.pushsection .discard.intra_function_call;		\
+	.long 999b;						\
+	.popsection;
+
 #else /* !CONFIG_STACK_VALIDATION */
 
 #define STACK_FRAME_NON_STANDARD(func)
+#define ANNOTATE_INTRA_FUNCTION_CALL
 
 #endif /* CONFIG_STACK_VALIDATION */
 
diff --git a/tools/objtool/Documentation/stack-validation.txt b/tools/objtool/Documentation/stack-validation.txt
index de094670050b..09f863fd32d2 100644
--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/stack-validation.txt
@@ -290,6 +290,14 @@ they mean, and suggestions for how to fix them.
       https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
 
 
+9. file.o: warning: unsupported intra-function call
+
+   This warning means that a direct call is done to a destination which
+   is not at the beginning of a function. If this is a legit call, you
+   can remove this warning by putting the ANNOTATE_INTRA_FUNCTION_CALL
+   directive right before the call.
+
+
 If the error doesn't seem to make sense, it could be a bug in objtool.
 Feel free to ask the objtool maintainer for help.
 
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index a62e032863a8..f4d70b8835c4 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -437,6 +437,12 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 
 	case 0xe8:
 		*type = INSN_CALL;
+		/*
+		 * For the impact on the stack, a call behaves like
+		 * a push of an immediate value (the return address).
+		 */
+		op->src.type = OP_SRC_CONST;
+		op->dest.type = OP_DEST_PUSH;
 		break;
 
 	case 0xfc:
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 8f0525d5d895..ad362c5de281 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -654,44 +654,58 @@ static int add_jump_destinations(struct objtool_file *file)
 	return 0;
 }
 
+static int configure_call(struct objtool_file *file, struct instruction *insn)
+{
+	unsigned long dest_off;
+
+	dest_off = insn->offset + insn->len + insn->immediate;
+	insn->call_dest = find_func_by_offset(insn->sec, dest_off);
+	if (!insn->call_dest)
+		insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
+
+	if (insn->call_dest) {
+		/* regular call */
+		if (insn->func && insn->call_dest->type != STT_FUNC) {
+			WARN_FUNC("unsupported call to non-function",
+				  insn->sec, insn->offset);
+			return -1;
+		}
+		return 0;
+	}
+
+	/* intra-function call */
+	if (!insn->intra_function_call)
+		WARN_FUNC("intra-function call", insn->sec, insn->offset);
+
+	dest_off = insn->offset + insn->len + insn->immediate;
+	insn->jump_dest = find_insn(file, insn->sec, dest_off);
+	if (!insn->jump_dest) {
+		WARN_FUNC("can't find call dest at %s+0x%lx",
+			  insn->sec, insn->offset,
+			  insn->sec->name, dest_off);
+		return -1;
+	}
+
+	return 0;
+}
+
 /*
  * Find the destination instructions for all calls.
  */
 static int add_call_destinations(struct objtool_file *file)
 {
 	struct instruction *insn;
-	unsigned long dest_off;
 	struct rela *rela;
 
 	for_each_insn(file, insn) {
-		if (insn->type != INSN_CALL)
+		if (insn->type != INSN_CALL || insn->ignore)
 			continue;
 
 		rela = find_rela_by_dest_range(file->elf, insn->sec,
 					       insn->offset, insn->len);
 		if (!rela) {
-			dest_off = insn->offset + insn->len + insn->immediate;
-			insn->call_dest = find_func_by_offset(insn->sec, dest_off);
-			if (!insn->call_dest)
-				insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
-
-			if (insn->ignore)
-				continue;
-
-			if (!insn->call_dest) {
-				WARN_FUNC("unsupported intra-function call",
-					  insn->sec, insn->offset);
-				if (retpoline)
-					WARN("If this is a retpoline, please patch it in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE.");
-				return -1;
-			}
-
-			if (insn->func && insn->call_dest->type != STT_FUNC) {
-				WARN_FUNC("unsupported call to non-function",
-					  insn->sec, insn->offset);
+			if (configure_call(file, insn))
 				return -1;
-			}
-
 		} else if (rela->sym->type == STT_SECTION) {
 			insn->call_dest = find_func_by_offset(rela->sym->sec,
 							      rela->addend+4);
@@ -1337,6 +1351,42 @@ 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 direct call",
+				  insn->sec, insn->offset);
+			return -1;
+		}
+
+		insn->intra_function_call = true;
+	}
+
+	return 0;
+}
+
 static void mark_rodata(struct objtool_file *file)
 {
 	struct section *sec;
@@ -1392,6 +1442,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;
@@ -2155,7 +2209,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;
@@ -2164,6 +2219,29 @@ 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) {
+				/*
+				 * The call instruction can update the stack
+				 * state. Then make the intra-function call
+				 * behaves like and unconditional jump.
+				 */
+				ret = update_insn_state(insn, &state);
+				if (ret)
+					return ret;
+
+				ret = validate_branch(file, func, insn,
+						      insn->jump_dest, state);
+				if (ret) {
+					if (backtrace) {
+						BT_FUNC("(intra-function call)",
+							insn);
+					}
+					return ret;
+				}
+
+				return 0;
+			}
+
 			break;
 
 		case INSN_JUMP_CONDITIONAL:
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 953db38dfc35..6a80903fc4aa 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -36,6 +36,7 @@ struct instruction {
 	int alt_group;
 	bool dead_end, ignore, ignore_alts;
 	bool hint, save, restore;
+	bool intra_function_call;
 	bool retpoline_safe;
 	u8 visited;
 	struct symbol *call_dest;
-- 
2.18.2

[PATCH V3 4/9] objtool: Handle return instruction with intra-function call

[Alexandre Chartre]

Intra-function calls are implemented in objtool like unconditional
jumps. Keep track of intra-functin calls return addresses so that
objtool can make a return instruction continues the flow at the
right location.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 tools/objtool/arch/x86/decode.c |   7 +++
 tools/objtool/check.c           | 104 ++++++++++++++++++++++++++++++--
 tools/objtool/check.h           |   3 +
 3 files changed, 110 insertions(+), 4 deletions(-)

diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index f4d70b8835c4..76b593bb2e4f 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -427,6 +427,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 	case 0xc2:
 	case 0xc3:
 		*type = INSN_RETURN;
+		/*
+		 * For the impact on the stack, a ret behaves like
+		 * a pop of the return address.
+		 */
+		op->src.type = OP_SRC_POP;
+		op->dest.type = OP_DEST_REG;
+		op->dest.reg = CFI_RA;
 		break;
 
 	case 0xca: /* retf */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index ad362c5de281..8b1df659cd68 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -26,9 +26,50 @@ struct alternative {
 	bool skip_orig;
 };
 
+/*
+ * List to keep track of intra-function call return addresses.
+ * The list is a simple static array because we don't expect
+ * to have a lot of nested intra-function calls.
+ */
+#define RADDR_COUNT_MAX		32
+#define RADDR_ALTERED		((void *)-1)
+
+static struct instruction *raddr_list[RADDR_COUNT_MAX];
+static int raddr_count;
+
 const char *objname;
 struct cfi_state initial_func_cfi;
 
+static void raddr_clear(void)
+{
+	raddr_count = 0;
+}
+
+static bool raddr_push(struct instruction *insn)
+{
+	if (raddr_count == RADDR_COUNT_MAX) {
+		WARN_FUNC("return address list is full",
+			  insn->sec, insn->offset);
+		return false;
+	}
+
+	raddr_list[raddr_count++] = insn;
+	return true;
+}
+
+static bool raddr_pop(struct instruction **insn)
+{
+	if (raddr_count == 0)
+		return false;
+
+	*insn = raddr_list[--raddr_count];
+	return true;
+}
+
+static int validate_branch(struct objtool_file *file, struct symbol *func,
+			   struct instruction *from,
+			   struct instruction *first, struct insn_state state);
+
 struct instruction *find_insn(struct objtool_file *file,
 			      struct section *sec, unsigned long offset)
 {
@@ -2039,8 +2080,52 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
 	return validate_call(insn, state);
 }
 
-static int validate_return(struct symbol *func, struct instruction *insn, struct insn_state *state)
+static int validate_return_address(struct objtool_file *file,
+				   struct symbol *func,
+				   struct instruction *insn,
+				   struct insn_state *state)
 {
+	struct instruction *raddr_insn;
+	int ret;
+
+	while (raddr_pop(&raddr_insn)) {
+		/*
+		 * We are branching somewhere and so processing
+		 * a return instruction. So update the stack
+		 * state for this instruction.
+		 */
+		update_insn_state(insn, state);
+
+		/*
+		 * If the return address has no instruction then
+		 * that's the end of the function.
+		 */
+		if (!raddr_insn)
+			break;
+
+		/*
+		 * If we are branching to a defined address then
+		 * just do an unconditional jump there.
+		 */
+		ret = validate_branch(file, func, insn,
+				      raddr_insn, *state);
+		if (ret) {
+			if (backtrace)
+				BT_FUNC("(ret-branch)", insn);
+			return ret;
+		}
+
+		return 0;
+	}
+
+	return -1;
+}
+
+static int validate_return(struct objtool_file *file, struct symbol *func,
+			   struct instruction *insn, struct insn_state *state)
+{
+	int ret;
+
 	if (state->uaccess && !func_uaccess_safe(func)) {
 		WARN_FUNC("return with UACCESS enabled",
 			  insn->sec, insn->offset);
@@ -2059,6 +2144,11 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
 		return 1;
 	}
 
+	/* check if we have return address to branch to */
+	ret = validate_return_address(file, func, insn, state);
+	if (ret >= 0)
+		return ret;
+
 	if (func && has_modified_stack_frame(state)) {
 		WARN_FUNC("return with modified stack frame",
 			  insn->sec, insn->offset);
@@ -2200,7 +2290,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 		switch (insn->type) {
 
 		case INSN_RETURN:
-			return validate_return(func, insn, &state);
+			return validate_return(file, func, insn, &state);
 
 		case INSN_CALL:
 		case INSN_CALL_DYNAMIC:
@@ -2223,12 +2313,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				/*
 				 * The call instruction can update the stack
 				 * state. Then make the intra-function call
-				 * behaves like and unconditional jump.
+				 * behaves like and unconditional jump. We
+				 * track the return address to handle any
+				 * return instruction.
 				 */
 				ret = update_insn_state(insn, &state);
 				if (ret)
 					return ret;
 
+				if (!raddr_push(next_insn))
+					return 1;
+
 				ret = validate_branch(file, func, insn,
 						      insn->jump_dest, state);
 				if (ret) {
@@ -2383,6 +2478,7 @@ static int validate_unwind_hints(struct objtool_file *file)
 
 	for_each_insn(file, insn) {
 		if (insn->hint && !insn->visited) {
+			raddr_clear();
 			ret = validate_branch(file, insn->func,
 					      NULL, insn, state);
 			if (ret && backtrace)
@@ -2522,7 +2618,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
 			continue;
 
 		state.uaccess = func->uaccess_safe;
-
+		raddr_clear();
 		ret = validate_branch(file, func, NULL, insn, state);
 		if (ret && backtrace)
 			BT_FUNC("<=== (func)", insn);
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 6a80903fc4aa..f7dbecd46bed 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -23,6 +23,7 @@ struct insn_state {
 	unsigned int uaccess_stack;
 	int drap_reg, drap_offset;
 	struct cfi_reg vals[CFI_NUM_REGS];
+	bool stack_altered;
 };
 
 struct instruction {
@@ -39,6 +40,8 @@ struct instruction {
 	bool intra_function_call;
 	bool retpoline_safe;
 	u8 visited;
+	u8 raddr_delete;
+	u8 raddr_alter;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct instruction *first_jump_src;
-- 
2.18.2

[PATCH V3 5/9] objtool: Add return address unwind hints

[Alexandre Chartre]

Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
hint macros to flag instructions which remove or modify return
addresses.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 arch/x86/include/asm/orc_types.h       |  2 +
 arch/x86/include/asm/unwind_hints.h    | 23 +++++++++
 tools/arch/x86/include/asm/orc_types.h |  2 +
 tools/objtool/check.c                  | 67 ++++++++++++++++++++++++++
 4 files changed, 94 insertions(+)

diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index 6e060907c163..5c1141175d51 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -60,6 +60,8 @@
 #define ORC_TYPE_REGS_IRET		2
 #define UNWIND_HINT_TYPE_SAVE		3
 #define UNWIND_HINT_TYPE_RESTORE	4
+#define UNWIND_HINT_TYPE_RADDR_ALTER	6
+#define UNWIND_HINT_TYPE_RADDR_DELETE	7
 
 #ifndef __ASSEMBLY__
 /*
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index f5e2eb12cb71..5211d2c5b7a2 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -94,6 +94,23 @@
 	UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
 .endm
 
+/*
+ * RADDR_DELETE: Used on instructions (other than ret instruction)
+ * which remove a return address from the stack. count is the number
+ * of return address which are removed.
+ */
+.macro UNWIND_HINT_RADDR_DELETE count=1
+	UNWIND_HINT type=UNWIND_HINT_TYPE_RADDR_DELETE sp_offset=\count
+.endm
+
+/*
+ * RADDR_ALTER: Used on instructions which change a return address on
+ * the stack. count is the number of return address which are changed.
+ */
+.macro UNWIND_HINT_RADDR_ALTER count=1
+	UNWIND_HINT type=UNWIND_HINT_TYPE_RADDR_ALTER sp_offset=\count
+.endm
+
 #else /* !__ASSEMBLY__ */
 
 #define UNWIND_HINT(sp_reg, sp_offset, type, end)		\
@@ -112,6 +129,12 @@
 
 #define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
 
+#define UNWIND_HINT_RADDR_ALTER(count)	\
+	UNWIND_HINT(0, count, UNWIND_HINT_TYPE_RADDR_ALTER, 0)
+
+#define UNWIND_HINT_RADDR_DELETE(count)	\
+	UNWIND_HINT(0, count, UNWIND_HINT_TYPE_RADDR_DELETE, 0)
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_X86_UNWIND_HINTS_H */
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index 6e060907c163..5c1141175d51 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -60,6 +60,8 @@
 #define ORC_TYPE_REGS_IRET		2
 #define UNWIND_HINT_TYPE_SAVE		3
 #define UNWIND_HINT_TYPE_RESTORE	4
+#define UNWIND_HINT_TYPE_RADDR_ALTER	6
+#define UNWIND_HINT_TYPE_RADDR_DELETE	7
 
 #ifndef __ASSEMBLY__
 /*
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 8b1df659cd68..0574ce8e232d 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -66,6 +66,28 @@ static bool raddr_pop(struct instruction **insn)
 	return true;
 }
 
+static bool raddr_delete(int count)
+{
+	if (raddr_count < count)
+		return false;
+
+	raddr_count -= count;
+	return true;
+}
+
+static bool raddr_alter(int count)
+{
+	int i;
+
+	if (raddr_count < count)
+		false;
+
+	for (i = 0; i < count; i++)
+		raddr_list[raddr_count - 1 - i] = RADDR_ALTERED;
+
+	return true;
+}
+
 static int validate_branch(struct objtool_file *file, struct symbol *func,
 			   struct instruction *from,
 			   struct instruction *first, struct insn_state state);
@@ -1314,6 +1336,14 @@ static int read_unwind_hints(struct objtool_file *file)
 			insn->restore = true;
 			insn->hint = true;
 			continue;
+
+		} else if (hint->type == UNWIND_HINT_TYPE_RADDR_DELETE) {
+			insn->raddr_delete = hint->sp_offset;
+			continue;
+
+		} else if (hint->type == UNWIND_HINT_TYPE_RADDR_ALTER) {
+			insn->raddr_alter = hint->sp_offset;
+			continue;
 		}
 
 		insn->hint = true;
@@ -1526,6 +1556,13 @@ static bool has_modified_stack_frame(struct insn_state *state)
 	    state->drap)
 		return true;
 
+	/*
+	 * If the stack was altered then don't check registers because
+	 * a callee-saved registers might have been pushed on the stack.
+	 */
+	if (state->stack_altered)
+		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)
@@ -2103,6 +2140,15 @@ static int validate_return_address(struct objtool_file *file,
 		if (!raddr_insn)
 			break;
 
+		/*
+		 * If this is a dynamic branch then we expect this
+		 * branch to return or not, depending on the defined
+		 * list of RA we have, so just continue the processing
+		 * of the RA list.
+		 */
+		if (raddr_insn == RADDR_ALTERED)
+			continue;
+
 		/*
 		 * If we are branching to a defined address then
 		 * just do an unconditional jump there.
@@ -2287,6 +2333,27 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				return 0;
 		}
 
+		if (insn->raddr_delete) {
+			/* delete return address */
+			if (!raddr_delete(insn->raddr_delete)) {
+				WARN_FUNC("fail to delete %d return address",
+					  insn->sec, insn->offset,
+					  insn->raddr_delete);
+				return 1;
+			}
+			state.stack_altered = true;
+
+		} else if (insn->raddr_alter) {
+			/* alter return address */
+			if (!raddr_alter(insn->raddr_alter)) {
+				WARN_FUNC("fail to alter %d return address",
+					  insn->sec, insn->offset,
+					  insn->raddr_alter);
+				return 1;
+			}
+			state.stack_altered = true;
+		}
+
 		switch (insn->type) {
 
 		case INSN_RETURN:
-- 
2.18.2

[PATCH V3 6/9] objtool: Report inconsistent stack changes in alternative

[Alexandre Chartre]

To allow a valid stack unwinding, an alternative should have code
where the same stack changes happens at the same places as in the
original code. Add a check in objtool to validate that stack changes
in alternative are effectively consitent with the original code.

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 tools/objtool/check.c | 155 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 155 insertions(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0574ce8e232d..9488a9c7be24 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2724,6 +2724,156 @@ static int validate_reachable_instructions(struct objtool_file *file)
 	return 0;
 }
 
+static int validate_alternative_state(struct objtool_file *file,
+				      struct instruction *first,
+				      struct instruction *prev,
+				      struct instruction *current,
+				      bool include_current)
+{
+	struct instruction *alt_insn, *alt_first;
+	unsigned long roff_prev, roff_curr, roff;
+	int count, err = 0, err_alt, alt_num;
+	struct alternative *alt;
+	const char *err_str;
+
+	/*
+	 * Check that instructions in alternatives between the corresponding
+	 * previous and current instructions, have the same stack state.
+	 */
+
+	/* use relative offset to find corresponding alt instructions */
+	roff_prev = prev->offset - first->offset;
+	roff_curr = current->offset - first->offset;
+	alt_num = 0;
+
+	list_for_each_entry(alt, &first->alts, list) {
+		if (!alt->insn->alt_group)
+			continue;
+
+		alt_num++;
+		alt_first = alt->insn;
+		count = 0;
+		err_alt = 0;
+
+		for (alt_insn = alt_first;
+		     alt_insn && alt_insn->alt_group == alt_first->alt_group;
+		     alt_insn = next_insn_same_sec(file, alt_insn)) {
+
+			roff = alt_insn->offset - alt_first->offset;
+			if (roff < roff_prev)
+				continue;
+
+			if (roff > roff_curr ||
+			    (roff == roff_curr && !include_current))
+				break;
+
+			count++;
+			/*
+			 * The first instruction we check must be aligned with
+			 * the corresponding "prev" instruction because stack
+			 * change should happen at the same offset.
+			 */
+			if (count == 1 && roff != roff_prev) {
+				err_str = "misaligned alternative state change";
+				err_alt++;
+			}
+
+			if (!err_alt && memcmp(&prev->state, &alt_insn->state,
+					       sizeof(struct insn_state))) {
+				err_str = "invalid alternative state";
+				err_alt++;
+			}
+
+			/*
+			 * On error, report the error and stop checking
+			 * this alternative but continue checking other
+			 * alternatives.
+			 */
+			if (err_alt) {
+				if (!err) {
+					WARN_FUNC("error in alternative",
+						  first->sec, first->offset);
+				}
+				WARN_FUNC("in alternative %d",
+					  alt_first->sec, alt_first->offset,
+					  alt_num);
+				WARN_FUNC("%s", alt_insn->sec, alt_insn->offset,
+					  err_str);
+
+				err += err_alt;
+				break;
+			}
+		}
+	}
+
+	return err;
+}
+
+static int validate_alternative(struct objtool_file *file)
+{
+	struct instruction *first, *prev, *next, *insn;
+	bool in_alternative;
+	int err;
+
+	err = 0;
+	first = prev = NULL;
+	in_alternative = false;
+	for_each_insn(file, insn) {
+		if (insn->ignore || !insn->alt_group || insn->ignore_alts)
+			continue;
+
+		if (!in_alternative) {
+			if (list_empty(&insn->alts))
+				continue;
+
+			/*
+			 * Setup variables to start the processing of a
+			 * new alternative.
+			 */
+			first = insn;
+			prev = insn;
+			err = 0;
+			in_alternative = true;
+
+		} else if (!err && memcmp(&prev->state, &insn->state,
+					  sizeof(struct insn_state))) {
+			/*
+			 * The stack state has changed and no error was
+			 * reported for this alternative. Check that the
+			 * stack state is the same in all alternatives
+			 * between the last check and up to this instruction.
+			 *
+			 * Once we have an error, stop checking the
+			 * alternative because once the stack state is
+			 * inconsistent, it will likely be inconsistent
+			 * for other instructions as well.
+			 */
+			err = validate_alternative_state(file, first,
+							 prev, insn, false);
+			prev = insn;
+		}
+
+		/*
+		 * If the next instruction is in the same alternative then
+		 * continue with processing this alternative. Otherwise
+		 * that's the end of this alternative so check there is no
+		 * stack state changes in remaining instructions (if no
+		 * error was reported yet).
+		 */
+		next = list_next_entry(insn, list);
+		if (next && !next->ignore &&
+		    next->alt_group == first->alt_group)
+			continue;
+
+		if (!err)
+			err = validate_alternative_state(file, first,
+							 prev, insn, true);
+		in_alternative = false;
+	}
+
+	return 0;
+}
+
 static struct objtool_file file;
 
 int check(const char *_objname, bool orc)
@@ -2769,6 +2919,11 @@ int check(const char *_objname, bool orc)
 		goto out;
 	warnings += ret;
 
+	ret = validate_alternative(&file);
+	if (ret < 0)
+		goto out;
+	warnings += ret;
+
 	if (!warnings) {
 		ret = validate_reachable_instructions(&file);
 		if (ret < 0)
-- 
2.18.2

[PATCH V3 7/9] x86/speculation: Change __FILL_RETURN_BUFFER to work with objtool

[Alexandre Chartre]

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

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
---
 arch/x86/include/asm/nospec-branch.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 07e95dcb40ad..b0db793fd083 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__
 
-- 
2.18.2

[PATCH V3 8/9] x86/speculation: Add return address unwind hints to retpoline and RSB stuffing

[Alexandre Chartre]

The retpoline and RSB stuffing code modify the stack to change return
addresses of intra-function calls. Add return address unwind hints so
that objtool can be aware of these tweaks. This requires a little
refactoring of the __FILL_RETURN_BUFFER macro for adding hints to
assembly and C code.

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

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index b0db793fd083..20594ea99c21 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -9,6 +9,7 @@
 #include <asm/alternative-asm.h>
 #include <asm/cpufeatures.h>
 #include <asm/msr-index.h>
+#include <asm/unwind_hints.h>
 
 /*
  * This should be used immediately before a retpoline alternative. It tells
@@ -43,7 +44,7 @@
  * the optimal version — two calls, each with their own speculation
  * trap should their return address end up getting used, in a loop.
  */
-#define __FILL_RETURN_BUFFER(reg, nr, sp)	\
+#define __FILL_RETURN_BUFFER_BEGIN(reg, nr, sp)	\
 	mov	$(nr/2), reg;			\
 771:						\
 	call	772f;				\
@@ -58,12 +59,19 @@
 	lfence;					\
 	jmp	775b;				\
 774:						\
-	dec	reg;				\
+	dec	reg;
+
+#define __FILL_RETURN_BUFFER_END(sp)		\
 	add	$(BITS_PER_LONG/8) * 2, sp;	\
 	jnz	771b;
 
 #ifdef __ASSEMBLY__
 
+#define __FILL_RETURN_BUFFER(reg, nr, sp)	\
+	__FILL_RETURN_BUFFER_BEGIN(reg, nr, sp)	\
+	UNWIND_HINT_RADDR_DELETE 2;		\
+	__FILL_RETURN_BUFFER_END(sp)
+
 /*
  * This should be used immediately before an indirect jump/call. It tells
  * objtool the subsequent indirect jump/call is vouched safe for retpoline
@@ -88,6 +96,7 @@
 	lfence
 	jmp	.Lspec_trap_\@
 .Ldo_rop_\@:
+	UNWIND_HINT_RADDR_ALTER
 	mov	\reg, (%_ASM_SP)
 	ret
 .endm
@@ -237,6 +246,11 @@ enum ssb_mitigation {
 extern char __indirect_thunk_start[];
 extern char __indirect_thunk_end[];
 
+#define __FILL_RETURN_BUFFER(reg, nr, sp)			\
+	__stringify(__FILL_RETURN_BUFFER_BEGIN(reg, nr, sp))	\
+	UNWIND_HINT_RADDR_DELETE(2)				\
+	__stringify(__FILL_RETURN_BUFFER_END(sp))
+
 /*
  * On VMEXIT we must ensure that no RSB predictions learned in the guest
  * can be followed in the host, by overwriting the RSB completely. Both
@@ -250,7 +264,7 @@ static inline void vmexit_fill_RSB(void)
 
 	asm volatile (ANNOTATE_NOSPEC_ALTERNATIVE
 		      ALTERNATIVE("jmp 910f",
-				  __stringify(__FILL_RETURN_BUFFER(%0, RSB_CLEAR_LOOPS, %1)),
+				  __FILL_RETURN_BUFFER(%0, RSB_CLEAR_LOOPS, %1),
 				  X86_FEATURE_RETPOLINE)
 		      "910:"
 		      : "=r" (loops), ASM_CALL_CONSTRAINT
-- 
2.18.2

[PATCH V3 9/9] 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, 14 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 20594ea99c21..89ae2f9cc873 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -3,6 +3,7 @@
 #ifndef _ASM_X86_NOSPEC_BRANCH_H_
 #define _ASM_X86_NOSPEC_BRANCH_H_
 
+#include <linux/frame.h>
 #include <linux/static_key.h>
 
 #include <asm/alternative.h>
@@ -20,6 +21,15 @@
 #define ANNOTATE_NOSPEC_ALTERNATIVE \
 	ANNOTATE_IGNORE_ALTERNATIVE
 
+/*
+ * 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.
+ */
+#define INTRA_FUNCTION_CALL \
+	ANNOTATE_INTRA_FUNCTION_CALL call
+
 /*
  * Fill the CPU return stack buffer.
  *
@@ -47,13 +57,13 @@
 #define __FILL_RETURN_BUFFER_BEGIN(reg, nr, sp)	\
 	mov	$(nr/2), reg;			\
 771:						\
-	call	772f;				\
+	INTRA_FUNCTION_CALL 772f;		\
 773:	/* speculation trap */			\
 	pause;					\
 	lfence;					\
 	jmp	773b;				\
 772:						\
-	call	774f;				\
+	INTRA_FUNCTION_CALL 774f;		\
 775:	/* speculation trap */			\
 	pause;					\
 	lfence;					\
@@ -90,7 +100,7 @@
  * invocation below less ugly.
  */
 .macro RETPOLINE_JMP reg:req
-	call	.Ldo_rop_\@
+	INTRA_FUNCTION_CALL .Ldo_rop_\@
 .Lspec_trap_\@:
 	pause
 	lfence
@@ -110,7 +120,7 @@
 .Ldo_retpoline_jmp_\@:
 	RETPOLINE_JMP \reg
 .Ldo_call_\@:
-	call	.Ldo_retpoline_jmp_\@
+	INTRA_FUNCTION_CALL .Ldo_retpoline_jmp_\@
 .endm
 
 /*
-- 
2.18.2

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

[Julien Thierry]

Hi Alexandre,

On 4/14/20 11:36 AM, Alexandre Chartre wrote:
> Change objtool to support intra-function calls. On x86, an intra-function
> call is represented in objtool as a push onto the stack (of the return
> address), and a jump to the destination address. That way the stack
> information is correctly updated and the call flow is still accurate.
> 
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> ---
>   include/linux/frame.h                         |  11 ++
>   .../Documentation/stack-validation.txt        |   8 ++
>   tools/objtool/arch/x86/decode.c               |   6 +
>   tools/objtool/check.c                         | 126 ++++++++++++++----
>   tools/objtool/check.h                         |   1 +
>   5 files changed, 128 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/frame.h b/include/linux/frame.h
> index 02d3ca2d9598..c7178e6c9d48 100644
> --- a/include/linux/frame.h
> +++ b/include/linux/frame.h
> @@ -15,9 +15,20 @@
>   	static void __used __section(.discard.func_stack_frame_non_standard) \
>   		*__func_stack_frame_non_standard_##func = func
>   
> +/*
> + * This macro indicates that the following intra-function call is valid.
> + * Any non-annotated intra-function call will cause objtool to issue warning.
> + */
> +#define ANNOTATE_INTRA_FUNCTION_CALL				\
> +	999:							\
> +	.pushsection .discard.intra_function_call;		\

Very nit-pickish but my brain wants to add an 's' to that section name. 
Like for the sections "unwind_hints" or "altinstructions".

> +	.long 999b;						\
> +	.popsection;
> +
>   #else /* !CONFIG_STACK_VALIDATION */
>   
>   #define STACK_FRAME_NON_STANDARD(func)
> +#define ANNOTATE_INTRA_FUNCTION_CALL
>   
>   #endif /* CONFIG_STACK_VALIDATION */
>   
> diff --git a/tools/objtool/Documentation/stack-validation.txt b/tools/objtool/Documentation/stack-validation.txt
> index de094670050b..09f863fd32d2 100644
> --- a/tools/objtool/Documentation/stack-validation.txt
> +++ b/tools/objtool/Documentation/stack-validation.txt
> @@ -290,6 +290,14 @@ they mean, and suggestions for how to fix them.
>         https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
>   
>   
> +9. file.o: warning: unsupported intra-function call
> +
> +   This warning means that a direct call is done to a destination which
> +   is not at the beginning of a function. If this is a legit call, you
> +   can remove this warning by putting the ANNOTATE_INTRA_FUNCTION_CALL
> +   directive right before the call.
> +
> +
>   If the error doesn't seem to make sense, it could be a bug in objtool.
>   Feel free to ask the objtool maintainer for help.
>   
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index a62e032863a8..f4d70b8835c4 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -437,6 +437,12 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>   
>   	case 0xe8:
>   		*type = INSN_CALL;
> +		/*
> +		 * For the impact on the stack, a call behaves like
> +		 * a push of an immediate value (the return address).
> +		 */
> +		op->src.type = OP_SRC_CONST;
> +		op->dest.type = OP_DEST_PUSH;
>   		break;
>   
>   	case 0xfc:
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 8f0525d5d895..ad362c5de281 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -654,44 +654,58 @@ static int add_jump_destinations(struct objtool_file *file)
>   	return 0;
>   }
>   
> +static int configure_call(struct objtool_file *file, struct instruction *insn)

The name "configure_call()" is a bit vague. What about naming this 
set_local_call_destination() ? (Since it deals with call destinations 
that don't need relocations and sets their actual target destination).

> +{
> +	unsigned long dest_off;
> +
> +	dest_off = insn->offset + insn->len + insn->immediate;
> +	insn->call_dest = find_func_by_offset(insn->sec, dest_off);
> +	if (!insn->call_dest)
> +		insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
> +
> +	if (insn->call_dest) {
> +		/* regular call */
> +		if (insn->func && insn->call_dest->type != STT_FUNC) {
> +			WARN_FUNC("unsupported call to non-function",
> +				  insn->sec, insn->offset);
> +			return -1;
> +		}
> +		return 0;
> +	}
> +
> +	/* intra-function call */
> +	if (!insn->intra_function_call)
> +		WARN_FUNC("intra-function call", insn->sec, insn->offset);
> +
> +	dest_off = insn->offset + insn->len + insn->immediate;
> +	insn->jump_dest = find_insn(file, insn->sec, dest_off);
> +	if (!insn->jump_dest) {
> +		WARN_FUNC("can't find call dest at %s+0x%lx",
> +			  insn->sec, insn->offset,
> +			  insn->sec->name, dest_off);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>   /*
>    * Find the destination instructions for all calls.
>    */
>   static int add_call_destinations(struct objtool_file *file)
>   {
>   	struct instruction *insn;
> -	unsigned long dest_off;
>   	struct rela *rela;
>   
>   	for_each_insn(file, insn) {
> -		if (insn->type != INSN_CALL)
> +		if (insn->type != INSN_CALL || insn->ignore)
>   			continue;
>   
>   		rela = find_rela_by_dest_range(file->elf, insn->sec,
>   					       insn->offset, insn->len);
>   		if (!rela) {
> -			dest_off = insn->offset + insn->len + insn->immediate;
> -			insn->call_dest = find_func_by_offset(insn->sec, dest_off);
> -			if (!insn->call_dest)
> -				insn->call_dest = find_symbol_by_offset(insn->sec, dest_off);
> -
> -			if (insn->ignore)
> -				continue;
> -
> -			if (!insn->call_dest) {
> -				WARN_FUNC("unsupported intra-function call",
> -					  insn->sec, insn->offset);
> -				if (retpoline)
> -					WARN("If this is a retpoline, please patch it in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE.");
> -				return -1;
> -			}
> -
> -			if (insn->func && insn->call_dest->type != STT_FUNC) {
> -				WARN_FUNC("unsupported call to non-function",
> -					  insn->sec, insn->offset);
> +			if (configure_call(file, insn))
>   				return -1;
> -			}
> -
>   		} else if (rela->sym->type == STT_SECTION) {
>   			insn->call_dest = find_func_by_offset(rela->sym->sec,
>   							      rela->addend+4);
> @@ -1337,6 +1351,42 @@ 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 direct call",
> +				  insn->sec, insn->offset);
> +			return -1;
> +		}
> +
> +		insn->intra_function_call = true;

Maybe you could directly lookup the target instruction here and set 
insn->jump_dest here.

Then you wouldn't need as much changes in add_call_destination(), just 
adding the following in the "!rela" branch:

	if (insn->intra_function_call)
		continue;

> +	}
> +
> +	return 0;
> +}
> +
>   static void mark_rodata(struct objtool_file *file)
>   {
>   	struct section *sec;
> @@ -1392,6 +1442,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;
> @@ -2155,7 +2209,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;
> @@ -2164,6 +2219,29 @@ 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) {
> +				/*
> +				 * The call instruction can update the stack
> +				 * state. Then make the intra-function call
> +				 * behaves like and unconditional jump.
> +				 */
> +				ret = update_insn_state(insn, &state);
> +				if (ret)
> +					return ret;
> +
> +				ret = validate_branch(file, func, insn,
> +						      insn->jump_dest, state);
> +				if (ret) {
> +					if (backtrace) {
> +						BT_FUNC("(intra-function call)",
> +							insn);
> +					}
> +					return ret;
> +				}
> +
> +				return 0;
> +			}
> +
>   			break;
>   
>   		case INSN_JUMP_CONDITIONAL:
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index 953db38dfc35..6a80903fc4aa 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -36,6 +36,7 @@ struct instruction {
>   	int alt_group;
>   	bool dead_end, ignore, ignore_alts;
>   	bool hint, save, restore;
> +	bool intra_function_call;
>   	bool retpoline_safe;
>   	u8 visited;
>   	struct symbol *call_dest;
> 

Cheers,

-- 
Julien Thierry

Re: [PATCH V3 4/9] objtool: Handle return instruction with intra-function call

[Julien Thierry]

Hi Alexandre,

On 4/14/20 11:36 AM, Alexandre Chartre wrote:
> Intra-function calls are implemented in objtool like unconditional
> jumps. Keep track of intra-functin calls return addresses so that

intra-function*

> objtool can make a return instruction continues the flow at the
> right location.
> 
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> ---
>   tools/objtool/arch/x86/decode.c |   7 +++
>   tools/objtool/check.c           | 104 ++++++++++++++++++++++++++++++--
>   tools/objtool/check.h           |   3 +
>   3 files changed, 110 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index f4d70b8835c4..76b593bb2e4f 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -427,6 +427,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>   	case 0xc2:
>   	case 0xc3:
>   		*type = INSN_RETURN;
> +		/*
> +		 * For the impact on the stack, a ret behaves like
> +		 * a pop of the return address.
> +		 */
> +		op->src.type = OP_SRC_POP;
> +		op->dest.type = OP_DEST_REG;
> +		op->dest.reg = CFI_RA;
>   		break;
>   
>   	case 0xca: /* retf */
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index ad362c5de281..8b1df659cd68 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -26,9 +26,50 @@ struct alternative {
>   	bool skip_orig;
>   };
>   
> +/*
> + * List to keep track of intra-function call return addresses.
> + * The list is a simple static array because we don't expect
> + * to have a lot of nested intra-function calls.
> + */
> +#define RADDR_COUNT_MAX		32
> +#define RADDR_ALTERED		((void *)-1)
> +
> +static struct instruction *raddr_list[RADDR_COUNT_MAX];
> +static int raddr_count;

Could this be part of the insn_state?

> +
>   const char *objname;
>   struct cfi_state initial_func_cfi;
>   
> +static void raddr_clear(void)
> +{
> +	raddr_count = 0;
> +}
> +
> +static bool raddr_push(struct instruction *insn)
> +{
> +	if (raddr_count == RADDR_COUNT_MAX) {
> +		WARN_FUNC("return address list is full",
> +			  insn->sec, insn->offset);
> +		return false;
> +	}
> +
> +	raddr_list[raddr_count++] = insn;
> +	return true;
> +}
> +
> +static bool raddr_pop(struct instruction **insn)
> +{
> +	if (raddr_count == 0)
> +		return false;
> +
> +	*insn = raddr_list[--raddr_count];
> +	return true;
> +}
> +
> +static int validate_branch(struct objtool_file *file, struct symbol *func,
> +			   struct instruction *from,
> +			   struct instruction *first, struct insn_state state);
> +
>   struct instruction *find_insn(struct objtool_file *file,
>   			      struct section *sec, unsigned long offset)
>   {
> @@ -2039,8 +2080,52 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
>   	return validate_call(insn, state);
>   }
>   
> -static int validate_return(struct symbol *func, struct instruction *insn, struct insn_state *state)
> +static int validate_return_address(struct objtool_file *file,
> +				   struct symbol *func,
> +				   struct instruction *insn,
> +				   struct insn_state *state)
>   {
> +	struct instruction *raddr_insn;
> +	int ret;
> +
> +	while (raddr_pop(&raddr_insn)) {

Why is this a loop? Either there is something on the return address 
stack, it gets validated as a branch and the function returns, or 
nothing is on the stack and the function returns.

There doesn't seem to be a point where the loop gets to a second iteration.

> +		/*
> +		 * We are branching somewhere and so processing
> +		 * a return instruction. So update the stack
> +		 * state for this instruction.
> +		 */
> +		update_insn_state(insn, state);
> +
> +		/*
> +		 * If the return address has no instruction then
> +		 * that's the end of the function.
> +		 */
> +		if (!raddr_insn)

I can't think of a valid case where raddr_pop() returns "true" for a 
NULL return address. If you check for that case, it should probably be a 
warning/error rather than silently returning (especially since you'd 
have updated the state).

> +			break;
> +
> +		/*
> +		 * If we are branching to a defined address then
> +		 * just do an unconditional jump there.
> +		 */
> +		ret = validate_branch(file, func, insn,
> +				      raddr_insn, *state);
> +		if (ret) {
> +			if (backtrace)
> +				BT_FUNC("(ret-branch)", insn);
> +			return ret;
> +		}
> +
> +		return 0;
> +	}
> +
> +	return -1;

Returning "-1" is a bit confusing. One might think this is reporting an 
error, but the only caller actually uses it as "I'll just carry on with 
the rest of my checks".

Maybe validate_return() could call validate_return_address() as follows:

	if (radd_pop(&raddr_insn))
		return validate_return_address(file, func, insn, state, raddr_insn);


> +}
> +
> +static int validate_return(struct objtool_file *file, struct symbol *func,
> +			   struct instruction *insn, struct insn_state *state)
> +{
> +	int ret;
> +
>   	if (state->uaccess && !func_uaccess_safe(func)) {
>   		WARN_FUNC("return with UACCESS enabled",
>   			  insn->sec, insn->offset);
> @@ -2059,6 +2144,11 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
>   		return 1;
>   	}
>   
> +	/* check if we have return address to branch to */
> +	ret = validate_return_address(file, func, insn, state);
> +	if (ret >= 0)
> +		return ret;
> +
>   	if (func && has_modified_stack_frame(state)) {
>   		WARN_FUNC("return with modified stack frame",
>   			  insn->sec, insn->offset);
> @@ -2200,7 +2290,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   		switch (insn->type) {
>   
>   		case INSN_RETURN:
> -			return validate_return(func, insn, &state);
> +			return validate_return(file, func, insn, &state);
>   
>   		case INSN_CALL:
>   		case INSN_CALL_DYNAMIC:
> @@ -2223,12 +2313,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   				/*
>   				 * The call instruction can update the stack
>   				 * state. Then make the intra-function call
> -				 * behaves like and unconditional jump.
> +				 * behaves like and unconditional jump. We
> +				 * track the return address to handle any
> +				 * return instruction.
>   				 */
>   				ret = update_insn_state(insn, &state);
>   				if (ret)
>   					return ret;
>   
> +				if (!raddr_push(next_insn))
> +					return 1;
> +
>   				ret = validate_branch(file, func, insn,
>   						      insn->jump_dest, state);
>   				if (ret) {
> @@ -2383,6 +2478,7 @@ static int validate_unwind_hints(struct objtool_file *file)
>   
>   	for_each_insn(file, insn) {
>   		if (insn->hint && !insn->visited) {
> +			raddr_clear();
>   			ret = validate_branch(file, insn->func,
>   					      NULL, insn, state);
>   			if (ret && backtrace)
> @@ -2522,7 +2618,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
>   			continue;
>   
>   		state.uaccess = func->uaccess_safe;
> -
> +		raddr_clear();
>   		ret = validate_branch(file, func, NULL, insn, state);
>   		if (ret && backtrace)
>   			BT_FUNC("<=== (func)", insn);
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index 6a80903fc4aa..f7dbecd46bed 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -23,6 +23,7 @@ struct insn_state {
>   	unsigned int uaccess_stack;
>   	int drap_reg, drap_offset;
>   	struct cfi_reg vals[CFI_NUM_REGS];
> +	bool stack_altered;

This isn't used in this patch.

>   };
>   
>   struct instruction {
> @@ -39,6 +40,8 @@ struct instruction {
>   	bool intra_function_call;
>   	bool retpoline_safe;
>   	u8 visited;
> +	u8 raddr_delete;
> +	u8 raddr_alter;

These fields (and the RADDR_ALTERED) don't seem to be used elsewhere in 
this patch. I guess they should be part of patch 5.

>   	struct symbol *call_dest;
>   	struct instruction *jump_dest;
>   	struct instruction *first_jump_src;
> 

Cheers,

-- 
Julien Thierry

Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in alternative

[Julien Thierry]

Hi Alexandre,

On 4/14/20 11:36 AM, Alexandre Chartre wrote:
> To allow a valid stack unwinding, an alternative should have code
> where the same stack changes happens at the same places as in the
> original code. Add a check in objtool to validate that stack changes
> in alternative are effectively consitent with the original code.
> 
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> ---
>   tools/objtool/check.c | 155 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 155 insertions(+)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 0574ce8e232d..9488a9c7be24 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2724,6 +2724,156 @@ static int validate_reachable_instructions(struct objtool_file *file)
>   	return 0;
>   }
>   
> +static int validate_alternative_state(struct objtool_file *file,
> +				      struct instruction *first,
> +				      struct instruction *prev,
> +				      struct instruction *current,
> +				      bool include_current)
> +{
> +	struct instruction *alt_insn, *alt_first;
> +	unsigned long roff_prev, roff_curr, roff;
> +	int count, err = 0, err_alt, alt_num;
> +	struct alternative *alt;
> +	const char *err_str;
> +
> +	/*
> +	 * Check that instructions in alternatives between the corresponding
> +	 * previous and current instructions, have the same stack state.
> +	 */
> +
> +	/* use relative offset to find corresponding alt instructions */
> +	roff_prev = prev->offset - first->offset;
> +	roff_curr = current->offset - first->offset;
> +	alt_num = 0;
> +
> +	list_for_each_entry(alt, &first->alts, list) {
> +		if (!alt->insn->alt_group)
> +			continue;
> +
> +		alt_num++;
> +		alt_first = alt->insn;
> +		count = 0;
> +		err_alt = 0;

Unless I'm missing something, err_alt is wither 1 or 0, so perhaps a 
boolean would be more appropriate.

> +
> +		for (alt_insn = alt_first;
> +		     alt_insn && alt_insn->alt_group == alt_first->alt_group;
> +		     alt_insn = next_insn_same_sec(file, alt_insn)) {
> +
> +			roff = alt_insn->offset - alt_first->offset;
> +			if (roff < roff_prev)
> +				continue;
> +
> +			if (roff > roff_curr ||
> +			    (roff == roff_curr && !include_current))
> +				break;
> +
> +			count++;
> +			/*
> +			 * The first instruction we check must be aligned with
> +			 * the corresponding "prev" instruction because stack
> +			 * change should happen at the same offset.
> +			 */
> +			if (count == 1 && roff != roff_prev) {
> +				err_str = "misaligned alternative state change";
> +				err_alt++;
> +			}
> +
> +			if (!err_alt && memcmp(&prev->state, &alt_insn->state,
> +					       sizeof(struct insn_state))) {

In insn_state_match(), not the whole of the insn_state is taken into 
account. In particular, uaccess_stack, uaccess, df and end are not 
compared. Also, stack_size (but maybe that should be included in 
insn_state_match() ) and vals are not checked.

Is there a reason we'd check those for alternatives but not in the 
normal case? And does your alternative validation work with uaccess check?

> +				err_str = "invalid alternative state";
> +				err_alt++;
> +			}
> +
> +			/*
> +			 * On error, report the error and stop checking
> +			 * this alternative but continue checking other
> +			 * alternatives.
> +			 */
> +			if (err_alt) {
> +				if (!err) {
> +					WARN_FUNC("error in alternative",
> +						  first->sec, first->offset);
> +				}
> +				WARN_FUNC("in alternative %d",
> +					  alt_first->sec, alt_first->offset,
> +					  alt_num);
> +				WARN_FUNC("%s", alt_insn->sec, alt_insn->offset,
> +					  err_str);
> +
> +				err += err_alt;
> +				break;
> +			}
> +		}
> +	}
> +
> +	return err;
> +}
> +
> +static int validate_alternative(struct objtool_file *file)
> +{
> +	struct instruction *first, *prev, *next, *insn;
> +	bool in_alternative;
> +	int err;
> +
> +	err = 0;
> +	first = prev = NULL;
> +	in_alternative = false;
> +	for_each_insn(file, insn) {
> +		if (insn->ignore || !insn->alt_group || insn->ignore_alts)
> +			continue;
> +
> +		if (!in_alternative) {
> +			if (list_empty(&insn->alts))
> +				continue;
> +
> +			/*
> +			 * Setup variables to start the processing of a
> +			 * new alternative.
> +			 */
> +			first = insn;
> +			prev = insn;
> +			err = 0;
> +			in_alternative = true;
> +
> +		} else if (!err && memcmp(&prev->state, &insn->state,
> +					  sizeof(struct insn_state))) {
> +			/*
> +			 * The stack state has changed and no error was
> +			 * reported for this alternative. Check that the
> +			 * stack state is the same in all alternatives
> +			 * between the last check and up to this instruction.
> +			 *
> +			 * Once we have an error, stop checking the
> +			 * alternative because once the stack state is
> +			 * inconsistent, it will likely be inconsistent
> +			 * for other instructions as well.
> +			 */
> +			err = validate_alternative_state(file, first,
> +							 prev, insn, false);
> +			prev = insn;
> +		}
> +
> +		/*
> +		 * If the next instruction is in the same alternative then
> +		 * continue with processing this alternative. Otherwise
> +		 * that's the end of this alternative so check there is no
> +		 * stack state changes in remaining instructions (if no
> +		 * error was reported yet).
> +		 */
> +		next = list_next_entry(insn, list);
> +		if (next && !next->ignore &&
> +		    next->alt_group == first->alt_group)
> +			continue;
> +
> +		if (!err)
> +			err = validate_alternative_state(file, first,
> +							 prev, insn, true);
> +		in_alternative = false;
> +	}
> +
> +	return 0;
> +}
> +
>   static struct objtool_file file;
>   
>   int check(const char *_objname, bool orc)
> @@ -2769,6 +2919,11 @@ int check(const char *_objname, bool orc)
>   		goto out;
>   	warnings += ret;
>   
> +	ret = validate_alternative(&file);
> +	if (ret < 0)
> +		goto out;
> +	warnings += ret;
> +
>   	if (!warnings) {
>   		ret = validate_reachable_instructions(&file);
>   		if (ret < 0)
> 

Cheers,

-- 
Julien Thierry

Re: [PATCH V3 5/9] objtool: Add return address unwind hints

[Peter Zijlstra]

On Tue, Apr 14, 2020 at 12:36:14PM +0200, Alexandre Chartre wrote:
> Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
> hint macros to flag instructions which remove or modify return
> addresses.

I'm confused by this thing; why? AFAICT the rest of the patches are
actually simpler without this one.

Re: [PATCH V3 5/9] objtool: Add return address unwind hints

[Alexandre Chartre]

On 4/14/20 6:16 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 12:36:14PM +0200, Alexandre Chartre wrote:
>> Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
>> hint macros to flag instructions which remove or modify return
>> addresses.
> 
> I'm confused by this thing; why? AFAICT the rest of the patches are
> actually simpler without this one.
> 

This is required to indicate to objtool that assembly instructions are
changing return addresses. For example, in patch 8:

For retpoline:

@@ -88,6 +96,7 @@
  	lfence
  	jmp	.Lspec_trap_\@
  .Ldo_rop_\@:
+	UNWIND_HINT_RADDR_ALTER
  	mov	\reg, (%_ASM_SP)
  	ret
  .endm

The unwind hint indicates that the return address has been altered, so the
code won't return to the return address which was on the stack.

Also for RSB stuffing, this is used to indicated that the stack adjustment
instruction (add $(BITS_PER_LONG/8) * 2, sp) is actually removing the last
two return addresses.

alex.

Re: [PATCH V3 5/9] objtool: Add return address unwind hints

[Peter Zijlstra]

On Tue, Apr 14, 2020 at 06:40:12PM +0200, Alexandre Chartre wrote:
> 
> On 4/14/20 6:16 PM, Peter Zijlstra wrote:
> > On Tue, Apr 14, 2020 at 12:36:14PM +0200, Alexandre Chartre wrote:
> > > Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
> > > hint macros to flag instructions which remove or modify return
> > > addresses.
> > 
> > I'm confused by this thing; why? AFAICT the rest of the patches are
> > actually simpler without this one.
> > 
> 
> This is required to indicate to objtool that assembly instructions are
> changing return addresses. For example, in patch 8:
> 
> For retpoline:
> 
> @@ -88,6 +96,7 @@
>  	lfence
>  	jmp	.Lspec_trap_\@
>  .Ldo_rop_\@:
> +	UNWIND_HINT_RADDR_ALTER
>  	mov	\reg, (%_ASM_SP)
>  	ret
>  .endm
> 
> The unwind hint indicates that the return address has been altered, so the
> code won't return to the return address which was on the stack.

But if you hadn't added that return stack stuff in the first place,
you'd not have needed that HINT.

So what actual problem is it solving?

Re: [PATCH V3 5/9] objtool: Add return address unwind hints

[Alexandre Chartre]

On 4/14/20 7:56 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 06:40:12PM +0200, Alexandre Chartre wrote:
>>
>> On 4/14/20 6:16 PM, Peter Zijlstra wrote:
>>> On Tue, Apr 14, 2020 at 12:36:14PM +0200, Alexandre Chartre wrote:
>>>> Add the UNWIND_HINT_RADDR_DELETE and UNWIND_HINT_RADDR_ALTER unwind
>>>> hint macros to flag instructions which remove or modify return
>>>> addresses.
>>>
>>> I'm confused by this thing; why? AFAICT the rest of the patches are
>>> actually simpler without this one.
>>>
>>
>> This is required to indicate to objtool that assembly instructions are
>> changing return addresses. For example, in patch 8:
>>
>> For retpoline:
>>
>> @@ -88,6 +96,7 @@
>>   	lfence
>>   	jmp	.Lspec_trap_\@
>>   .Ldo_rop_\@:
>> +	UNWIND_HINT_RADDR_ALTER
>>   	mov	\reg, (%_ASM_SP)
>>   	ret
>>   .endm
>>
>> The unwind hint indicates that the return address has been altered, so the
>> code won't return to the return address which was on the stack.
> 
> But if you hadn't added that return stack stuff in the first place,
> you'd not have needed that HINT.
> 
> So what actual problem is it solving?
> 

The return stack stuff is here to correctly handle intra-function call so that
we can figure out where the ret of an intra-function call should return. We
don't have this challenge with regular functions because we know that a ret
inside such function just indicates the end of the function.

But when there's an intra-function call, a ret instruction can either:
   - continue after the intra-function call (if the stack was unchanged)
   - jump somewhere else (if the return address was changed) and eventually
     return to the next return address
   - indicate the end of the function (if the return address was removed).

So, all this is needed to correctly follow the flow of the code and properly
record stack changes.

alex.

Re: [PATCH V3 5/9] objtool: Add return address unwind hints

[Peter Zijlstra]

On Tue, Apr 14, 2020 at 08:31:23PM +0200, Alexandre Chartre wrote:
> On 4/14/20 7:56 PM, Peter Zijlstra wrote:

> > So what actual problem is it solving?
> > 
> 
> The return stack stuff is here to correctly handle intra-function call so that
> we can figure out where the ret of an intra-function call should return. We
> don't have this challenge with regular functions because we know that a ret
> inside such function just indicates the end of the function.
> 
> But when there's an intra-function call, a ret instruction can either:
>   - continue after the intra-function call (if the stack was unchanged)
>   - jump somewhere else (if the return address was changed) and eventually
>     return to the next return address
>   - indicate the end of the function (if the return address was removed).
> 
> So, all this is needed to correctly follow the flow of the code and properly
> record stack changes.

But which intra-function calls are you worried about here? The RSB
stuffing ones we have to explicitly forget and the retpoline ones we
can't follow because they're indirect calls.

So again, who cares about that stack?

Re: [PATCH V3 5/9] objtool: Add return address unwind hints

[Alexandre Chartre]

On 4/14/20 8:42 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 08:31:23PM +0200, Alexandre Chartre wrote:
>> On 4/14/20 7:56 PM, Peter Zijlstra wrote:
> 
>>> So what actual problem is it solving?
>>>
>>
>> The return stack stuff is here to correctly handle intra-function call so that
>> we can figure out where the ret of an intra-function call should return. We
>> don't have this challenge with regular functions because we know that a ret
>> inside such function just indicates the end of the function.
>>
>> But when there's an intra-function call, a ret instruction can either:
>>    - continue after the intra-function call (if the stack was unchanged)
>>    - jump somewhere else (if the return address was changed) and eventually
>>      return to the next return address
>>    - indicate the end of the function (if the return address was removed).
>>
>> So, all this is needed to correctly follow the flow of the code and properly
>> record stack changes.
> 
> But which intra-function calls are you worried about here? The RSB
> stuffing ones we have to explicitly forget and the retpoline ones we
> can't follow because they're indirect calls.
> 
> So again, who cares about that stack?
> 

This provides a generic code to handle any intra-function call. Currently we have
the RSB stuffing ones which are forgotten with the UNWIND_HINT_TYPE_RADDR_DELETE
directive. And for retpoline, they will not return if we have an indirect jump
(JMP_NOSPEC) but they will return if we have an indirect call (CALL_NOSPEC). The
code can handle both cases. For example, if we were to have a CALL_NOSPEC invocation
which is not in an alternative then objtool can now correctly handle it.

alex.

Re: [PATCH V3 5/9] objtool: Add return address unwind hints

[Peter Zijlstra]

On Tue, Apr 14, 2020 at 09:27:27PM +0200, Alexandre Chartre wrote:
> This provides a generic code to handle any intra-function call. Currently we have
> the RSB stuffing ones which are forgotten with the UNWIND_HINT_TYPE_RADDR_DELETE
> directive. And for retpoline, they will not return if we have an indirect jump
> (JMP_NOSPEC) but they will return if we have an indirect call (CALL_NOSPEC). The
> code can handle both cases. For example, if we were to have a CALL_NOSPEC invocation
> which is not in an alternative then objtool can now correctly handle it.

The specialness of CALL_NOSPEC goes away with my proposed retpoline
rework as well. I really don't think we need something as complicated as
this.

Fundamentally validate_branch() will continue after a CALL instruction;
so I'm thikning the worst that can happen from not following a
(theoretical direct return) is a false-positive unreachable code
warning, and we can trivially fix those with exisiting hints.

Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in alternative

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

Hi Alexandre,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on linus/master v5.7-rc1 next-20200414]
[cannot apply to tip/x86/core linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Alexandre-Chartre/objtool-changes-to-check-retpoline-code/20200415-015312
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 354a2fb98a37f696a1cffdc58dbff48a02e7c611
config: x86_64-randconfig-b002-20200415 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> mm/kasan/common.o: warning: objtool: kasan_report()+0x13: error in alternative
>> mm/kasan/common.o: warning: objtool: .altinstr_replacement+0xa: in alternative 1
>> mm/kasan/common.o: warning: objtool: .altinstr_replacement+0xb: invalid alternative state
   mm/kasan/common.o: warning: objtool: kasan_report()+0x3b: error in alternative
   mm/kasan/common.o: warning: objtool: .altinstr_replacement+0xf: in alternative 1
   mm/kasan/common.o: warning: objtool: .altinstr_replacement+0x10: invalid alternative state

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33318 bytes --]

Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in alternative

[kbuild test robot]

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

Hi Alexandre,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on linus/master v5.7-rc1 next-20200414]
[cannot apply to tip/x86/core linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Alexandre-Chartre/objtool-changes-to-check-retpoline-code/20200415-015312
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 354a2fb98a37f696a1cffdc58dbff48a02e7c611
config: x86_64-randconfig-b002-20200415 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> mm/kasan/common.o: warning: objtool: kasan_report()+0x13: error in alternative
>> mm/kasan/common.o: warning: objtool: .altinstr_replacement+0xa: in alternative 1
>> mm/kasan/common.o: warning: objtool: .altinstr_replacement+0xb: invalid alternative state
   mm/kasan/common.o: warning: objtool: kasan_report()+0x3b: error in alternative
   mm/kasan/common.o: warning: objtool: .altinstr_replacement+0xf: in alternative 1
   mm/kasan/common.o: warning: objtool: .altinstr_replacement+0x10: invalid alternative state

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33318 bytes --]

Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in alternative

[kbuild test robot]

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

Hi Alexandre,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on linus/master v5.7-rc1 next-20200414]
[cannot apply to tip/x86/core linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Alexandre-Chartre/objtool-changes-to-check-retpoline-code/20200415-015312
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 354a2fb98a37f696a1cffdc58dbff48a02e7c611
config: x86_64-randconfig-g003-20200414 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> kernel/trace/trace_branch.o: warning: objtool: ftrace_likely_update()+0x4: error in alternative
>> kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x0: in alternative 1
>> kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x1: invalid alternative state
   kernel/trace/trace_branch.o: warning: objtool: ftrace_likely_update()+0x21: error in alternative
   kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x5: in alternative 1
   kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x6: invalid alternative state
--
>> lib/ubsan.o: warning: objtool: ubsan_type_mismatch_common()+0xc: error in alternative
>> lib/ubsan.o: warning: objtool: .altinstr_replacement+0x0: in alternative 1
>> lib/ubsan.o: warning: objtool: .altinstr_replacement+0x1: invalid alternative state
   lib/ubsan.o: warning: objtool: ubsan_type_mismatch_common()+0x10a: error in alternative
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0x6: in alternative 1
>> lib/ubsan.o: warning: objtool: .altinstr_replacement+0x8: misaligned alternative state change
>> lib/ubsan.o: warning: objtool: __ubsan_handle_shift_out_of_bounds()+0x2b: error in alternative
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0x9: in alternative 1
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0xa: invalid alternative state
   lib/ubsan.o: warning: objtool: __ubsan_handle_shift_out_of_bounds()+0x103: error in alternative
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0xf: in alternative 1
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0x11: misaligned alternative state change

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32811 bytes --]

Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in alternative

[kbuild test robot]

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

Hi Alexandre,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on linus/master v5.7-rc1 next-20200414]
[cannot apply to tip/x86/core linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Alexandre-Chartre/objtool-changes-to-check-retpoline-code/20200415-015312
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 354a2fb98a37f696a1cffdc58dbff48a02e7c611
config: x86_64-randconfig-g003-20200414 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> kernel/trace/trace_branch.o: warning: objtool: ftrace_likely_update()+0x4: error in alternative
>> kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x0: in alternative 1
>> kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x1: invalid alternative state
   kernel/trace/trace_branch.o: warning: objtool: ftrace_likely_update()+0x21: error in alternative
   kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x5: in alternative 1
   kernel/trace/trace_branch.o: warning: objtool: .altinstr_replacement+0x6: invalid alternative state
--
>> lib/ubsan.o: warning: objtool: ubsan_type_mismatch_common()+0xc: error in alternative
>> lib/ubsan.o: warning: objtool: .altinstr_replacement+0x0: in alternative 1
>> lib/ubsan.o: warning: objtool: .altinstr_replacement+0x1: invalid alternative state
   lib/ubsan.o: warning: objtool: ubsan_type_mismatch_common()+0x10a: error in alternative
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0x6: in alternative 1
>> lib/ubsan.o: warning: objtool: .altinstr_replacement+0x8: misaligned alternative state change
>> lib/ubsan.o: warning: objtool: __ubsan_handle_shift_out_of_bounds()+0x2b: error in alternative
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0x9: in alternative 1
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0xa: invalid alternative state
   lib/ubsan.o: warning: objtool: __ubsan_handle_shift_out_of_bounds()+0x103: error in alternative
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0xf: in alternative 1
   lib/ubsan.o: warning: objtool: .altinstr_replacement+0x11: misaligned alternative state change

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32811 bytes --]

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

[Miroslav Benes]

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

"unsupported intra-function call" ?

Miroslav

Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in alternative

[Peter Zijlstra]

On Tue, Apr 14, 2020 at 12:36:15PM +0200, Alexandre Chartre wrote:
> To allow a valid stack unwinding, an alternative should have code
> where the same stack changes happens at the same places as in the
> original code. Add a check in objtool to validate that stack changes
> in alternative are effectively consitent with the original code.

This thing is completely buggered, it warns all over the place, even for
obviously correct alternatives like:

0000000000000310 <return_to_handler>:
 310:   48 83 ec 18             sub    $0x18,%rsp
 314:   48 89 04 24             mov    %rax,(%rsp)
 318:   48 89 54 24 08          mov    %rdx,0x8(%rsp)
 31d:   48 89 ef                mov    %rbp,%rdi
 320:   e8 00 00 00 00          callq  325 <return_to_handler+0x15>
                        321: R_X86_64_PLT32     ftrace_return_to_handler-0x4
 325:   48 89 c7                mov    %rax,%rdi
 328:   48 8b 54 24 08          mov    0x8(%rsp),%rdx
 32d:   48 8b 04 24             mov    (%rsp),%rax
 331:   48 83 c4 18             add    $0x18,%rsp
 335:   ff e7                   jmpq   *%rdi
 337:   90                      nop
 338:   90                      nop
 339:   90                      nop


Where 335 has two alternatives:

   0:   e9 00 00 00 00          jmpq   5 <.altinstr_replacement+0x5>
                        1: R_X86_64_PLT32       __x86_retpoline_rdi-0x4

and

   5:   0f ae e8                lfence
   8:   ff e7                   jmpq   *%rdi


And it then comes back with:

  defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .entry.text+0x335: error in alternative
  defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x5: in alternative 2
  defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x8: misaligned alternative state change

which is just utter crap, JMP has no (CFI) state change.

Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in alternative

[Alexandre Chartre]

On 4/16/20 4:18 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 12:36:15PM +0200, Alexandre Chartre wrote:
>> To allow a valid stack unwinding, an alternative should have code
>> where the same stack changes happens at the same places as in the
>> original code. Add a check in objtool to validate that stack changes
>> in alternative are effectively consitent with the original code.
> 
> This thing is completely buggered, it warns all over the place, even for
> obviously correct alternatives like:
> 
> 0000000000000310 <return_to_handler>:
>   310:   48 83 ec 18             sub    $0x18,%rsp
>   314:   48 89 04 24             mov    %rax,(%rsp)
>   318:   48 89 54 24 08          mov    %rdx,0x8(%rsp)
>   31d:   48 89 ef                mov    %rbp,%rdi
>   320:   e8 00 00 00 00          callq  325 <return_to_handler+0x15>
>                          321: R_X86_64_PLT32     ftrace_return_to_handler-0x4
>   325:   48 89 c7                mov    %rax,%rdi
>   328:   48 8b 54 24 08          mov    0x8(%rsp),%rdx
>   32d:   48 8b 04 24             mov    (%rsp),%rax
>   331:   48 83 c4 18             add    $0x18,%rsp
>   335:   ff e7                   jmpq   *%rdi
>   337:   90                      nop
>   338:   90                      nop
>   339:   90                      nop
> 
> 
> Where 335 has two alternatives:
> 
>     0:   e9 00 00 00 00          jmpq   5 <.altinstr_replacement+0x5>
>                          1: R_X86_64_PLT32       __x86_retpoline_rdi-0x4
> 
> and
> 
>     5:   0f ae e8                lfence
>     8:   ff e7                   jmpq   *%rdi
> 
> 
> And it then comes back with:
> 
>    defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .entry.text+0x335: error in alternative
>    defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x5: in alternative 2
>    defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x8: misaligned alternative state change
> 
> which is just utter crap, JMP has no (CFI) state change.

I think that's because the original nop instructions are not reached, so
they probably have an undefined stack state. Hence there's a different
stack state between 335 (jmpq *%rdi) and 337 (nop).

With the alternative, we have:

335:         lfence
338:         jmpq *%rdi

So now instruction 338 is reached with the stack state we had at 335
which is different from the original stack state at 338 (undefined)
with the unreached instruction.

Don't we have a state change recorded at 337 because the instruction
is seen as not reached?


alex.

[tip: objtool/core] objtool: Add support for intra-function calls

[tip-bot2 for Alexandre Chartre]

The following commit has been merged into the objtool/core branch of tip:

Commit-ID:     8aa8eb2a8f5b3305a95f39957dd2b715fa668e21
Gitweb:        https://git.kernel.org/tip/8aa8eb2a8f5b3305a95f39957dd2b715fa668e21
Author:        Alexandre Chartre <alexandre.chartre@oracle.com>
AuthorDate:    Tue, 14 Apr 2020 12:36:12 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 30 Apr 2020 20:14:33 +02:00

objtool: Add support for intra-function calls

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

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lkml.kernel.org/r/20200414103618.12657-4-alexandre.chartre@oracle.com
---
 include/linux/frame.h                            | 11 ++-
 tools/objtool/Documentation/stack-validation.txt |  8 ++-
 tools/objtool/arch/x86/decode.c                  |  8 ++-
 tools/objtool/check.c                            | 79 ++++++++++++++-
 4 files changed, 102 insertions(+), 4 deletions(-)

diff --git a/include/linux/frame.h b/include/linux/frame.h
index 02d3ca2..303cda6 100644
--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -15,9 +15,20 @@
 	static void __used __section(.discard.func_stack_frame_non_standard) \
 		*__func_stack_frame_non_standard_##func = func
 
+/*
+ * This macro indicates that the following intra-function call is valid.
+ * Any non-annotated intra-function call will cause objtool to issue a warning.
+ */
+#define ANNOTATE_INTRA_FUNCTION_CALL				\
+	999:							\
+	.pushsection .discard.intra_function_calls;		\
+	.long 999b;						\
+	.popsection;
+
 #else /* !CONFIG_STACK_VALIDATION */
 
 #define STACK_FRAME_NON_STANDARD(func)
+#define ANNOTATE_INTRA_FUNCTION_CALL
 
 #endif /* CONFIG_STACK_VALIDATION */
 
diff --git a/tools/objtool/Documentation/stack-validation.txt b/tools/objtool/Documentation/stack-validation.txt
index 0189039..0542e46 100644
--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/stack-validation.txt
@@ -323,6 +323,14 @@ they mean, and suggestions for how to fix them.
     The easiest way to enforce this is to ensure alternatives do not contain
     any ORC entries, which in turn implies the above constraint.
 
+11. file.o: warning: unannotated intra-function call
+
+   This warning means that a direct call is done to a destination which
+   is not at the beginning of a function. If this is a legit call, you
+   can remove this warning by putting the ANNOTATE_INTRA_FUNCTION_CALL
+   directive right before the call.
+
+
 If the error doesn't seem to make sense, it could be a bug in objtool.
 Feel free to ask the objtool maintainer for help.
 
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index d7b5d10..4b504fc 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -496,6 +496,14 @@ int arch_decode_instruction(const struct elf *elf, const struct section *sec,
 
 	case 0xe8:
 		*type = INSN_CALL;
+		/*
+		 * For the impact on the stack, a CALL behaves like
+		 * a PUSH of an immediate value (the return address).
+		 */
+		ADD_OP(op) {
+			op->src.type = OP_SRC_CONST;
+			op->dest.type = OP_DEST_PUSH;
+		}
 		break;
 
 	case 0xfc:
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index d822858..32dea5f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -674,6 +674,16 @@ static int add_jump_destinations(struct objtool_file *file)
 	return 0;
 }
 
+static void remove_insn_ops(struct instruction *insn)
+{
+	struct stack_op *op, *tmp;
+
+	list_for_each_entry_safe(op, tmp, &insn->stack_ops, list) {
+		list_del(&op->list);
+		free(op);
+	}
+}
+
 /*
  * Find the destination instructions for all calls.
  */
@@ -699,10 +709,7 @@ static int add_call_destinations(struct objtool_file *file)
 				continue;
 
 			if (!insn->call_dest) {
-				WARN_FUNC("unsupported intra-function call",
-					  insn->sec, insn->offset);
-				if (retpoline)
-					WARN("If this is a retpoline, please patch it in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE.");
+				WARN_FUNC("unannotated intra-function call", insn->sec, insn->offset);
 				return -1;
 			}
 
@@ -725,6 +732,15 @@ static int add_call_destinations(struct objtool_file *file)
 			}
 		} else
 			insn->call_dest = rela->sym;
+
+		/*
+		 * Whatever stack impact regular CALLs have, should be undone
+		 * by the RETURN of the called function.
+		 *
+		 * Annotated intra-function calls retain the stack_ops but
+		 * are converted to JUMP, see read_intra_function_calls().
+		 */
+		remove_insn_ops(insn);
 	}
 
 	return 0;
@@ -1404,6 +1420,57 @@ static int read_instr_hints(struct objtool_file *file)
 	return 0;
 }
 
+static int read_intra_function_calls(struct objtool_file *file)
+{
+	struct instruction *insn;
+	struct section *sec;
+	struct rela *rela;
+
+	sec = find_section_by_name(file->elf, ".rela.discard.intra_function_calls");
+	if (!sec)
+		return 0;
+
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		unsigned long dest_off;
+
+		if (rela->sym->type != STT_SECTION) {
+			WARN("unexpected relocation symbol type in %s",
+			     sec->name);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("bad .discard.intra_function_call entry");
+			return -1;
+		}
+
+		if (insn->type != INSN_CALL) {
+			WARN_FUNC("intra_function_call not a direct call",
+				  insn->sec, insn->offset);
+			return -1;
+		}
+
+		/*
+		 * Treat intra-function CALLs as JMPs, but with a stack_op.
+		 * See add_call_destinations(), which strips stack_ops from
+		 * normal CALLs.
+		 */
+		insn->type = INSN_JUMP_UNCONDITIONAL;
+
+		dest_off = insn->offset + insn->len + insn->immediate;
+		insn->jump_dest = find_insn(file, insn->sec, dest_off);
+		if (!insn->jump_dest) {
+			WARN_FUNC("can't find call dest at %s+0x%lx",
+				  insn->sec, insn->offset,
+				  insn->sec->name, dest_off);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 static void mark_rodata(struct objtool_file *file)
 {
 	struct section *sec;
@@ -1459,6 +1526,10 @@ static int decode_sections(struct objtool_file *file)
 	if (ret)
 		return ret;
 
+	ret = read_intra_function_calls(file);
+	if (ret)
+		return ret;
+
 	ret = add_call_destinations(file);
 	if (ret)
 		return ret;

[tip: objtool/core] objtool: is_fentry_call() crashes if call has no destination

[tip-bot2 for Alexandre Chartre]

The following commit has been merged into the objtool/core branch of tip:

Commit-ID:     87cf61fe848ca8ddf091548671e168f52e8a718e
Gitweb:        https://git.kernel.org/tip/87cf61fe848ca8ddf091548671e168f52e8a718e
Author:        Alexandre Chartre <alexandre.chartre@oracle.com>
AuthorDate:    Tue, 14 Apr 2020 12:36:10 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 30 Apr 2020 20:14:32 +02:00

objtool: is_fentry_call() crashes if call has no destination

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

Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lkml.kernel.org/r/20200414103618.12657-2-alexandre.chartre@oracle.com
---
 tools/objtool/check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index fa9bf36..8af8de2 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1484,7 +1484,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;