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

[Björn Töpel]

Puranjay Mohan <puranjay12@xxxxxxxxx> 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 <bjorn@xxxxxxxxxx>