x86/cfi: Fix a few kCFI issues found with GCC

Hi!

While testing x86 kernels built by GCC with the coming kCFI patches I tripped over three problems. Since the kCFI call sequence was optimized in v7.3-rc1, the trap handler decodes the check one byte off, so a kCFI failure in a CONFIG_FINEIBT=y kernel running in kCFI mode reports whatever %rbx holds as the target, with Clang as well as GCC (1/3). GNU as warns about the "cs" prefix in the FineIBT paranoid caller template, which Clang's integrated assembler accepts silently (2/3). And objtool's kCFI check treats GCC's jump-table jump for the BPF interpreter's computed goto as an unchecked indirect call (3/3).

Thanks!

-Kees

Kees Cook (3):
x86/kcfi: Decode the kCFI check from the trap address x86/kcfi: Emit the FineIBT paranoid "cs" prefix as a byte
objtool: Skip jump table jumps in the kCFI indirect call check

 arch/x86/kernel/alternative.c |  3 ++-
 arch/x86/kernel/cfi.c         | 14 +++++++-------
 tools/objtool/check.c         |  8 ++++++++
 3 files changed, 17 insertions(+), 8 deletions(-)

2.55.0

Since commit b2250d63e6422 ("x86/kcfi: Optimize call sequence"), a CONFIG_FINEIBT=y kernel running in kCFI mode rewrites each kCFI call site at boot so that a failed check traps on the 0xd6 byte of "test $0xd6, %al", one byte past where the compiler's "ud2" was. handle_cfi_failure() steps "addr" back one byte to find the trap in .kcfi_traps, but decode_cfi_insn() still decodes from regs->ip, so it reads each instruction starting at its second byte.
"movl $-<id>, %r10d" then decodes as "movl $-<id>, %edx", so the reported type is right by accident, but "addl -15(%r11), %r10d" decodes as "addl -15(%rbx), %edx", so the reported target is whatever %rbx holds:

CFI failure at lkdtm_indirect_call+0x16/0x20 (target: 0x12; expected type: 0xc79a2d1b)

Pass the adjusted address to decode_cfi_insn() and decode from it.

Tested ARCH=x86_64 defconfig with CONFIG_CFI=y and CONFIG_LKDTM=y under QEMU, with LKDTM's CFI_FORWARD_PROTO and both Clang 24.0.0git and GCC 17.0.0 (experimental, with the kCFI series). With CONFIG_FINEIBT=y in kCFI mode, the report gives %rbx as the target without this change, and the expected lkdtm_increment_int() with it. With cfi=fineibt, with cfi=fineibt,paranoid, and with CONFIG_FINEIBT=n, it gives lkdtm_increment_int() both with and without this change.

---
 arch/x86/kernel/cfi.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 1df26dfdce08..8e5c4ed264a0 100644
@@ -10,11 +10,11 @@
 #include <asm/insn-eval.h>
 
 /*
- * Returns the target address and the expected type when regs->ip points
+ * Returns the target address and the expected type when addr points
  * to a compiler-generated CFI trap.
  */
-static bool decode_cfi_insn(struct pt_regs *regs, unsigned long *target,
-			    u32 *type)
+static bool decode_cfi_insn(struct pt_regs *regs, unsigned long addr,
+			    unsigned long *target, u32 *type)
 {
 	char buffer[MAX_INSN_SIZE];
 	struct insn insn;

@@ -29,13 +29,13 @@ static bool decode_cfi_insn(struct pt_regs *regs, unsigned long *target,
 	 *   movl    -<id>, %r10d       ; 6 bytes
 	 *   addl    -<pos>(%reg), %r10d; 4 bytes
 	 *   je      .Ltmp1             ; 2 bytes
-	 *   ud2                        ; <- regs->ip
+	 *   ud2                        ; <- addr
 	 *   .Ltmp1:
 	 *
 	 * We can decode the expected type and the target address from the
 	 * movl/addl instructions.
 	 */
-	if (copy_from_kernel_nofault(buffer, (void *)regs->ip - 12, MAX_INSN_SIZE))
+	if (copy_from_kernel_nofault(buffer, (void *)addr - 12, MAX_INSN_SIZE))
 		return false;
 	if (insn_decode_kernel(&insn, &buffer[offset]))
 		return false;

@@ -44,7 +44,7 @@ static bool decode_cfi_insn(struct pt_regs *regs, unsigned long *target,
 
 	*type = -(u32)insn.immediate.value;
 
-	if (copy_from_kernel_nofault(buffer, (void *)regs->ip - 6, MAX_INSN_SIZE))
+	if (copy_from_kernel_nofault(buffer, (void *)addr - 6, MAX_INSN_SIZE))
 		return false;
 	if (insn_decode_kernel(&insn, &buffer[offset]))
 		return false;

@@ -82,7 +82,7 @@ enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
 				return BUG_TRAP_TYPE_NONE;
 		}
 
-		if (!decode_cfi_insn(regs, &target, &type))
+		if (!decode_cfi_insn(regs, addr, &target, &type))
 			return report_cfi_failure_noaddr(regs, addr);
 
 		break;

2.55.0

The "cs" prefix on the LEA in the FineIBT paranoid caller template is
padding: it places the LEA's 0xf0 immediate byte where it doubles as the LOCK prefix that makes the failing Jcc raise #UD. GNU as warns about it in every CONFIG_FINEIBT=y build:

Warning: segment override on `lea' is ineffectual

Clang's integrated assembler does not warn, and both assemble the same bytes. Emit the prefix with ".byte 0x2e" to silence GNU as.

Build tested ARCH=x86_64 defconfig with CONFIG_CFI=y, with GCC 17.0.0 (experimental, with the kCFI series) and GNU as 2.47, and with Clang 24.0.0git: GNU as no longer warns, and the template assembles to the same bytes as before with both. Under QEMU with cfi=fineibt,paranoid, the paranoid caller check catches LKDTM's CFI_FORWARD_PROTO with both.

---
 arch/x86/kernel/alternative.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 91b1cdd16569..1300ca50e770 100644
@@ -1448,7 +1448,8 @@ asm(	".pushsection .rodata				\n"
 	"fineibt_paranoid_start:			\n"
 	"	mov	$0x12345678, %eax		\n"
 	"	cmpl	-11(%r11), %eax			\n"
-	"	cs lea	-0x10(%r11), %r11		\n"
+	"	.byte	0x2e				\n"
+	"	lea	-0x10(%r11), %r11		\n"
 	"#fineibt_caller_size:                          \n"
 	"	jne	fineibt_paranoid_start+0xd	\n"
 	"fineibt_paranoid_ind:				\n"

2.55.0

Shees...

This is a binutils bug.  Can't we fix it here?

The claim in the warning is simply not true.  This is an example where it absolutely does matter, and it also matters for legibility that we can avoid writing raw hex.

~Andrew

I can try to send a binutils bug fix?

Well, we'd have to keep some kind of work-around for earlier binutils for a long while...

Kees Cook

With MITIGATION_RETPOLINE=y, GCC lowers the computed goto in ___bpf_prog_run() to a jump through __x86_indirect_thunk_rdx. objtool puts that jump on the retpoline call list along with the indirect calls, so with CONFIG_CFI=y the kCFI check wants a UD2 before it:

vmlinux.o: warning: objtool: ___bpf_prog_run+0x1e: no-cfi indirect call!

The jump goes through the "jumptable" array, which objtool already reads from .data.rel.ro.c_jump_table, so its targets stay inside the function and need no kCFI check. Skip jump table jumps in the check. Clang does not emit this jump: with retpolines, it lowers the computed goto without an indirect jump.

Build tested ARCH=x86_64 defconfig with CONFIG_LKDTM=y and CONFIG_CFI=y, with GCC 17.0.0 (experimental, with the kCFI series).

---
 tools/objtool/check.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index 464f6c9d9ff0..d35c5305075f 100644
@@ -4199,6 +4199,14 @@ static int validate_retpoline(struct objtool_file *file)
 	list_for_each_entry(insn, &file->retpoline_call_list, call_node) {
 		struct symbol *sym = insn_sym(insn);
 
+		/*
+		 * A jump through a jump table stays inside the function, so
+		 * it has no kCFI check.  GCC turns the computed goto in
+		 * ___bpf_prog_run() into one under MITIGATION_RETPOLINE.
+		 */
+		if (is_jump_table_jump(insn))
+			continue;
+
 		if (sym && (is_notype_sym(sym) ||
 			    is_func_sym(sym)) && !sym->nocfi) {
 			struct instruction *prev =

2.55.0

I am confused.. I don't see a computed goto in __bpf_prog_run(), and we build with -fno-jump-tables.

It's the interpreter, ___bpf_prog_run() (three underscores) in kernel/bpf/core.c, not the __bpf_prog_run() wrapper in filter.h:

	static const void * const jumptable[256] __annotate_jump_table = {
		[0 ... 255] = &&default_label,
		...
	};
	...
	select_insn:
		goto *jumptable[insn->code];

So it's a manually constructed jump table, not the "normal" table-from-switch stuff that -fno-jump-tables suppresses (though see below about Clang). objtool already knows about it, with annotations added back in 2019 in commit e55a73251da33 ("bpf: Fix ORC unwinding in non-JIT BPF code").

With retpolines, GCC emits a single dispatch that every handler uses:

	movzbl (%rbx),%esi
	movzbl %sil,%edx
	mov    jumptable(,%rdx,8),%rdx
	jmp    __x86_indirect_thunk_rdx

Clang never has this jump: with retpolines, LLVM's IndirectBrExpand pass turns the label addresses into indexes and the "goto *" into a switch, which Clang's -fno-jump-tables then makes a compare tree. So it's only constructed this way under GCC, but since now we can turn on kCFI with GCC, the objtool warning about indirects without kcfi protection trips.

Kees Cook

Durr, clearly I wasn't wearing my glasses or something.

OK, then the next question; do we *want* to allow a 'naked' indirect branch to survive?

We can simply disable the whole interpreter when kcfi=y. Isn't that thing recommended disabled anyway?

I originally struggled about this. I'd really rather that GCC not codgen like this at all. On the other hand, it's a bounded load and jump through an .rodata table, so that's kind of a manual CFI. :P (And if it gets targeted via ROP, it doesn't matter: everything is a target then.)

I don't think that's realistic, as every GCC-building distro will (I hope) enable KCFI as soon as it's in GCC. It's present with CONFIG_BPF_JIT_ALWAYS_ON=y, and only Debian has it off:

  │ Debian sid and trixie                      │ not set
  │ Ubuntu noble                               │ y
  │ Fedora, CentOS Stream 9 and 10             │ y
  │ Arch                                       │ y
  │ openSUSE Tumbleweed, SLE15-SP7             │ y
  │ Alpine (lts and virt)                      │ y
  │ Android GKI android16-6.12 (arm64 and x86) │ y

Kees Cook

With IBT enabled, I would image GCC will also spray ENDBR around like there is no tomorrow :/

What an unfortunate situation this is :(

But I don't have a better suggestion. Your patch it is then.

And that would in turn cause more problems for FineIBT evasion, since that more or less depends on ENDBR being followed by the hash check.

With FRED we got rid of the 'naked' ENDBR instances on entry, but now we get them here :-(