| Message ID | 20230406164505.1046801-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 953d9f5beaf75e88c69a13d70ce424cd606a29f5 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Improve verifier for cond_op and spilled loop index variables | expand |
On 4/6/23 12:45 PM, Yonghong Song wrote: > Currently, the verifier does not handle '<const> <cond_op> <non_const>' well. > For example, > ... > 10: (79) r1 = *(u64 *)(r10 -16) ; R1_w=scalar() R10=fp0 > 11: (b7) r2 = 0 ; R2_w=0 > 12: (2d) if r2 > r1 goto pc+2 > 13: (b7) r0 = 0 > 14: (95) exit > 15: (65) if r1 s> 0x1 goto pc+3 > 16: (0f) r0 += r1 > ... > At insn 12, verifier decides both true and false branch are possible, but > actually only false branch is possible. > > Currently, the verifier already supports patterns '<non_const> <cond_op> <const>. > Add support for patterns '<const> <cond_op> <non_const>' in a similar way. > > Also fix selftest 'verifier_bounds_mix_sign_unsign/bounds checks mixing signed and unsigned, variant 10' > due to this change. > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- I still think there's a refactoring opportunity here, but I see your comments on the related thread in v1 of this series, and don't think it's a blocker to find cleanest refactor. Acked-by: Dave Marchevsky <davemarchevsky@fb.com>
On Thu, Apr 6, 2023 at 11:10 AM Dave Marchevsky <davemarchevsky@meta.com> wrote: > > On 4/6/23 12:45 PM, Yonghong Song wrote: > > Currently, the verifier does not handle '<const> <cond_op> <non_const>' well. > > For example, > > ... > > 10: (79) r1 = *(u64 *)(r10 -16) ; R1_w=scalar() R10=fp0 > > 11: (b7) r2 = 0 ; R2_w=0 > > 12: (2d) if r2 > r1 goto pc+2 > > 13: (b7) r0 = 0 > > 14: (95) exit > > 15: (65) if r1 s> 0x1 goto pc+3 > > 16: (0f) r0 += r1 > > ... > > At insn 12, verifier decides both true and false branch are possible, but > > actually only false branch is possible. > > > > Currently, the verifier already supports patterns '<non_const> <cond_op> <const>. > > Add support for patterns '<const> <cond_op> <non_const>' in a similar way. > > > > Also fix selftest 'verifier_bounds_mix_sign_unsign/bounds checks mixing signed and unsigned, variant 10' > > due to this change. > > > > Signed-off-by: Yonghong Song <yhs@fb.com> > > --- > > I still think there's a refactoring opportunity here, but I see your comments > on the related thread in v1 of this series, and don't think it's a blocker > to find cleanest refactor. Agreed, but current implementation is not wrong, so: Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Acked-by: Dave Marchevsky <davemarchevsky@fb.com>
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 5c6b90e384a5..3660b573048a 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -13356,6 +13356,18 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env, src_reg->var_off.value, opcode, is_jmp32); + } else if (dst_reg->type == SCALAR_VALUE && + is_jmp32 && tnum_is_const(tnum_subreg(dst_reg->var_off))) { + pred = is_branch_taken(src_reg, + tnum_subreg(dst_reg->var_off).value, + flip_opcode(opcode), + is_jmp32); + } else if (dst_reg->type == SCALAR_VALUE && + !is_jmp32 && tnum_is_const(dst_reg->var_off)) { + pred = is_branch_taken(src_reg, + dst_reg->var_off.value, + flip_opcode(opcode), + is_jmp32); } else if (reg_is_pkt_pointer_any(dst_reg) && reg_is_pkt_pointer_any(src_reg) && !is_jmp32) { diff --git a/tools/testing/selftests/bpf/progs/verifier_bounds_mix_sign_unsign.c b/tools/testing/selftests/bpf/progs/verifier_bounds_mix_sign_unsign.c index 91a66357896a..4f40144748a5 100644 --- a/tools/testing/selftests/bpf/progs/verifier_bounds_mix_sign_unsign.c +++ b/tools/testing/selftests/bpf/progs/verifier_bounds_mix_sign_unsign.c @@ -354,7 +354,7 @@ __naked void signed_and_unsigned_variant_10(void) call %[bpf_map_lookup_elem]; \ if r0 == 0 goto l0_%=; \ r1 = *(u64*)(r10 - 16); \ - r2 = 0; \ + r2 = -1; \ if r2 > r1 goto l1_%=; \ r0 = 0; \ exit; \
Currently, the verifier does not handle '<const> <cond_op> <non_const>' well. For example, ... 10: (79) r1 = *(u64 *)(r10 -16) ; R1_w=scalar() R10=fp0 11: (b7) r2 = 0 ; R2_w=0 12: (2d) if r2 > r1 goto pc+2 13: (b7) r0 = 0 14: (95) exit 15: (65) if r1 s> 0x1 goto pc+3 16: (0f) r0 += r1 ... At insn 12, verifier decides both true and false branch are possible, but actually only false branch is possible. Currently, the verifier already supports patterns '<non_const> <cond_op> <const>. Add support for patterns '<const> <cond_op> <non_const>' in a similar way. Also fix selftest 'verifier_bounds_mix_sign_unsign/bounds checks mixing signed and unsigned, variant 10' due to this change. Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/verifier.c | 12 ++++++++++++ .../bpf/progs/verifier_bounds_mix_sign_unsign.c | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-)