[RFC bpf PATCH 0/2] bpf: Fix memory access tags in helper prototypes

Zesen Liu posted 2 patches 1 month, 2 weeks ago
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(-)
[RFC bpf PATCH 0/2] bpf: Fix memory access tags in helper prototypes
Posted by Zesen Liu 1 month, 2 weeks ago
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,
-- 
Zesen Liu <ftyghome@gmail.com>
Re: [RFC bpf PATCH 0/2] bpf: Fix memory access tags in helper prototypes
Posted by Amery Hung 1 month, 2 weeks ago
On Sat, Dec 20, 2025 at 3:35 AM Zesen Liu <ftyghome@gmail.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@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,
> --
> Zesen Liu <ftyghome@gmail.com>
>
>
Re: [RFC bpf PATCH 0/2] bpf: Fix memory access tags in helper prototypes
Posted by Zesen Liu 1 month, 2 weeks ago
Apologies for the resend due to an incorrect configuration in my mail client.

Thanks for your comment!

> On Dec 23, 2025, at 03:29, Amery Hung <ameryhung@gmail.com> wrote:
> 
> 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.


Actually, that is not the case. I tested this again, and ARG_PTR_TO_MEM
without MEM_RDONLY does reject read-only buffers. You can reproduce this
behavior with test_d_path_check_rdonly_mem in selftests by removing its
arg2’s MEM_WRITE tag.

> 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.

The current default semantic is in an inconsistent state: it implies
neither MEM_RDONLY nor MEM_WRITE. A naked ARG_PTR_TO_MEM rejects
read-only buffers, yet tells the verifier that the helper does not modify the memory.

I see two ways to resolve this ambiguity:
1) Enforce explicit memory access tags (disallow naked ARG_PTR_TO_MEM) as I
proposed eariler; or
2) Change ARG_PTR_TO_MEM semantics to behave exactly like
   ARG_PTR_TO_MEM | MEM_RDONLY.

I would appreciate your thoughts on this. :)


Thanks,
Zesen Liu