Due to FineIBT weakness, add an additional mitigation for BHI.
Use the 5 bytes of the nop at -1 and the 4 byte poison to squirrel in
a BHI mitigation.
Relies on clang-cfi to emit an additional piece of magic in the kCFI
pre-amble, identifying which function arguments are pointers.
An additional u8 (next to the existing u32) is emitted like:
movl 0x12345678, %eax // CFI hash
movb 0x12, %al // CFI args
This u8 is a bitmask, where BIT(n) indicates the n'th argument is a
pointer, notably the 6 possible argument registers are:
rdi, rsi, rdx, rcx, r8 and r9
Single bit can be inlined, while 2-4 bits have combinatoric stubs with
the required magic in. Anything more will fall back to
__bhi_args_all which additionally poisons %rsp for good measure, in
case things overflowed to the stack.
FineIBT+ FineIBT+BHI
__cfi_foo: __cfi_foo:
endbr64 endbr64
subl $0x12345678, %r10d subl $0x12345678, %r10d
jz foo+4 jz +2
ud2 ud2
nop call __bhi_args_foo
foo: foo+4:
ud1 0x0(%eax), %eax
... ...
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/include/asm/cfi.h | 1
arch/x86/kernel/alternative.c | 82 ++++++++++++++++++++++++++++++++++++++----
arch/x86/net/bpf_jit_comp.c | 16 ++++++--
tools/objtool/check.c | 16 ++++----
4 files changed, 98 insertions(+), 17 deletions(-)
--- a/arch/x86/include/asm/cfi.h
+++ b/arch/x86/include/asm/cfi.h
@@ -97,6 +97,7 @@ enum cfi_mode {
CFI_OFF, /* Taditional / IBT depending on .config */
CFI_KCFI, /* Optionally CALL_PADDING, IBT, RETPOLINE */
CFI_FINEIBT, /* see arch/x86/kernel/alternative.c */
+ CFI_FINEIBT_BHI,
};
extern enum cfi_mode cfi_mode;
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -932,7 +932,31 @@ __noendbr bool is_endbr(u32 *val)
if (get_kernel_nofault(endbr, val))
return false;
- return __is_endbr(endbr);
+ if (__is_endbr(endbr))
+ return true;
+
+#if defined(CONFIG_FINEIBT) && defined(CONFIG_X86_KERNEL_IBT_PLUS)
+ if (cfi_mode != CFI_FINEIBT_BHI)
+ return false;
+
+ /*
+ * The BHI clobbers 'replace' the ENDBR poison, but dynamic call
+ * patching (static_call, kprobes, etc..) still need to be able
+ * to identify and skip the foo()+0 'endbr'.
+ */
+
+ /* REX CMOVNE, see bhi_args_1() */
+ if ((endbr & 0xc2fffff9) == 0xc2450f49)
+ return true;
+
+ /* CALL __bhi_args_* */
+ void *dest = (void *)val + 4 + (s32)endbr;
+ if (dest >= (void *)__bhi_args_6c1 &&
+ dest <= (void *)__bhi_args_all)
+ return true;
+#endif
+
+ return false;
}
static void poison_cfi(void *addr);
@@ -1190,6 +1214,8 @@ static __init int cfi_parse_cmdline(char
cfi_mode = CFI_KCFI;
} else if (!strcmp(str, "fineibt")) {
cfi_mode = CFI_FINEIBT;
+ } else if (IS_ENABLED(CONFIG_X86_KERNEL_IBT_PLUS) && !strcmp(str, "fineibt+bhi")) {
+ cfi_mode = CFI_FINEIBT_BHI;
} else if (!strcmp(str, "norand")) {
cfi_rand = false;
} else {
@@ -1208,10 +1234,9 @@ early_param("cfi", cfi_parse_cmdline);
*
* __cfi_\func: __cfi_\func:
* movl $0x12345678,%eax // 5 endbr64 // 4
- * nop subl $0x12345678,%r10d // 7
+ * movb $0x12,%al // 2 subl $0x12345678,%r10d // 7
* nop jz 1f // 2
* nop ud2 // 2
- * nop 1: nop // 1
* nop
* nop
* nop
@@ -1279,6 +1304,17 @@ static u32 decode_preamble_hash(void *ad
return 0; /* invalid hash value */
}
+static u8 decode_preamble_args(void *addr)
+{
+ u8 *p = addr;
+
+ /* b0 12 movb $0x12, %al */
+ if (p[5] == 0xb0)
+ return p[6];
+
+ return 0xff; /* invalid args */
+}
+
static u32 decode_caller_hash(void *addr)
{
u8 *p = addr;
@@ -1371,6 +1407,7 @@ static int cfi_rewrite_preamble(s32 *sta
for (s = start; s < end; s++) {
void *addr = (void *)s + *s;
u32 hash;
+ u8 args;
/*
* When the function doesn't start with ENDBR the compiler will
@@ -1385,11 +1422,25 @@ static int cfi_rewrite_preamble(s32 *sta
addr, addr, 5, addr))
return -EINVAL;
+ args = decode_preamble_args(addr);
+
text_poke_early(addr, fineibt_preamble_start, fineibt_preamble_size);
WARN_ON(*(u32 *)(addr + fineibt_preamble_hash) != 0x12345678);
text_poke_early(addr + fineibt_preamble_hash, &hash, 4);
*(u8 *)(addr + fineibt_preamble_jccd8) += 4;
+
+ if (cfi_mode != CFI_FINEIBT_BHI)
+ continue;
+
+ WARN_ONCE(args == 0xff, "no CFI args found at %pS %px %*ph\n",
+ addr, addr, 7, addr);
+
+ /*
+ * Stash the ARGS byte in the NOP at __cfi_foo+15, see
+ * cfi_rewrite_endbr().
+ */
+ *(u8 *)(addr + fineibt_preamble_size - 1) = args;
}
return 0;
@@ -1401,11 +1452,26 @@ static void cfi_rewrite_endbr(s32 *start
for (s = start; s < end; s++) {
void *addr = (void *)s + *s;
+ u8 args;
if (!is_endbr(addr + 16))
continue;
- poison_endbr(addr + 16);
+ if (cfi_mode != CFI_FINEIBT_BHI) {
+ poison_endbr(addr + 16);
+ continue;
+ }
+
+ /* recover the args byte */
+ args = *(u8 *)(addr + fineibt_preamble_size - 1);
+ *(u8 *)(addr + fineibt_preamble_size - 1) = BYTES_NOP1;
+ if (args) {
+ /* only skip the UD2 */
+ *(u8 *)(addr + fineibt_preamble_jccd8) = 2;
+
+ /* write BHI clobber in the 5 bytes that hold: nop + poison */
+ bhi_args(args, addr + fineibt_preamble_size - 1);
+ }
}
}
@@ -1506,6 +1572,7 @@ static void __apply_fineibt(s32 *start_r
return;
case CFI_FINEIBT:
+ case CFI_FINEIBT_BHI:
/* place the FineIBT preamble at func()-16 */
ret = cfi_rewrite_preamble(start_cfi, end_cfi);
if (ret)
@@ -1519,8 +1586,10 @@ static void __apply_fineibt(s32 *start_r
/* 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_mode == CFI_FINEIBT_BHI ? "+BHI" : "");
+ }
return;
default:
@@ -1548,6 +1617,7 @@ static void poison_cfi(void *addr)
*/
switch (cfi_mode) {
case CFI_FINEIBT:
+ case CFI_FINEIBT_BHI:
/*
* FineIBT prefix should start with an ENDBR.
*/
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -401,10 +401,17 @@ static void emit_fineibt(u8 **pprog, u32
EMIT_ENDBR();
EMIT3_off32(0x41, 0x81, 0xea, hash); /* subl $hash, %r10d */
- EMIT2(0x74, 0x07); /* jz.d8 +7 */
- EMIT2(0x0f, 0x0b); /* ud2 */
- EMIT1(0x90); /* nop */
- EMIT_ENDBR_POISON();
+ if (cfi_mode == CFI_FINEIBT_BHI) {
+ EMIT2(0x74, 0x02); /* jz.d8 +2 */
+ EMIT2(0x0f, 0x0b); /* ud2 */
+ EMIT1(0x2e); /* cs */
+ EMIT4(0x49, 0x0f, 0x45, 0xfa); /* cmovne %r10, %rdi */
+ } else {
+ EMIT2(0x74, 0x07); /* jz.d8 +7 */
+ EMIT2(0x0f, 0x0b); /* ud2 */
+ EMIT1(0x90); /* nop */
+ EMIT_ENDBR_POISON();
+ }
*pprog = prog;
}
@@ -438,6 +445,7 @@ static void emit_cfi(u8 **pprog, u32 has
switch (cfi_mode) {
case CFI_FINEIBT:
+ case CFI_FINEIBT_BHI:
emit_fineibt(&prog, hash);
break;
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4397,11 +4397,9 @@ static int validate_ibt_insn(struct objt
continue;
off = reloc->sym->offset;
- if (reloc_type(reloc) == R_X86_64_PC32 ||
- reloc_type(reloc) == R_X86_64_PLT32)
- off += arch_dest_reloc_offset(reloc_addend(reloc));
- else
- off += reloc_addend(reloc);
+ off += reloc_addend(reloc);
+ if (arch_pc_relative_reloc(reloc))
+ off = arch_dest_reloc_offset(off);
dest = find_insn(file, reloc->sym->sec, off);
if (!dest)
@@ -4456,10 +4454,14 @@ static int validate_ibt_insn(struct objt
static int validate_ibt_data_reloc(struct objtool_file *file,
struct reloc *reloc)
{
+ long offset = reloc->sym->offset;
struct instruction *dest;
- dest = find_insn(file, reloc->sym->sec,
- reloc->sym->offset + reloc_addend(reloc));
+ offset += reloc_addend(reloc);
+ if (reloc_type(reloc) == R_X86_64_PLT32) // the fuck ?!
+ offset = arch_dest_reloc_offset(offset);
+
+ dest = find_insn(file, reloc->sym->sec, offset);
if (!dest)
return 0;On Fri, Sep 27, 2024 at 09:49:10PM +0200, Peter Zijlstra wrote:
> @@ -1190,6 +1214,8 @@ static __init int cfi_parse_cmdline(char
> cfi_mode = CFI_KCFI;
> } else if (!strcmp(str, "fineibt")) {
> cfi_mode = CFI_FINEIBT;
> + } else if (IS_ENABLED(CONFIG_X86_KERNEL_IBT_PLUS) && !strcmp(str, "fineibt+bhi")) {
> + cfi_mode = CFI_FINEIBT_BHI;
> } else if (!strcmp(str, "norand")) {
> cfi_rand = false;
> } else {
Do we need to hook this in with bugs.c somehow so it skips the other BHI
mitigations?
--
Josh
On Fri, Sep 27, 2024 at 06:50:06PM -0700, Josh Poimboeuf wrote:
> On Fri, Sep 27, 2024 at 09:49:10PM +0200, Peter Zijlstra wrote:
> > @@ -1190,6 +1214,8 @@ static __init int cfi_parse_cmdline(char
> > cfi_mode = CFI_KCFI;
> > } else if (!strcmp(str, "fineibt")) {
> > cfi_mode = CFI_FINEIBT;
> > + } else if (IS_ENABLED(CONFIG_X86_KERNEL_IBT_PLUS) && !strcmp(str, "fineibt+bhi")) {
> > + cfi_mode = CFI_FINEIBT_BHI;
> > } else if (!strcmp(str, "norand")) {
> > cfi_rand = false;
> > } else {
>
> Do we need to hook this in with bugs.c somehow so it skips the other BHI
> mitigations?
Yeah.. those didn't exist when I started this code :-) But yeah, once we
get to the point of doing this patch for real -- the compiler(s) have
the required features implemented properly and everyrhing, this should
be hooked up better.
> > On Fri, Sep 27, 2024 at 09:49:10PM +0200, Peter Zijlstra wrote:
> > > Due to FineIBT weakness, add an additional mitigation for BHI.
> > >
> > > Use the 5 bytes of the nop at -1 and the 4 byte poison to squirrel in a BHI mitigation.
> > >
> > > Relies on clang-cfi to emit an additional piece of magic in the kCFI pre-amble, identifying which function arguments are pointers.
> > >
> > > An additional u8 (next to the existing u32) is emitted like:
> > >
> > > movl 0x12345678, %eax // CFI hash
> > > movb 0x12, %al // CFI args
> > >
> > > This u8 is a bitmask, where BIT(n) indicates the n'th argument is a pointer, notably the 6 possible argument registers are:
> > >
> > > rdi, rsi, rdx, rcx, r8 and r9
> > >
> > > Single bit can be inlined, while 2-4 bits have combinatoric stubs with the required magic in. Anything more will fall back to __bhi_args_all which additionally poisons %rsp for good measure, in case things overflowed to the stack.
Hi Peter,
Instead of:
movl 0x12345678, %eax // CFI hash
movb 0x12, %al // CFI args
Can we do this?
movb 0x12, %al // CFI args
movl 0x12345678, %eax // CFI hash
The latter ordering does not affect the kCFI hash's location, whereas the former ordering shifts the location of the kCFI hash backward by two bytes, which creates more work for the compiler.
Also, when I build LLVM and Linux with these patches, my kernel crashes. The root cause appears to be a call from bpf_dispatcher_nop_func:
0xffffffff813e69f1 <+209>: mov %rbx,%rdi
0xffffffff813e69f4 <+212>: mov $0x9b5328da,%r10d
0xffffffff813e69fa <+218>: sub $0x10,%r11
0xffffffff813e69fe <+222>: nopl 0x0(%rax)
0xffffffff813e6a02 <+226>: call *%r11 # This is the problematic call
to some function that I don't see in the kernel image, and that I suspect is being generated at runtime:
0xffffffffa0000794: int3 # The call instruction lands here...
0xffffffffa0000795: int3
0xffffffffa0000796: int3
0xffffffffa0000797: int3
0xffffffffa0000798: int3
0xffffffffa0000799: int3
0xffffffffa000079a: int3
0xffffffffa000079b: int3
0xffffffffa000079c: int3
0xffffffffa000079d: int3
0xffffffffa000079e: int3
0xffffffffa000079f: int3
0xffffffffa00007a0: int3
0xffffffffa00007a1: int3
0xffffffffa00007a2: int3
0xffffffffa00007a3: int3
0xffffffffa00007a4: endbr64 # ... but it should land here
0xffffffffa00007a8: sub $0x9b5328da,%r10d
0xffffffffa00007af: je 0xffffffffa00007b3
0xffffffffa00007b1: ud2
0xffffffffa00007b3: cs cmovne %r10,%rdi
0xffffffffa00007b8: nopl 0x0(%rax,%rax,1)
0xffffffffa00007bd: nopl (%rax)
0xffffffffa00007c0: push %rbp
0xffffffffa00007c1: mov %rsp,%rbp
0xffffffffa00007c4: endbr64
Regards,
Scott Constable
> On Fri, Sep 27, 2024 at 06:50:06PM -0700, Josh Poimboeuf wrote:
> > On Fri, Sep 27, 2024 at 09:49:10PM +0200, Peter Zijlstra wrote:
> > > @@ -1190,6 +1214,8 @@ static __init int cfi_parse_cmdline(char
> > > cfi_mode = CFI_KCFI;
> > > } else if (!strcmp(str, "fineibt")) {
> > > cfi_mode = CFI_FINEIBT;
> > > + } else if (IS_ENABLED(CONFIG_X86_KERNEL_IBT_PLUS) && !strcmp(str, "fineibt+bhi")) {
> > > + cfi_mode = CFI_FINEIBT_BHI;
> > > } else if (!strcmp(str, "norand")) {
> > > cfi_rand = false;
> > > } else {
> >
> > Do we need to hook this in with bugs.c somehow so it skips the other
> > BHI mitigations?
>
> Yeah.. those didn't exist when I started this code :-) But yeah, once we get to the point of doing this patch for real -- the compiler(s) have the required features implemented properly and everyrhing, this should be hooked up better.
© 2016 - 2024 Red Hat, Inc.