Re: [PATCH 5.15 33/33] selftests/bpf: Add test for reg2btf_ids out of bounds access

[Greg Kroah-Hartman]

On Fri, Jun 24, 2022 at 06:33:57PM +0800, Po-Hsu Lin wrote:
> On Fri, Apr 29, 2022 at 6:47 PM Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > From: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> >
> > commit 13c6a37d409db9abc9c0bfc6d0a2f07bf0fff60e upstream.
> >
> > This test tries to pass a PTR_TO_BTF_ID_OR_NULL to the release function,
> > which would trigger a out of bounds access without the fix in commit
> > 45ce4b4f9009 ("bpf: Fix crash due to out of bounds access into reg2btf_ids.")
> > but after the fix, it should only index using base_type(reg->type),
> > which should be less than __BPF_REG_TYPE_MAX, and also not permit any
> > type flags to be set for the reg->type.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > Link: https://lore.kernel.org/bpf/20220220023138.2224652-1-memxor@xxxxxxxxx
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  tools/testing/selftests/bpf/verifier/calls.c |   19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > --- a/tools/testing/selftests/bpf/verifier/calls.c
> > +++ b/tools/testing/selftests/bpf/verifier/calls.c
> > @@ -108,6 +108,25 @@
> >         .errstr = "R0 min value is outside of the allowed memory range",
> >  },
> >  {
> > +       "calls: trigger reg2btf_ids[reg->type] for reg->type > __BPF_REG_TYPE_MAX",
> > +       .insns = {
> > +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_10),
> > +       BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, -8),
> > +       BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 0),
> > +       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
> > +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> > +       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, BPF_PSEUDO_KFUNC_CALL, 0, 0),
> > +       BPF_EXIT_INSN(),
> > +       },
> > +       .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> > +       .result = REJECT,
> > +       .errstr = "arg#0 pointer type STRUCT prog_test_ref_kfunc must point",
> > +       .fixup_kfunc_btf_id = {
> > +               { "bpf_kfunc_call_test_acquire", 3 },
> > +               { "bpf_kfunc_call_test_release", 5 },
> > +       },
> > +},
> > +{
> >         "calls: overlapping caller/callee",
> >         .insns = {
> >         BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 0),
> >
> >
> 
> Hello Greg,
> 
> When I tried to build the bpf selftest from 5.15.49 source tree on a
> Ubuntu Jammy instance running with 5.15.49-051549-generic, I got the
> following error message:
> 
> In file included from
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/tests.h:21,
>                  from test_verifier.c:432:
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:124:10:
> error: ‘struct bpf_test’ has no member named ‘fixup_kfunc_btf_id’
>   124 |         .fixup_kfunc_btf_id = {
>       |          ^~~~~~~~~~~~~~~~~~
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:124:9:
> warning: braces around scalar initializer
>   124 |         .fixup_kfunc_btf_id = {
>       |         ^
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:124:9:
> note: (near initialization for ‘tests[150].errstr_unpriv’)
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:125:17:
> warning: braces around scalar initializer
>   125 |                 { "bpf_kfunc_call_test_acquire", 3 },
>       |                 ^
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:125:17:
> note: (near initialization for ‘tests[150].errstr_unpriv’)
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:125:50:
> warning: excess elements in scalar initializer
>   125 |                 { "bpf_kfunc_call_test_acquire", 3 },
>       |                                                  ^
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:125:50:
> note: (near initialization for ‘tests[150].errstr_unpriv’)
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:126:17:
> warning: braces around scalar initializer
>   126 |                 { "bpf_kfunc_call_test_release", 5 },
>       |                 ^
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:126:17:
> note: (near initialization for ‘tests[150].errstr_unpriv’)
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:126:50:
> warning: excess elements in scalar initializer
>   126 |                 { "bpf_kfunc_call_test_release", 5 },
>       |                                                  ^
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:126:50:
> note: (near initialization for ‘tests[150].errstr_unpriv’)
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:126:17:
> warning: excess elements in scalar initializer
>   126 |                 { "bpf_kfunc_call_test_release", 5 },
>       |                 ^
> /home/ubuntu/linux/tools/testing/selftests/bpf/verifier/calls.c:126:17:
> note: (near initialization for ‘tests[150].errstr_unpriv’)
> make: *** [Makefile:508:
> /home/ubuntu/linux/tools/testing/selftests/bpf/test_verifier] Error 1
> 
> Which is introduced by this commit f59e6886c "selftests/bpf: Add test
> for reg2btf_ids out of bounds access" on 5.15. With this commit
> reverted, there will be another error in progs/timer_crash.c like in
> 5.10 [1]:
> 
> progs/timer_crash.c:8:19: error: field has incomplete type 'struct bpf_timer'
>         struct bpf_timer timer;
>                          ^
> /home/ubuntu/linux/tools/testing/selftests/bpf/tools/include/bpf/bpf_helper_defs.h:39:8:
> note: forward declaration of 'struct bpf_timer'
> struct bpf_timer;
>        ^
> 1 error generated.
> 
> Maybe commit "selftests/bpf: Add test for bpf_timer overwriting crash"
> should be reverted on 5.15 as well.

Should the test be fixed instead?

I'll take patches for either, thanks.

greg k-h