linux-kernel - Re: x86 entry perf unwinding failure (missing IRET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200428141614.GA13616@hirez.programming.kicks-ass.net>
Date:   Tue, 28 Apr 2020 16:16:14 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Jann Horn <jannh@...gle.com>, Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        the arch/x86 maintainers <x86@...nel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        alexandre.chartre@...cle.com
Subject: Re: x86 entry perf unwinding failure (missing IRET_REGS annotation
 on stack switch?)

On Tue, Apr 28, 2020 at 02:46:27PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 28, 2020 at 02:04:50AM -0500, Josh Poimboeuf wrote:

> > I'm thinking something like this should fix it.  Peter, does this look
> > ok?
> 
> Unfortunate. But also, I fear, insufficient. Specifically consider
> things like:
> 
> 	ALTERNATIVE "jmp 1f",
> 		"alt...
> 		"..."
> 		"...insn", X86_FEAT_foo
> 	1:
> 
> This results in something like:
> 
> 
> 	.text	.altinstr_replacement
> 	e8 xx	...
> 	90
> 	90
> 	...
> 	90
> 
> Where all our normal single byte nops (0x90) are unreachable with
> undefined CFI, but the alternative might have CFI, which is never
> propagated.
> 
> We ran into this with the validate_alternative stuff from Alexandre.

> So rather than hacking around this issue, should we not make
> create_orc() smarter?
> 
> I'm trying to come up with something, but so far I'm just making a mess.

Like this, it's horrid, but it seems to work.

What do you think of the approach? I'll work on cleaning it up if you
don't hate it too much ;-)


---

diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index 9d2bf2daaaa6..2a853ae994ea 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -10,17 +10,129 @@
 #include "check.h"
 #include "warn.h"
 
-int create_orc(struct objtool_file *file)
+static bool same_cfi(struct cfi_state *a, struct cfi_state *b)
 {
+	return memcmp(a, b, sizeof(*a));
+}
+
+static struct instruction *next_insn_same_sec(struct objtool_file *file,
+					      struct instruction *insn)
+{
+	struct instruction *next = list_next_entry(insn, list);
+
+	if (!next || &next->list == &file->insn_list || next->sec != insn->sec)
+		return NULL;
+
+	return next;
+}
+
+struct alternative {
+	struct list_head list;
 	struct instruction *insn;
+	bool skip_orig;
+};
+
+static int alt_cfi(struct objtool_file *file,
+		   struct instruction *prev_insn,
+		   struct instruction *orig_insn,
+		   struct instruction *alt_insn)
+{
+	unsigned long orig_offset = orig_insn->offset;
+	unsigned long alt_offset = alt_insn->offset;
+	struct instruction *next_insn;
+	bool orig_orc, alt_orc;
+
+	orig_orc = !prev_insn || same_cfi(&orig_insn->cfi, &prev_insn->cfi);
+	alt_orc  = !prev_insn || same_cfi(&alt_insn->cfi,  &prev_insn->cfi);
+
+again:
+	if ((orig_orc || alt_orc)) {
+		if (orig_insn->offset - orig_offset != alt_insn->offset - alt_offset) {
+			WARN_FUNC("alternative has unaligned ORC", orig_insn->sec, orig_insn->offset);
+			return -1;
+		}
+
+		if (orig_insn->visited) {
+			if (same_cfi(&orig_insn->cfi, &alt_insn->cfi)) {
+				WARN_FUNC("alternative violates ORC invariance", orig_insn->sec, orig_insn->offset);
+				return -1;
+			}
+		} else {
+			/*
+			 * We're in unreachable NOPs, allow the alternative to
+			 * override the CFI/ORC data.
+			 */
+			orig_insn->cfi = alt_insn->cfi;
+		}
+	}
+
+	next_insn = next_insn_same_sec(file, alt_insn);
+	if (!next_insn)
+		return 0;
+
+	if (next_insn->offset == -1 /*FAKE_JUMP_OFFSET*/)
+		return 0;
+
+	alt_orc = same_cfi(&alt_insn->cfi, &next_insn->cfi);
+	alt_insn = next_insn;
+
+	do {
+		next_insn = next_insn_same_sec(file, orig_insn);
+		if (!next_insn)
+			return 0;
+		if (!next_insn->alt_group)
+			return 0;
+
+		orig_orc = next_insn->visited && same_cfi(&orig_insn->cfi, &next_insn->cfi);
+		orig_insn = next_insn;
+	} while (orig_insn->offset - orig_offset < alt_insn->offset - alt_offset);
+
+	goto again;
+}
+
+static void orig_fill(struct objtool_file *file,
+		      struct instruction *prev_insn,
+		      struct instruction *insn)
+{
+	for (;;) {
+		if (!insn->visited)
+			insn->cfi = prev_insn->cfi;
+
+		prev_insn = insn;
+		insn = next_insn_same_sec(file, insn);
+		if (!insn)
+			return;
+		if (!insn->alt_group)
+			return;
+	}
+}
+
+int create_orc(struct objtool_file *file)
+{
+	struct instruction *insn, *prev_insn = NULL;
 
 	for_each_insn(file, insn) {
 		struct orc_entry *orc = &insn->orc;
 		struct cfi_reg *cfa = &insn->cfi.cfa;
 		struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];
+		int ret;
 
 		orc->end = insn->cfi.end;
 
+		if (insn->alt_group && !insn->ignore_alts) {
+			struct alternative *alt;
+
+			list_for_each_entry(alt, &insn->alts, list) {
+				if (alt->insn->offset == -1 /*FAKE_JUMP_OFFSET*/)
+					continue;
+				ret = alt_cfi(file, prev_insn, insn, alt->insn);
+				if (ret)
+					return ret;
+			}
+
+			orig_fill(file, prev_insn, insn);
+		}
+
 		if (cfa->base == CFI_UNDEFINED) {
 			orc->sp_reg = ORC_REG_UNDEFINED;
 			continue;
@@ -76,6 +188,8 @@ int create_orc(struct objtool_file *file)
 		orc->sp_offset = cfa->offset;
 		orc->bp_offset = bp->offset;
 		orc->type = insn->cfi.type;
+
+		prev_insn = insn;
 	}
 
 	return 0;