Message-ID: <YhF4da/oHQ4nDyPM@hirez.programming.kicks-ass.net>
Date:   Sun, 20 Feb 2022 00:08:37 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     x86@...nel.org, joao@...rdrivepizza.com, hjl.tools@...il.com,
        andrew.cooper3@...rix.com, linux-kernel@...r.kernel.org,
        ndesaulniers@...gle.com, keescook@...omium.org,
        samitolvanen@...gle.com, mark.rutland@....com,
        alyssa.milburn@...el.com
Subject: Re: [PATCH 07/29] x86/entry: Sprinkle ENDBR dust

On Fri, Feb 18, 2022 at 04:23:38PM -0800, Josh Poimboeuf wrote:
> Another possibly better and less intrusive way of doing this would be
> for objtool to realize that any UNWIND_HINT_IRET_REGS at the beginning
> of a SYM_CODE_START (global non-function code symbol) needs ENDBR.

This; I likes that. I reverted this patch from the tree (very much
including the annotations), redid the objtool check and regenerated the
missing ENDBR given the objtool output.

I think the few missing ENDBRs in this are due to using x86_64-defconfig
instead of allmodconfig. I'll try on monday after spooling up a real
build machine :-)

---
 arch/x86/entry/entry_64.S           | 32 +++++++++++++++-----------------
 arch/x86/entry/entry_64_compat.S    |  3 +--
 arch/x86/include/asm/idtentry.h     | 19 +++++++------------
 arch/x86/include/asm/segment.h      |  7 +------
 arch/x86/include/asm/unwind_hints.h | 18 +++++-------------
 arch/x86/kernel/head_64.S           | 13 ++++++-------
 arch/x86/kernel/unwind_orc.c        |  3 +--
 include/linux/objtool.h             |  5 ++---
 tools/include/linux/objtool.h       |  5 ++---
 tools/objtool/check.c               | 13 ++++++++-----
 tools/objtool/orc_dump.c            |  3 +--
 11 files changed, 49 insertions(+), 72 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 77e222f2061e..d69239c638a2 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -39,7 +39,6 @@
 #include <asm/trapnr.h>
 #include <asm/nospec-branch.h>
 #include <asm/fsgsbase.h>
-#include <asm/ibt.h>
 #include <linux/err.h>
 
 #include "calling.h"
@@ -87,8 +86,8 @@
 
 SYM_CODE_START(entry_SYSCALL_64)
 	UNWIND_HINT_EMPTY
-
 	ENDBR
+
 	swapgs
 	/* tss.sp2 is scratch space. */
 	movq	%rsp, PER_CPU_VAR(cpu_tss_rw + TSS_sp2)
@@ -353,7 +352,7 @@ SYM_CODE_END(ret_from_fork)
  */
 .macro idtentry vector asmsym cfunc has_error_code:req
 SYM_CODE_START(\asmsym)
-	UNWIND_HINT_IRET_REGS offset=\has_error_code*8 entry=1
+	UNWIND_HINT_IRET_REGS offset=\has_error_code*8
 	ENDBR
 	ASM_CLAC
 
@@ -371,7 +370,7 @@ SYM_CODE_START(\asmsym)
 		.rept	6
 		pushq	5*8(%rsp)
 		.endr
-		UNWIND_HINT_IRET_REGS offset=8 entry=0
+		UNWIND_HINT_IRET_REGS offset=8
 .Lfrom_usermode_no_gap_\@:
 	.endif
 
@@ -421,7 +420,7 @@ SYM_CODE_END(\asmsym)
  */
 .macro idtentry_mce_db vector asmsym cfunc
 SYM_CODE_START(\asmsym)
-	UNWIND_HINT_IRET_REGS entry=1
+	UNWIND_HINT_IRET_REGS
 	ENDBR
 	ASM_CLAC
 
@@ -477,7 +476,7 @@ SYM_CODE_END(\asmsym)
  */
 .macro idtentry_vc vector asmsym cfunc
 SYM_CODE_START(\asmsym)
-	UNWIND_HINT_IRET_REGS entry=1
+	UNWIND_HINT_IRET_REGS
 	ENDBR
 	ASM_CLAC
 
@@ -539,7 +538,7 @@ SYM_CODE_END(\asmsym)
  */
 .macro idtentry_df vector asmsym cfunc
 SYM_CODE_START(\asmsym)
-	UNWIND_HINT_IRET_REGS offset=8 entry=1
+	UNWIND_HINT_IRET_REGS offset=8
 	ENDBR
 	ASM_CLAC
 
@@ -641,8 +640,7 @@ SYM_INNER_LABEL(restore_regs_and_return_to_kernel, SYM_L_GLOBAL)
 	INTERRUPT_RETURN
 
 SYM_INNER_LABEL_ALIGN(native_iret, SYM_L_GLOBAL)
-	UNWIND_HINT_IRET_REGS entry=0
-	ENDBR // paravirt_iret
+	UNWIND_HINT_IRET_REGS
 	/*
 	 * Are we returning to a stack segment from the LDT?  Note: in
 	 * 64-bit mode SS:RSP on the exception stack is always valid.
@@ -720,7 +718,7 @@ SYM_INNER_LABEL(native_irq_return_iret, SYM_L_GLOBAL)
 	popq	%rdi				/* Restore user RDI */
 
 	movq	%rax, %rsp
-	UNWIND_HINT_IRET_REGS offset=8 entry=0
+	UNWIND_HINT_IRET_REGS offset=8
 
 	/*
 	 * At this point, we cannot write to the stack any more, but we can
@@ -837,13 +835,13 @@ SYM_CODE_START(xen_failsafe_callback)
 	movq	8(%rsp), %r11
 	addq	$0x30, %rsp
 	pushq	$0				/* RIP */
-	UNWIND_HINT_IRET_REGS offset=8 entry=0
+	UNWIND_HINT_IRET_REGS offset=8
 	jmp	asm_exc_general_protection
 1:	/* Segment mismatch => Category 1 (Bad segment). Retry the IRET. */
 	movq	(%rsp), %rcx
 	movq	8(%rsp), %r11
 	addq	$0x30, %rsp
-	UNWIND_HINT_IRET_REGS entry=0
+	UNWIND_HINT_IRET_REGS
 	pushq	$-1 /* orig_ax = -1 => not a system call */
 	PUSH_AND_CLEAR_REGS
 	ENCODE_FRAME_POINTER
@@ -1078,7 +1076,7 @@ SYM_CODE_END(error_return)
  *	      when PAGE_TABLE_ISOLATION is in use.  Do not clobber.
  */
 SYM_CODE_START(asm_exc_nmi)
-	UNWIND_HINT_IRET_REGS entry=1
+	UNWIND_HINT_IRET_REGS
 	ENDBR
 
 	/*
@@ -1144,13 +1142,13 @@ SYM_CODE_START(asm_exc_nmi)
 	SWITCH_TO_KERNEL_CR3 scratch_reg=%rdx
 	movq	%rsp, %rdx
 	movq	PER_CPU_VAR(cpu_current_top_of_stack), %rsp
-	UNWIND_HINT_IRET_REGS base=%rdx offset=8 entry=0
+	UNWIND_HINT_IRET_REGS base=%rdx offset=8
 	pushq	5*8(%rdx)	/* pt_regs->ss */
 	pushq	4*8(%rdx)	/* pt_regs->rsp */
 	pushq	3*8(%rdx)	/* pt_regs->flags */
 	pushq	2*8(%rdx)	/* pt_regs->cs */
 	pushq	1*8(%rdx)	/* pt_regs->rip */
-	UNWIND_HINT_IRET_REGS entry=0
+	UNWIND_HINT_IRET_REGS
 	pushq   $-1		/* pt_regs->orig_ax */
 	PUSH_AND_CLEAR_REGS rdx=(%rdx)
 	ENCODE_FRAME_POINTER
@@ -1306,7 +1304,7 @@ SYM_CODE_START(asm_exc_nmi)
 	.rept 5
 	pushq	11*8(%rsp)
 	.endr
-	UNWIND_HINT_IRET_REGS entry=0
+	UNWIND_HINT_IRET_REGS
 
 	/* Everything up to here is safe from nested NMIs */
 
@@ -1322,7 +1320,7 @@ SYM_CODE_START(asm_exc_nmi)
 	pushq	$__KERNEL_CS	/* CS */
 	pushq	$1f		/* RIP */
 	iretq			/* continues at repeat_nmi below */
-	UNWIND_HINT_IRET_REGS entry=0
+	UNWIND_HINT_IRET_REGS
 1:
 #endif
 
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 316e0fa119b4..86caf7872a25 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -48,8 +48,8 @@
  */
 SYM_CODE_START(entry_SYSENTER_compat)
 	UNWIND_HINT_EMPTY
-	/* Interrupts are off on entry. */
 	ENDBR
+	/* Interrupts are off on entry. */
 	SWAPGS
 
 	pushq	%rax
@@ -344,7 +344,6 @@ SYM_CODE_END(entry_SYSCALL_compat)
  */
 SYM_CODE_START(entry_INT80_compat)
 	UNWIND_HINT_EMPTY
-	ENDBR
 	/*
 	 * Interrupts are off on entry.
 	 */
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index 9127e1e3c439..1157ee6f98d7 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -5,11 +5,7 @@
 /* Interrupts/Exceptions */
 #include <asm/trapnr.h>
 
-#ifdef CONFIG_X86_IBT
-#define IDT_ALIGN	16
-#else
-#define IDT_ALIGN	8
-#endif
+#define IDT_ALIGN	(8 * (1 + IS_ENABLED(CONFIG_X86_IBT)))
 
 #ifndef __ASSEMBLY__
 #include <linux/entry-common.h>
@@ -486,7 +482,7 @@ __visible noinstr void func(struct pt_regs *regs,			\
 
 /*
  * ASM code to emit the common vector entry stubs where each stub is
- * packed into 8 bytes.
+ * packed into IDT_ALIGN bytes.
  *
  * Note, that the 'pushq imm8' is emitted via '.byte 0x6a, vector' because
  * GCC treats the local vector variable as unsigned int and would expand
@@ -498,17 +494,16 @@ __visible noinstr void func(struct pt_regs *regs,			\
  * point is to mask off the bits above bit 7 because the push is sign
  * extending.
  */
-
 	.align IDT_ALIGN
 SYM_CODE_START(irq_entries_start)
     vector=FIRST_EXTERNAL_VECTOR
     .rept NR_EXTERNAL_VECTORS
-	UNWIND_HINT_IRET_REGS entry=1
-0 :
+	UNWIND_HINT_IRET_REGS
 	ENDBR
+0 :
 	.byte	0x6a, vector
 	jmp	asm_common_interrupt
-	/* Ensure that the above is 8 bytes max */
+	/* Ensure that the above is IDT_ALIGN bytes max */
 	.fill 0b + IDT_ALIGN - ., 1, 0x90
 	vector = vector+1
     .endr
@@ -519,12 +514,12 @@ SYM_CODE_END(irq_entries_start)
 SYM_CODE_START(spurious_entries_start)
     vector=FIRST_SYSTEM_VECTOR
     .rept NR_SYSTEM_VECTORS
-	UNWIND_HINT_IRET_REGS entry=1
+	UNWIND_HINT_IRET_REGS
 0 :
 	ENDBR
 	.byte	0x6a, vector
 	jmp	asm_spurious_interrupt
-	/* Ensure that the above is 8 bytes max */
+	/* Ensure that the above is IDT_ALIGN bytes max */
 	.fill 0b + IDT_ALIGN - ., 1, 0x90
 	vector = vector+1
     .endr
diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
index 6a8a5bcbf14d..3a09647788bd 100644
--- a/arch/x86/include/asm/segment.h
+++ b/arch/x86/include/asm/segment.h
@@ -4,7 +4,6 @@
 
 #include <linux/const.h>
 #include <asm/alternative.h>
-#include <asm/ibt.h>
 
 /*
  * Constructor for a conventional segment GDT (or LDT) entry.
@@ -276,11 +275,7 @@ static inline void vdso_read_cpunode(unsigned *cpu, unsigned *node)
  * vector has no error code (two bytes), a 'push $vector_number' (two
  * bytes), and a jump to the common entry code (up to five bytes).
  */
-#ifdef CONFIG_X86_IBT
-#define EARLY_IDT_HANDLER_SIZE 13
-#else
-#define EARLY_IDT_HANDLER_SIZE 9
-#endif
+#define EARLY_IDT_HANDLER_SIZE (9 + 4*IS_ENABLED(CONFIG_X86_IBT))
 
 /*
  * xen_early_idt_handler_array is for Xen pv guests: for each entry in
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index d5b401c2f9e9..8b33674288ea 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -11,7 +11,7 @@
 	UNWIND_HINT sp_reg=ORC_REG_UNDEFINED type=UNWIND_HINT_TYPE_CALL end=1
 .endm
 
-.macro UNWIND_HINT_REGS base=%rsp offset=0 indirect=0 extra=1 partial=0 entry=1
+.macro UNWIND_HINT_REGS base=%rsp offset=0 indirect=0 extra=1 partial=0
 	.if \base == %rsp
 		.if \indirect
 			.set sp_reg, ORC_REG_SP_INDIRECT
@@ -33,17 +33,9 @@
 	.set sp_offset, \offset
 
 	.if \partial
-		.if \entry
-		.set type, UNWIND_HINT_TYPE_REGS_ENTRY
-		.else
-		.set type, UNWIND_HINT_TYPE_REGS_EXIT
-		.endif
+		.set type, UNWIND_HINT_TYPE_REGS_PARTIAL
 	.elseif \extra == 0
-		.if \entry
-		.set type, UNWIND_HINT_TYPE_REGS_ENTRY
-		.else
-		.set type, UNWIND_HINT_TYPE_REGS_EXIT
-		.endif
+		.set type, UNWIND_HINT_TYPE_REGS_PARTIAL
 		.set sp_offset, \offset + (16*8)
 	.else
 		.set type, UNWIND_HINT_TYPE_REGS
@@ -52,8 +44,8 @@
 	UNWIND_HINT sp_reg=sp_reg sp_offset=sp_offset type=type
 .endm
 
-.macro UNWIND_HINT_IRET_REGS base=%rsp offset=0 entry=1
-	UNWIND_HINT_REGS base=\base offset=\offset partial=1 entry=\entry
+.macro UNWIND_HINT_IRET_REGS base=%rsp offset=0
+	UNWIND_HINT_REGS base=\base offset=\offset partial=1
 .endm
 
 .macro UNWIND_HINT_FUNC
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 92e759ae9030..816bc70c9e71 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -25,7 +25,6 @@
 #include <asm/export.h>
 #include <asm/nospec-branch.h>
 #include <asm/fixmap.h>
-#include <asm/ibt.h>
 
 /*
  * We are not able to switch in one step to the final KERNEL ADDRESS SPACE
@@ -332,8 +331,7 @@ SYM_CODE_END(start_cpu0)
  * when .init.text is freed.
  */
 SYM_CODE_START_NOALIGN(vc_boot_ghcb)
-	UNWIND_HINT_IRET_REGS offset=8 entry=1
-	ENDBR
+	UNWIND_HINT_IRET_REGS offset=8
 
 	/* Build pt_regs */
 	PUSH_AND_CLEAR_REGS
@@ -377,24 +375,25 @@ SYM_CODE_START(early_idt_handler_array)
 	i = 0
 	.rept NUM_EXCEPTION_VECTORS
 	.if ((EXCEPTION_ERRCODE_MASK >> i) & 1) == 0
-		UNWIND_HINT_IRET_REGS entry=1
+		UNWIND_HINT_IRET_REGS
 		ENDBR
 		pushq $0	# Dummy error code, to make stack frame uniform
 	.else
-		UNWIND_HINT_IRET_REGS offset=8 entry=1
+		UNWIND_HINT_IRET_REGS offset=8
 		ENDBR
 	.endif
 	pushq $i		# 72(%rsp) Vector number
 	jmp early_idt_handler_common
-	UNWIND_HINT_IRET_REGS entry=0
+	UNWIND_HINT_IRET_REGS
 	i = i + 1
 	.fill early_idt_handler_array + i*EARLY_IDT_HANDLER_SIZE - ., 1, 0xcc
 	.endr
-	UNWIND_HINT_IRET_REGS offset=16 entry=0
 SYM_CODE_END(early_idt_handler_array)
 	ANNOTATE_NOENDBR // early_idt_handler_array[NUM_EXCEPTION_VECTORS]
 
 SYM_CODE_START_LOCAL(early_idt_handler_common)
+	UNWIND_HINT_IRET_REGS offset=16
+	ANNOTATE_NOENDBR
 	/*
 	 * The stack is the hardware frame, an error code or zero, and the
 	 * vector number.
diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index fbf112c5485c..2de3c8c5eba9 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -566,8 +566,7 @@ bool unwind_next_frame(struct unwind_state *state)
 		state->signal = true;
 		break;
 
-	case UNWIND_HINT_TYPE_REGS_ENTRY:
-	case UNWIND_HINT_TYPE_REGS_EXIT:
+	case UNWIND_HINT_TYPE_REGS_PARTIAL:
 		if (!deref_stack_iret_regs(state, sp, &state->ip, &state->sp)) {
 			orc_warn_current("can't access iret registers at %pB\n",
 					 (void *)orig_ip);
diff --git a/include/linux/objtool.h b/include/linux/objtool.h
index 5281e02c2326..fd9d90ec0e48 100644
--- a/include/linux/objtool.h
+++ b/include/linux/objtool.h
@@ -35,9 +35,8 @@ struct unwind_hint {
  */
 #define UNWIND_HINT_TYPE_CALL		0
 #define UNWIND_HINT_TYPE_REGS		1
-#define UNWIND_HINT_TYPE_REGS_ENTRY	2
-#define UNWIND_HINT_TYPE_REGS_EXIT	3
-#define UNWIND_HINT_TYPE_FUNC		4
+#define UNWIND_HINT_TYPE_REGS_PARTIAL	2
+#define UNWIND_HINT_TYPE_FUNC		3
 
 #ifdef CONFIG_STACK_VALIDATION
 
diff --git a/tools/include/linux/objtool.h b/tools/include/linux/objtool.h
index c48d45733071..aca52db2f3f3 100644
--- a/tools/include/linux/objtool.h
+++ b/tools/include/linux/objtool.h
@@ -35,9 +35,8 @@ struct unwind_hint {
  */
 #define UNWIND_HINT_TYPE_CALL		0
 #define UNWIND_HINT_TYPE_REGS		1
-#define UNWIND_HINT_TYPE_REGS_ENTRY	2
-#define UNWIND_HINT_TYPE_REGS_EXIT	3
-#define UNWIND_HINT_TYPE_FUNC		4
+#define UNWIND_HINT_TYPE_REGS_PARTIAL	2
+#define UNWIND_HINT_TYPE_FUNC		3
 
 #ifdef CONFIG_STACK_VALIDATION
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 414c8a1dd868..5db0f66ab8fe 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2488,8 +2488,7 @@ static int update_cfi_state(struct instruction *insn,
 	}
 
 	if (cfi->type == UNWIND_HINT_TYPE_REGS ||
-	    cfi->type == UNWIND_HINT_TYPE_REGS_ENTRY ||
-	    cfi->type == UNWIND_HINT_TYPE_REGS_EXIT)
+	    cfi->type == UNWIND_HINT_TYPE_REGS_PARTIAL)
 		return update_cfi_state_regs(insn, cfi, op);
 
 	switch (op->dest.type) {
@@ -3254,9 +3253,13 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 		if (insn->hint) {
 			state.cfi = *insn->cfi;
 			if (ibt) {
-				if (insn->cfi->type == UNWIND_HINT_TYPE_REGS_ENTRY &&
-				    insn->type != INSN_ENDBR) {
-					WARN_FUNC("IRET_ENTRY hint without ENDBR", insn->sec, insn->offset);
+				struct symbol *sym;
+				if (insn->cfi->type == UNWIND_HINT_TYPE_REGS_PARTIAL &&
+				    (sym = find_symbol_by_offset(insn->sec, insn->offset)) &&
+				    insn->type != INSN_ENDBR && !insn->noendbr) {
+					WARN_FUNC("IRET_REGS hint without ENDBR: %s",
+						  insn->sec, insn->offset,
+						  sym->name);
 				}
 			}
 		} else {
diff --git a/tools/objtool/orc_dump.c b/tools/objtool/orc_dump.c
index 145cef3535c2..f5a8508c42d6 100644
--- a/tools/objtool/orc_dump.c
+++ b/tools/objtool/orc_dump.c
@@ -43,8 +43,7 @@ static const char *orc_type_name(unsigned int type)
 		return "call";
 	case UNWIND_HINT_TYPE_REGS:
 		return "regs";
-	case UNWIND_HINT_TYPE_REGS_ENTRY:
-	case UNWIND_HINT_TYPE_REGS_EXIT:
+	case UNWIND_HINT_TYPE_REGS_PARTIAL:
 		return "regs (partial)";
 	default:
 		return "?";

Powered by blists - more mailing lists