On Thu, May 4, 2023 at 3:44 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, May 04, 2023 at 03:19:08PM -0700, Andrii Nakryiko wrote:
> > 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?
>
> recalling how jmp_history works and reading this comment when we process any call:
> /* when we exit from subprog, we need to record non-linear history */
> mark_jmp_point(env, t + 1);
>
> so for static subprog the history will be:
> call
> jmps inside subprog
> insn after call.
>
> for global it will be:
> call
> insn after call.
>
> I was thinking that we can do simple check that call + 1 == subseq_idx for globals.
> For statics that should never be the case.
Got it. Yes, I think you are right, and it's simple to check and
enforce. Will add.
>
> We don't have to do it. Mostly checking my understanding of patches and jmp history.
>
> >
> > >
> > > > + /* 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.
>
> Got it. Makes sense now.
