Precision backtracking treats only R0 as a return register at a call/return boundary, so once the verifier starts modeling R2 that way, marking the second half of such a return precise would trip the "unexpected regs" checks in backtrack_insn() and reject a valid program. Marking the upper half precise, for example by branching on it after a call to a static subprogram, walks backtracking into the callee and reaches its BPF_EXIT with R2 still set in the mask. Handle R2 like R0 in boundaries where a call defines the return registers. R2 differs from R0 in that it is an argument register as well, so it is part of the BPF_REGMASK_ARGS check and has to be cleared before that check rather than next to R0. Clear it unconditionally, rather than only where the callee or the kfunc really does return a pair. That gives up the "unexpected regs" assertion for R2, and in exchange keeps backtracking free of any BTF lookup. Nothing is lost: a callee that does not return a pair leaves the caller's R2 uninitialized, so the main verification pass has already rejected any program that reads it, and backtracking is never asked for its precision. At BPF_EXIT the return registers are sampled before the callback path clears R1-R5. That clear does not touch R0, but it does cover R2, and running it first would drop a pair return whenever the instruction following the call happens to be one that invokes a callback. Suggested-by: Eduard Zingerman Acked-by: Eduard Zingerman Signed-off-by: Yonghong Song --- kernel/bpf/backtrack.c | 46 +++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/kernel/bpf/backtrack.c b/kernel/bpf/backtrack.c index a2b18a9f1694..653db80bcc47 100644 --- a/kernel/bpf/backtrack.c +++ b/kernel/bpf/backtrack.c @@ -423,6 +423,10 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, */ verifier_bug_if(idx + 1 != subseq_idx, env, "extra insn from subprog"); + /* global subprog always sets R0 */ + bt_clear_reg(bt, BPF_REG_0); + /* and if it does not set R2, main pass would catch it */ + bt_clear_reg(bt, BPF_REG_2); /* r1-r5 are invalidated after subprog call, * so for global func call it shouldn't be set * anymore @@ -432,8 +436,6 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, bt_reg_mask(bt)); return -EFAULT; } - /* global subprog always sets R0 */ - bt_clear_reg(bt, BPF_REG_0); return 0; } else { /* static subprog call instruction, which @@ -506,6 +508,8 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, return -ENOTSUPP; /* regular helper call sets R0 */ bt_clear_reg(bt, BPF_REG_0); + /* kfunc might also set R2 */ + bt_clear_reg(bt, BPF_REG_2); if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) { /* if backtracking was looking for registers R1-R5 * they should have been found already. @@ -520,7 +524,25 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, return -EFAULT; } } else if (opcode == BPF_EXIT) { - bool r0_precise; + bool from_subprog_call, r0_precise, r2_precise; + + /* BPF_EXIT in subprog or callback always returns + * right after the call instruction, so by checking + * whether the instruction at subseq_idx-1 is subprog + * call or not we can distinguish actual exit from + * *subprog* from exit from *callback*. In the former + * case, we need to propagate the precision of the + * return registers, if necessary. In the latter we + * never do that. + */ + from_subprog_call = subseq_idx - 1 >= 0 && + bpf_pseudo_call(&env->prog->insnsi[subseq_idx - 1]); + + /* Sample the return registers before the callback + * handling below clears R1-R5. + */ + r0_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_0); + r2_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_2); /* Backtracking to a nested function call, 'idx' is a part of * the inner frame 'subseq_idx' is a part of the outer frame. @@ -533,30 +555,22 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, if (subseq_idx >= 0 && bpf_calls_callback(env, subseq_idx)) for (i = BPF_REG_1; i <= BPF_REG_5; i++) bt_clear_reg(bt, i); + + bt_clear_reg(bt, BPF_REG_0); + bt_clear_reg(bt, BPF_REG_2); if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) { verifier_bug(env, "backtracking exit unexpected regs %x", bt_reg_mask(bt)); return -EFAULT; } - /* BPF_EXIT in subprog or callback always returns - * right after the call instruction, so by checking - * whether the instruction at subseq_idx-1 is subprog - * call or not we can distinguish actual exit from - * *subprog* from exit from *callback*. In the former - * case, we need to propagate r0 precision, if - * necessary. In the former we never do that. - */ - r0_precise = subseq_idx - 1 >= 0 && - bpf_pseudo_call(&env->prog->insnsi[subseq_idx - 1]) && - bt_is_reg_set(bt, BPF_REG_0); - - bt_clear_reg(bt, BPF_REG_0); if (bt_subprog_enter(bt)) return -EFAULT; if (r0_precise) bt_set_reg(bt, BPF_REG_0); + if (r2_precise) + bt_set_reg(bt, BPF_REG_2); /* r6-r9 and stack slots will stay set in caller frame * bitmasks until we return back from callee(s) */ -- 2.53.0-Meta