[bpf-next,v7,2/2] arm64/cfi,bpf: Support kCFI + BPF on arm64

Commit Message

Maxwell Bland June 17, 2024, 4:31 p.m. UTC

From: Puranjay Mohan <[e-mail]>

Currently, bpf_dispatcher_*_func() is marked with `__nocfi` therefore
calling BPF programs from this interface doesn't cause CFI warnings.

When BPF programs are called directly from C: from BPF helpers or
struct_ops, CFI warnings are generated.

Implement proper CFI prologues for the BPF programs and callbacks and
drop __nocfi for arm64. Fix the trampoline generation code to emit kCFI
prologue when a struct_ops trampoline is being prepared.

Signed-off-by: Puranjay Mohan <[e-mail]>
Signed-off-by: Maxwell Bland <[e-mail]>
---
 arch/arm64/include/asm/cfi.h    | 23 +++++++++++++++++++++++
 arch/arm64/kernel/alternative.c | 18 ++++++++++++++++++
 arch/arm64/net/bpf_jit_comp.c   | 21 ++++++++++++++++++---
 3 files changed, 59 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm64/include/asm/cfi.h

Comments

Puranjay Mohan June 18, 2024, 1:30 p.m. UTC | #1

Hi Maxwell,

I am happy to test your code everytime but it would be great if you test
the code before posting it on the list. Otherwise it would take multiple
revisions for the patches to be accepted. I understand that testing this
is non-trivial because you need clang and everything and you also need
ARM64 hardware or Qemu setup. But if you enjoy kernel development you
will find all this is worth it.

> +u32 cfi_get_func_hash(void *func)
> +{
> +	u32 *hashp = func - cfi_get_offset();
> +	return READ_ONCE(*hashp);

The above assumes that hashp is always a valid address, and when it is
not, it crashes the kernel:

Building these patches with clang and with CONFIG_CFI_CLANG=y and then
running `sudo ./test_progs -a dummy_st_ops` crashes the kernel like:

Internal error: Oops: 0000000096000006 [#1] SMP
Modules linked in: bpf_testmod(OE) nls_ascii nls_cp437 aes_ce_blk aes_ce_cipher ghash_ce sha1_ce button sunrpc sch_fq_codel dm_mod dax configfs dmi_sysfs sha2_ce sha256_arm64
CPU: 47 PID: 5746 Comm: test_progs Tainted: G        W  OE      6.10.0-rc2+ #41
Hardware name: Amazon EC2 c6g.16xlarge/, BIOS 1.0 11/1/2018
pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : cfi_get_func_hash+0xc/0x1c
lr : prepare_trampoline+0xcc/0xf44
sp : ffff80008b1637e0
x29: ffff80008b163810 x28: ffff0003c9e4b0c0 x27: 0000000000000000
x26: 0000000000000010 x25: ffff0003d4ab7000 x24: 0000000000000040
x23: 0000000000000018 x22: 0000000000000037 x21: 0000000000000001
x20: 0000000000000020 x19: ffff80008b163870 x18: 0000000000000000
x17: 00000000ad6b63b6 x16: 00000000ad6b63b6 x15: ffff80008002eed4
x14: ffff80008002eff4 x13: ffff80008b160000 x12: ffff80008b164000
x11: 0000000000000082 x10: 0000000000000010 x9 : ffff80008004b724
x8 : 0000000000000110 x7 : 0000000000000000 x6 : 0000000000000001
x5 : 0000000000000110 x4 : 0000000000000001 x3 : 0000000000000000
x2 : ffff0003d4ab7000 x1 : 000000000000003f x0 : 0000000000000000
Call trace:
 cfi_get_func_hash+0xc/0x1c
 arch_bpf_trampoline_size+0xe8/0x158
 bpf_struct_ops_prepare_trampoline+0x8
[...]

Here is my understanding of the above:

We are trying to get the cfi hash from <func_addr> - 4, but clang
doesn't emit the cfi hash for all functions, and in case the cfi hash is
not emitted, <func_addr> - 4 could be an invalid address or point to
something that is not a cfi hash.

In my original patch I had:

u32 cfi_get_func_hash(void *func)
{
	u32 hash;

	if (get_kernel_nofault(hash, func - cfi_get_offset()))
		return 0;

	return hash;
}

I think we need to keep the get_kernel_nofault() to fix this issue.

cfi_get_func_hash() in arch/x86/kernel/alternative.c also uses
get_kernel_nofault() and I think it is there for the same reason.

Thanks,
Puranjay

Patch

diff --git a/arch/arm64/include/asm/cfi.h b/arch/arm64/include/asm/cfi.h
new file mode 100644
index 000000000000..670e191f8628
--- /dev/null
+++ b/arch/arm64/include/asm/cfi.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARM64_CFI_H
+#define _ASM_ARM64_CFI_H
+
+#ifdef CONFIG_CFI_CLANG
+#define __bpfcall
+static inline int cfi_get_offset(void)
+{
+	return 4;
+}
+#define cfi_get_offset cfi_get_offset
+extern u32 cfi_bpf_hash;
+extern u32 cfi_bpf_subprog_hash;
+extern u32 cfi_get_func_hash(void *func);
+#else
+#define cfi_bpf_hash 0U
+#define cfi_bpf_subprog_hash 0U
+static inline u32 cfi_get_func_hash(void *func)
+{
+	return 0;
+}
+#endif /* CONFIG_CFI_CLANG */
+#endif /* _ASM_ARM64_CFI_H */
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 8ff6610af496..d7a58eca7665 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -8,11 +8,13 @@ 
 
 #define pr_fmt(fmt) "alternatives: " fmt
 
+#include <linux/cfi_types.h>
 #include <linux/init.h>
 #include <linux/cpu.h>
 #include <linux/elf.h>
 #include <asm/cacheflush.h>
 #include <asm/alternative.h>
+#include <asm/cfi.h>
 #include <asm/cpufeature.h>
 #include <asm/insn.h>
 #include <asm/module.h>
@@ -298,3 +300,19 @@  noinstr void alt_cb_patch_nops(struct alt_instr *alt, __le32 *origptr,
 		updptr[i] = cpu_to_le32(aarch64_insn_gen_nop());
 }
 EXPORT_SYMBOL(alt_cb_patch_nops);
+
+#ifdef CONFIG_CFI_CLANG
+struct bpf_insn;
+/* Must match bpf_func_t / DEFINE_BPF_PROG_RUN() */
+extern unsigned int __bpf_prog_runX(const void *ctx,
+				    const struct bpf_insn *insn);
+DEFINE_CFI_TYPE(cfi_bpf_hash, __bpf_prog_runX);
+/* Must match bpf_callback_t */
+extern u64 __bpf_callback_fn(u64, u64, u64, u64, u64);
+DEFINE_CFI_TYPE(cfi_bpf_subprog_hash, __bpf_callback_fn);
+u32 cfi_get_func_hash(void *func)
+{
+	u32 *hashp = func - cfi_get_offset();
+	return READ_ONCE(*hashp);
+}
+#endif
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 720336d28856..211e1c29f004 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -17,6 +17,7 @@ 
 #include <asm/asm-extable.h>
 #include <asm/byteorder.h>
 #include <asm/cacheflush.h>
+#include <asm/cfi.h>
 #include <asm/debug-monitors.h>
 #include <asm/insn.h>
 #include <asm/patching.h>
@@ -162,6 +163,12 @@  static inline void emit_bti(u32 insn, struct jit_ctx *ctx)
 		emit(insn, ctx);
 }
 
+static inline void emit_kcfi(u32 hash, struct jit_ctx *ctx)
+{
+	if (IS_ENABLED(CONFIG_CFI_CLANG))
+		emit(hash, ctx);
+}
+
 /*
  * Kernel addresses in the vmalloc space use at most 48 bits, and the
  * remaining bits are guaranteed to be 0x1. So we can compose the address
@@ -311,7 +318,6 @@  static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf,
 	const u8 tcc = bpf2a64[TCALL_CNT];
 	const u8 fpb = bpf2a64[FP_BOTTOM];
 	const u8 arena_vm_base = bpf2a64[ARENA_VM_START];
-	const int idx0 = ctx->idx;
 	int cur_offset;
 
 	/*
@@ -337,6 +343,9 @@  static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf,
 	 *
 	 */
 
+	emit_kcfi(is_main_prog ? cfi_bpf_hash : cfi_bpf_subprog_hash, ctx);
+	const int idx0 = ctx->idx;
+
 	/* bpf function may be invoked by 3 instruction types:
 	 * 1. bl, attached via freplace to bpf prog via short jump
 	 * 2. br, attached via freplace to bpf prog via long jump
@@ -1849,9 +1858,9 @@  struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 		jit_data->ro_header = ro_header;
 	}
 
-	prog->bpf_func = (void *)ctx.ro_image;
+	prog->bpf_func = (void *)ctx.ro_image + cfi_get_offset();
 	prog->jited = 1;
-	prog->jited_len = prog_size;
+	prog->jited_len = prog_size - cfi_get_offset();
 
 	if (!prog->is_func || extra_pass) {
 		int i;
@@ -2104,6 +2113,12 @@  static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
 	/* return address locates above FP */
 	retaddr_off = stack_size + 8;
 
+	if (flags & BPF_TRAMP_F_INDIRECT) {
+		/*
+		 * Indirect call for bpf_struct_ops
+		 */
+		emit_kcfi(cfi_get_func_hash(func_addr), ctx);
+	}
 	/* bpf trampoline may be invoked by 3 instruction types:
 	 * 1. bl, attached to bpf prog or kernel function via short jump
 	 * 2. br, attached to bpf prog or kernel function via long jump