fix a verifier bug in check_attach_btf_id()
Subsystems (4)
cover fix a verifier bug in check_attach_btf_id()
Commit 5b92a28aae4d ("bpf: Support attaching tracing BPF program to other BPF programs") added support to attach tracing bpf program to other bpf programs. It had a bug when trying to get the address of the jited image if the main program does not have any callees, resulting in the following kernel segfault:
......
[79162.619208] BUG: kernel NULL pointer dereference, address:
0000000000000000
......
[79162.634255] Call Trace:
[79162.634974] ? _cond_resched+0x15/0x30
[79162.635686] ? kmem_cache_alloc_trace+0x162/0x220
[79162.636398] ? selinux_bpf_prog_alloc+0x1f/0x60
[79162.637111] bpf_prog_load+0x3de/0x690
[79162.637809] __do_sys_bpf+0x105/0x1740
[79162.638488] do_syscall_64+0x5b/0x180
[79162.639147] entry_SYSCALL_64_after_hwframe+0x44/0xa9Patch #1 fixed the problem with more explanation in the commit message. Patch #2 added a selftest which will fail without this patch.
Yonghong Song (2):
bpf: fix a bug to get subprog 0 jited image in check_attach_btf_id selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no
callees
kernel/bpf/verifier.c | 5 +- .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 70 ++++++++++++++----- .../bpf/progs/fexit_bpf2bpf_simple.c | 26 +++++++ .../selftests/bpf/progs/test_pkt_md_access.c | 4 +- 4 files changed, 85 insertions(+), 20 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/fexit_bpf2bpf_simple.c
Signature · 1 line
2.17.1
PATCH 1/2 bpf: fix a bug when getting subprog 0 jited image in check_attach_btf_id
For jited bpf program, if the subprogram count is 1, i.e., there is no callees in the program, prog->aux->func will be NULL and prog->bpf_func points to image address of the program.
If there is more than one subprogram, prog->aux->func is populated, and subprogram 0 can be accessed through either prog->bpf_func or prog->aux->func[0]. Other subprograms should be accessed through prog->aux->func[subprog_id].
This patch fixed a bug in check_attach_btf_id(), where prog->aux->func[subprog_id] is used to access any subprogram which caused a segfault like below:
[79162.619208] BUG: kernel NULL pointer dereference, address: 0000000000000000 ...... [79162.634255] Call Trace: [79162.634974] ? _cond_resched+0x15/0x30 [79162.635686] ? kmem_cache_alloc_trace+0x162/0x220 [79162.636398] ? selinux_bpf_prog_alloc+0x1f/0x60 [79162.637111] bpf_prog_load+0x3de/0x690 [79162.637809] __do_sys_bpf+0x105/0x1740 [79162.638488] do_syscall_64+0x5b/0x180 [79162.639147] entry_SYSCALL_64_after_hwframe+0x44/0xa9 ......
--- kernel/bpf/verifier.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/bpf/verifier.c +4 -1
@@ -9636,7 +9636,10 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) ret = -EINVAL ; goto out; } - addr = (long ) tgt_prog->aux->func[subprog]->bpf_func; +if (subprog ==0 ) + addr = (long ) tgt_prog->bpf_func; +else + addr = (long ) tgt_prog->aux->func[subprog]->bpf_func; }else { addr =kallsyms_lookup_name (tname);if (!addr) {
Signature · 1 line
2.17.1
Re: [PATCH bpf 1/2] bpf: fix a bug when getting subprog 0 jited image in check_attach_btf_id
Quoted 37 lines · Yonghong Song
For jited bpf program, if the subprogram count is 1, i.e., there is no callees in the program,
prog->aux->funcwill be NULL andprog->bpf_funcpoints to image address of the program.If there is more than one subprogram,
prog->aux->funcis populated, and subprogram 0 can be accessed through eitherprog->bpf_funcor prog->aux->func[0]. Other subprograms should be accessed through prog->aux->func[subprog_id].This patch fixed a bug in
check_attach_btf_id(), where prog->aux->func[subprog_id] is used to access any subprogram which caused a segfault like below:[79162.619208] BUG: kernel NULL pointer dereference, address: 0000000000000000 ...... [79162.634255] Call Trace: [79162.634974] ? _cond_resched+0x15/0x30 [79162.635686] ? kmem_cache_alloc_trace+0x162/0x220 [79162.636398] ? selinux_bpf_prog_alloc+0x1f/0x60 [79162.637111] bpf_prog_load+0x3de/0x690 [79162.637809] __do_sys_bpf+0x105/0x1740 [79162.638488] do_syscall_64+0x5b/0x180 [79162.639147] entry_SYSCALL_64_after_hwframe+0x44/0xa9 ......Fixes 5b92a28aae4d ("bpf: Support attaching tracing BPF program to other BPF programs")Reported-by Eelco Chaudron <[email protected]>Signed-off-by Yonghong Song <[email protected]>--- kernel/bpf/verifier.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)kernel/bpf/verifier.c +0
-0
kernel/bpf/verifier.c static int check_attach_btf_id(struct bpf_verifier_env *env) · line 9642
Quoted 8 lines
kernel/bpf/verifier.c +4
-1@@ -9636,7 +9636,10 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) ret = -EINVAL ; goto out; } - addr = (long ) tgt_prog->aux->func[subprog]->bpf_func; +if (subprog ==0 ) + addr = (long ) tgt_prog->bpf_func; +else + addr = (long ) tgt_prog->aux->func[subprog]->bpf_func;
That is exactly the code I had while developing, but then decided to simplify it, since tgt_prog->aux->func[0]->bpf_func == tgt_prog->bpf_func. Oh well.
Thanks for the fix!
PATCH 2/2 selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no callees
In reply to Yonghong SongThe existing fexit_bpf2bpf test covers the target progrm with callees. This patch added a test for the target program without callees.
--- .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 70 ++++++++++++++----- .../bpf/progs/fexit_bpf2bpf_simple.c | 26 +++++++ .../selftests/bpf/progs/test_pkt_md_access.c | 4 +- 3 files changed, 81 insertions(+), 19 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/fexit_bpf2bpf_simple.c
tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c +53 -17
@@ -2,25 +2,21 @@/* Copyright (c) 2019 Facebook */ #include <test_progs.h> -#define PROG_CNT 3 - -void test_fexit_bpf2bpf (void ) +static void test_fexit_bpf2bpf_common (const char *obj_file, +const char *target_obj_file, +int prog_cnt, +const char **prog_name) { -const char *prog_name[PROG_CNT ] = { -"fexit/test_pkt_access" , -"fexit/test_pkt_access_subprog1" , -"fexit/test_pkt_access_subprog2" , - };struct bpf_object *obj =NULL , *pkt_obj;int err, pkt_fd, i; -struct bpf_link *link[PROG_CNT ] = {}; -struct bpf_program *prog[PROG_CNT ]; +struct bpf_link **link =NULL ; +struct bpf_program **prog =NULL ;__u32 duration, retval;struct bpf_map *data_map;const int zero =0 ; -u64 result[PROG_CNT ]; +u64 *result =NULL ; - err =bpf_prog_load ("./test_pkt_access.o" ,BPF_PROG_TYPE_UNSPEC , + err =bpf_prog_load (target_obj_file,BPF_PROG_TYPE_UNSPEC , &pkt_obj, &pkt_fd);if (CHECK (err,"prog_load sched cls" ,"err %d errno %d\n" , err, errno))return ;
@@ -28,7 +24,7 @@ void test_fexit_bpf2bpf(void) .attach_prog_fd = pkt_fd, ); - obj =bpf_object__open_file ("./fexit_bpf2bpf.o" , &opts); + obj =bpf_object__open_file (obj_file, &opts);if (CHECK (IS_ERR_OR_NULL (obj),"obj_open" ,"failed to open fexit_bpf2bpf: %ld\n" ,PTR_ERR (obj)))
@@ -38,7 +34,14 @@ void test_fexit_bpf2bpf(void)if (CHECK (err,"obj_load" ,"err %d\n" , err)) goto close_prog; -for (i =0 ; i <PROG_CNT ; i++) { + link =calloc (sizeof (struct bpf_link *), prog_cnt); + prog =calloc (sizeof (struct bpf_program *), prog_cnt); + result =malloc (prog_cnt *sizeof (u64)); +if (CHECK (!link || !prog || !result,"alloc_memory" , +"failed to alloc memory" )) + goto close_prog; + +for (i =0 ; i < prog_cnt; i++) { prog[i] =bpf_object__find_program_by_title (obj, prog_name[i]);if (CHECK (!prog[i],"find_prog" ,"prog %s not found\n" , prog_name[i])) goto close_prog;
@@ -56,21 +59,54 @@ void test_fexit_bpf2bpf(void)"err %d errno %d retval %d duration %d\n" , err, errno, retval, duration); - err =bpf_map_lookup_elem (bpf_map__fd (data_map), &zero, &result); + err =bpf_map_lookup_elem (bpf_map__fd (data_map), &zero, result);if (CHECK (err,"get_result" ,"failed to get output data: %d\n" , err)) goto close_prog; -for (i =0 ; i <PROG_CNT ; i++) +for (i =0 ; i < prog_cnt; i++)if (CHECK (result[i] !=1 ,"result" ,"fexit_bpf2bpf failed err %ld\n" , result[i])) goto close_prog; close_prog: -for (i =0 ; i <PROG_CNT ; i++) +for (i =0 ; i < prog_cnt; i++)if (!IS_ERR_OR_NULL (link[i]))bpf_link__destroy (link[i]);if (!IS_ERR_OR_NULL (obj))bpf_object__close (obj);bpf_object__close (pkt_obj); +free (link); +free (prog); +free (result); +} + +static void test_target_no_callees (void ) +{ +const char *prog_name[] = { +"fexit/test_pkt_md_access" , + }; +test_fexit_bpf2bpf_common ("./fexit_bpf2bpf_simple.o" , +"./test_pkt_md_access.o" , +ARRAY_SIZE (prog_name), + prog_name); +} + +static void test_target_yes_callees (void ) +{ +const char *prog_name[] = { +"fexit/test_pkt_access" , +"fexit/test_pkt_access_subprog1" , +"fexit/test_pkt_access_subprog2" , + }; +test_fexit_bpf2bpf_common ("./fexit_bpf2bpf.o" , +"./test_pkt_access.o" , +ARRAY_SIZE (prog_name), + prog_name); +} + +void test_fexit_bpf2bpf (void ) +{ +test_target_no_callees (); +test_target_yes_callees (); }
tools/testing/selftests/bpf/progs/fexit_bpf2bpf_simple.c new file +26 -0
@@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2019 Facebook */ +#include <linux/bpf.h> +#include "bpf_helpers.h" +#include "bpf_trace_helpers.h" + +struct sk_buff { +unsigned int len; +}; + +__u64 test_result =0 ; +BPF_TRACE_2 ("fexit/test_pkt_md_access" , test_main2, + struct sk_buff *, skb, int, ret) +{ +int len; + +__builtin_preserve_access_index (({ + len = skb->len; + })); +if (len !=74 || ret !=0 ) +return 0 ; + + test_result =1 ; +return 0 ; +} +char _license[] SEC("license") = "GPL";
tools/testing/selftests/bpf/progs/test_pkt_md_access.c +2 -2
@@ -27,8 +27,8 @@ int _version SEC("version") = 1; }#endif -SEC("test1") -int process (struct __sk_buff *skb) +SEC("classifier/test_pkt_md_access") +int test_pkt_md_access (struct __sk_buff *skb) {TEST_FIELD (__u8, len,0xFF );TEST_FIELD (__u16, len,0xFFFF );
Signature · 1 line
2.17.1
Re: [PATCH bpf 2/2] selftests/bpf: add a fexit/bpf2bpf test with target bpf prog no callees
Quoted 7 lines · Yonghong Song
tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c +1
-1- obj =bpf_object__open_file ("./fexit_bpf2bpf.o" , &opts); + obj =bpf_object__open_file (obj_file, &opts);if (CHECK (IS_ERR_OR_NULL (obj),"obj_open" ,"failed to open fexit_bpf2bpf: %ld\n" ,PTR_ERR (obj)))
tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c void test_fexit_bpf2bpf(void) · line 42
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c +6
-1@@ -38,7 +34,14 @@ void test_fexit_bpf2bpf(void)if (CHECK (err,"obj_load" ,"err %d\n" , err)) goto close_prog; -for (i =0 ; i <PROG_CNT ; i++) { + link =calloc (sizeof (struct bpf_link *), prog_cnt); + prog =calloc (sizeof (struct bpf_program *), prog_cnt); + result =malloc (prog_cnt *sizeof (u64)); +if (CHECK (!link || !prog || !result,"alloc_memory" , +"failed to alloc memory" )) + goto close_prog;
bpf_object__open_file() can fail when jit is off and for() loop in close_prog will segfault. I fixed it up by moving above 3 mallocs before bpf_object__open_file() and applied both patches. Thanks!