[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87jzmfb9qg.fsf@all.your.base.are.belong.to.us>
Date: Wed, 06 Mar 2024 17:33:59 +0100
From: Björn Töpel <bjorn@...nel.org>
To: Puranjay Mohan <puranjay12@...il.com>, Paul Walmsley
<paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou
<aou@...s.berkeley.edu>, Alexei Starovoitov <ast@...nel.org>, Daniel
Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
<eddyz87@...il.com>, Song Liu <song@...nel.org>, Yonghong Song
<yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, KP
Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo
<haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, Luke Nelson
<luke.r.nels@...il.com>, Xi Wang <xi.wang@...il.com>, Sami Tolvanen
<samitolvanen@...gle.com>, Peter Zijlstra <peterz@...radead.org>, Kees
Cook <keescook@...omium.org>, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Cc: puranjay12@...il.com
Subject: Re: [PATCH bpf-next 0/1] Support kCFI + BPF on riscv64
Puranjay Mohan <puranjay12@...il.com> 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@...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@...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@...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@...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@...nel.org>
Powered by blists - more mailing lists