2024-03-03 17:02:42

by Puranjay Mohan

[permalink] [raw]
Subject: [PATCH bpf-next 0/1] Support kCFI + BPF on riscv64

With CONFIG_CFI_CLANG, the compiler injects a type preamble immediately
before each function and a check to validate the target function type
before indirect calls:

; type preamble
.word <id>
function:
...
; indirect call check
lw t1, -4(a0)
lui t2, <hi20>
addiw t2, t2, <lo12>
beq t1, t2, .Ltmp0
ebreak
.Ltmp0:
jarl a0

BPF JIT currently doesn't emit this preamble before BPF programs and when
the calling fuction tries to load the type id from the preamble, it finds
an invalid value there.

This will cause CFI failures like in the following bpf selftest:

root@rv-selftester:~/bpf# ./test_progs -a "rbtree_success"

CFI failure at bpf_rbtree_add_impl+0x148/0x350 (target: bpf_prog_fb8b097ab47d164a_less+0x0/0x42; expected type: 0x00000000)
WARNING: CPU: 1 PID: 278 at bpf_rbtree_add_impl+0x148/0x350
Modules linked in: bpf_testmod(OE) drm fuse dm_mod backlight i2c_core configfs drm_panel_orientation_quirks ip_tables x_tables
CPU: 1 PID: 278 Comm: test_progs Tainted: P OE 6.8.0-rc1 #1
Hardware name: riscv-virtio,qemu (DT)
epc : bpf_rbtree_add_impl+0x148/0x350
ra : bpf_prog_27b36e47d273751e_rbtree_first_and_remove+0x1aa/0x35e
epc : ffffffff805acc0c ra : ffffffff780077fa sp : ff2000000110b9d0
gp : ffffffff868d6218 tp : ff60000085772a40 t0 : ffffffff86849660
t1 : 0000000000000000 t2 : ffffffff9e4709a9 s0 : ff2000000110ba50
s1 : ff60000089c14958 a0 : ff60000089c14758 a1 : ff60000089c14958
a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
a5 : 0000000000000000 a6 : ff6000008aba4b30 a7 : ffffffff86849640
s2 : ff6000008aba4b30 s3 : ff60000089c14758 s4 : ffffffff780079f0
s5 : 0000000000000000 s6 : ffffffff84c01080 s7 : ff6000008aba4b30
s8 : 0000000000000000 s9 : 0000000000000000 s10: 0000000000000001
s11: 0000000000000000 t3 : ffffffff868499e0 t4 : ffffffff868499c0
t5 : ffffffff86849840 t6 : ffffffff86849860
status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
[<ffffffff805acc0c>] bpf_rbtree_add_impl+0x148/0x350
[<ffffffff780077fa>] bpf_prog_27b36e47d273751e_rbtree_first_and_remove+0x1aa/0x35e
[<ffffffff8294f32c>] bpf_test_run+0x2a4/0xa3c
[<ffffffff8294d032>] bpf_prog_test_run_skb+0x47a/0xe52
[<ffffffff805083ee>] bpf_prog_test_run+0x170/0x548
[<ffffffff805029c8>] __sys_bpf+0x2d2/0x378
[<ffffffff804ff570>] __riscv_sys_bpf+0x5c/0x120
[<ffffffff8000e8fe>] syscall_handler+0x62/0xe4
[<ffffffff83362df6>] do_trap_ecall_u+0xc6/0x27c
[<ffffffff833822c4>] ret_from_exception+0x0/0x64
---[ end trace 0000000000000000 ]---

The calling function tries to load the type id hash from target_func - 4.
If this memory address is not mapped then it can cause a page fault and
crash the kernel.

This behaviour can be seen by running the 'dummy_st_ops' selftest:

root@rv-selftester:~/bpf# ./test_progs -a dummy_st_ops

Unable to handle kernel paging request at virtual address ffffffff78204ffc
Oops [#1]
Modules linked in: bpf_testmod(OE) drm fuse backlight i2c_core drm_panel_orientation_quirks dm_mod configfs ip_tables x_tables [last unloaded: bpf_testmod(OE)]
CPU: 3 PID: 356 Comm: test_progs Tainted: P OE 6.8.0-rc1 #1
Hardware name: riscv-virtio,qemu (DT)
epc : bpf_struct_ops_test_run+0x28c/0x5fc
ra : bpf_struct_ops_test_run+0x26c/0x5fc
epc : ffffffff82958010 ra : ffffffff82957ff0 sp : ff200000007abc80
gp : ffffffff868d6218 tp : ff6000008d87b840 t0 : 000000000000000f
t1 : 0000000000000000 t2 : 000000002005793e s0 : ff200000007abcf0
s1 : ff6000008a90fee0 a0 : 0000000000000000 a1 : 0000000000000000
a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
a5 : ffffffff868dba26 a6 : 0000000000000001 a7 : 0000000052464e43
s2 : 00007ffffc0a95f0 s3 : ff6000008a90fe80 s4 : ff60000084c24c00
s5 : ffffffff78205000 s6 : ff60000088750648 s7 : ff20000000035008
s8 : fffffffffffffff4 s9 : ffffffff86200610 s10: 0000000000000000
s11: 0000000000000000 t3 : ffffffff8483dc30 t4 : ffffffff8483dc10
t5 : ffffffff8483dbf0 t6 : ffffffff8483dbd0
status: 0000000200000120 badaddr: ffffffff78204ffc cause: 000000000000000d
[<ffffffff82958010>] bpf_struct_ops_test_run+0x28c/0x5fc
[<ffffffff805083ee>] bpf_prog_test_run+0x170/0x548
[<ffffffff805029c8>] __sys_bpf+0x2d2/0x378
[<ffffffff804ff570>] __riscv_sys_bpf+0x5c/0x120
[<ffffffff8000e8fe>] syscall_handler+0x62/0xe4
[<ffffffff83362df6>] do_trap_ecall_u+0xc6/0x27c
[<ffffffff833822c4>] ret_from_exception+0x0/0x64
Code: b603 0109 b683 0189 b703 0209 8493 0609 157d 8d65 (a303) ffca
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Fatal exception
SMP: stopping secondary CPUs

This patch improves the BPF JIT for the riscv64 architecture to emit kCFI
type id before BPF programs and struct ops trampolines.

After applying this patch, the above two selftests pass without any issues.

root@rv-selftester:~/bpf# ./test_progs -a "rbtree_success,dummy_st_ops"
#70/1 dummy_st_ops/dummy_st_ops_attach:OK
#70/2 dummy_st_ops/dummy_init_ret_value:OK
#70/3 dummy_st_ops/dummy_init_ptr_arg:OK
#70/4 dummy_st_ops/dummy_multiple_args:OK
#70/5 dummy_st_ops/dummy_sleepable:OK
#70/6 dummy_st_ops/test_unsupported_field_sleepable:OK
#70 dummy_st_ops:OK
#189/1 rbtree_success/rbtree_add_nodes:OK
#189/2 rbtree_success/rbtree_add_and_remove:OK
#189/3 rbtree_success/rbtree_first_and_remove:OK
#189/4 rbtree_success/rbtree_api_release_aliasing:OK
#189 rbtree_success:OK
Summary: 2/10 PASSED, 0 SKIPPED, 0 FAILED

root@rv-selftester:~/bpf# zcat /proc/config.gz | grep CONFIG_CFI_CLANG
CONFIG_CFI_CLANG=y

Puranjay Mohan (1):
riscv64/cfi,bpf: Support kCFI + BPF on riscv64

arch/riscv/include/asm/cfi.h | 17 +++++++++++
arch/riscv/kernel/cfi.c | 53 +++++++++++++++++++++++++++++++++
arch/riscv/net/bpf_jit.h | 2 +-
arch/riscv/net/bpf_jit_comp32.c | 2 +-
arch/riscv/net/bpf_jit_comp64.c | 14 ++++++++-
arch/riscv/net/bpf_jit_core.c | 9 +++---
6 files changed, 90 insertions(+), 7 deletions(-)

--
2.40.1



2024-03-03 17:02:58

by Puranjay Mohan

[permalink] [raw]
Subject: [PATCH bpf-next 1/1] riscv64/cfi,bpf: Support kCFI + BPF on riscv64

The riscv BPF JIT doesn't emit proper kCFI prologues for BPF programs
and struct_ops trampolines when CONFIG_CFI_CLANG is enabled.

This causes CFI failures when calling BPF programs and can even crash
the kernel due to invalid memory accesses.

Example crash:

root@rv-selftester:~/bpf# ./test_progs -a dummy_st_ops

Unable to handle kernel paging request at virtual address ffffffff78204ffc
Oops [#1]
Modules linked in: bpf_testmod(OE) [....]
CPU: 3 PID: 356 Comm: test_progs Tainted: P OE 6.8.0-rc1 #1
Hardware name: riscv-virtio,qemu (DT)
epc : bpf_struct_ops_test_run+0x28c/0x5fc
ra : bpf_struct_ops_test_run+0x26c/0x5fc
epc : ffffffff82958010 ra : ffffffff82957ff0 sp : ff200000007abc80
gp : ffffffff868d6218 tp : ff6000008d87b840 t0 : 000000000000000f
t1 : 0000000000000000 t2 : 000000002005793e s0 : ff200000007abcf0
s1 : ff6000008a90fee0 a0 : 0000000000000000 a1 : 0000000000000000
a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
a5 : ffffffff868dba26 a6 : 0000000000000001 a7 : 0000000052464e43
s2 : 00007ffffc0a95f0 s3 : ff6000008a90fe80 s4 : ff60000084c24c00
s5 : ffffffff78205000 s6 : ff60000088750648 s7 : ff20000000035008
s8 : fffffffffffffff4 s9 : ffffffff86200610 s10: 0000000000000000
s11: 0000000000000000 t3 : ffffffff8483dc30 t4 : ffffffff8483dc10
t5 : ffffffff8483dbf0 t6 : ffffffff8483dbd0
status: 0000000200000120 badaddr: ffffffff78204ffc cause: 000000000000000d
[<ffffffff82958010>] bpf_struct_ops_test_run+0x28c/0x5fc
[<ffffffff805083ee>] bpf_prog_test_run+0x170/0x548
[<ffffffff805029c8>] __sys_bpf+0x2d2/0x378
[<ffffffff804ff570>] __riscv_sys_bpf+0x5c/0x120
[<ffffffff8000e8fe>] syscall_handler+0x62/0xe4
[<ffffffff83362df6>] do_trap_ecall_u+0xc6/0x27c
[<ffffffff833822c4>] ret_from_exception+0x0/0x64
Code: b603 0109 b683 0189 b703 0209 8493 0609 157d 8d65 (a303) ffca
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Fatal exception
SMP: stopping secondary CPUs

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

Signed-off-by: Puranjay Mohan <[email protected]>
---
arch/riscv/include/asm/cfi.h | 17 +++++++++++
arch/riscv/kernel/cfi.c | 53 +++++++++++++++++++++++++++++++++
arch/riscv/net/bpf_jit.h | 2 +-
arch/riscv/net/bpf_jit_comp32.c | 2 +-
arch/riscv/net/bpf_jit_comp64.c | 14 ++++++++-
arch/riscv/net/bpf_jit_core.c | 9 +++---
6 files changed, 90 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/include/asm/cfi.h b/arch/riscv/include/asm/cfi.h
index 8f7a62257044..fb9696d7a3f2 100644
--- a/arch/riscv/include/asm/cfi.h
+++ b/arch/riscv/include/asm/cfi.h
@@ -13,11 +13,28 @@ struct pt_regs;

#ifdef CONFIG_CFI_CLANG
enum bug_trap_type handle_cfi_failure(struct pt_regs *regs);
+#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
static inline enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)
{
return BUG_TRAP_TYPE_NONE;
}
+
+#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_RISCV_CFI_H */
diff --git a/arch/riscv/kernel/cfi.c b/arch/riscv/kernel/cfi.c
index 6ec9dbd7292e..64bdd3e1ab8c 100644
--- a/arch/riscv/kernel/cfi.c
+++ b/arch/riscv/kernel/cfi.c
@@ -75,3 +75,56 @@ enum bug_trap_type handle_cfi_failure(struct pt_regs *regs)

return report_cfi_failure(regs, regs->epc, &target, type);
}
+
+#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);
+
+/*
+ * Force a reference to the external symbol so the compiler generates
+ * __kcfi_typid.
+ */
+__ADDRESSABLE(__bpf_prog_runX);
+
+/* u32 __ro_after_init cfi_bpf_hash = __kcfi_typeid___bpf_prog_runX; */
+asm (
+" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
+" .type cfi_bpf_hash,@object \n"
+" .globl cfi_bpf_hash \n"
+" .p2align 2, 0x0 \n"
+"cfi_bpf_hash: \n"
+" .word __kcfi_typeid___bpf_prog_runX \n"
+" .size cfi_bpf_hash, 4 \n"
+" .popsection \n"
+);
+
+/* Must match bpf_callback_t */
+extern u64 __bpf_callback_fn(u64, u64, u64, u64, u64);
+
+__ADDRESSABLE(__bpf_callback_fn);
+
+/* u32 __ro_after_init cfi_bpf_subprog_hash = __kcfi_typeid___bpf_callback_fn; */
+asm (
+" .pushsection .data..ro_after_init,\"aw\",@progbits \n"
+" .type cfi_bpf_subprog_hash,@object \n"
+" .globl cfi_bpf_subprog_hash \n"
+" .p2align 2, 0x0 \n"
+"cfi_bpf_subprog_hash: \n"
+" .word __kcfi_typeid___bpf_callback_fn \n"
+" .size cfi_bpf_subprog_hash, 4 \n"
+" .popsection \n"
+);
+
+u32 cfi_get_func_hash(void *func)
+{
+ u32 hash;
+
+ if (get_kernel_nofault(hash, func - cfi_get_offset()))
+ return 0;
+
+ return hash;
+}
+#endif
diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
index 8b35f12a4452..f4b6b3b9edda 100644
--- a/arch/riscv/net/bpf_jit.h
+++ b/arch/riscv/net/bpf_jit.h
@@ -1223,7 +1223,7 @@ static inline void emit_bswap(u8 rd, s32 imm, struct rv_jit_context *ctx)

#endif /* __riscv_xlen == 64 */

-void bpf_jit_build_prologue(struct rv_jit_context *ctx);
+void bpf_jit_build_prologue(struct rv_jit_context *ctx, bool is_subprog);
void bpf_jit_build_epilogue(struct rv_jit_context *ctx);

int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
diff --git a/arch/riscv/net/bpf_jit_comp32.c b/arch/riscv/net/bpf_jit_comp32.c
index 529a83b85c1c..f5ba73bb153d 100644
--- a/arch/riscv/net/bpf_jit_comp32.c
+++ b/arch/riscv/net/bpf_jit_comp32.c
@@ -1301,7 +1301,7 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
return 0;
}

-void bpf_jit_build_prologue(struct rv_jit_context *ctx)
+void bpf_jit_build_prologue(struct rv_jit_context *ctx, bool is_subprog)
{
const s8 *fp = bpf2rv32[BPF_REG_FP];
const s8 *r1 = bpf2rv32[BPF_REG_1];
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 869e4282a2c4..aac190085472 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -11,6 +11,7 @@
#include <linux/memory.h>
#include <linux/stop_machine.h>
#include <asm/patch.h>
+#include <asm/cfi.h>
#include "bpf_jit.h"

#define RV_FENTRY_NINSNS 2
@@ -455,6 +456,12 @@ static int emit_call(u64 addr, bool fixed_addr, struct rv_jit_context *ctx)
return emit_jump_and_link(RV_REG_RA, off, fixed_addr, ctx);
}

+static inline void emit_kcfi(u32 hash, struct rv_jit_context *ctx)
+{
+ if (IS_ENABLED(CONFIG_CFI_CLANG))
+ emit(hash, ctx);
+}
+
static void emit_atomic(u8 rd, u8 rs, s16 off, s32 imm, bool is64,
struct rv_jit_context *ctx)
{
@@ -869,6 +876,8 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
emit_sd(RV_REG_SP, stack_size - 16, RV_REG_FP, ctx);
emit_addi(RV_REG_FP, RV_REG_SP, stack_size, ctx);
} else {
+ /* emit kcfi hash */
+ emit_kcfi(cfi_get_func_hash(func_addr), ctx);
/* For the trampoline called directly, just handle
* the frame of trampoline.
*/
@@ -1711,7 +1720,7 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
return 0;
}

-void bpf_jit_build_prologue(struct rv_jit_context *ctx)
+void bpf_jit_build_prologue(struct rv_jit_context *ctx, bool is_subprog)
{
int i, stack_adjust = 0, store_offset, bpf_stack_adjust;

@@ -1740,6 +1749,9 @@ void bpf_jit_build_prologue(struct rv_jit_context *ctx)

store_offset = stack_adjust - 8;

+ /* emit kcfi type preamble immediately before the first insn */
+ emit_kcfi(is_subprog ? cfi_bpf_subprog_hash : cfi_bpf_hash, ctx);
+
/* nops reserved for auipc+jalr pair */
for (i = 0; i < RV_FENTRY_NINSNS; i++)
emit(rv_nop(), ctx);
diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
index 7b70ccb7fec3..6b3acac30c06 100644
--- a/arch/riscv/net/bpf_jit_core.c
+++ b/arch/riscv/net/bpf_jit_core.c
@@ -10,6 +10,7 @@
#include <linux/filter.h>
#include <linux/memory.h>
#include <asm/patch.h>
+#include <asm/cfi.h>
#include "bpf_jit.h"

/* Number of iterations to try until offsets converge. */
@@ -100,7 +101,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
pass++;
ctx->ninsns = 0;

- bpf_jit_build_prologue(ctx);
+ bpf_jit_build_prologue(ctx, bpf_is_subprog(prog));
ctx->prologue_len = ctx->ninsns;

if (build_body(ctx, extra_pass, ctx->offset)) {
@@ -160,7 +161,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
ctx->ninsns = 0;
ctx->nexentries = 0;

- bpf_jit_build_prologue(ctx);
+ bpf_jit_build_prologue(ctx, bpf_is_subprog(prog));
if (build_body(ctx, extra_pass, NULL)) {
prog = orig_prog;
goto out_free_hdr;
@@ -170,9 +171,9 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
if (bpf_jit_enable > 1)
bpf_jit_dump(prog->len, prog_size, pass, ctx->insns);

- prog->bpf_func = (void *)ctx->ro_insns;
+ prog->bpf_func = (void *)ctx->ro_insns + 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) {
if (WARN_ON(bpf_jit_binary_pack_finalize(prog, jit_data->ro_header,
--
2.40.1


2024-03-06 16:34:32

by Björn Töpel

[permalink] [raw]
Subject: Re: [PATCH bpf-next 0/1] Support kCFI + BPF on riscv64

Puranjay Mohan <[email protected]> writes:

> With CONFIG_CFI_CLANG, the compiler injects a type preamble immediately
> before each function and a check to validate the target function type
> before indirect calls:
>
> ; type preamble
> .word <id>
> function:
> ...
> ; indirect call check
> lw t1, -4(a0)
> lui t2, <hi20>
> addiw t2, t2, <lo12>
> beq t1, t2, .Ltmp0
> ebreak
> .Ltmp0:
> jarl a0
>
> BPF JIT currently doesn't emit this preamble before BPF programs and when
> the calling fuction tries to load the type id from the preamble, it finds
> an invalid value there.
>
> This will cause CFI failures like in the following bpf selftest:
>
> root@rv-selftester:~/bpf# ./test_progs -a "rbtree_success"
>
> CFI failure at bpf_rbtree_add_impl+0x148/0x350 (target: bpf_prog_fb8b097ab47d164a_less+0x0/0x42; expected type: 0x00000000)
> WARNING: CPU: 1 PID: 278 at bpf_rbtree_add_impl+0x148/0x350
> Modules linked in: bpf_testmod(OE) drm fuse dm_mod backlight i2c_core configfs drm_panel_orientation_quirks ip_tables x_tables
> CPU: 1 PID: 278 Comm: test_progs Tainted: P OE 6.8.0-rc1 #1
> Hardware name: riscv-virtio,qemu (DT)
> epc : bpf_rbtree_add_impl+0x148/0x350
> ra : bpf_prog_27b36e47d273751e_rbtree_first_and_remove+0x1aa/0x35e
> epc : ffffffff805acc0c ra : ffffffff780077fa sp : ff2000000110b9d0
> gp : ffffffff868d6218 tp : ff60000085772a40 t0 : ffffffff86849660
> t1 : 0000000000000000 t2 : ffffffff9e4709a9 s0 : ff2000000110ba50
> s1 : ff60000089c14958 a0 : ff60000089c14758 a1 : ff60000089c14958
> a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
> a5 : 0000000000000000 a6 : ff6000008aba4b30 a7 : ffffffff86849640
> s2 : ff6000008aba4b30 s3 : ff60000089c14758 s4 : ffffffff780079f0
> s5 : 0000000000000000 s6 : ffffffff84c01080 s7 : ff6000008aba4b30
> s8 : 0000000000000000 s9 : 0000000000000000 s10: 0000000000000001
> s11: 0000000000000000 t3 : ffffffff868499e0 t4 : ffffffff868499c0
> t5 : ffffffff86849840 t6 : ffffffff86849860
> status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> [<ffffffff805acc0c>] bpf_rbtree_add_impl+0x148/0x350
> [<ffffffff780077fa>] bpf_prog_27b36e47d273751e_rbtree_first_and_remove+0x1aa/0x35e
> [<ffffffff8294f32c>] bpf_test_run+0x2a4/0xa3c
> [<ffffffff8294d032>] bpf_prog_test_run_skb+0x47a/0xe52
> [<ffffffff805083ee>] bpf_prog_test_run+0x170/0x548
> [<ffffffff805029c8>] __sys_bpf+0x2d2/0x378
> [<ffffffff804ff570>] __riscv_sys_bpf+0x5c/0x120
> [<ffffffff8000e8fe>] syscall_handler+0x62/0xe4
> [<ffffffff83362df6>] do_trap_ecall_u+0xc6/0x27c
> [<ffffffff833822c4>] ret_from_exception+0x0/0x64
> ---[ end trace 0000000000000000 ]---
>
> The calling function tries to load the type id hash from target_func - 4.
> If this memory address is not mapped then it can cause a page fault and
> crash the kernel.
>
> This behaviour can be seen by running the 'dummy_st_ops' selftest:
>
> root@rv-selftester:~/bpf# ./test_progs -a dummy_st_ops
>
> Unable to handle kernel paging request at virtual address ffffffff78204ffc
> Oops [#1]
> Modules linked in: bpf_testmod(OE) drm fuse backlight i2c_core drm_panel_orientation_quirks dm_mod configfs ip_tables x_tables [last unloaded: bpf_testmod(OE)]
> CPU: 3 PID: 356 Comm: test_progs Tainted: P OE 6.8.0-rc1 #1
> Hardware name: riscv-virtio,qemu (DT)
> epc : bpf_struct_ops_test_run+0x28c/0x5fc
> ra : bpf_struct_ops_test_run+0x26c/0x5fc
> epc : ffffffff82958010 ra : ffffffff82957ff0 sp : ff200000007abc80
> gp : ffffffff868d6218 tp : ff6000008d87b840 t0 : 000000000000000f
> t1 : 0000000000000000 t2 : 000000002005793e s0 : ff200000007abcf0
> s1 : ff6000008a90fee0 a0 : 0000000000000000 a1 : 0000000000000000
> a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
> a5 : ffffffff868dba26 a6 : 0000000000000001 a7 : 0000000052464e43
> s2 : 00007ffffc0a95f0 s3 : ff6000008a90fe80 s4 : ff60000084c24c00
> s5 : ffffffff78205000 s6 : ff60000088750648 s7 : ff20000000035008
> s8 : fffffffffffffff4 s9 : ffffffff86200610 s10: 0000000000000000
> s11: 0000000000000000 t3 : ffffffff8483dc30 t4 : ffffffff8483dc10
> t5 : ffffffff8483dbf0 t6 : ffffffff8483dbd0
> status: 0000000200000120 badaddr: ffffffff78204ffc cause: 000000000000000d
> [<ffffffff82958010>] bpf_struct_ops_test_run+0x28c/0x5fc
> [<ffffffff805083ee>] bpf_prog_test_run+0x170/0x548
> [<ffffffff805029c8>] __sys_bpf+0x2d2/0x378
> [<ffffffff804ff570>] __riscv_sys_bpf+0x5c/0x120
> [<ffffffff8000e8fe>] syscall_handler+0x62/0xe4
> [<ffffffff83362df6>] do_trap_ecall_u+0xc6/0x27c
> [<ffffffff833822c4>] ret_from_exception+0x0/0x64
> Code: b603 0109 b683 0189 b703 0209 8493 0609 157d 8d65 (a303) ffca
> ---[ end trace 0000000000000000 ]---
> Kernel panic - not syncing: Fatal exception
> SMP: stopping secondary CPUs
>
> This patch improves the BPF JIT for the riscv64 architecture to emit kCFI
> type id before BPF programs and struct ops trampolines.
>
> After applying this patch, the above two selftests pass without any issues.
>
> root@rv-selftester:~/bpf# ./test_progs -a "rbtree_success,dummy_st_ops"
> #70/1 dummy_st_ops/dummy_st_ops_attach:OK
> #70/2 dummy_st_ops/dummy_init_ret_value:OK
> #70/3 dummy_st_ops/dummy_init_ptr_arg:OK
> #70/4 dummy_st_ops/dummy_multiple_args:OK
> #70/5 dummy_st_ops/dummy_sleepable:OK
> #70/6 dummy_st_ops/test_unsupported_field_sleepable:OK
> #70 dummy_st_ops:OK
> #189/1 rbtree_success/rbtree_add_nodes:OK
> #189/2 rbtree_success/rbtree_add_and_remove:OK
> #189/3 rbtree_success/rbtree_first_and_remove:OK
> #189/4 rbtree_success/rbtree_api_release_aliasing:OK
> #189 rbtree_success:OK
> Summary: 2/10 PASSED, 0 SKIPPED, 0 FAILED
>
> root@rv-selftester:~/bpf# zcat /proc/config.gz | grep CONFIG_CFI_CLANG
> CONFIG_CFI_CLANG=y

Apologies for the slow review. Nice work!

Acked-by: Björn Töpel <[email protected]>

2024-03-06 22:44:47

by patchwork-bot+netdevbpf

[permalink] [raw]
Subject: Re: [PATCH bpf-next 0/1] Support kCFI + BPF on riscv64

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <[email protected]>:

On Sun, 3 Mar 2024 17:02:06 +0000 you wrote:
> With CONFIG_CFI_CLANG, the compiler injects a type preamble immediately
> before each function and a check to validate the target function type
> before indirect calls:
>
> ; type preamble
> .word <id>
> function:
> ...
> ; indirect call check
> lw t1, -4(a0)
> lui t2, <hi20>
> addiw t2, t2, <lo12>
> beq t1, t2, .Ltmp0
> ebreak
> .Ltmp0:
> jarl a0
>
> [...]

Here is the summary with links:
- [bpf-next,1/1] riscv64/cfi,bpf: Support kCFI + BPF on riscv64
https://git.kernel.org/bpf/bpf-next/c/dfd646d117b7

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



2024-03-07 13:38:05

by Pu Lehui

[permalink] [raw]
Subject: Re: [PATCH bpf-next 1/1] riscv64/cfi,bpf: Support kCFI + BPF on riscv64


On 2024/3/4 1:02, Puranjay Mohan wrote:
> The riscv BPF JIT doesn't emit proper kCFI prologues for BPF programs

[SNIP]

>
> -void bpf_jit_build_prologue(struct rv_jit_context *ctx)
> +void bpf_jit_build_prologue(struct rv_jit_context *ctx, bool is_subprog)

Not tracked in time. Some nits, although it has been merged. We don't
need to add new parameters here since we can fetch prog in ctx. Others,
it looks great.

> {
> int i, stack_adjust = 0, store_offset, bpf_stack_adjust;
>
> @@ -1740,6 +1749,9 @@ void bpf_jit_build_prologue(struct rv_jit_context *ctx)
>
> store_offset = stack_adjust - 8;