On Fri, Feb 14, 2025 at 10:57:51AM +0100, Peter Zijlstra wrote:
> On Thu, Feb 13, 2025 at 12:53:28PM -0800, Kees Cook wrote:
>
> > Right, the "if they can control a function pointer" is the part I'm
> > focusing on. This attack depends on making an indirect call with a
> > controlled pointer. Non-FineIBT CFI will protect against that step,
> > so I think this is only an issue for IBT-only and FineIBT, but not CFI
> > nor CFI+IBT.
>
> Yes, the whole caller side validation should stop this.
And I think we can retro-fit that in FineIBT. Notably the current call
sites look like:
0000000000000060 <fineibt_caller>:
60: 41 ba 78 56 34 12 mov $0x12345678,%r10d
66: 49 83 eb 10 sub $0x10,%r11
6a: 0f 1f 40 00 nopl 0x0(%rax)
6e: 41 ff d3 call *%r11
71: 0f 1f 00 nopl (%rax)
Of which the last 6 bytes are the retpoline site (starting at 0x6e). It
is trivially possible to re-arrange things to have both nops next to one
another, giving us 7 bytes to muck about with.
And I think we can just about manage to do a caller side hash validation
in them bytes like:
0000000000000080 <fineibt_paranoid>:
80: 41 ba 78 56 34 12 mov $0x12345678,%r10d
86: 49 83 eb 10 sub $0x10,%r11
8a: 45 3b 53 07 cmp 0x7(%r11),%r10d
8e: 74 01 je 91 <fineibt_paranoid+0x11>
90: ea (bad)
91: 41 ff d3 call *%r11
And while this is somewhat daft, it would close the hole vs this entry
point swizzle afaict, no?
Patch against tip/x86/core (which includes the latest ibt bits as per
this morning).
Boots and builds the next kernel on my ADL.
---
arch/x86/include/asm/bug.h | 1 +
arch/x86/include/asm/cfi.h | 8 ++--
arch/x86/kernel/alternative.c | 107 +++++++++++++++++++++++++++++++++++++++---
arch/x86/kernel/cfi.c | 4 +-
arch/x86/kernel/traps.c | 13 ++++-
5 files changed, 120 insertions(+), 13 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 1a5e4b372694..bc8a2ca3c82e 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -25,6 +25,7 @@
#define BUG_UD2 0xfffe
#define BUG_UD1 0xfffd
#define BUG_UD1_UBSAN 0xfffc
+#define BUG_EA 0xffea
#ifdef CONFIG_GENERIC_BUG
diff --git a/arch/x86/include/asm/cfi.h b/arch/x86/include/asm/cfi.h
index 7dd5ab239c87..550f75450e43 100644
--- a/arch/x86/include/asm/cfi.h
+++ b/arch/x86/include/asm/cfi.h
@@ -104,7 +104,7 @@ extern enum cfi_mode cfi_mode;
struct pt_regs;
#ifdef CONFIG_CFI_CLANG
-enum bug_trap_type handle_cfi_failure(struct pt_regs *regs);
+enum bug_trap_type handle_cfi_failure(int ud_type, struct pt_regs *regs);
#define __bpfcall
extern u32 cfi_bpf_hash;
extern u32 cfi_bpf_subprog_hash;
@@ -127,10 +127,10 @@ 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);
+extern bool decode_fineibt_insn(int ud_type, 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)
+decode_fineibt_insn(int ud_type, struct pt_regs *regs, unsigned long *target,
u32 *type)
{
return false;
}
@@ -138,7 +138,7 @@ decode_fineibt_insn(struct pt_regs *regs, unsigned long
*target, u32 *type)
#endif
#else
-static inline enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
+static inline enum bug_trap_type handle_cfi_failure(int ud_type, struct
pt_regs *regs)
{
return BUG_TRAP_TYPE_NONE;
}
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 247ee5ffbff4..9e327b5e9f75 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -741,6 +741,11 @@ void __init_or_module noinline apply_retpolines(s32
*start, s32 *end)
op2 = insn.opcode.bytes[1];
switch (op1) {
+ case 0x70 ... 0x7f: /* Jcc.d8 */
+ /* See cfi_paranoid. */
+ WARN_ON_ONCE(cfi_mode != CFI_FINEIBT);
+ continue;
+
case CALL_INSN_OPCODE:
case JMP32_INSN_OPCODE:
break;
@@ -983,6 +988,8 @@ u32 cfi_get_func_hash(void *func)
static bool cfi_rand __ro_after_init = true;
static u32 cfi_seed __ro_after_init;
+static bool cfi_paranoid __ro_after_init = false;
+
/*
* Re-hash the CFI hash with a boot-time seed while making sure the result is
* not a valid ENDBR instruction.
@@ -1022,6 +1029,8 @@ static __init int cfi_parse_cmdline(char *str)
cfi_mode = CFI_FINEIBT;
} else if (!strcmp(str, "norand")) {
cfi_rand = false;
+ } else if (!strcmp(str, "paranoid")) {
+ cfi_paranoid = true;
} else {
pr_err("Ignoring unknown cfi option (%s).", str);
}
@@ -1097,6 +1106,29 @@ extern u8 fineibt_caller_end[];
#define fineibt_caller_jmp (fineibt_caller_size - 2)
+asm( ".pushsection .rodata \n"
+ "fineibt_paranoid_start: \n"
+ " movl $0x12345678, %r10d \n"
+ " sub $16, %r11 \n"
+ " cmpl 7(%r11), %r10d \n"
+ " je fineibt_paranoid_call \n"
+ "fineibt_paranoid_trap: \n"
+ " .byte 0xea \n"
+ "fineibt_paranoid_call: \n"
+ " call *%r11 \n"
+ "fineibt_paranoid_end: \n"
+ ".popsection \n"
+);
+
+extern u8 fineibt_paranoid_start[];
+extern u8 fineibt_paranoid_trap[];
+extern u8 fineibt_paranoid_call[];
+extern u8 fineibt_paranoid_end[];
+
+#define fineibt_paranoid_size (fineibt_paranoid_end - fineibt_paranoid_start)
+#define fineibt_paranoid_ud (fineibt_paranoid_trap - fineibt_paranoid_start)
+#define fineibt_paranoid_ind (fineibt_paranoid_call - fineibt_paranoid_start)
+
static u32 decode_preamble_hash(void *addr)
{
u8 *p = addr;
@@ -1260,18 +1292,48 @@ static int cfi_rewrite_callers(s32 *start, s32 *end)
{
s32 *s;
+ BUG_ON(fineibt_paranoid_size != 20);
+
for (s = start; s < end; s++) {
void *addr = (void *)s + *s;
+ struct insn insn;
+ u8 bytes[20];
u32 hash;
+ int ret;
+ u8 op;
addr -= fineibt_caller_size;
hash = decode_caller_hash(addr);
- if (hash) {
+ if (!hash)
+ continue;
+
+ if (!cfi_paranoid) {
text_poke_early(addr, fineibt_caller_start,
fineibt_caller_size);
WARN_ON(*(u32 *)(addr + fineibt_caller_hash) !=
0x12345678);
text_poke_early(addr + fineibt_caller_hash, &hash, 4);
+ /* rely on apply_retpolines() */
+ continue;
}
- /* rely on apply_retpolines() */
+
+ /* cfi_paranoid */
+ ret = insn_decode_kernel(&insn, addr + fineibt_caller_size);
+ if (WARN_ON_ONCE(ret < 0))
+ continue;
+
+ op = insn.opcode.bytes[0];
+ if (op != CALL_INSN_OPCODE && op != JMP32_INSN_OPCODE) {
+ WARN_ON_ONCE(1);
+ continue;
+ }
+
+ memcpy(bytes, fineibt_paranoid_start, fineibt_paranoid_size);
+ memcpy(bytes + fineibt_caller_hash, &hash, 4);
+
+ ret = emit_indirect(op, 11, bytes + fineibt_paranoid_ind);
+ if (WARN_ON_ONCE(ret != 3))
+ continue;
+
+ text_poke_early(addr, bytes, fineibt_paranoid_size);
}
return 0;
@@ -1288,8 +1350,11 @@ static void __apply_fineibt(s32 *start_retpoline, s32
*end_retpoline,
if (cfi_mode == CFI_AUTO) {
cfi_mode = CFI_KCFI;
- if (HAS_KERNEL_IBT && cpu_feature_enabled(X86_FEATURE_IBT))
+ if (HAS_KERNEL_IBT && cpu_feature_enabled(X86_FEATURE_IBT)) {
+ if (!cpu_feature_enabled(X86_FEATURE_FRED))
+ cfi_paranoid = true;
cfi_mode = CFI_FINEIBT;
+ }
}
/*
@@ -1346,8 +1411,10 @@ static void __apply_fineibt(s32 *start_retpoline, s32
*end_retpoline,
/* now that nobody targets func()+0, remove ENDBR there */
cfi_rewrite_endbr(start_cfi, end_cfi);
- if (builtin)
- pr_info("Using FineIBT CFI\n");
+ if (builtin) {
+ pr_info("Using FineIBT %s CFI\n",
+ cfi_paranoid ? "paranoid" : "");
+ }
return;
default:
@@ -1420,7 +1487,8 @@ static void poison_cfi(void *addr)
* 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)
+static bool decode_fineibt_preamble(int ud_type, struct pt_regs *regs,
+ unsigned long *target, u32 *type)
{
unsigned long addr = regs->ip - fineibt_preamble_ud2;
u32 endbr, hash;
@@ -1440,6 +1508,33 @@ bool decode_fineibt_insn(struct pt_regs *regs, unsigned
long *target, u32 *type)
return false;
}
+/*
+ * regs->ip points to a 0xea instruction from the fineibt_paranoid_start[]
+ * sequence.
+ */
+static bool decode_fineibt_paranoid(int ud_type, struct pt_regs *regs,
+ unsigned long *target, u32 *type)
+{
+ unsigned long addr = regs->ip - fineibt_paranoid_ud;
+ u32 hash;
+
+ __get_kernel_nofault(&hash, addr + fineibt_caller_hash, u32, Efault);
+ *target = regs->r11 + 16;
+ *type = regs->r10;
+ return true;
+
+Efault:
+ return false;
+}
+
+bool decode_fineibt_insn(int ud_type, struct pt_regs *regs,
+ unsigned long *target, u32 *type)
+{
+ if (ud_type == BUG_EA)
+ return decode_fineibt_paranoid(ud_type, regs, target, type);
+ return decode_fineibt_preamble(ud_type, regs, target, type);
+}
+
#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 f6905bef0af8..f9eb7465eec6 100644
--- a/arch/x86/kernel/cfi.c
+++ b/arch/x86/kernel/cfi.c
@@ -65,7 +65,7 @@ static bool decode_cfi_insn(struct pt_regs *regs, unsigned
long *target,
* 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.
*/
-enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
+enum bug_trap_type handle_cfi_failure(int ud_type, struct pt_regs *regs)
{
unsigned long target;
u32 type;
@@ -81,7 +81,7 @@ enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
break;
case CFI_FINEIBT:
- if (!decode_fineibt_insn(regs, &target, &type))
+ if (!decode_fineibt_insn(ud_type, regs, &target, &type))
return BUG_TRAP_TYPE_NONE;
break;
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 05b86c05e446..500030ab8036 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -113,6 +113,10 @@ __always_inline int decode_bug(unsigned long addr, s32
*imm, int *len)
v = *(u8 *)(addr++);
if (v == INSN_ASOP)
v = *(u8 *)(addr++);
+ if (v == 0xea) {
+ *len = addr - start;
+ return BUG_EA;
+ }
if (v != OPCODE_ESCAPE)
return BUG_NONE;
@@ -308,9 +312,16 @@ static noinstr bool handle_bug(struct pt_regs *regs)
raw_local_irq_enable();
switch (ud_type) {
+ case BUG_EA:
+ if (handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
+ regs->ip += ud_len;
+ handled = true;
+ }
+ break;
+
case BUG_UD2:
if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN ||
- handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
+ handle_cfi_failure(ud_type, regs) == BUG_TRAP_TYPE_WARN) {
regs->ip += ud_len;
handled = true;
}
