Hi,
This series adds missing memory access tags (MEM_RDONLY or MEM_WRITE) to several bpf helper function prototypes that use ARG_PTR_TO_MEM but lack the correct type annotation.
Missing memory access tags in helper prototypes can lead to critical correctness issues when the verifier tries to perform code optimization. After commit 37cce22dbd51 ("bpf: verifier: Refactor helper access type tracking"), the verifier relies on the memory access tags, rather than treating all arguments in helper functions as potentially modifying the pointed-to memory.
We have already seen several reports regarding this:
- commit ac44dcc788b9 ("bpf: Fix verifier assumptions of bpf_d_path's output buffer") adds MEM_WRITE to bpf_d_path; - commit 2eb7648558a7 ("bpf: Specify access type of bpf_sysctl_get_name args") adds MEM_WRITE to bpf_sysctl_get_name.
This series looks through all prototypes in the kernel and completes the tags. In addition, this series also adds selftests for some of these functions.
I marked the series as RFC since the introduced selftests are fragile and ad hoc (similar to the previously added selftests). The original goal of these tests is to reproduce the case where the verifier wrongly optimizes reads after the helper function is called. However, triggering the error often requires carefully designed code patterns. For example, I had to explicitly use "if (xx != 0)" in my attached tests, because using memcmp will not reproduce the issue. This makes the reproduction heavily dependent on the verifier's internal optimization logic and clutters the selftests with specific, unnatural patterns.
Some cases are also hard to trigger by selftests. For example, I couldn't find a triggering pattern for bpf_read_branch_records, since the execution of program seems to be messed up by wrong tags. For bpf_skb_fib_lookup, I also failed to reproduce it because the argument needs content on entry, but the verifier seems to only enable this optimization for fully empty buffers.
Since adding selftests does not help with existing issues or prevent future occurrences of similar problems, I believe one way to resolve it is to statically restrict ARG_PTR_TO_MEM from appearing without memory access tags. Using ARG_PTR_TO_MEM alone without tags is nonsensical because:
- If the helper does not change the argument, missing MEM_RDONLY causes the verifier to incorrectly reject a read-only buffer. - If the helper does change the argument, missing MEM_WRITE causes the verifier to incorrectly assume the memory is unchanged, leading to potential errors.
I am still wondering, if we agree on the above, how should we enforce this restriction? Should we let ARG_PTR_TO_MEM imply MEM_WRITE semantics by default, and change ARG_PTR_TO_MEM | MEM_RDONLY to ARG_CONST_PTR_TO_MEM? Or should we add a check in the verifier to ensure ARG_PTR_TO_MEM always comes with an access tag (though this seems to only catch errors at runtime/testing)?
Any insights and comments are welcome. If the individual fix patches for the prototypes look correct, I would also really appreciate it if they could be merged ahead of the discussion.
Thanks,
Zesen Liu
Signed-off-by: Zesen Liu ftyghome@gmail.com --- Zesen Liu (2): bpf: Fix memory access tags in helper prototypes selftests/bpf: add regression tests for snprintf and get_stack helpers
kernel/bpf/helpers.c | 2 +- kernel/trace/bpf_trace.c | 6 +++--- net/core/filter.c | 8 ++++---- tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c | 15 +++++++++++++-- tools/testing/selftests/bpf/prog_tests/snprintf.c | 6 ++++++ tools/testing/selftests/bpf/prog_tests/snprintf_btf.c | 3 +++ tools/testing/selftests/bpf/progs/netif_receive_skb.c | 13 ++++++++++++- tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c | 11 ++++++++++- tools/testing/selftests/bpf/progs/test_snprintf.c | 12 ++++++++++++ 9 files changed, 64 insertions(+), 12 deletions(-) --- base-commit: 22cc16c04b7893d8fc22810599f49a305d600b9e change-id: 20251220-helper_proto-fb6e64182467
Best regards,
After commit 37cce22dbd51 ("bpf: verifier: Refactor helper access type tracking"), the verifier started relying on the access type tags in helper function prototypes to perform memory access optimizations.
Currently, several helper functions utilizing ARG_PTR_TO_MEM lack the corresponding MEM_RDONLY or MEM_WRITE tags. This omission causes the verifier to incorrectly assume that the buffer contents are unchanged across the helper call. Consequently, the verifier may optimize away subsequent reads based on this wrong assumption, leading to correctness issues.
Similar issues were recently addressed for specific helpers in commit ac44dcc788b9 ("bpf: Fix verifier assumptions of bpf_d_path's output buffer") and commit 2eb7648558a7 ("bpf: Specify access type of bpf_sysctl_get_name args").
Fix these prototypes by adding the correct memory access tags.
Fixes: 37cce22dbd51 ("bpf: verifier: Refactor helper access type tracking") Co-developed-by: Shuran Liu electronlsr@gmail.com Signed-off-by: Shuran Liu electronlsr@gmail.com Co-developed-by: Peili Gao gplhust955@gmail.com Signed-off-by: Peili Gao gplhust955@gmail.com Co-developed-by: Haoran Ni haoran.ni.cs@gmail.com Signed-off-by: Haoran Ni haoran.ni.cs@gmail.com Signed-off-by: Zesen Liu ftyghome@gmail.com --- kernel/bpf/helpers.c | 2 +- kernel/trace/bpf_trace.c | 6 +++--- net/core/filter.c | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index db72b96f9c8c..f66284f8ec2c 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -1077,7 +1077,7 @@ const struct bpf_func_proto bpf_snprintf_proto = { .func = bpf_snprintf, .gpl_only = true, .ret_type = RET_INTEGER, - .arg1_type = ARG_PTR_TO_MEM_OR_NULL, + .arg1_type = ARG_PTR_TO_MEM_OR_NULL | MEM_WRITE, .arg2_type = ARG_CONST_SIZE_OR_ZERO, .arg3_type = ARG_PTR_TO_CONST_STR, .arg4_type = ARG_PTR_TO_MEM | PTR_MAYBE_NULL | MEM_RDONLY, diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index fe28d86f7c35..59c2394981c7 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1022,7 +1022,7 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { .func = bpf_snprintf_btf, .gpl_only = false, .ret_type = RET_INTEGER, - .arg1_type = ARG_PTR_TO_MEM, + .arg1_type = ARG_PTR_TO_MEM | MEM_WRITE, .arg2_type = ARG_CONST_SIZE, .arg3_type = ARG_PTR_TO_MEM | MEM_RDONLY, .arg4_type = ARG_CONST_SIZE, @@ -1526,7 +1526,7 @@ static const struct bpf_func_proto bpf_read_branch_records_proto = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM_OR_NULL, + .arg2_type = ARG_PTR_TO_MEM_OR_NULL | MEM_WRITE, .arg3_type = ARG_CONST_SIZE_OR_ZERO, .arg4_type = ARG_ANYTHING, }; @@ -1661,7 +1661,7 @@ static const struct bpf_func_proto bpf_get_stack_proto_raw_tp = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM | MEM_RDONLY, + .arg2_type = ARG_PTR_TO_UNINIT_MEM, .arg3_type = ARG_CONST_SIZE_OR_ZERO, .arg4_type = ARG_ANYTHING, }; diff --git a/net/core/filter.c b/net/core/filter.c index 616e0520a0bb..6e07bb994aa7 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -6399,7 +6399,7 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_MEM | MEM_WRITE, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, }; @@ -6454,7 +6454,7 @@ static const struct bpf_func_proto bpf_skb_fib_lookup_proto = { .gpl_only = true, .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_CTX, - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_MEM | MEM_WRITE, .arg3_type = ARG_CONST_SIZE, .arg4_type = ARG_ANYTHING, }; @@ -8010,7 +8010,7 @@ static const struct bpf_func_proto bpf_tcp_raw_gen_syncookie_ipv4_proto = { .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_FIXED_SIZE_MEM, .arg1_size = sizeof(struct iphdr), - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_MEM | MEM_RDONLY, .arg3_type = ARG_CONST_SIZE_OR_ZERO, };
@@ -8042,7 +8042,7 @@ static const struct bpf_func_proto bpf_tcp_raw_gen_syncookie_ipv6_proto = { .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_FIXED_SIZE_MEM, .arg1_size = sizeof(struct ipv6hdr), - .arg2_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_PTR_TO_MEM | MEM_RDONLY, .arg3_type = ARG_CONST_SIZE_OR_ZERO, };
Add regression tests for bpf_snprintf(), bpf_snprintf_btf(), and bpf_get_stack() to cover incorrect verifier assumptions caused by incorrect function prototypes.
These tests reproduce the scenario where the verifier previously incorrectly assumed that the destination buffer remained unwritten across the helper call. The tests call these helpers and verify that subsequent reads see the updated data, ensuring that the verifier correctly marks the memory as clobbered and does not optimize away the reads based on stale assumptions.
Co-developed-by: Shuran Liu electronlsr@gmail.com Signed-off-by: Shuran Liu electronlsr@gmail.com Co-developed-by: Peili Gao gplhust955@gmail.com Signed-off-by: Peili Gao gplhust955@gmail.com Co-developed-by: Haoran Ni haoran.ni.cs@gmail.com Signed-off-by: Haoran Ni haoran.ni.cs@gmail.com Signed-off-by: Zesen Liu ftyghome@gmail.com --- tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c | 15 +++++++++++++-- tools/testing/selftests/bpf/prog_tests/snprintf.c | 6 ++++++ tools/testing/selftests/bpf/prog_tests/snprintf_btf.c | 3 +++ tools/testing/selftests/bpf/progs/netif_receive_skb.c | 13 ++++++++++++- tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c | 11 ++++++++++- tools/testing/selftests/bpf/progs/test_snprintf.c | 12 ++++++++++++ 6 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c index 858e0575f502..7c2774b49138 100644 --- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c +++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c @@ -87,13 +87,13 @@ void test_get_stack_raw_tp(void) const char *file = "./test_get_stack_rawtp.bpf.o"; const char *file_err = "./test_get_stack_rawtp_err.bpf.o"; const char *prog_name = "bpf_prog1"; - int i, err, prog_fd, exp_cnt = MAX_CNT_RAWTP; + int i, err, prog_fd, exp_cnt = MAX_CNT_RAWTP, key = 0, valid_top_stack = 0; struct perf_buffer *pb = NULL; struct bpf_link *link = NULL; struct timespec tv = {0, 10}; struct bpf_program *prog; struct bpf_object *obj; - struct bpf_map *map; + struct bpf_map *map, *bss_map; cpu_set_t cpu_set;
err = bpf_prog_test_load(file_err, BPF_PROG_TYPE_RAW_TRACEPOINT, &obj, &prog_fd); @@ -135,6 +135,17 @@ void test_get_stack_raw_tp(void) for (i = 0; i < MAX_CNT_RAWTP; i++) nanosleep(&tv, NULL);
+ bss_map = bpf_object__find_map_by_name(obj, ".bss"); + if (CHECK(!bss_map, "find .bss map", "not found\n")) + goto close_prog; + + err = bpf_map_lookup_elem(bpf_map__fd(bss_map), &key, &valid_top_stack); + if (CHECK(err, "lookup .bss", "err %d errno %d\n", err, errno)) + goto close_prog; + + if (!ASSERT_EQ(valid_top_stack, 1, "valid_top_stack")) + goto close_prog; + while (exp_cnt > 0) { err = perf_buffer__poll(pb, 100); if (err < 0 && CHECK(err < 0, "pb__poll", "err %d\n", err)) diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c index 594441acb707..80d6f2655b5f 100644 --- a/tools/testing/selftests/bpf/prog_tests/snprintf.c +++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c @@ -33,6 +33,9 @@
#define EXP_NO_BUF_RET 29
+#define EXP_STACK_OUT "stack_out" +#define EXP_STACK_RET sizeof(EXP_STACK_OUT) + static void test_snprintf_positive(void) { char exp_addr_out[] = EXP_ADDR_OUT; @@ -79,6 +82,9 @@ static void test_snprintf_positive(void)
ASSERT_EQ(skel->bss->nobuf_ret, EXP_NO_BUF_RET, "no_buf_ret");
+ ASSERT_EQ(skel->bss->stack_ret, EXP_STACK_RET, "stack_ret"); + ASSERT_STREQ(skel->bss->stack_out, EXP_STACK_OUT, "stack_out"); + cleanup: test_snprintf__destroy(skel); } diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf_btf.c b/tools/testing/selftests/bpf/prog_tests/snprintf_btf.c index dd41b826be30..a2e400a4880d 100644 --- a/tools/testing/selftests/bpf/prog_tests/snprintf_btf.c +++ b/tools/testing/selftests/bpf/prog_tests/snprintf_btf.c @@ -55,6 +55,9 @@ void serial_test_snprintf_btf(void) bss->ran_subtests)) goto cleanup;
+ if (!ASSERT_EQ(bss->stack_out_test_passed, 1, "stack output test failed")) + goto cleanup; + cleanup: netif_receive_skb__destroy(skel); } diff --git a/tools/testing/selftests/bpf/progs/netif_receive_skb.c b/tools/testing/selftests/bpf/progs/netif_receive_skb.c index 9e067dcbf607..f78d5f56f6c9 100644 --- a/tools/testing/selftests/bpf/progs/netif_receive_skb.c +++ b/tools/testing/selftests/bpf/progs/netif_receive_skb.c @@ -12,9 +12,11 @@ long ret = 0; int num_subtests = 0; int ran_subtests = 0; +int stack_out_test_passed = 0; bool skip = false;
-#define STRSIZE 2048 +#define STRSIZE 2048 +#define STACK_STRSIZE 64 #define EXPECTED_STRSIZE 256
#if defined(bpf_target_s390) @@ -98,6 +100,7 @@ int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb) __u32 key = 0; int i, __ret; char *str; + char stack_out[STACK_STRSIZE] = { };
#if __has_builtin(__builtin_btf_type_id) str = bpf_map_lookup_elem(&strdata, &key); @@ -124,6 +127,13 @@ int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb) ret = -ERANGE; }
+ /* Check when writing to a buffer on the stack */ + p.type_id = bpf_core_type_id_kernel(struct sk_buff); + p.ptr = skb; + ret = bpf_snprintf_btf(stack_out, STACK_STRSIZE, &p, sizeof(p), 0); + if (ret >= 0 && stack_out[0] != '\0') + stack_out_test_passed = 1; + /* Verify type display for various types. */
/* simple int */ @@ -242,6 +252,7 @@ int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb) TEST_BTF(str, struct bpf_insn, BTF_F_NONAME, "{1,0x2,0x3,4,5,}", {.code = 1, .dst_reg = 0x2, .src_reg = 0x3, .off = 4, .imm = 5,}); + #else skip = true; #endif diff --git a/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c b/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c index b6a6eb279e54..57723dc823a0 100644 --- a/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c +++ b/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c @@ -54,14 +54,17 @@ struct { __type(value, __u64[2 * MAX_STACK_RAWTP]); } rawdata_map SEC(".maps");
+int valid_top_stack = 0; + SEC("raw_tracepoint/sys_enter") int bpf_prog1(void *ctx) { int max_len, max_buildid_len, total_size; struct stack_trace_t *data; - long usize, ksize; + long usize, ksize, top_usize; void *raw_data; __u32 key = 0; + __u64 top_user_stack = 0;
data = bpf_map_lookup_elem(&stackdata_map, &key); if (!data) @@ -88,6 +91,12 @@ int bpf_prog1(void *ctx) if (usize < 0) return 0;
+ /* checks if the verifier correctly marks the stack variable as written. */ + top_usize = bpf_get_stack(ctx, &top_user_stack, sizeof(__u64), + BPF_F_USER_STACK); + if (top_usize > 0 && top_user_stack != 0) + valid_top_stack = 1; + ksize = bpf_get_stack(ctx, raw_data + usize, max_len - usize, 0); if (ksize < 0) return 0; diff --git a/tools/testing/selftests/bpf/progs/test_snprintf.c b/tools/testing/selftests/bpf/progs/test_snprintf.c index 8fda07544023..ce78fd7add03 100644 --- a/tools/testing/selftests/bpf/progs/test_snprintf.c +++ b/tools/testing/selftests/bpf/progs/test_snprintf.c @@ -32,6 +32,9 @@ long noarg_ret = 0;
long nobuf_ret = 0;
+char stack_out[64] = {}; +long stack_ret = 0; + extern const void schedule __ksym;
SEC("raw_tp/sys_enter") @@ -42,6 +45,7 @@ int handler(const void *ctx) const __u8 ex_ipv6[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}; static const char str1[] = "str1"; static const char longstr[] = "longstr"; + char buf[64] = {};
if ((int)bpf_get_current_pid_tgid() != pid) return 0; @@ -71,6 +75,14 @@ int handler(const void *ctx) /* No buffer */ nobuf_ret = BPF_SNPRINTF(NULL, 0, "only interested in length %d", 60);
+ /* Write to a buffer on the stack */ + stack_ret = BPF_SNPRINTF(buf, sizeof(buf), "stack_out"); + /* The condition is necessary to check if the verifier + * correctly marks the stack memory as written. + */ + if (buf[0] != '\0') + __builtin_memcpy(stack_out, buf, 64); + return 0; }
linux-kselftest-mirror@lists.linaro.org
-
Zesen Liu