diff mbox series

[bpf-next,v2,3/3] selftests/bpf: Add selftest for may_goto

Message ID 20250213131214.164982-4-mrpre@163.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: Fix array bounds error with may_goto and add selftest | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-48 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 26 (+1) this patch: 26 (+1)
netdev/cc_maintainers success CCed 17 of 17 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning CHECK: Lines should not end with a '('
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiayuan Chen Feb. 13, 2025, 1:12 p.m. UTC 
Add test cases to ensure the maximum stack size can be properly limited to
512.

Test result:
echo "0" > /proc/sys/net/core/bpf_jit_enable
./test_progs -t verifier_stack_ptr
verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:SKIP
verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:OK

echo "1" > /proc/sys/net/core/bpf_jit_enable
verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:OK
verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:SKIP

Signed-off-by: Jiayuan Chen <mrpre@163.com>
---
 .../selftests/bpf/progs/verifier_stack_ptr.c  | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)

Comments

Alexei Starovoitov Feb. 13, 2025, 4:04 p.m. UTC | #1 
On Thu, Feb 13, 2025 at 5:13 AM Jiayuan Chen <mrpre@163.com> wrote:
>
> Add test cases to ensure the maximum stack size can be properly limited to
> 512.
>
> Test result:
> echo "0" > /proc/sys/net/core/bpf_jit_enable
> ./test_progs -t verifier_stack_ptr
> verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:SKIP
> verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:OK
>
> echo "1" > /proc/sys/net/core/bpf_jit_enable
> verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:OK
> verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:SKIP

echo '0|1' is not longer necessary ?
The commit log seems obsolete?

pw-bot: cr

> Signed-off-by: Jiayuan Chen <mrpre@163.com>
> ---
>  .../selftests/bpf/progs/verifier_stack_ptr.c  | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> index 417c61cd4b19..8ffe5a01d140 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> @@ -481,4 +481,54 @@ l1_%=:     r0 = 42;                                        \
>         : __clobber_all);
>  }
>
> +SEC("socket")
> +__description("PTR_TO_STACK stack size > 512")
> +__failure __msg("invalid write to stack R1 off=-520 size=8")
> +__naked void stack_check_size_gt_512(void)
> +{
> +       asm volatile (
> +       "r1 = r10;"
> +       "r1 += -520;"
> +       "r0 = 42;"
> +       "*(u64*)(r1 + 0) = r0;"
> +       "exit;"
> +       ::: __clobber_all);
> +}
> +
> +#ifdef __BPF_FEATURE_MAY_GOTO
> +SEC("socket")
> +__description("PTR_TO_STACK stack size 512 with may_goto with jit")
> +__use_jit()
> +__success __retval(42)
> +__naked void stack_check_size_512_with_may_goto_jit(void)
> +{
> +       asm volatile (
> +       "r1 = r10;"
> +       "r1 += -512;"
> +       "r0 = 42;"
> +       "*(u32*)(r1 + 0) = r0;"
> +       "may_goto l0_%=;"
> +       "r2 = 100;"
> +"l0_%=:        exit;"
> +       ::: __clobber_all);
> +}
> +
> +SEC("socket")
> +__description("PTR_TO_STACK stack size 512 with may_goto without jit")
> +__use_interp()
> +__failure __msg("stack size 520(extra 8) is too large")
> +__naked void stack_check_size_512_with_may_goto(void)
> +{
> +       asm volatile (
> +       "r1 = r10;"
> +       "r1 += -512;"
> +       "r0 = 42;"
> +       "*(u32*)(r1 + 0) = r0;"
> +       "may_goto l0_%=;"
> +       "r2 = 100;"
> +"l0_%=:        exit;"
> +       ::: __clobber_all);
> +}
> +#endif
> +
>  char _license[] SEC("license") = "GPL";
> --
> 2.47.1
>
Jiayuan Chen Feb. 13, 2025, 4:41 p.m. UTC | #2 
On Thu, Feb 13, 2025 at 08:04:05AM -0800, Alexei Starovoitov wrote:
> On Thu, Feb 13, 2025 at 5:13 AM Jiayuan Chen <mrpre@163.com> wrote:
> >
> > Add test cases to ensure the maximum stack size can be properly limited to
> > 512.
> >
> > Test result:
> > echo "0" > /proc/sys/net/core/bpf_jit_enable
> > ./test_progs -t verifier_stack_ptr
> > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:SKIP
> > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:OK
> >
> > echo "1" > /proc/sys/net/core/bpf_jit_enable
> > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:OK
> > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:SKIP
> 
> echo '0|1' is not longer necessary ?
> The commit log seems obsolete?
> 
> pw-bot: cr

It looks like the problem only arises when CONFIG_BPF_JIT_ALWAYS_ON is
turned off, and we're only restricting the stack size when
prog->jit_requested is false. To test this, I simulated different
scenarios by echoing '0' or '1' to see how the program would behave when
jit_requested is enabled or disabled.

As expected, when I echoed '0', the program failed verification, and when
I echoed '1', it ran smoothly.

Thanks
> 
> > Signed-off-by: Jiayuan Chen <mrpre@163.com>
> > ---
> >  .../selftests/bpf/progs/verifier_stack_ptr.c  | 50 +++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> > index 417c61cd4b19..8ffe5a01d140 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
> > @@ -481,4 +481,54 @@ l1_%=:     r0 = 42;                                        \
> >         : __clobber_all);
> >  }
> >
> > +SEC("socket")
> > +__description("PTR_TO_STACK stack size > 512")
> > +__failure __msg("invalid write to stack R1 off=-520 size=8")
> > +__naked void stack_check_size_gt_512(void)
> > +{
> > +       asm volatile (
> > +       "r1 = r10;"
> > +       "r1 += -520;"
> > +       "r0 = 42;"
> > +       "*(u64*)(r1 + 0) = r0;"
> > +       "exit;"
> > +       ::: __clobber_all);
> > +}
> > +
> > +#ifdef __BPF_FEATURE_MAY_GOTO
> > +SEC("socket")
> > +__description("PTR_TO_STACK stack size 512 with may_goto with jit")
> > +__use_jit()
> > +__success __retval(42)
> > +__naked void stack_check_size_512_with_may_goto_jit(void)
> > +{
> > +       asm volatile (
> > +       "r1 = r10;"
> > +       "r1 += -512;"
> > +       "r0 = 42;"
> > +       "*(u32*)(r1 + 0) = r0;"
> > +       "may_goto l0_%=;"
> > +       "r2 = 100;"
> > +"l0_%=:        exit;"
> > +       ::: __clobber_all);
> > +}
> > +
> > +SEC("socket")
> > +__description("PTR_TO_STACK stack size 512 with may_goto without jit")
> > +__use_interp()
> > +__failure __msg("stack size 520(extra 8) is too large")
> > +__naked void stack_check_size_512_with_may_goto(void)
> > +{
> > +       asm volatile (
> > +       "r1 = r10;"
> > +       "r1 += -512;"
> > +       "r0 = 42;"
> > +       "*(u32*)(r1 + 0) = r0;"
> > +       "may_goto l0_%=;"
> > +       "r2 = 100;"
> > +"l0_%=:        exit;"
> > +       ::: __clobber_all);
> > +}
> > +#endif
> > +
> >  char _license[] SEC("license") = "GPL";
> > --
> > 2.47.1
> >
Alexei Starovoitov Feb. 14, 2025, 12:58 a.m. UTC | #3 
On Thu, Feb 13, 2025 at 8:42 AM Jiayuan Chen <mrpre@163.com> wrote:
>
> On Thu, Feb 13, 2025 at 08:04:05AM -0800, Alexei Starovoitov wrote:
> > On Thu, Feb 13, 2025 at 5:13 AM Jiayuan Chen <mrpre@163.com> wrote:
> > >
> > > Add test cases to ensure the maximum stack size can be properly limited to
> > > 512.
> > >
> > > Test result:
> > > echo "0" > /proc/sys/net/core/bpf_jit_enable
> > > ./test_progs -t verifier_stack_ptr
> > > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:SKIP
> > > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:OK
> > >
> > > echo "1" > /proc/sys/net/core/bpf_jit_enable
> > > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto with jit:OK
> > > verifier_stack_ptr/PTR_TO_STACK stack size 512 with may_goto without jit:SKIP
> >
> > echo '0|1' is not longer necessary ?
> > The commit log seems obsolete?
> >
> > pw-bot: cr
>
> It looks like the problem only arises when CONFIG_BPF_JIT_ALWAYS_ON is
> turned off, and we're only restricting the stack size when
> prog->jit_requested is false. To test this, I simulated different
> scenarios by echoing '0' or '1' to see how the program would behave when
> jit_requested is enabled or disabled.
>
> As expected, when I echoed '0', the program failed verification, and when
> I echoed '1', it ran smoothly.

I misunderstood the tags in patch 2. I thought:

+#define __use_jit() __attribute__((btf_decl_tag("comment:run_mode=jit")))
+#define __use_interp()
__attribute__((btf_decl_tag("comment:run_mode=interpreter")))

"use jit" actually means use jit.

while what it's doing is different:

+ if ((jit_enabled && spec->run_mode & INTERP) ||
+    (!jit_enabled && spec->run_mode & JIT)) {
+     test__skip();
+     return;
+ }
+

The tags should probably be named __load_if_JITed and __load_if_interpreted
or something like that.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
index 417c61cd4b19..8ffe5a01d140 100644
--- a/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
+++ b/tools/testing/selftests/bpf/progs/verifier_stack_ptr.c
@@ -481,4 +481,54 @@  l1_%=:	r0 = 42;					\
 	: __clobber_all);
 }
 
+SEC("socket")
+__description("PTR_TO_STACK stack size > 512")
+__failure __msg("invalid write to stack R1 off=-520 size=8")
+__naked void stack_check_size_gt_512(void)
+{
+	asm volatile (
+	"r1 = r10;"
+	"r1 += -520;"
+	"r0 = 42;"
+	"*(u64*)(r1 + 0) = r0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+#ifdef __BPF_FEATURE_MAY_GOTO
+SEC("socket")
+__description("PTR_TO_STACK stack size 512 with may_goto with jit")
+__use_jit()
+__success __retval(42)
+__naked void stack_check_size_512_with_may_goto_jit(void)
+{
+	asm volatile (
+	"r1 = r10;"
+	"r1 += -512;"
+	"r0 = 42;"
+	"*(u32*)(r1 + 0) = r0;"
+	"may_goto l0_%=;"
+	"r2 = 100;"
+"l0_%=:	exit;"
+	::: __clobber_all);
+}
+
+SEC("socket")
+__description("PTR_TO_STACK stack size 512 with may_goto without jit")
+__use_interp()
+__failure __msg("stack size 520(extra 8) is too large")
+__naked void stack_check_size_512_with_may_goto(void)
+{
+	asm volatile (
+	"r1 = r10;"
+	"r1 += -512;"
+	"r0 = 42;"
+	"*(u32*)(r1 + 0) = r0;"
+	"may_goto l0_%=;"
+	"r2 = 100;"
+"l0_%=:	exit;"
+	::: __clobber_all);
+}
+#endif
+
 char _license[] SEC("license") = "GPL";