On Tue, Apr 25, 2023 at 04:49:09PM -0700, Andrii Nakryiko wrote:
> if (insn->code == 0)
> return 0;
> @@ -3424,14 +3449,72 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx,
> if (class == BPF_STX)
> bt_set_reg(bt, sreg);
> } else if (class == BPF_JMP || class == BPF_JMP32) {
> - if (opcode == BPF_CALL) {
> - if (insn->src_reg == BPF_PSEUDO_CALL)
> - return -ENOTSUPP;
> - /* BPF helpers that invoke callback subprogs are
> - * equivalent to BPF_PSEUDO_CALL above
> + if (bpf_pseudo_call(insn)) {
> + int subprog_insn_idx, subprog;
> + bool is_global;
> +
> + subprog_insn_idx = idx + insn->imm + 1;
> + subprog = find_subprog(env, subprog_insn_idx);
> + if (subprog < 0)
> + return -EFAULT;
> + is_global = subprog_is_global(env, subprog);
> +
> + if (is_global) {
could you add a warn_on here that checks that jmp history doesn't have insns from subprog.
> + /* r1-r5 are invalidated after subprog call,
> + * so for global func call it shouldn't be set
> + * anymore
> + */
> + if (bt_reg_mask(bt) & BPF_REGMASK_ARGS)
> + return -EFAULT;
This shouldn't be happening, but backtracking is delicate.
Could you add verbose("backtracking bug") here, so we know why the prog got rejected.
I'd probably do -ENOTSUPP, but EFAULT is ok too.
> + /* global subprog always sets R0 */
> + bt_clear_reg(bt, BPF_REG_0);
> + return 0;
> + } else {
> + /* static subprog call instruction, which
> + * means that we are exiting current subprog,
> + * so only r1-r5 could be still requested as
> + * precise, r0 and r6-r10 or any stack slot in
> + * the current frame should be zero by now
> + */
> + if (bt_reg_mask(bt) & ~BPF_REGMASK_ARGS)
> + return -EFAULT;
same here.
> + /* we don't track register spills perfectly,
> + * so fallback to force-precise instead of failing */
> + if (bt_stack_mask(bt) != 0)
> + return -ENOTSUPP;
> + /* propagate r1-r5 to the caller */
> + for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> + if (bt_is_reg_set(bt, i)) {
> + bt_clear_reg(bt, i);
> + bt_set_frame_reg(bt, bt->frame - 1, i);
> + }
> + }
> + if (bt_subprog_exit(bt))
> + return -EFAULT;
> + return 0;
> + }
> + } else if ((bpf_helper_call(insn) &&
> + is_callback_calling_function(insn->imm) &&
> + !is_async_callback_calling_function(insn->imm)) ||
> + (bpf_pseudo_kfunc_call(insn) && is_callback_calling_kfunc(insn->imm))) {
> + /* callback-calling helper or kfunc call, which means
> + * we are exiting from subprog, but unlike the subprog
> + * call handling above, we shouldn't propagate
> + * precision of r1-r5 (if any requested), as they are
> + * not actually arguments passed directly to callback
> + * subprogs
> */
> - if (insn->src_reg == 0 && is_callback_calling_function(insn->imm))
> + if (bt_reg_mask(bt) & ~BPF_REGMASK_ARGS)
> + return -EFAULT;
> + if (bt_stack_mask(bt) != 0)
> return -ENOTSUPP;
> + /* clear r1-r5 in callback subprog's mask */
> + for (i = BPF_REG_1; i <= BPF_REG_5; i++)
> + bt_clear_reg(bt, i);
> + if (bt_subprog_exit(bt))
> + return -EFAULT;
> + return 0;
jmp history will include callback insn, right?
So skip of jmp history is missing here ?
> + } else if (opcode == BPF_CALL) {
> /* kfunc with imm==0 is invalid and fixup_kfunc_call will
> * catch this error later. Make backtracking conservative
> * with ENOTSUPP.
> @@ -3449,7 +3532,39 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx,
> return -EFAULT;
> }
> } else if (opcode == BPF_EXIT) {
> - return -ENOTSUPP;
> + bool r0_precise;
> +
> + if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) {
> + /* if backtracing was looking for registers R1-R5
> + * they should have been found already.
> + */
> + verbose(env, "BUG regs %x\n", bt_reg_mask(bt));
> + WARN_ONCE(1, "verifier backtracking bug");
> + return -EFAULT;
> + }
> +
> + /* BPF_EXIT in subprog or callback always jump right
I'd say 'subprog always returns right after the call'. 'jump' is a bit confusing here,
since it doesn't normally used to describe function return address.
> + * after the call instruction, so by check 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);
> +
The rest all makes sense.
