On Thu, May 4, 2023 at 12:41 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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.
wouldn't this be very expensive to go over the entire jmp history to
check that no jump point there overlaps with the global function? Or
what do you have in mind specifically for this check?
>
> > + /* 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.
Will add verbose(). EFAULT because valid code should never use r1-r5
after call. Invalid code should be rejected before that, and if not,
then it is really a bug and best to bail out.
>
> > + /* 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.
ack
>
> > + /* 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 ?
This is, say, `call bpf_loop;` instruction. Which means we just got
out of bpf_loop's callback's jump history (so already "skipped" them,
except we didn't skip, we properly processed them). So there is
nothing to skip anymore. We are in the parent program already.
This is exactly the situation described in the third example in the
commit description. We are at instruction #11 here.
Note how we don't bail out on r1-r5 being set, because it's expected
that callback might require some of its input arguments to be precise
and that's not an error condition. This is similar to handling global
function, where r1-r5 might be still required to be precise when we
already processed the entire jump history.
It's a bit mind bending to reason about all this, because everything
is backwards.
>
> > + } 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.
this was described in the context of "jump history", but I'll adjust
the wording to "return".
>
> > + * 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.
Cool. Thanks for taking a thorough look. I'm a bit confused on what
exactly you'd like me to check on global subprog call, so please
elaborate. I'll address all the other feedback meanwhile.
