Message-ID: <CAMB2axPpB7Km=_7J_QTcQV8SvuFMKqg-_fCyRHEZKNfr7WL2Gg@mail.gmail.com>
Date: Mon, 22 Dec 2025 11:29:17 -0800
From: Amery Hung <ameryhung@...il.com>
To: Zesen Liu <ftyghome@...il.com>
Cc: 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@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Matt Bobrowski <mattbobrowski@...gle.com>, 
	Steven Rostedt <rostedt@...dmis.org>, Masami Hiramatsu <mhiramat@...nel.org>, 
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, Daniel Xu <dxu@...uu.xyz>, Shuah Khan <shuah@...nel.org>, 
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, Shuran Liu <electronlsr@...il.com>, 
	Peili Gao <gplhust955@...il.com>, Haoran Ni <haoran.ni.cs@...il.com>
Subject: Re: [RFC bpf PATCH 0/2] bpf: Fix memory access tags in helper prototypes

On Sat, Dec 20, 2025 at 3:35 AM Zesen Liu <ftyghome@...il.com> 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 <ftyghome@...il.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,
> --
> Zesen Liu <ftyghome@...il.com>
>
>

Powered by blists - more mailing lists