Hi! Respin of the FineIBT-BHI patches. Scott has managed to get LLVM bits merged: https://github.com/llvm/llvm-project/commit/e223485c9b38a5579991b8cebb6a200153eee245 Which prompted me to update these patches once again. They boot and build the next kernel on my ADL when booted with: cfi=fineibt+bhi Aside from the last two patches -- which implement the FineIBT-BHI scheme proper -- I'm planning on getting these patches merged 'soon'. Scott, what those last two patches need, aside from a lot more testing, is a coherent writeup of how the mitigation works and ideally also a few numbers proving the performance gains are worth it. Last version at: https://lore.kernel.org/all/20240927194856.096003183@infradead.org/T/#u Current patches: git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/fineibt-bhi Patches apply on top of tip/master.
Hi Peter, On Fri, Feb 7, 2025 at 4:28 AM Peter Zijlstra <peterz@infradead.org> wrote: > > Hi! > > Respin of the FineIBT-BHI patches. > > Scott has managed to get LLVM bits merged: > > https://github.com/llvm/llvm-project/commit/e223485c9b38a5579991b8cebb6a200153eee245 > > Which prompted me to update these patches once again. > > They boot and build the next kernel on my ADL when booted with: cfi=fineibt+bhi > > Aside from the last two patches -- which implement the FineIBT-BHI scheme > proper -- I'm planning on getting these patches merged 'soon'. > > Scott, what those last two patches need, aside from a lot more testing, is a > coherent writeup of how the mitigation works and ideally also a few numbers > proving the performance gains are worth it. > > Last version at: > > https://lore.kernel.org/all/20240927194856.096003183@infradead.org/T/#u > > Current patches: > > git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/fineibt-bhi > > Patches apply on top of tip/master. I gave this a spin with a ToT Clang; LKDTM 's CFI_FORWARD_PROTO test now traps in __bhi_args_1 as expected, and the changes look good to me. The is_endbr() clean-up also fixes the gendwarfksyms+ftrace build issue reported earlier. Feel free to add: Reviewed-by: Sami Tolvanen <samitolvanen@google.com> One thing I realized is that CONFIG_CFI_PERMISSIVE doesn't actually do anything when FineIBT is used since we lose track of CFI trap locations. I'm not sure if that's worth fixing, but perhaps we could disable FineIBT when permissive mode is enabled to avoid confusion? Sami
On Mon, Feb 10, 2025 at 10:29:22AM -0800, Sami Tolvanen wrote: > Hi Peter, > > On Fri, Feb 7, 2025 at 4:28 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > > Hi! > > > > Respin of the FineIBT-BHI patches. > > > > Scott has managed to get LLVM bits merged: > > > > https://github.com/llvm/llvm-project/commit/e223485c9b38a5579991b8cebb6a200153eee245 > > > > Which prompted me to update these patches once again. > > > > They boot and build the next kernel on my ADL when booted with: cfi=fineibt+bhi > > > > Aside from the last two patches -- which implement the FineIBT-BHI scheme > > proper -- I'm planning on getting these patches merged 'soon'. > > > > Scott, what those last two patches need, aside from a lot more testing, is a > > coherent writeup of how the mitigation works and ideally also a few numbers > > proving the performance gains are worth it. > > > > Last version at: > > > > https://lore.kernel.org/all/20240927194856.096003183@infradead.org/T/#u > > > > Current patches: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/fineibt-bhi > > > > Patches apply on top of tip/master. > > I gave this a spin with a ToT Clang; LKDTM 's CFI_FORWARD_PROTO test > now traps in __bhi_args_1 as expected, and the changes look good to > me. The is_endbr() clean-up also fixes the gendwarfksyms+ftrace build > issue reported earlier. Feel free to add: > > Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Thanks! > One thing I realized is that CONFIG_CFI_PERMISSIVE doesn't actually do > anything when FineIBT is used since we lose track of CFI trap > locations. I'm not sure if that's worth fixing, but perhaps we could > disable FineIBT when permissive mode is enabled to avoid confusion? Hmm, yeah, that's one thing I keep forgetting about. Let me try and fix it and see how ugly it gets before offering an opinion :-)
On Thu, Feb 13, 2025 at 11:48:02AM +0100, Peter Zijlstra wrote:
> > One thing I realized is that CONFIG_CFI_PERMISSIVE doesn't actually do
> > anything when FineIBT is used since we lose track of CFI trap
> > locations. I'm not sure if that's worth fixing, but perhaps we could
> > disable FineIBT when permissive mode is enabled to avoid confusion?
>
> Hmm, yeah, that's one thing I keep forgetting about. Let me try and fix
> it and see how ugly it gets before offering an opinion :-)
Completely untested and on top of this series minus the last two patches
-- basically what I just pushed into:
git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/core
WDYT?
---
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index e285933506e9..1fec1e445a25 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1410,6 +1410,36 @@ static void poison_cfi(void *addr)
}
}
+bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type)
+{
+ /*
+ * FineIBT preamble:
+ *
+ * __cfi_foo:
+ * endbr64
+ * subl $0x12345678, %r10d
+ * jz 1f
+ * ud2
+ * 1: nop
+ * foo:
+ *
+ * regs->ip points to the UD2 instruction.
+ */
+ unsigned long addr = regs->ip - (4+7+2);
+ u32 hash;
+
+ if (!is_endbr((void *)addr)) {
+Efault:
+ return false;
+ }
+
+ *target = addr + fineibt_preamble_size;
+
+ __get_kernel_nofault(&hash, *(u32 *)(addr + fineibt_preamble_hash), u32, Efault);
+ *type = (u32)regs->r10 + hash;
+ return true;
+}
+
#else
static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
@@ -1421,6 +1451,11 @@ static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
static void poison_cfi(void *addr) { }
#endif
+bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type)
+{
+ return false;
+}
+
#endif
void apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
diff --git a/arch/x86/kernel/cfi.c b/arch/x86/kernel/cfi.c
index e6bf78fac146..4c682076c656 100644
--- a/arch/x86/kernel/cfi.c
+++ b/arch/x86/kernel/cfi.c
@@ -61,6 +61,8 @@ static bool decode_cfi_insn(struct pt_regs *regs, unsigned long *target,
return true;
}
+extern bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type);
+
/*
* Checks if a ud2 trap is because of a CFI failure, and handles the trap
* if needed. Returns a bug_trap_type value similarly to report_bug.
@@ -70,11 +72,25 @@ enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
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);
}
On Thu, Feb 13, 2025 at 12:45:47PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 13, 2025 at 11:48:02AM +0100, Peter Zijlstra wrote:
>
> > > One thing I realized is that CONFIG_CFI_PERMISSIVE doesn't actually do
> > > anything when FineIBT is used since we lose track of CFI trap
> > > locations. I'm not sure if that's worth fixing, but perhaps we could
> > > disable FineIBT when permissive mode is enabled to avoid confusion?
> >
> > Hmm, yeah, that's one thing I keep forgetting about. Let me try and fix
> > it and see how ugly it gets before offering an opinion :-)
>
> Completely untested and on top of this series minus the last two patches
> -- basically what I just pushed into:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/core
>
> WDYT?
>
> ---
>
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index e285933506e9..1fec1e445a25 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1410,6 +1410,36 @@ static void poison_cfi(void *addr)
> }
> }
>
> +bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type)
> +{
> + /*
> + * FineIBT preamble:
> + *
> + * __cfi_foo:
> + * endbr64
> + * subl $0x12345678, %r10d
> + * jz 1f
> + * ud2
> + * 1: nop
> + * foo:
> + *
> + * regs->ip points to the UD2 instruction.
> + */
> + unsigned long addr = regs->ip - (4+7+2);
> + u32 hash;
> +
> + if (!is_endbr((void *)addr)) {
> +Efault:
> + return false;
> + }
> +
> + *target = addr + fineibt_preamble_size;
> +
> + __get_kernel_nofault(&hash, *(u32 *)(addr + fineibt_preamble_hash), u32, Efault);
You have an extra * here, should be just (u32 *).
> + *type = (u32)regs->r10 + hash;
> + return true;
> +}
> +
> #else
>
> static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
> @@ -1421,6 +1451,11 @@ static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
> static void poison_cfi(void *addr) { }
> #endif
>
> +bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type)
> +{
> + return false;
> +}
> +
> #endif
>
> void apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
> diff --git a/arch/x86/kernel/cfi.c b/arch/x86/kernel/cfi.c
> index e6bf78fac146..4c682076c656 100644
> --- a/arch/x86/kernel/cfi.c
> +++ b/arch/x86/kernel/cfi.c
> @@ -61,6 +61,8 @@ static bool decode_cfi_insn(struct pt_regs *regs, unsigned long *target,
> return true;
> }
>
> +extern bool decode_fineibt_insn(struct pt_regs *regs, unsigned long *target, u32 *type);
> +
> /*
> * Checks if a ud2 trap is because of a CFI failure, and handles the trap
> * if needed. Returns a bug_trap_type value similarly to report_bug.
> @@ -70,11 +72,25 @@ enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
> 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);
> }
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@google.com>
Sami
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@google.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@infradead.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@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Sami Tolvanen <samitolvanen@google.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);
}
The following commit has been merged into the x86/core branch of tip:
Commit-ID: 882b86fd4e0d49bf91148dbadcdbece19ded40e6
Gitweb: https://git.kernel.org/tip/882b86fd4e0d49bf91148dbadcdbece19ded40e6
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 13 Feb 2025 12:45:47 +01:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 14 Feb 2025 10:32:07 +01:00
x86/ibt: Handle FineIBT in handle_cfi_failure()
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@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Link: https://lkml.kernel.org/r/20250214092619.GB21726@noisy.programming.kicks-ass.net
---
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(-)
diff --git a/arch/x86/include/asm/cfi.h b/arch/x86/include/asm/cfi.h
index 31d19c8..7dd5ab2 100644
--- 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)
{
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index e285933..247ee5f 100644
--- 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,
diff --git a/arch/x86/kernel/cfi.c b/arch/x86/kernel/cfi.c
index e6bf78f..f6905be 100644
--- a/arch/x86/kernel/cfi.c
+++ b/arch/x86/kernel/cfi.c
@@ -70,11 +70,25 @@ enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
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);
}© 2016 - 2026 Red Hat, Inc.