* [PATCH bpf-next 0/2] bpf: Preserve stack frame number for commuted stack arithmetic @ 2026-07-20 16:37 Yiyang Chen 2026-07-20 16:37 ` [PATCH bpf-next 1/2] bpf: Preserve stack frame number for commuted arithmetic Yiyang Chen 2026-07-20 16:37 ` [PATCH bpf-next 2/2] selftests/bpf: Cover stack frame number after scalar plus fp Yiyang Chen 0 siblings, 2 replies; 6+ messages in thread From: Yiyang Chen @ 2026-07-20 16:37 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi Cc: Yiyang Chen, John Fastabend, Martin KaFai Lau, Song Liu, Yonghong Song, Jiri Olsa, Shuah Khan, Emil Tsalapatis, bpf, linux-kselftest, linux-kernel The verifier uses reg->frameno to distinguish stack pointers that have the same offset but refer to different call frames. The scalar += pointer path copies pointer type and id to the destination register, but it also needs to preserve frameno when the source pointer is PTR_TO_STACK. Preserve the frame number for commuted stack pointer arithmetic and add a verifier regression test where a callee derives its frame pointer through scalar += fp before overwriting and reloading the same stack slot. Yiyang Chen (2): bpf: Preserve stack frame number for commuted arithmetic selftests/bpf: Cover stack frame number after scalar plus fp kernel/bpf/verifier.c | 7 +++- .../selftests/bpf/prog_tests/verifier.c | 2 - .../bpf/prog_tests/verifier_basic_stack.c | 28 +++++++++++++ .../bpf/progs/verifier_basic_stack.c | 41 +++++++++++++++++++ 4 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/verifier_basic_stack.c base-commit: cfce77b63375dac81d53f2f85593c548415206b7 -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 1/2] bpf: Preserve stack frame number for commuted arithmetic 2026-07-20 16:37 [PATCH bpf-next 0/2] bpf: Preserve stack frame number for commuted stack arithmetic Yiyang Chen @ 2026-07-20 16:37 ` Yiyang Chen 2026-07-20 18:47 ` Eduard Zingerman 2026-07-20 16:37 ` [PATCH bpf-next 2/2] selftests/bpf: Cover stack frame number after scalar plus fp Yiyang Chen 1 sibling, 1 reply; 6+ messages in thread From: Yiyang Chen @ 2026-07-20 16:37 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi Cc: Yiyang Chen, John Fastabend, Martin KaFai Lau, Song Liu, Yonghong Song, Jiri Olsa, Shuah Khan, Emil Tsalapatis, bpf, linux-kselftest, linux-kernel When scalar += pointer is handled in adjust_ptr_min_max_vals(), the destination register inherits the pointer type and id from the source pointer. For PTR_TO_STACK, the inherited pointer state also has to carry the stack frame number. Without the frame number copy, a stack pointer derived inside a callee through scalar += fp can be recorded as pointing to frame 0. Stack reads and writes through that register can then update or consult the caller frame while the actual instruction uses the callee frame. Copy the frame number when PTR_TO_STACK state is inherited by the commuted arithmetic form. Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn> --- kernel/bpf/verifier.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 52be0a118cce0..58017141d52b7 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -13796,11 +13796,14 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, return -EACCES; } - /* In case of 'scalar += pointer', dst_reg inherits pointer type and id. - * The id may be overwritten later if we create a new variable offset. + /* In case of 'scalar += pointer', dst_reg inherits pointer type, id, + * and for stack pointers also the frame number. The id may be overwritten + * later if we create a new variable offset. */ dst_reg->type = ptr_reg->type; dst_reg->id = ptr_reg->id; + if (base_type(ptr_reg->type) == PTR_TO_STACK) + dst_reg->frameno = ptr_reg->frameno; if (!check_reg_sane_offset_scalar(env, off_reg, ptr_reg->type) || !check_reg_sane_offset_ptr(env, ptr_reg, ptr_reg->type)) -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Preserve stack frame number for commuted arithmetic 2026-07-20 16:37 ` [PATCH bpf-next 1/2] bpf: Preserve stack frame number for commuted arithmetic Yiyang Chen @ 2026-07-20 18:47 ` Eduard Zingerman 2026-07-21 7:51 ` Yiyang Chen 0 siblings, 1 reply; 6+ messages in thread From: Eduard Zingerman @ 2026-07-20 18:47 UTC (permalink / raw) To: Yiyang Chen, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kumar Kartikeya Dwivedi Cc: John Fastabend, Martin KaFai Lau, Song Liu, Yonghong Song, Jiri Olsa, Shuah Khan, Emil Tsalapatis, bpf, linux-kselftest, linux-kernel On Mon, 2026-07-20 at 16:37 +0000, Yiyang Chen wrote: > When scalar += pointer is handled in adjust_ptr_min_max_vals(), the > destination register inherits the pointer type and id from the source > pointer. For PTR_TO_STACK, the inherited pointer state also has to carry > the stack frame number. > > Without the frame number copy, a stack pointer derived inside a callee > through scalar += fp can be recorded as pointing to frame 0. Stack reads > and writes through that register can then update or consult the caller > frame while the actual instruction uses the callee frame. > > Copy the frame number when PTR_TO_STACK state is inherited by the > commuted arithmetic form. Could you please infer a "Fixes" tag? > Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn> > --- > kernel/bpf/verifier.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 52be0a118cce0..58017141d52b7 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -13796,11 +13796,14 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, > return -EACCES; > } > > - /* In case of 'scalar += pointer', dst_reg inherits pointer type and id. > - * The id may be overwritten later if we create a new variable offset. > + /* In case of 'scalar += pointer', dst_reg inherits pointer type, id, > + * and for stack pointers also the frame number. The id may be overwritten > + * later if we create a new variable offset. > */ > dst_reg->type = ptr_reg->type; > dst_reg->id = ptr_reg->id; > + if (base_type(ptr_reg->type) == PTR_TO_STACK) > + dst_reg->frameno = ptr_reg->frameno; This patch fixes a real issue and this a surprisingly buggy piece of code. Looking at other fields defined in bpf_reg_state, it appears that several additional modifications are necessary: - dst_reg->delta = 0 - dst_reg->parent_id = ptr_reg->parent_id Given such an error prone nature, I think it would be better to: - stash the value of the off_reg in a temporary variable and adjust off_reg pointer accordingly (see bpf_verifier_env for a collection of similar temporaries). - do *dst_reg = *ptr_reg instead of the fixup. Wdyt? > > if (!check_reg_sane_offset_scalar(env, off_reg, ptr_reg->type) || > !check_reg_sane_offset_ptr(env, ptr_reg, ptr_reg->type)) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Preserve stack frame number for commuted arithmetic 2026-07-20 18:47 ` Eduard Zingerman @ 2026-07-21 7:51 ` Yiyang Chen 0 siblings, 0 replies; 6+ messages in thread From: Yiyang Chen @ 2026-07-21 7:51 UTC (permalink / raw) To: eddyz87 Cc: andrii, ast, bpf, chenyy23, daniel, emil, john.fastabend, jolsa, linux-kernel, linux-kselftest, martin.lau, memxor, shuah, song, yonghong.song Hi Eduard, Totally agree. I'll respin v2 to preserve the full pointer state instead of copying only selected fields, and use a temporary scalar offset register for the commuted `scalar += pointer` case as you suggested. I also re-checked the history before adding the `Fixes:` tag. The tightest candidate I found is: `Fixes: f1174f77b50c ("bpf/verifier: rework value tracking")` That commit introduced the unified `scalar += pointer` handling and the pointer-state inheritance path. `f4d7e40a5b15` added the multi-frame stack model later, so it is related but less precise as a `Fixes:` target. Will send v2 soon. Thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Cover stack frame number after scalar plus fp 2026-07-20 16:37 [PATCH bpf-next 0/2] bpf: Preserve stack frame number for commuted stack arithmetic Yiyang Chen 2026-07-20 16:37 ` [PATCH bpf-next 1/2] bpf: Preserve stack frame number for commuted arithmetic Yiyang Chen @ 2026-07-20 16:37 ` Yiyang Chen 2026-07-20 17:18 ` Eduard Zingerman 1 sibling, 1 reply; 6+ messages in thread From: Yiyang Chen @ 2026-07-20 16:37 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi Cc: Yiyang Chen, John Fastabend, Martin KaFai Lau, Song Liu, Yonghong Song, Jiri Olsa, Shuah Khan, Emil Tsalapatis, bpf, linux-kselftest, linux-kernel Add a verifier test where a callee spills a map value pointer. It then derives its frame pointer through scalar += fp. It overwrites the same stack slot through the derived pointer. The final reload must be treated as a scalar after the overwrite. The verifier should reject the program when the reloaded value is dereferenced. Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn> --- .../selftests/bpf/prog_tests/verifier.c | 2 - .../bpf/prog_tests/verifier_basic_stack.c | 28 +++++++++++++ .../bpf/progs/verifier_basic_stack.c | 41 +++++++++++++++++++ 3 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/verifier_basic_stack.c diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c index be97f6887f0e7..0483ae340726c 100644 --- a/tools/testing/selftests/bpf/prog_tests/verifier.c +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c @@ -11,7 +11,6 @@ #include "verifier_arena_globals2.skel.h" #include "verifier_array_access.skel.h" #include "verifier_async_cb_context.skel.h" -#include "verifier_basic_stack.skel.h" #include "verifier_bitfield_write.skel.h" #include "verifier_bounds.skel.h" #include "verifier_bounds_deduction.skel.h" @@ -165,7 +164,6 @@ void test_verifier_arena(void) { RUN(verifier_arena); } void test_verifier_arena_large(void) { RUN(verifier_arena_large); } void test_verifier_arena_globals1(void) { RUN(verifier_arena_globals1); } void test_verifier_arena_globals2(void) { RUN(verifier_arena_globals2); } -void test_verifier_basic_stack(void) { RUN(verifier_basic_stack); } void test_verifier_bitfield_write(void) { RUN(verifier_bitfield_write); } void test_verifier_bounds(void) { RUN(verifier_bounds); } void test_verifier_bounds_deduction(void) { RUN(verifier_bounds_deduction); } diff --git a/tools/testing/selftests/bpf/prog_tests/verifier_basic_stack.c b/tools/testing/selftests/bpf/prog_tests/verifier_basic_stack.c new file mode 100644 index 0000000000000..279f389729b8e --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/verifier_basic_stack.c @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <test_progs.h> + +#include "cap_helpers.h" +#include "verifier_basic_stack.skel.h" + +void test_verifier_basic_stack(void) +{ + struct test_loader tester = {}; + __u64 old_caps; + int err; + + /* test_verifier tests are executed w/o CAP_SYS_ADMIN, do the same here */ + err = cap_disable_effective(1ULL << CAP_SYS_ADMIN, &old_caps); + if (err) { + PRINT_FAIL("failed to drop CAP_SYS_ADMIN: %i, %s\n", err, strerror(-err)); + return; + } + + test_loader__run_subtests(&tester, "verifier_basic_stack", + verifier_basic_stack__elf_bytes); + test_loader_fini(&tester); + + err = cap_enable_effective(old_caps, NULL); + if (err) + PRINT_FAIL("failed to restore CAP_SYS_ADMIN: %i, %s\n", err, strerror(-err)); +} diff --git a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c index fb62e09f21146..634183fc86883 100644 --- a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c +++ b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c @@ -97,4 +97,45 @@ __naked void misaligned_read_from_stack(void) " ::: __clobber_all); } +SEC("socket") +__description("stack pointer arithmetic preserves frame number") +__failure __msg("R7 invalid mem access 'scalar'") +__naked void stack_ptr_arith_preserves_frameno(void) +{ + asm volatile (" \ + r3 = 0; \ + *(u64 *)(r10 - 8) = r3; \ + r1 = %[map_hash_8b] ll; \ + r2 = r10; \ + r2 += -8; \ + call %[bpf_map_lookup_elem]; \ + if r0 != 0 goto +2; \ + r0 = 0; \ + exit; \ + r1 = r0; \ + r2 = 0; \ + r3 = 0; \ + call stack_ptr_arith_preserves_frameno_subprog; \ + r0 = 0; \ + exit; \ +" : + : __imm(bpf_map_lookup_elem), + __imm_addr(map_hash_8b) + : __clobber_all); +} + +static __used __naked void stack_ptr_arith_preserves_frameno_subprog(void) +{ + asm volatile (" \ + *(u64 *)(r10 - 8) = r1; \ + r6 = -8; \ + r6 += r10; \ + *(u64 *)(r6 + 0) = r2; \ + r7 = *(u64 *)(r10 - 8); \ + *(u64 *)(r7 + 0) = r3; \ + r0 = 0; \ + exit; \ +" ::: __clobber_all); +} + char _license[] SEC("license") = "GPL"; -- 2.34.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Cover stack frame number after scalar plus fp 2026-07-20 16:37 ` [PATCH bpf-next 2/2] selftests/bpf: Cover stack frame number after scalar plus fp Yiyang Chen @ 2026-07-20 17:18 ` Eduard Zingerman 0 siblings, 0 replies; 6+ messages in thread From: Eduard Zingerman @ 2026-07-20 17:18 UTC (permalink / raw) To: Yiyang Chen, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kumar Kartikeya Dwivedi Cc: John Fastabend, Martin KaFai Lau, Song Liu, Yonghong Song, Jiri Olsa, Shuah Khan, Emil Tsalapatis, bpf, linux-kselftest, linux-kernel On Mon, 2026-07-20 at 16:37 +0000, Yiyang Chen wrote: ... > +++ b/tools/testing/selftests/bpf/prog_tests/verifier_basic_stack.c > @@ -0,0 +1,28 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +#include <test_progs.h> > + > +#include "cap_helpers.h" > +#include "verifier_basic_stack.skel.h" > + > +void test_verifier_basic_stack(void) > +{ > + struct test_loader tester = {}; > + __u64 old_caps; > + int err; > + > + /* test_verifier tests are executed w/o CAP_SYS_ADMIN, do the same here */ > + err = cap_disable_effective(1ULL << CAP_SYS_ADMIN, &old_caps); > + if (err) { > + PRINT_FAIL("failed to drop CAP_SYS_ADMIN: %i, %s\n", err, strerror(-err)); Why is this necessary? It should be possible to prepare a test case demonstrating the bug w/o this change. > + return; > + } > + > + test_loader__run_subtests(&tester, "verifier_basic_stack", > + verifier_basic_stack__elf_bytes); > + test_loader_fini(&tester); > + > + err = cap_enable_effective(old_caps, NULL); > + if (err) > + PRINT_FAIL("failed to restore CAP_SYS_ADMIN: %i, %s\n", err, strerror(-err)); > +} ... ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-07-21 7:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-07-20 16:37 [PATCH bpf-next 0/2] bpf: Preserve stack frame number for commuted stack arithmetic Yiyang Chen 2026-07-20 16:37 ` [PATCH bpf-next 1/2] bpf: Preserve stack frame number for commuted arithmetic Yiyang Chen 2026-07-20 18:47 ` Eduard Zingerman 2026-07-21 7:51 ` Yiyang Chen 2026-07-20 16:37 ` [PATCH bpf-next 2/2] selftests/bpf: Cover stack frame number after scalar plus fp Yiyang Chen 2026-07-20 17:18 ` Eduard Zingerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®