linux-kernel - Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

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: <20200327010001.i3kebxb4um422ycb@treble>
Date:   Thu, 26 Mar 2020 20:00:01 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org, x86@...nel.org,
        mhiramat@...nel.org, mbenes@...e.cz
Subject: Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

On Thu, Mar 26, 2020 at 08:57:18PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 26, 2020 at 04:49:38PM +0100, Peter Zijlstra wrote:
> > > The 'insn == first' check isn't ideal, but at least it works (I think?).
> > 
> > It works, yes, for exactly this one case.
>
> How's this? Ignore the ignore_cfi bits, that's a 'failed' experiment.

It still seems complex to me.

What do you think about this?  If we store save_insn in the state when
we see insn->save, the restore logic becomes a lot easier.  Then if we
get a restore without a save, we can just ignore the restore hint in
that path.  Later, when we see the restore insn again from the save
path, we can then compare the insn state with the saved state to make
sure they match.

This assumes no crazy save/restore scenarios.  It also means that the
restore path has to be reachable from the save path, for which I had to
make a change to make IRETQ *not* a dead end if there's a restore hint
immediately after it.

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e637a4a38d2a..e9becd50f148 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1265,7 +1265,6 @@ static int read_unwind_hints(struct objtool_file *file)
 
 		} else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
 			insn->restore = true;
-			insn->hint = true;
 			continue;
 		}
 
@@ -2003,7 +2002,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			   struct instruction *first, struct insn_state state)
 {
 	struct alternative *alt;
-	struct instruction *insn, *next_insn;
+	struct instruction *insn, *next_insn, *save_insn = NULL;
 	struct section *sec;
 	u8 visited;
 	int ret;
@@ -2034,54 +2033,32 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 
 		visited = 1 << state.uaccess;
 		if (insn->visited) {
-			if (!insn->hint && !insn_state_match(insn, &state))
+			if (!insn->hint && !insn->restore &&
+			    !insn_state_match(insn, &state)) {
 				return 1;
+			}
+
+			if (insn->restore && save_insn) {
+				if (!insn_state_match(insn, &save_insn->state))
+					return 1;
+				save_insn = NULL;
+			}
 
 			if (insn->visited & visited)
 				return 0;
 		}
 
-		if (insn->hint) {
-			if (insn->restore) {
-				struct instruction *save_insn, *i;
-
-				i = insn;
-				save_insn = NULL;
-				func_for_each_insn_continue_reverse(file, func, i) {
-					if (i->save) {
-						save_insn = i;
-						break;
-					}
-				}
-
-				if (!save_insn) {
-					WARN_FUNC("no corresponding CFI save for CFI restore",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				if (!save_insn->visited) {
-					/*
-					 * Oops, no state to copy yet.
-					 * Hopefully we can reach this
-					 * instruction from another branch
-					 * after the save insn has been
-					 * visited.
-					 */
-					if (insn == first)
-						return 0;
-
-					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
-						  sec, insn->offset);
-					return 1;
-				}
+		if (insn->save)
+			save_insn = insn;
 
-				insn->state = save_insn->state;
-			}
+		if (insn->restore && save_insn) {
+			insn->state = save_insn->state;
+			save_insn = NULL;
+		}
 
+		if (insn->hint)
 			state = insn->state;
-
-		} else
+		else
 			insn->state = state;
 
 		insn->visited |= visited;
@@ -2191,12 +2168,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			break;
 
 		case INSN_CONTEXT_SWITCH:
-			if (func && (!next_insn || !next_insn->hint)) {
-				WARN_FUNC("unsupported instruction in callable function",
-					  sec, insn->offset);
-				return 1;
+			if (!next_insn || !next_insn->restore) {
+				if (func) {
+					WARN_FUNC("unsupported instruction in callable function",
+							sec, insn->offset);
+					return 1;
+				}
+
+				return 0;
 			}
-			return 0;
+
+			break;
 
 		case INSN_STACK:
 			if (update_insn_state(insn, &state))
@@ -2293,7 +2275,7 @@ static int validate_unwind_hints(struct objtool_file *file)
 	clear_insn_state(&state);
 
 	for_each_insn(file, insn) {
-		if (insn->hint && !insn->visited) {
+		if ((insn->hint || insn->save || insn->restore) && !insn->visited) {
 			ret = validate_branch(file, insn->func, insn, state);
 			if (ret && backtrace)
 				BT_FUNC("<=== (hint)", insn);