linux-kernel - Re: [PATCH 18/35] objtool: Another static block fail

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: <20180119164229.GR2228@hirez.programming.kicks-ass.net>
Date:   Fri, 19 Jan 2018 17:42:29 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     David Woodhouse <dwmw2@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Dave Hansen <dave.hansen@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Jun Nakajima <jun.nakajima@...el.com>,
        Asit Mallick <asit.k.mallick@...el.com>,
        Jason Baron <jbaron@...mai.com>
Subject: Re: [PATCH 18/35] objtool: Another static block fail

On Thu, Jan 18, 2018 at 02:48:18PM +0100, Peter Zijlstra wrote:
> @@ -1216,7 +1218,8 @@ static bool __grow_static_block(struct o
>  
>  	switch (insn->type) {
>  	case INSN_JUMP_UNCONDITIONAL:
> -		/* mark this instruction, terminate this section */
> +		/* follow the jump, mark this instruction, terminate this section */
> +		jmp_grow_static_block(file, insn->jump_dest);

Hmm we should only do this when the jump is backwards, if its forwards
we should just mark the destination instruction and let the main
iteration loop take it.

>  		insn->static_jump_dest = true;
>  		return false;
>  

something like the below merged in maybe; I'm going to look at this
again later, my brain is completely useless today :/

Josh, how would you feel about adding the bits required to make objtool
a full disassembler? That would make it far easier to visualse the state
and I don't think its _that_ much more, all the difficult bits are
already done afaict, all we need is infrastructure to print the already
fully decoded instruction.


---
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1210,16 +1210,16 @@ static int read_retpoline_hints(struct o
 static void jmp_grow_static_block(struct objtool_file *file, struct instruction *insn);
 
 static bool __grow_static_block(struct objtool_file *file,
-				struct instruction *insn, bool ign_bt)
+				struct instruction *insn)
 {
-	/* if a jump can come in here, terminate */
-	if (insn->branch_target && !ign_bt)
+	/* if a !static jump can come in here, terminate */
+	if (insn->branch_target && !insn->static_jump_dest)
 		return false;
 
 	switch (insn->type) {
 	case INSN_JUMP_UNCONDITIONAL:
 		/* follow the jump, mark this instruction, terminate this section */
-		jmp_grow_static_block(file, insn->jump_dest);
+		jmp_grow_static_block(file, insn);
 		insn->static_jump_dest = true;
 		return false;
 
@@ -1243,27 +1243,43 @@ static bool __grow_static_block(struct o
 
 static void jmp_grow_static_block(struct objtool_file *file, struct instruction *insn)
 {
-	bool ignore = true;
+	struct instruction *dest = insn->jump_dest;
+	bool static_block = true;
 
-	/* !jump_dest */
-	if (!insn)
+	if (!dest)
 		return;
 
 	/* more than a single site jumps here, can't be certain */
-	if (insn->branch_target > 1)
+	if (dest->branch_target > 1)
 		return;
 
-	for (; &insn->list != &file->insn_list;
-	     insn = list_next_entry(insn, list)) {
-
+	if (dest->offset > insn->offset) {
 		/*
-		 * Per definition the first insn of a jump is a branch target,
-		 * don't terminate because of that.
+		 * fwd jump, the main iteration will still get there.
+		 * make sure it continues the section there.
 		 */
-		if (!__grow_static_block(file, insn, ignore))
-			break;
+		dest->static_jump_dest = true;
+		return;
+	}
 
-		ignore = false;
+	/* backwards jump, we need to revisit the instructions */
+	for (; static_block && dest != insn; dest = list_next_entry(dest, list)) {
+
+		static_block = __grow_static_block(file, dest);
+
+		if (insn->type == INSN_CALL) {
+			struct symbol *func = insn->call_dest;
+			if (!func)
+				continue;
+
+			/*
+			 * we flipped the call to static, update the stats.
+			 */
+			if (insn->static_jump_dest) {
+				func->non_static_call--;
+				func->static_call++;
+			}
+		}
 	}
 }
 
@@ -1276,7 +1292,7 @@ static int grow_static_blocks(struct obj
 
 	for_each_insn(file, insn) {
 		if (static_block || insn->static_jump_dest)
-			static_block = __grow_static_block(file, insn, !static_block);
+			static_block = __grow_static_block(file, insn);
 
 		if (insn->type == INSN_CALL) {
 			func = insn->call_dest;
@@ -1284,9 +1300,9 @@ static int grow_static_blocks(struct obj
 				continue;
 
 			if (static_block)
-				func->static_call = true;
+				func->static_call++;
 			else
-				func->non_static_call = true;
+				func->non_static_call++;
 		}
 	}
 
@@ -1301,7 +1317,7 @@ static int grow_static_blocks(struct obj
 			/* static && !non_static -- only static callers */
 
 			func_for_each_insn(file, func, insn) {
-				if (!__grow_static_block(file, insn, false))
+				if (!__grow_static_block(file, insn))
 					break;
 			}
 		}
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -61,7 +61,7 @@ struct symbol {
 	unsigned char bind, type;
 	unsigned long offset;
 	unsigned int len;
-	bool static_call, non_static_call;
+	unsigned int static_call, non_static_call;
 };
 
 struct rela {