On Mon, Jun 13, 2022 at 4:34 PM Yonghong Song <yhs@xxxxxx> wrote:
>
> With latest llvm15, test_varlen failed with the following verifier log:
>
> 17: (85) call bpf_probe_read_kernel_str#115 ; R0_w=scalar(smin=-4095,smax=256)
> 18: (bf) r1 = r0 ; R0_w=scalar(id=1,smin=-4095,smax=256) R1_w=scalar(id=1,smin=-4095,smax=256)
> 19: (67) r1 <<= 32 ; R1_w=scalar(smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32_max=)
> 20: (bf) r2 = r1 ; R1_w=scalar(id=2,smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32)
> 21: (c7) r2 s>>= 32 ; R2=scalar(smin=-2147483648,smax=256)
> ; if (len >= 0) {
> 22: (c5) if r2 s< 0x0 goto pc+7 ; R2=scalar(umax=256,var_off=(0x0; 0x1ff))
> ; payload4_len1 = len;
> 23: (18) r2 = 0xffffc90000167418 ; R2_w=map_value(off=1048,ks=4,vs=1572,imm=0)
> 25: (63) *(u32 *)(r2 +0) = r0 ; R0=scalar(id=1,smin=-4095,smax=256) R2_w=map_value(off=1048,ks=4,vs=1572,imm=0)
> 26: (77) r1 >>= 32 ; R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
> ; payload += len;
> 27: (18) r6 = 0xffffc90000167424 ; R6_w=map_value(off=1060,ks=4,vs=1572,imm=0)
> 29: (0f) r6 += r1 ; R1_w=Pscalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0)
> ; len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in2[0]);
> 30: (bf) r1 = r6 ; R1_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,um)
> 31: (b7) r2 = 256 ; R2_w=256
> 32: (18) r3 = 0xffffc90000164100 ; R3_w=map_value(off=256,ks=4,vs=1056,imm=0)
> 34: (85) call bpf_probe_read_kernel_str#115
> R1 unbounded memory access, make sure to bounds check any such access
> processed 27 insns (limit 1000000) max_states_per_insn 0 total_states 2 peak_states 2 mark_read 1
> -- END PROG LOAD LOG --
> libbpf: failed to load program 'handler32_signed'
>
> The failure is due to
> 20: (bf) r2 = r1 ; R1_w=scalar(id=2,smax=1099511627776,umax=18446744069414584320,var_off=(0x0; 0xffffffff00000000),s32_min=0,s32_max=0,u32)
> 21: (c7) r2 s>>= 32 ; R2=scalar(smin=-2147483648,smax=256)
> 22: (c5) if r2 s< 0x0 goto pc+7 ; R2=scalar(umax=256,var_off=(0x0; 0x1ff))
> 26: (77) r1 >>= 32 ; R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff))
> 29: (0f) r6 += r1 ; R1_w=Pscalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R6_w=map_value(off=1060,ks=4,vs=1572,umax=4294967295,var_off=(0)
> where r1 has conservative value range compared to r2 and r1 is used later.
>
> In llvm, commit [1] triggered the above code generation and caused
> verification failure.
>
> It may take a while for llvm to address this issue. In the main time,
> let us change the variable 'len' type to 'long' and adjust condition properly.
> Tested with llvm14 and latest llvm15, both worked fine.
>
> [1] https://reviews.llvm.org/D126647
>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
> tools/testing/selftests/bpf/progs/test_varlen.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/test_varlen.c b/tools/testing/selftests/bpf/progs/test_varlen.c
> index 913acdffd90f..3987ff174f1f 100644
> --- a/tools/testing/selftests/bpf/progs/test_varlen.c
> +++ b/tools/testing/selftests/bpf/progs/test_varlen.c
> @@ -41,20 +41,20 @@ int handler64_unsigned(void *regs)
> {
> int pid = bpf_get_current_pid_tgid() >> 32;
> void *payload = payload1;
> - u64 len;
> + long len;
>
> /* ignore irrelevant invocations */
> if (test_pid != pid || !capture)
> return 0;
>
> len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in1[0]);
> - if (len <= MAX_LEN) {
> + if (len >= 0) {
> payload += len;
> payload1_len1 = len;
> }
>
> len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in2[0]);
> - if (len <= MAX_LEN) {
> + if (len >= 0) {
> payload += len;
> payload1_len2 = len;
> }
> @@ -123,7 +123,7 @@ int handler32_signed(void *regs)
> {
> int pid = bpf_get_current_pid_tgid() >> 32;
> void *payload = payload4;
> - int len;
> + long len;
The whole point of handler32_signed and handler64_unsigned was to test
that this code patterns works for signed 32-bit and unsigned 64-bit
variables. By the time clang is fixed we might forget to undo these
changes in test_varlen.c. Maybe it's better to leave code as is? We
already blacklisted this test for CI anyways, so it doesn't affect
kernel-patches CI runs.
>
> /* ignore irrelevant invocations */
> if (test_pid != pid || !capture)
> --
> 2.30.2
>
