Yonghong Song <yhs@xxxxxxxx> 于2023年1月2日周一 03:20写道: > > > > On 12/30/22 1:44 AM, Hao Sun wrote: >> >> >> Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> 于2022年12月30日周五 06:16写道: >>> >>> On Tue, Dec 27, 2022 at 9:24 PM Yonghong Song <yhs@xxxxxxxx> wrote: >>>> >>>> >>>> >>>> On 12/20/22 4:30 PM, Andrii Nakryiko wrote: >>>>> On Mon, Dec 19, 2022 at 11:13 AM <sdf@xxxxxxxxxx> wrote: >>>>>> >>>>>> On 12/19, Hao Sun wrote: >>>>>>> Hi, >>>>>> >>>>>>> The following backtracking bug can be triggered on the latest bpf-next and >>>>>>> Linux 6.1 with the C prog provided. I don't have enough knowledge about >>>>>>> this part in the verifier, don't know how to fix this. >>>>>> >>>>>> Maybe something related to commit be2ef8161572 ("bpf: allow precision >>>>>> tracking >>>>>> for programs with subprogs") and/or the related ones? >>>>>> >>>>>> >>>>>>> This can be reproduced on: >>>>>> >>>>>>> HEAD commit: 0e43662e61f2 tools/resolve_btfids: Use pkg-config to locate >>>>>>> libelf >>>>>>> git tree: bpf-next >>>>>>> console log: https://pastebin.com/raw/45hZ7iqm >>>>>>> kernel config: https://pastebin.com/raw/0pu1CHRm >>>>>>> C reproducer: https://pastebin.com/raw/tqsiezvT >>>>>> >>>>>>> func#0 @0 >>>>>>> 0: R1=ctx(off=0,imm=0) R10=fp0 >>>>>>> 0: (18) r2 = 0x8000000000000 ; R2_w=2251799813685248 >>>>>>> 2: (18) r6 = 0xffff888027358000 ; >>>>>>> R6_w=map_ptr(off=0,ks=3032,vs=3664,imm=0) >>>>>>> 4: (18) r7 = 0xffff88802735a000 ; >>>>>>> R7_w=map_ptr(off=0,ks=156,vs=2624,imm=0) >>>>>>> 6: (18) r8 = 0xffff88802735e000 ; >>>>>>> R8_w=map_ptr(off=0,ks=2396,vs=76,imm=0) >>>>>>> 8: (18) r9 = 0x8e9700000000 ; R9_w=156779191205888 >>>>>>> 10: (36) if w9 >= 0xffffffe3 goto pc+1 >>>>>>> last_idx 10 first_idx 0 >>>>>>> regs=200 stack=0 before 8: (18) r9 = 0x8e9700000000 >>>>>>> 11: R9_w=156779191205888 >>>>>>> 11: (85) call #0 >>>>>>> 12: (cc) w2 s>>= w7 >>>>> >>>>> w2 should have been set to NOT_INIT (because r1-r5 are clobbered by >>>>> calls) and rejected here as !read_ok (see check_reg_arg()) before >>>>> attempting to mark precision for r2. Can you please try to debug and >>>>> understand why that didn't happen here? >>>> >>>> The verifier is doing the right thing here and the 'call #0' does >>>> implicitly cleared r1-r5. >>>> >>>> So for 'w2 s>>= w7', since w2 is used, the verifier tries to find >>>> its definition by backtracing. It encountered 'call #0', which clears >>> >>> and that's what I'm saying is incorrect. Normally we'd get !read_ok >>> error because s>>= is both READ and WRITE on w2, which is >>> uninitialized after call instruction according to BPF ABI. And that's >>> what actually seems to happen correctly in my (simpler) tests locally. >>> But something is special about this specific repro that somehow either >>> bypasses this logic, or attempts to mark precision before we get to >>> that test. That's what we should investigate. I haven't tried to run >>> this specific repro locally yet, so can't tell for sure. >>> >> >> So, the reason why w2 is not marked as uninit is that the kfunc call in >> the BPF program is invalid, "call #0", imm is zero, right? > > Yes, "call #0" is invalid. As the code below > >> /* skip for now, but return error when we find this in > fixup_kfunc_call */ >> if (!insn->imm) >> return 0; > > The error report will be delayed later in fixup_kfunc_call(). > > static int fixup_kfunc_call(struct bpf_verifier_env *env, struct > bpf_insn *insn, > struct bpf_insn *insn_buf, int insn_idx, > int *cnt) > { > const struct bpf_kfunc_desc *desc; > > if (!insn->imm) { > verbose(env, "invalid kernel function call not > eliminated in verifier pass\n"); > return -EINVAL; > } > > >> In check_kfunc_call(), it skips this error temporarily: >> >> /* skip for now, but return error when we find this in fixup_kfunc_call */ >> if (!insn->imm) >> return 0; >> >> So the kfunc call is the previous instruction before "w2 s>>= w7", this >> leads to the warning in backtrack_insn(): >> >> /* regular helper call sets R0 */ >> *reg_mask &= ~1; >> if (*reg_mask & 0x3f) { >> /* if backtracing was looking for registers R1-R5 >> * they should have been found already. >> */ >> verbose(env, "BUG regs %x\n", *reg_mask); >> WARN_ONCE(1, "verifier backtracking bug”); >> return -EFAULT; >> } > > The main triggering the backtrack_insn() is due to > > } else { > /* scalar += pointer > * This is legal, but we have to > reverse our > * src/dest handling in computing the range > */ > err = mark_chain_precision(env, > insn->dst_reg); > if (err) > return err; > return adjust_ptr_min_max_vals(env, insn, > src_reg, > dst_reg); > } > > > unc#0 @0 > 0: R1=ctx(off=0,imm=0) R10=fp0 > 0: (18) r2 = 0x8000000000000 ; R2_w=2251799813685248 > 2: (18) r6 = 0xffff888100d29000 ; > R6_w=map_ptr(off=0,ks=3032,vs=3664,imm=0) > 4: (18) r7 = 0xffff888100d2a000 ; > R7_w=map_ptr(off=0,ks=156,vs=2624,imm=0) > 6: (18) r8 = 0xffff888100d2ac00 ; > R8_w=map_ptr(off=0,ks=2396,vs=76,imm=0) > 8: (18) r9 = 0x8e9700000000 ; R9_w=156779191205888 > 10: (36) if w9 >= 0xffffffe3 goto pc+1 > last_idx 10 first_idx 0 > regs=200 stack=0 before 8: (18) r9 = 0x8e9700000000 > 11: R9_w=156779191205888 > 11: (85) call #0 > 12: (cc) w2 s>>= w7 > last_idx 12 first_idx 12 > parent didn't have regs=4 stack=0 marks: R1=ctx(off=0,imm=0) > R2_rw=P2251799813685248 R6_w=map_ptr(off=0,ks=3032,vs=3664,imm=0) > R7_rw=map_ptr(off=0,ks=156,vs=2624,imm=0) R8_w=map_ptr(off=0,ks=2396,v0 > last_idx 11 first_idx 0 > regs=4 stack=0 before 11: (85) call #0 > BUG regs 4 > > For insn 12, 'w2 s>>= w7', w2 is a scalar and w7 is a map_ptr. Hence, > based on the above verifier code, mark_chain_precision() is triggered. > > Not sure what is the purpose of this test. But to make it succeed, > first "call #0" need to change to a valid kfunc call, and second, you > might want to change 'w2 s>>= w7' to e.g., 'w9 s>>= w7' to avoid > precision tracking. > The purpose is not to make the test "succeed", the verifier temporarily skips the invalid kfunc insn "call #0", but this insn triggered a warning in backtrack_insn(), while it is supposed to reject the program either due to insn#12 32bit ptr alu or insn#11 invalid kfunc. Maybe something like the bellow, after applying the patch, the reproducer is rejected: func#0 @0 0: R1=ctx(off=0,imm=0) R10=fp0 0: (18) r2 = 0x8000000000000 ; R2_w=2251799813685248 2: (18) r6 = 0xffff88817d563000 ; R6_w=map_ptr(off=0,ks=3032,vs=3664,imm=0) 4: (18) r7 = 0xffff888171ee9000 ; R7_w=map_ptr(off=0,ks=156,vs=2624,imm=0) 6: (18) r8 = 0xffff888171ee8000 ; R8_w=map_ptr(off=0,ks=2396,vs=76,imm=0) 8: (18) r9 = 0x8e9700000000 ; R9_w=156779191205888 10: (36) if w9 >= 0xffffffe3 goto pc+1 last_idx 10 first_idx 0 regs=200 stack=0 before 8: (18) r9 = 0x8e9700000000 11: R9_w=156779191205888 11: (85) call #0 12: (cc) w2 s>>= w7 last_idx 12 first_idx 12 parent didn't have regs=4 stack=0 marks: R1=ctx(off=0,imm=0) R2_rw=P2251799813685248 R6_w=map_ptr(off=0,ks=3032,vs=3664,imm=0) R7_rw=map_ptr(off=0,ks=156,vs=2624,imm=0) R8_w=map_ptr(off=0,ks=2396,vs=76,imm=0) R9_w=156779191205888 R10=fp0 last_idx 11 first_idx 0 regs=4 stack=0 before 11: (85) call #0 regs=4 stack=0 before 10: (36) if w9 >= 0xffffffe3 goto pc+1 regs=4 stack=0 before 8: (18) r9 = 0x8e9700000000 regs=4 stack=0 before 6: (18) r8 = 0xffff888171ee8000 regs=4 stack=0 before 4: (18) r7 = 0xffff888171ee9000 regs=4 stack=0 before 2: (18) r6 = 0xffff88817d563000 regs=4 stack=0 before 0: (18) r2 = 0x8000000000000 R2 32-bit pointer arithmetic prohibited processed 8 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1 diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 4a25375ebb0d..abc7e96d826f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2743,6 +2743,9 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, *reg_mask |= sreg; } else if (class == BPF_JMP || class == BPF_JMP32) { if (opcode == BPF_CALL) { + /* skip for now, should return error when we find this in fixup_kfunc_call */ + if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL && insn->imm == 0) + return 0; if (insn->src_reg == BPF_PSEUDO_CALL) return -ENOTSUPP; /* BPF helpers that invoke callback subprogs are >> >> Any idea or hint on how to fix this? >>