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 New
Headers show
Series bpf: Fix array bounds error with may_goto and add selftest | expand

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";