Re: bpf selftest failed in 5.4.210 kernel

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Tue, Aug 23, 2022 at 11:25:15PM +0300, Ovidiu Panait wrote:
> Hi Jean-Philippe,
> 
> On 8/23/22 21:34, Jean-Philippe Brucker wrote:
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > 
> > On Tue, Aug 23, 2022 at 10:31:40AM +0300, RAJESH DASARI wrote:
> > > Sorry for the confusion, results are indeed confusing to me .
> > > If I try with git bisect I get
> > > 
> > > git bisect bad
> > > 9d6f67365d9cdb389fbdac2bb5b00e59e345930e is the first bad commit
> > For me bisecting points to:
> > 
> > (A)     7c1134c7da99 ("bpf: Verifer, adjust_scalar_min_max_vals to always call update_reg_bounds()")
> > 
> > This changes the BPF verifier output and (as expected) breaks the
> > test_align selftest. That's why in the same series [1] another patch fixed
> > test_align. In v5.4.y, that patch is:
> > 
> > (B)     6a9b3f0f3bad ("selftests/bpf: Fix test_align verifier log patterns")
> > 
> > Unfortunately commit (B) addresses multiple verifier changes, not solely
> > (A). My guess is those changes were in series [1] and haven't been
> > backported to v5.4. So multiple solutions:
> > 
> > * Partially revert (B), only keeping the changes needed by (A)
> > * Revert (A) and (B)
> > * Add the missing commits that (B) also addresses
> > 
> > I don't know which, I suppose it depends on the intent behind backporting
> > (A). Ovidiu?
> The intent behind backporting 7c1134c7da99 ("bpf: Verifer,
> adjust_scalar_min_max_vals to always call update_reg_bounds()") was to fix
> CVE-2021-4159.
> 
> If we revert test 11 changes brought in by 6a9b3f0f3bad ("selftests/bpf: Fix
> test_align verifier log patterns") backport, all test_align testcases pass
> on my side:
> 
> diff --git a/tools/testing/selftests/bpf/test_align.c
> b/tools/testing/selftests/bpf/test_align.c
> index c9c9bdce9d6d..4726e3eca9b2 100644
> --- a/tools/testing/selftests/bpf/test_align.c
> +++ b/tools/testing/selftests/bpf/test_align.c
> @@ -580,18 +580,18 @@ static struct bpf_align_test tests[] = {
>                         /* Adding 14 makes R6 be (4n+2) */
>                         {11,
> "R6_w=inv(id=0,umin_value=14,umax_value=74,var_off=(0x2; 0x7c))"},
>                         /* Subtracting from packet pointer overflows ubounds
> */
> -                       {13, "R5_w=pkt(id=1,off=0,r=8,umin_value=18446744073709551542,umax_value=18446744073709551602,var_off=(0xffffffffffffff82;
> 0x7c)"},
> +                       {13, "R5_w=pkt(id=1,off=0,r=8,umin_value=18446744073709551542,umax_value=18446744073709551602,var_off=(0xffffffffffffff82;
> 0x7c))"},
>                         /* New unknown value in R7 is (4n), >= 76 */
>                         {15,
> "R7_w=inv(id=0,umin_value=76,umax_value=1096,var_off=(0x0; 0x7fc))"},
>                         /* Adding it to packet pointer gives nice bounds
> again */
> -                       {16,
> "R5_w=pkt(id=2,off=0,r=0,umin_value=2,umax_value=1082,var_off=(0x2;
> 0xfffffffc)"},
> +                       {16,
> "R5_w=pkt(id=2,off=0,r=0,umin_value=2,umax_value=1082,var_off=(0x2;
> 0x7fc))"},
>                         /* At the time the word size load is performed from
> R5,
>                          * its total fixed offset is NET_IP_ALIGN + reg->off
> (0)
>                          * which is 2.  Then the variable offset is (4n+2),
> so
>                          * the total offset is 4-byte aligned and meets the
>                          * load's requirements.
>                          */
> -                       {20,
> "R5=pkt(id=2,off=0,r=4,umin_value=2,umax_value=1082,var_off=(0x2;
> 0xfffffffc)"},
> +                       {20,
> "R5=pkt(id=2,off=0,r=4,umin_value=2,umax_value=1082,var_off=(0x2; 0x7fc))"},
>                 },
>         },
>  };
> 
> root@intel-x86-64:~/bpf# ./test_align
> Test   0: mov ... PASS
> Test   1: shift ... PASS
> Test   2: addsub ... PASS
> Test   3: mul ... PASS
> Test   4: unknown shift ... PASS
> Test   5: unknown mul ... PASS
> Test   6: packet const offset ... PASS
> Test   7: packet variable offset ... PASS
> Test   8: packet variable offset 2 ... PASS
> Test   9: dubious pointer arithmetic ... PASS
> Test  10: variable subtraction ... PASS
> Test  11: pointer variable subtraction ... PASS
> Results: 12 pass 0 fail
> > In any case 6098562ed9df ("selftests/bpf: Fix "dubious pointer arithmetic"
> > test") can be reverted, I can send that once we figure out the rest.
> 
> In my testing, with [1] and [2] applied, but without [3], the following
> test_align selftest would still fail:
> 
> Test   9: dubious pointer arithmetic ... Failed to find match 9:
> R5=inv(id=0,umin_value=2,umax_value=9223372034707292158,var_off=(0x2;
> 0x7fffffff7ffffffc)

Right thanks for the details, so I think the cleanest is to revert [3] and
partially revert [2], tests 11 and part of 9. I'll send that out

Thanks,
Jean

> 
> 
> [1] 7c1134c7da99 ("bpf: Verifer, adjust_scalar_min_max_vals to always call
> update_reg_bounds()")
> [2] 6a9b3f0f3bad ("selftests/bpf: Fix test_align verifier log patterns")
> [3] 6098562ed9df ("selftests/bpf: Fix "dubious pointer arithmetic" test")
> 
> Ovidiu