linux-kernel - Re: [PATCH 00/11] x86/ibt: FineIBT-BHI

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: <20250214092619.GB21726@noisy.programming.kicks-ass.net>
Date: Fri, 14 Feb 2025 10:26:19 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Sami Tolvanen <samitolvanen@...gle.com>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, alyssa.milburn@...el.com,
	scott.d.constable@...el.com, joao@...rdrivepizza.com,
	andrew.cooper3@...rix.com, jpoimboe@...nel.org,
	jose.marchesi@...cle.com, hjl.tools@...il.com,
	ndesaulniers@...gle.com, nathan@...nel.org, ojeda@...nel.org,
	kees@...nel.org, alexei.starovoitov@...il.com, mhiramat@...nel.org
Subject: Re: [PATCH 00/11] x86/ibt: FineIBT-BHI

On Thu, Feb 13, 2025 at 07:12:02PM +0000, Sami Tolvanen wrote:

> > +	__get_kernel_nofault(&hash, *(u32 *)(addr + fineibt_preamble_hash), u32, Efault);
> 
> You have an extra * here, should be just (u32 *).

Doh :-), I started by doing a plain deref and then figured I should be
careful and wrap it in the nofault thing.


> Otherwise, LGTM.
> 
> One minor issue is that since the trap is in the preamble, we don't
> get caller information in the warning message:
> 
> [   19.080184] CFI failure at __cfi_lkdtm_increment_int+0xd/0x10
> (target: lkdtm_increment_int+0x0/0x20; expected type: 0x7e0c52a5)
> 
> But this is followed by a call trace, so it's not really necessary
> either. With the __get_kernel_nofault argument fixed:
> 
> Reviewed-by: Sami Tolvanen <samitolvanen@...gle.com>

Thanks.

I did some more clean-ups. I'll stick it on top of those patches slated
for x86/core.

---
Subject: x86/ibt: Handle FineIBT in handle_cfi_failure()
From: Peter Zijlstra <peterz@...radead.org>
Date: Thu, 13 Feb 2025 12:45:47 +0100

Sami reminded me that FineIBT failure does not hook into the regular
CFI failure case, and as such CFI_PERMISSIVE does not work.

Reported-by: Sami Tolvanen <samitolvanen@...gle.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
Reviewed-by: Sami Tolvanen <samitolvanen@...gle.com>
---
 arch/x86/include/asm/cfi.h    |   11 +++++++++++
 arch/x86/kernel/alternative.c |   30 ++++++++++++++++++++++++++++++
 arch/x86/kernel/cfi.c         |   22 ++++++++++++++++++----
 3 files changed, 59 insertions(+), 4 deletions(-)

--- a/arch/x86/include/asm/cfi.h
+++ b/arch/x86/include/asm/cfi.h
@@ -126,6 +126,17 @@ static inline int cfi_get_offset(void)
 
 extern u32 cfi_get_func_hash(void *func);
 
+#ifdef CONFIG_FINEIBT
+extern bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type);
+#else
+static inline bool
+decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type)
+{
+	return false;
+}
+
+#endif
+
 #else
 static inline enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
 {
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1065,6 +1065,7 @@ asm(	".pushsection .rodata			\n"
 	"	endbr64				\n"
 	"	subl	$0x12345678, %r10d	\n"
 	"	je	fineibt_preamble_end	\n"
+	"fineibt_preamble_ud2:			\n"
 	"	ud2				\n"
 	"	nop				\n"
 	"fineibt_preamble_end:			\n"
@@ -1072,9 +1073,11 @@ asm(	".pushsection .rodata			\n"
 );
 
 extern u8 fineibt_preamble_start[];
+extern u8 fineibt_preamble_ud2[];
 extern u8 fineibt_preamble_end[];
 
 #define fineibt_preamble_size (fineibt_preamble_end - fineibt_preamble_start)
+#define fineibt_preamble_ud2  (fineibt_preamble_ud2 - fineibt_preamble_start)
 #define fineibt_preamble_hash 7
 
 asm(	".pushsection .rodata			\n"
@@ -1410,6 +1413,33 @@ static void poison_cfi(void *addr)
 	}
 }
 
+/*
+ * regs->ip points to a UD2 instruction, return true and fill out target and
+ * type when this UD2 is from a FineIBT preamble.
+ *
+ * We check the preamble by checking for the ENDBR instruction relative to the
+ * UD2 instruction.
+ */
+bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type)
+{
+	unsigned long addr = regs->ip - fineibt_preamble_ud2;
+	u32 endbr, hash;
+
+	__get_kernel_nofault(&endbr, addr, u32, Efault);
+	if (endbr != gen_endbr())
+		return false;
+
+	*target = addr + fineibt_preamble_size;
+
+	__get_kernel_nofault(&hash, addr + fineibt_preamble_hash, u32, Efault);
+	*type = (u32)regs->r10 + hash;
+
+	return true;
+
+Efault:
+	return false;
+}
+
 #else
 
 static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
--- a/arch/x86/kernel/cfi.c
+++ b/arch/x86/kernel/cfi.c
@@ -70,11 +70,25 @@ enum bug_trap_type handle_cfi_failure(st
 	unsigned long target;
 	u32 type;
 
-	if (!is_cfi_trap(regs->ip))
-		return BUG_TRAP_TYPE_NONE;
+	switch (cfi_mode) {
+	case CFI_KCFI:
+		if (!is_cfi_trap(regs->ip))
+			return BUG_TRAP_TYPE_NONE;
+
+		if (!decode_cfi_insn(regs, &target, &type))
+			return report_cfi_failure_noaddr(regs, regs->ip);
+
+		break;
 
-	if (!decode_cfi_insn(regs, &target, &type))
-		return report_cfi_failure_noaddr(regs, regs->ip);
+	case CFI_FINEIBT:
+		if (!decode_fineibt_insn(regs, &target, &type))
+			return BUG_TRAP_TYPE_NONE;
+
+		break;
+
+	default:
+		return BUG_TRAP_TYPE_NONE;
+	}
 
 	return report_cfi_failure(regs, regs->ip, &target, type);
 }