diff mbox series

[6/6] objtool: Validate kCFI calls

Message ID 20250414113754.540779611@infradead.org
State New
Headers show
Series objtool: Detect and warn about indirect calls in __nocfi functions | expand

Commit Message

Peter Zijlstra April 14, 2025, 11:11 a.m. UTC
Validate that all indirect calls adhere to kCFI rules. Notably doing
nocfi indirect call to a cfi function is broken.

Apparently some Rust 'core' code violates this and explodes when ran
with FineIBT.

All the ANNOTATE_NOCFI sites are prime targets for attackers.

 - runtime EFI is especially henous because it also needs to disable
   IBT. Basically calling unknown code without CFI protection at
   runtime is a massice security issue.

 - Kexec image handover; if you can exploit this, you get to keep it :-)

 - KVM, once for the interrupt injection calling IDT gates directly.

 - KVM, once for the FASTOP emulation stuff.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/machine_kexec_64.c  |    1 
 arch/x86/kvm/emulate.c              |    4 +++
 arch/x86/kvm/vmx/vmenter.S          |    4 +++
 arch/x86/platform/efi/efi_stub_64.S |    4 +++
 drivers/misc/lkdtm/perms.c          |    2 +
 include/linux/objtool.h             |    3 ++
 include/linux/objtool_types.h       |    1 
 tools/include/linux/objtool_types.h |    1 
 tools/objtool/check.c               |   41 ++++++++++++++++++++++++++++++++++++
 tools/objtool/include/objtool/elf.h |    1 
 10 files changed, 62 insertions(+)

Comments

Josh Poimboeuf April 14, 2025, 11:43 p.m. UTC | #1
On Mon, Apr 14, 2025 at 01:11:46PM +0200, Peter Zijlstra wrote:
>  SYM_FUNC_START(__efi_call)
> +	/*
> +	 * The EFI code doesn't have any CFI :-(
> +	 */
> +	ANNOTATE_NOCFI
>  	pushq %rbp
>  	movq %rsp, %rbp
>  	and $~0xf, %rsp

This looks like an insn annotation but is actually a func annotation.
ANNOTATE_NOCFI_SYM() would be a lot clearer, for all the asm code.

Though maybe it's better for ANNOTATE_NOCFI to annotate the call site
itself (for asm) while ANNOTATE_NOCFI_SYM annotates the entire function
(in C).  So either there would be two separate annotypes or the
annotation would get interpreted based on whether it's at the beginning
of the function.

> +++ b/include/linux/objtool.h
> @@ -185,6 +185,8 @@
>   */
>  #define ANNOTATE_REACHABLE(label)	__ASM_ANNOTATE(label, ANNOTYPE_REACHABLE)
>  
> +#define ANNOTATE_NOCFI_SYM(sym)		asm(__ASM_ANNOTATE(sym, ANNOTYPE_NOCFI))

This needs a comment like the others.

> @@ -2436,6 +2438,15 @@ static int __annotate_late(struct objtoo
>  		insn->_jump_table = (void *)1;
>  		break;
>  
> +	case ANNOTYPE_NOCFI:
> +		sym = insn->sym;
> +		if (!sym) {
> +			WARN_INSN(insn, "dodgy NOCFI annotation");
> +			break;

Return an error?

> @@ -4006,6 +4017,36 @@ static int validate_retpoline(struct obj
> +	/*
> +	 * kCFI call sites look like:
> +	 *
> +	 *     movl $(-0x12345678), %r10d
> +	 *     addl -4(%r11), %r10d
> +	 *     jz 1f
> +	 *     ud2
> +	 *  1: cs call __x86_indirect_thunk_r11
> +	 *
> +	 * Verify all indirect calls are kCFI adorned by checking for the
> +	 * UD2. Notably, doing __nocfi calls to regular (cfi) functions is
> +	 * broken.

This "__nocfi calls" is confusing me.  IIUC, there are two completely
different meanings for "nocfi":

  - __nocfi: disable the kcfi function entry stuff
  
  - ANNOTATE_NOCFI_SYM: Regardless of whether the function is __nocfi,
    allow it to have non-CFI indirect call sites.

Can we call this ANNOTATE_NOCFI_SAFE or something?
diff mbox series

Patch

--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -442,6 +442,7 @@  void __nocfi machine_kexec(struct kimage
 
 	__ftrace_enabled_restore(save_ftrace_enabled);
 }
+ANNOTATE_NOCFI_SYM(machine_kexec);
 
 /* arch-dependent functionality related to kexec file-based syscall */
 
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5071,6 +5071,10 @@  static noinline int fastop(struct x86_em
 		return emulate_de(ctxt);
 	return X86EMUL_CONTINUE;
 }
+/*
+ * The ASM stubs don't have CFI on.
+ */
+ANNOTATE_NOCFI_SYM(fastop);
 
 void init_decode_cache(struct x86_emulate_ctxt *ctxt)
 {
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -363,5 +363,9 @@  SYM_FUNC_END(vmread_error_trampoline)
 .section .text, "ax"
 
 SYM_FUNC_START(vmx_do_interrupt_irqoff)
+	/*
+	 * Calling an IDT gate directly.
+	 */
+	ANNOTATE_NOCFI
 	VMX_DO_EVENT_IRQOFF CALL_NOSPEC _ASM_ARG1
 SYM_FUNC_END(vmx_do_interrupt_irqoff)
--- a/arch/x86/platform/efi/efi_stub_64.S
+++ b/arch/x86/platform/efi/efi_stub_64.S
@@ -11,6 +11,10 @@ 
 #include <asm/nospec-branch.h>
 
 SYM_FUNC_START(__efi_call)
+	/*
+	 * The EFI code doesn't have any CFI :-(
+	 */
+	ANNOTATE_NOCFI
 	pushq %rbp
 	movq %rsp, %rbp
 	and $~0xf, %rsp
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -9,6 +9,7 @@ 
 #include <linux/vmalloc.h>
 #include <linux/mman.h>
 #include <linux/uaccess.h>
+#include <linux/objtool.h>
 #include <asm/cacheflush.h>
 #include <asm/sections.h>
 
@@ -86,6 +87,7 @@  static noinline __nocfi void execute_loc
 	func();
 	pr_err("FAIL: func returned\n");
 }
+ANNOTATE_NOCFI_SYM(execute_location);
 
 static void execute_user_location(void *dst)
 {
--- a/include/linux/objtool.h
+++ b/include/linux/objtool.h
@@ -185,6 +185,8 @@ 
  */
 #define ANNOTATE_REACHABLE(label)	__ASM_ANNOTATE(label, ANNOTYPE_REACHABLE)
 
+#define ANNOTATE_NOCFI_SYM(sym)		asm(__ASM_ANNOTATE(sym, ANNOTYPE_NOCFI))
+
 #else
 #define ANNOTATE_NOENDBR		ANNOTATE type=ANNOTYPE_NOENDBR
 #define ANNOTATE_RETPOLINE_SAFE		ANNOTATE type=ANNOTYPE_RETPOLINE_SAFE
@@ -194,6 +196,7 @@ 
 #define ANNOTATE_INTRA_FUNCTION_CALL	ANNOTATE type=ANNOTYPE_INTRA_FUNCTION_CALL
 #define ANNOTATE_UNRET_BEGIN		ANNOTATE type=ANNOTYPE_UNRET_BEGIN
 #define ANNOTATE_REACHABLE		ANNOTATE type=ANNOTYPE_REACHABLE
+#define ANNOTATE_NOCFI			ANNOTATE type=ANNOTYPE_NOCFI
 #endif
 
 #if defined(CONFIG_NOINSTR_VALIDATION) && \
--- a/include/linux/objtool_types.h
+++ b/include/linux/objtool_types.h
@@ -66,5 +66,6 @@  struct unwind_hint {
 #define ANNOTYPE_INTRA_FUNCTION_CALL	7
 #define ANNOTYPE_REACHABLE		8
 #define ANNOTYPE_JUMP_TABLE		9
+#define ANNOTYPE_NOCFI			10
 
 #endif /* _LINUX_OBJTOOL_TYPES_H */
--- a/tools/include/linux/objtool_types.h
+++ b/tools/include/linux/objtool_types.h
@@ -66,5 +66,6 @@  struct unwind_hint {
 #define ANNOTYPE_INTRA_FUNCTION_CALL	7
 #define ANNOTYPE_REACHABLE		8
 #define ANNOTYPE_JUMP_TABLE		9
+#define ANNOTYPE_NOCFI			10
 
 #endif /* _LINUX_OBJTOOL_TYPES_H */
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2387,6 +2387,8 @@  static int __annotate_ifc(struct objtool
 
 static int __annotate_late(struct objtool_file *file, int type, struct instruction *insn)
 {
+	struct symbol *sym;
+
 	switch (type) {
 	case ANNOTYPE_NOENDBR:
 		/* early */
@@ -2436,6 +2438,15 @@  static int __annotate_late(struct objtoo
 		insn->_jump_table = (void *)1;
 		break;
 
+	case ANNOTYPE_NOCFI:
+		sym = insn->sym;
+		if (!sym) {
+			WARN_INSN(insn, "dodgy NOCFI annotation");
+			break;
+		}
+		insn->sym->nocfi = 1;
+		break;
+
 	default:
 		ERROR_INSN(insn, "Unknown annotation type: %d", type);
 		return -1;
@@ -4006,6 +4017,36 @@  static int validate_retpoline(struct obj
 		warnings++;
 	}
 
+	if (!opts.cfi)
+		return warnings;
+
+	/*
+	 * kCFI call sites look like:
+	 *
+	 *     movl $(-0x12345678), %r10d
+	 *     addl -4(%r11), %r10d
+	 *     jz 1f
+	 *     ud2
+	 *  1: cs call __x86_indirect_thunk_r11
+	 *
+	 * Verify all indirect calls are kCFI adorned by checking for the
+	 * UD2. Notably, doing __nocfi calls to regular (cfi) functions is
+	 * broken.
+	 */
+	list_for_each_entry(insn, &file->retpoline_call_list, call_node) {
+		struct symbol *sym = insn->sym;
+
+		if (sym && sym->type == STT_FUNC && !sym->nocfi) {
+			struct instruction *prev =
+				prev_insn_same_sym(file, insn);
+
+			if (!prev || prev->type != INSN_BUG) {
+				WARN_INSN(insn, "no-cfi indirect call!");
+				warnings++;
+			}
+		}
+	}
+
 	return warnings;
 }
 
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -70,6 +70,7 @@  struct symbol {
 	u8 local_label       : 1;
 	u8 frame_pointer     : 1;
 	u8 ignore	     : 1;
+	u8 nocfi             : 1;
 	struct list_head pv_target;
 	struct reloc *relocs;
 };