linux-kernel - Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET

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: <20200402081710.GJ20760@hirez.programming.kicks-ass.net>
Date:   Thu, 2 Apr 2020 10:17:10 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Julien Thierry <jthierry@...hat.com>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>, tglx@...utronix.de,
        linux-kernel@...r.kernel.org, x86@...nel.org, mhiramat@...nel.org,
        mbenes@...e.cz, Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:

> > Also, instead of adding a special "arch_exception_frame_size", I could
> > suggest:
> > - Picking this patch [1] from a completely arbitrary source
> > - Getting rid of INSN_STACK type, any instruction could then include stack
> > ops on top of their existing semantics, they can just have an empty list if
> > they don't touch SP/BP
> > - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
> > right amount
> 
> That's not the worst idea, lemme try that.

Something like so then?

--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -738,7 +738,6 @@ static inline void sync_core(void)
 	unsigned int tmp;
 
 	asm volatile (
-		UNWIND_HINT_SAVE
 		"mov %%ss, %0\n\t"
 		"pushq %q0\n\t"
 		"pushq %%rsp\n\t"
@@ -748,7 +747,6 @@ static inline void sync_core(void)
 		"pushq %q0\n\t"
 		"pushq $1f\n\t"
 		"iretq\n\t"
-		UNWIND_HINT_RESTORE
 		"1:"
 		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
 #endif
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -19,6 +19,7 @@ enum insn_type {
 	INSN_CALL,
 	INSN_CALL_DYNAMIC,
 	INSN_RETURN,
+	INSN_EXCEPTION_RETURN,
 	INSN_CONTEXT_SWITCH,
 	INSN_STACK,
 	INSN_BUG,
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf *
 		*type = INSN_RETURN;
 		break;
 
+	case 0xcf: /* iret */
+		*type = INSN_EXCEPTION_RETURN;
+
+		/* add $40, %rsp */
+		op->src.type = OP_SRC_ADD;
+		op->src.reg = CFI_SP;
+		op->src.offset = 5*8;
+		op->dest.type = OP_DEST_REG;
+		op->dest.reg = CFI_SP;
+		break;
+
 	case 0xca: /* retf */
 	case 0xcb: /* retf */
-	case 0xcf: /* iret */
 		*type = INSN_CONTEXT_SWITCH;
 		break;
 
@@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf *
 
 	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
 
-	if (*type == INSN_STACK)
+	if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
 		list_add_tail(&op->list, ops_list);
 	else
 		free(op);
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2224,6 +2224,20 @@ static int validate_branch(struct objtoo
 
 			break;
 
+		case INSN_EXCEPTION_RETURN:
+			if (handle_insn_ops(insn, &state))
+				return 1;
+
+			/*
+			 * This handles x86's sync_core() case, where we use an
+			 * IRET to self. All 'normal' IRET instructions are in
+			 * STT_NOTYPE entry symbols.
+			 */
+			if (func)
+				break;
+
+			return 0;
+
 		case INSN_CONTEXT_SWITCH:
 			if (func && (!next_insn || !next_insn->hint)) {
 				WARN_FUNC("unsupported instruction in callable function",