On Sat, Dec 20, 2025 at 3:35 AM Zesen Liu <[email protected]> wrote: > > 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.
Perhaps you are conflating one of your proposals here? This is fine currently. ARG_PTR_TO_MEM without any annotation is viewed as BPF_READ so passing a read-only buffer should work. > - 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)? I think it is better to make the MEM_WRITE, MEM_RDONLY annotation explicit and check it in the verifier. Flipping the default MEM_RDONLY semantic to MEM_WRITE does not prevent a similar bug in the future when we have helpers/optimizations/checks rely on an implicit semantic. > > 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 <[email protected]> > --- > 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, > -- > Zesen Liu <[email protected]> > >
