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.
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.
