All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] objtool: Fix switch-table detection
@ 2018-02-08 13:02 Peter Zijlstra
  2018-02-08 17:34 ` Linus Torvalds
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Peter Zijlstra @ 2018-02-08 13:02 UTC (permalink / raw)
  To: Linus Torvalds, Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
	linux-kernel
  Cc: Borislav Petkov


Linus reported that GCC-7.3 generated a switch-table construct that
confused objtool. It turns out that, in particular due to KASAN, it is
possible to have unrelated .rodata usage in between the .rodata setup
for the switch-table and the following indirect jump.

The simple linear reverse search from the indirect jump would hit upon
the KASAN .rodata usage first and fail to find a switch_table,
resulting in a spurious 'sibling call with modified stack frame'
warning.

Fix this by creating a 'jump-stack' which we can 'unwind' during
reversal, thereby skipping over much of the in-between code.

This is not fool proof by any means, but is sufficient to make the
known cases work. Future work would be to construct more comprehensive
flow analysis code.

Also, since Josh keeps asking, add myself to MAINTAINERS.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 MAINTAINERS           |    1 +
 tools/objtool/check.c |   41 +++++++++++++++++++++++++++++++++++++++--
 tools/objtool/check.h |    1 +
 3 files changed, 41 insertions(+), 2 deletions(-)

--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9847,6 +9847,7 @@ F:	drivers/nfc/nxp-nci
 
 OBJTOOL
 M:	Josh Poimboeuf <jpoimboe@redhat.com>
+M:	Peter Zijlstra <peterz@infradead.org>
 S:	Supported
 F:	tools/objtool/
 
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -851,8 +851,14 @@ static int add_switch_table(struct objto
  *    This is a fairly uncommon pattern which is new for GCC 6.  As of this
  *    writing, there are 11 occurrences of it in the allmodconfig kernel.
  *
+ *    As of GCC 7 there are quite a few more of these and the 'in between' code
+ *    is significant. Esp. with KASAN enabled some of the code between the mov
+ *    and jmpq uses .rodata itself, which can confuse things.
+ *
  *    TODO: Once we have DWARF CFI and smarter instruction decoding logic,
  *    ensure the same register is used in the mov and jump instructions.
+ *
+ *    NOTE: RETPOLINE made it harder still to decode dynamic jumps.
  */
 static struct rela *find_switch_table(struct objtool_file *file,
 				      struct symbol *func,
@@ -874,12 +880,25 @@ static struct rela *find_switch_table(st
 						text_rela->addend + 4);
 		if (!rodata_rela)
 			return NULL;
+
 		file->ignore_unreachables = true;
 		return rodata_rela;
 	}
 
 	/* case 3 */
-	func_for_each_insn_continue_reverse(file, func, insn) {
+	/*
+	 * Backward search using the @first_jump_src links, these help avoid
+	 * much of the 'in between' code. Which avoids us getting confused by
+	 * it.
+	 */
+	for (insn = list_prev_entry(insn, list);
+
+	     &insn->list != &file->insn_list &&
+	     insn->sec == func->sec &&
+	     insn->offset >= func->offset;
+
+	     insn = insn->first_jump_src ?: list_prev_entry(insn, list)) {
+
 		if (insn->type == INSN_JUMP_DYNAMIC)
 			break;
 
@@ -909,14 +928,32 @@ static struct rela *find_switch_table(st
 	return NULL;
 }
 
+
 static int add_func_switch_tables(struct objtool_file *file,
 				  struct symbol *func)
 {
-	struct instruction *insn, *prev_jump = NULL;
+	struct instruction *insn, *last = NULL, *prev_jump = NULL;
 	struct rela *rela, *prev_rela = NULL;
 	int ret;
 
 	func_for_each_insn(file, func, insn) {
+		if (!last)
+			last = insn;
+
+		/*
+		 * Store back-pointers for unconditional forward jumps such
+		 * that find_switch_table() can back-track using those and
+		 * avoid some potentially confusing code.
+		 */
+		if (insn->type == INSN_JUMP_UNCONDITIONAL && insn->jump_dest &&
+		    insn->offset > last->offset &&
+		    insn->jump_dest->offset > insn->offset &&
+		    !insn->jump_dest->first_jump_src) {
+
+			insn->jump_dest->first_jump_src = insn;
+			last = insn->jump_dest;
+		}
+
 		if (insn->type != INSN_JUMP_DYNAMIC)
 			continue;
 
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -47,6 +47,7 @@ struct instruction {
 	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
+	struct instruction *first_jump_src;
 	struct list_head alts;
 	struct symbol *func;
 	struct stack_op stack_op;

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] objtool: Fix switch-table detection
  2018-02-08 13:02 [PATCH] objtool: Fix switch-table detection Peter Zijlstra
@ 2018-02-08 17:34 ` Linus Torvalds
  2018-02-08 18:32 ` Josh Poimboeuf
  2018-02-09  7:25 ` [tip:x86/pti] " tip-bot for Peter Zijlstra
  2 siblings, 0 replies; 4+ messages in thread
From: Linus Torvalds @ 2018-02-08 17:34 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
	Linux Kernel Mailing List, Borislav Petkov

On Thu, Feb 8, 2018 at 5:02 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Fix this by creating a 'jump-stack' which we can 'unwind' during
> reversal, thereby skipping over much of the in-between code.

I'm assuming this will come through the normal tip trees, since that's
where objtool stuff generally comes from, and this is not very high
priority.

> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>

And assuming it's the same patch, just with the extra comment and
jump_orig -> first_jump_src rename (and MAINTAINERS update), that can
be a

  Reported-and-tested-by: Linus Torvalds <torvalds@linux-foundation.org>

since the previous version worked fine for me.

Thanks,

                Linus

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] objtool: Fix switch-table detection
  2018-02-08 13:02 [PATCH] objtool: Fix switch-table detection Peter Zijlstra
  2018-02-08 17:34 ` Linus Torvalds
@ 2018-02-08 18:32 ` Josh Poimboeuf
  2018-02-09  7:25 ` [tip:x86/pti] " tip-bot for Peter Zijlstra
  2 siblings, 0 replies; 4+ messages in thread
From: Josh Poimboeuf @ 2018-02-08 18:32 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Linus Torvalds, Thomas Gleixner, Ingo Molnar, linux-kernel,
	Borislav Petkov

On Thu, Feb 08, 2018 at 02:02:32PM +0100, Peter Zijlstra wrote:
> 
> Linus reported that GCC-7.3 generated a switch-table construct that
> confused objtool. It turns out that, in particular due to KASAN, it is
> possible to have unrelated .rodata usage in between the .rodata setup
> for the switch-table and the following indirect jump.
> 
> The simple linear reverse search from the indirect jump would hit upon
> the KASAN .rodata usage first and fail to find a switch_table,
> resulting in a spurious 'sibling call with modified stack frame'
> warning.
> 
> Fix this by creating a 'jump-stack' which we can 'unwind' during
> reversal, thereby skipping over much of the in-between code.
> 
> This is not fool proof by any means, but is sufficient to make the
> known cases work. Future work would be to construct more comprehensive
> flow analysis code.
> 
> Also, since Josh keeps asking, add myself to MAINTAINERS.
> 
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Thanks again for helping out with objtool maintainership!

No complaints from the 0-day bot, so:

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [tip:x86/pti] objtool: Fix switch-table detection
  2018-02-08 13:02 [PATCH] objtool: Fix switch-table detection Peter Zijlstra
  2018-02-08 17:34 ` Linus Torvalds
  2018-02-08 18:32 ` Josh Poimboeuf
@ 2018-02-09  7:25 ` tip-bot for Peter Zijlstra
  2 siblings, 0 replies; 4+ messages in thread
From: tip-bot for Peter Zijlstra @ 2018-02-09  7:25 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: bp, jpoimboe, mingo, hpa, peterz, linux-kernel, torvalds, tglx

Commit-ID:  99ce7962d52d1948ad6f2785e308d48e76e0a6ef
Gitweb:     https://git.kernel.org/tip/99ce7962d52d1948ad6f2785e308d48e76e0a6ef
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 8 Feb 2018 14:02:32 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Feb 2018 07:20:23 +0100

objtool: Fix switch-table detection

Linus reported that GCC-7.3 generated a switch-table construct that
confused objtool. It turns out that, in particular due to KASAN, it is
possible to have unrelated .rodata usage in between the .rodata setup
for the switch-table and the following indirect jump.

The simple linear reverse search from the indirect jump would hit upon
the KASAN .rodata usage first and fail to find a switch_table,
resulting in a spurious 'sibling call with modified stack frame'
warning.

Fix this by creating a 'jump-stack' which we can 'unwind' during
reversal, thereby skipping over much of the in-between code.

This is not fool proof by any means, but is sufficient to make the
known cases work. Future work would be to construct more comprehensive
flow analysis code.

Reported-and-tested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20180208130232.GF25235@hirez.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 tools/objtool/check.c | 41 +++++++++++++++++++++++++++++++++++++++--
 tools/objtool/check.h |  1 +
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 9cd028a..2e458eb 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -851,8 +851,14 @@ static int add_switch_table(struct objtool_file *file, struct symbol *func,
  *    This is a fairly uncommon pattern which is new for GCC 6.  As of this
  *    writing, there are 11 occurrences of it in the allmodconfig kernel.
  *
+ *    As of GCC 7 there are quite a few more of these and the 'in between' code
+ *    is significant. Esp. with KASAN enabled some of the code between the mov
+ *    and jmpq uses .rodata itself, which can confuse things.
+ *
  *    TODO: Once we have DWARF CFI and smarter instruction decoding logic,
  *    ensure the same register is used in the mov and jump instructions.
+ *
+ *    NOTE: RETPOLINE made it harder still to decode dynamic jumps.
  */
 static struct rela *find_switch_table(struct objtool_file *file,
 				      struct symbol *func,
@@ -874,12 +880,25 @@ static struct rela *find_switch_table(struct objtool_file *file,
 						text_rela->addend + 4);
 		if (!rodata_rela)
 			return NULL;
+
 		file->ignore_unreachables = true;
 		return rodata_rela;
 	}
 
 	/* case 3 */
-	func_for_each_insn_continue_reverse(file, func, insn) {
+	/*
+	 * Backward search using the @first_jump_src links, these help avoid
+	 * much of the 'in between' code. Which avoids us getting confused by
+	 * it.
+	 */
+	for (insn = list_prev_entry(insn, list);
+
+	     &insn->list != &file->insn_list &&
+	     insn->sec == func->sec &&
+	     insn->offset >= func->offset;
+
+	     insn = insn->first_jump_src ?: list_prev_entry(insn, list)) {
+
 		if (insn->type == INSN_JUMP_DYNAMIC)
 			break;
 
@@ -909,14 +928,32 @@ static struct rela *find_switch_table(struct objtool_file *file,
 	return NULL;
 }
 
+
 static int add_func_switch_tables(struct objtool_file *file,
 				  struct symbol *func)
 {
-	struct instruction *insn, *prev_jump = NULL;
+	struct instruction *insn, *last = NULL, *prev_jump = NULL;
 	struct rela *rela, *prev_rela = NULL;
 	int ret;
 
 	func_for_each_insn(file, func, insn) {
+		if (!last)
+			last = insn;
+
+		/*
+		 * Store back-pointers for unconditional forward jumps such
+		 * that find_switch_table() can back-track using those and
+		 * avoid some potentially confusing code.
+		 */
+		if (insn->type == INSN_JUMP_UNCONDITIONAL && insn->jump_dest &&
+		    insn->offset > last->offset &&
+		    insn->jump_dest->offset > insn->offset &&
+		    !insn->jump_dest->first_jump_src) {
+
+			insn->jump_dest->first_jump_src = insn;
+			last = insn->jump_dest;
+		}
+
 		if (insn->type != INSN_JUMP_DYNAMIC)
 			continue;
 
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index dbadb30..23a1d06 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -47,6 +47,7 @@ struct instruction {
 	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
+	struct instruction *first_jump_src;
 	struct list_head alts;
 	struct symbol *func;
 	struct stack_op stack_op;

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-02-09  7:30 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-08 13:02 [PATCH] objtool: Fix switch-table detection Peter Zijlstra
2018-02-08 17:34 ` Linus Torvalds
2018-02-08 18:32 ` Josh Poimboeuf
2018-02-09  7:25 ` [tip:x86/pti] " tip-bot for Peter Zijlstra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.