netdev - Re: [bpf-next PATCH 07/10] bpf: test_verifier, bpf_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200326063336.do6mibb7b5xwofz2@ast-mbp>
Date:   Wed, 25 Mar 2020 23:33:36 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     ecree@...arflare.com, yhs@...com, daniel@...earbox.net,
        netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [bpf-next PATCH 07/10] bpf: test_verifier, bpf_get_stack return
 value add <0

On Tue, Mar 24, 2020 at 10:39:55AM -0700, John Fastabend wrote:
> diff --git a/tools/testing/selftests/bpf/verifier/bpf_get_stack.c b/tools/testing/selftests/bpf/verifier/bpf_get_stack.c
> index f24d50f..24aa6a0 100644
> --- a/tools/testing/selftests/bpf/verifier/bpf_get_stack.c
> +++ b/tools/testing/selftests/bpf/verifier/bpf_get_stack.c
> @@ -7,7 +7,7 @@
>  	BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
>  	BPF_LD_MAP_FD(BPF_REG_1, 0),
>  	BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
> -	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 28),
> +	BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 29),
>  	BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
>  	BPF_MOV64_IMM(BPF_REG_9, sizeof(struct test_val)),
>  	BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
> @@ -16,6 +16,7 @@
>  	BPF_MOV64_IMM(BPF_REG_4, 256),
>  	BPF_EMIT_CALL(BPF_FUNC_get_stack),
>  	BPF_MOV64_IMM(BPF_REG_1, 0),
> +	BPF_JMP32_REG(BPF_JSLT, BPF_REG_0, BPF_REG_1, 20),
>  	BPF_MOV64_REG(BPF_REG_8, BPF_REG_0),
>  	BPF_ALU64_IMM(BPF_LSH, BPF_REG_8, 32),
>  	BPF_ALU64_IMM(BPF_ARSH, BPF_REG_8, 32),

Yep. The test is wrong.
But this is cheating ;)
JSLT should be after shifts.

The test should have been written as:
diff --git a/tools/testing/selftests/bpf/verifier/bpf_get_stack.c b/tools/testing/selftests/bpf/verifier/bpf_get_stack.c
index f24d50f09dbe..be0758c1bfbd 100644
--- a/tools/testing/selftests/bpf/verifier/bpf_get_stack.c
+++ b/tools/testing/selftests/bpf/verifier/bpf_get_stack.c
@@ -19,7 +19,7 @@
        BPF_MOV64_REG(BPF_REG_8, BPF_REG_0),
        BPF_ALU64_IMM(BPF_LSH, BPF_REG_8, 32),
        BPF_ALU64_IMM(BPF_ARSH, BPF_REG_8, 32),
-       BPF_JMP_REG(BPF_JSLT, BPF_REG_1, BPF_REG_8, 16),
+       BPF_JMP_REG(BPF_JSGT, BPF_REG_1, BPF_REG_8, 16),
        BPF_ALU64_REG(BPF_SUB, BPF_REG_9, BPF_REG_8),
        BPF_MOV64_REG(BPF_REG_2, BPF_REG_7),
        BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_8),

That was the intent of the test.
But in such form it won't work with the current patch set,
but it should.

More so the patches 1 and 5 make test_progs pass,
but test_progs-no_alu32 fails tests 20 and 61.
Because clang generates the following:
14: (85) call bpf_get_stack#67
15: (b7) r1 = 0
16: (bf) r8 = r0
17: (67) r8 <<= 32
18: (c7) r8 s>>= 32
19: (6d) if r1 s> r8 goto pc+16

(which is exactly what bpf_get_stack.c test tried to capture)

I guess no_alu32 may be passing for you because you have that special
clang optimization that replaces <<32 s>>32 with more efficient insn,
but not everyone will be using new clang, so we have to teach verifier
recognize <<32 s>>32.
Thankfully with your new infra for s32 it should be easy to do.
In scalar_min_max_lsh() we don't need to do dst_reg->smax_value = S64_MAX;
When we have _positive_ s32_max_value
we can set smax_value = s32_max_value << 32
and I think that will be correct.
scalar_min_max_arsh() shouldn't need any changes.
And the verifier will restore valid smax_value after <<32 s>>32 sequence.
And this test will pass (after fixing s/JSLT/JSGT/) and test_progs-no_alu32
will do too. wdyt?