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)

[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")

Thanks, Jean

[1] https://lore.kernel.org/bpf/158507130343.15666.8018068546764556975.stgit@joh...

...

If I try to test myself with multiple test scenarios(I have mentioned in the previous mails) for the bad commits , I see that bad commits are bpf: Verifer, adjust_scalar_min_max_vals to always call update_reg_bounds() selftests/bpf: Fix test_align verifier log patterns selftests/bpf: Fix "dubious pointer arithmetic" test

Thanks, Rajesh Dasari.

On Tue, Aug 23, 2022 at 10:04 AM Greg KH gregkh@linuxfoundation.org wrote:

...

On Mon, Aug 22, 2022 at 10:23:02PM +0300, RAJESH DASARI wrote:

...

Hi,

Please find the test scenarios which I have tried.

Test 1:

Running system Kernel version (tag/commit) : v5.4.210 Kernel source code checkout : v5.4.210 test_align test case execution status : Failure

Test 2:

Running system Kernel version (tag/commit) : v5.4.210 Kernel source code checkout : v5.4.209 test_align test case execution status : Failure

Test 3:

Running system Kernel version (tag/commit) : v5.4.209 Kernel source code checkout : v5.4.209 test_align test case execution status : Success

Test 4:

Running system Kernel version (tag/commit) : ACPI: APEI: Better fix to avoid spamming the console with old error logs ( Kernel compiled at this commit and system is booted with this change) Kernel source code checkout : v5.4.210 but reverted selftests/bpf: Fix test_align verifier log patterns and selftests/bpf: Fix "dubious pointer arithmetic" test. If I revert only the Fix "dubious pointer arithmetic" test, the testcase still fails. test_align test case execution status : Success

Test 5:

Running system Kernel version (tag/commit) : v5.4.210 but reverted commit (bpf: Verifer, adjust_scalar_min_max_vals to always call update_reg_bounds() ) Kernel source code checkout : v5.4.210 but reverted selftests/bpf: Fix test_align verifier log patterns and selftests/bpf: Fix "dubious pointer arithmetic" test. test_align test case execution status : Success

Test 6 :

Running system Kernel version (tag/commit) : bpf: Test_verifier, #70 error message updates for 32-bit right shift( Kernel compiled at this commit and system is booted with this change) Kernel source code checkout : v5.4.209 or v5.4.210 test_align test case execution status : Failure

I'm sorry, but I don't know what to do with this report at all.

Is there some failure somewhere? If you use 'git bisect' do you find the offending commit?

confused,

greg k-h

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