Re: [PATCH v2 bpf-next 09/20] bpf: Recognize cast_kern/user instructions in the verifier.

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Fri, Feb 9, 2024 at 5:13 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2024-02-08 at 20:05 -0800, Alexei Starovoitov wrote:
> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 3c77a3ab1192..5eeb9bf7e324 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
>
> [...]
>
> > @@ -13837,6 +13844,21 @@ static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
> >
> >       dst_reg = &regs[insn->dst_reg];
> >       src_reg = NULL;
> > +
> > +     if (dst_reg->type == PTR_TO_ARENA) {
> > +             struct bpf_insn_aux_data *aux = cur_aux(env);
> > +
> > +             if (BPF_CLASS(insn->code) == BPF_ALU64)
> > +                     /*
> > +                      * 32-bit operations zero upper bits automatically.
> > +                      * 64-bit operations need to be converted to 32.
> > +                      */
> > +                     aux->needs_zext = true;
>
> It should be possible to write an example, when the same insn is
> visited with both PTR_TO_ARENA and some other PTR type.
> Such examples should be rejected as is currently done in do_check()
> for BPF_{ST,STX} using save_aux_ptr_type().

Good catch. Fixed reg_type_mismatch_ok().
Didn't craft a unit test. That will be in a follow up.

> [...]
>
> > @@ -13954,16 +13976,17 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> >       } else if (opcode == BPF_MOV) {
> >
> >               if (BPF_SRC(insn->code) == BPF_X) {
> > -                     if (insn->imm != 0) {
> > -                             verbose(env, "BPF_MOV uses reserved fields\n");
> > -                             return -EINVAL;
> > -                     }
> > -
> >                       if (BPF_CLASS(insn->code) == BPF_ALU) {
> > -                             if (insn->off != 0 && insn->off != 8 && insn->off != 16) {
> > +                             if ((insn->off != 0 && insn->off != 8 && insn->off != 16) ||
> > +                                 insn->imm) {
> >                                       verbose(env, "BPF_MOV uses reserved fields\n");
> >                                       return -EINVAL;
> >                               }
> > +                     } else if (insn->off == BPF_ARENA_CAST_KERN || insn->off == BPF_ARENA_CAST_USER) {
> > +                             if (!insn->imm) {
> > +                                     verbose(env, "cast_kern/user insn must have non zero imm32\n");
> > +                                     return -EINVAL;
> > +                             }
> >                       } else {
> >                               if (insn->off != 0 && insn->off != 8 && insn->off != 16 &&
> >                                   insn->off != 32) {
>
> I think it is now necessary to check insn->imm here,
> as is it allows ALU64 move with non-zero imm.

great catch too. Fixed.

> > @@ -13993,7 +14016,12 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> >                       struct bpf_reg_state *dst_reg = regs + insn->dst_reg;
> >
> >                       if (BPF_CLASS(insn->code) == BPF_ALU64) {
> > -                             if (insn->off == 0) {
> > +                             if (insn->imm) {
> > +                                     /* off == BPF_ARENA_CAST_KERN || off == BPF_ARENA_CAST_USER */
> > +                                     mark_reg_unknown(env, regs, insn->dst_reg);
> > +                                     if (insn->off == BPF_ARENA_CAST_KERN)
> > +                                             dst_reg->type = PTR_TO_ARENA;
>
> This effectively allows casting anything to PTR_TO_ARENA.
> Do we want to check that src_reg somehow originates from arena?
> Might be tricky, a new type modifier bit or something like that.

Yes. Casting anything is fine.
I don't think we need to enforce anything.
Those insns will be llvm generated. If src_reg is somehow ptr_to_ctx
or something it's likely llvm bug or crazy manual type cast
by the user, but if they do so let them experience debug pains.
The kernel won't crash.

> > +                             } else if (insn->off == 0) {
> >                                       /* case: R1 = R2
> >                                        * copy register state to dest reg
> >                                        */
> > @@ -14059,6 +14087,9 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> >                                               dst_reg->subreg_def = env->insn_idx + 1;
> >                                               coerce_subreg_to_size_sx(dst_reg, insn->off >> 3);
> >                                       }
> > +                             } else if (src_reg->type == PTR_TO_ARENA) {
> > +                                     mark_reg_unknown(env, regs, insn->dst_reg);
> > +                                     dst_reg->type = PTR_TO_ARENA;
>
> This describes a case wX = wY, where rY is PTR_TO_ARENA,
> should rX be marked as SCALAR instead of PTR_TO_ARENA?

That was a leftover from earlier experiments when alu64->alu32 was
done early.
Removed this hunk now.

> [...]
>
> > @@ -18235,6 +18272,31 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
> >                               fdput(f);
> >                               return -EBUSY;
> >                       }
> > +                     if (map->map_type == BPF_MAP_TYPE_ARENA) {
> > +                             if (env->prog->aux->arena) {
>
> Does this have to be (env->prog->aux->arena && env->prog->aux->arena != map) ?

No. all maps in used_maps[] are unique.
Adding "env->prog->aux->arena != map" won't make any difference.
It will only be confusing.

> > +                                     verbose(env, "Only one arena per program\n");
> > +                                     fdput(f);
> > +                                     return -EBUSY;
> > +                             }
>
> [...]
>
> > @@ -18799,6 +18861,18 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> >                          insn->code == (BPF_ST | BPF_MEM | BPF_W) ||
> >                          insn->code == (BPF_ST | BPF_MEM | BPF_DW)) {
> >                       type = BPF_WRITE;
> > +             } else if (insn->code == (BPF_ALU64 | BPF_MOV | BPF_X) && insn->imm) {
> > +                     if (insn->off == BPF_ARENA_CAST_KERN ||
> > +                         (((struct bpf_map *)env->prog->aux->arena)->map_flags & BPF_F_NO_USER_CONV)) {
> > +                             /* convert to 32-bit mov that clears upper 32-bit */
> > +                             insn->code = BPF_ALU | BPF_MOV | BPF_X;
> > +                             /* clear off, so it's a normal 'wX = wY' from JIT pov */
> > +                             insn->off = 0;
> > +                     } /* else insn->off == BPF_ARENA_CAST_USER should be handled by JIT */
> > +                     continue;
> > +             } else if (env->insn_aux_data[i + delta].needs_zext) {
> > +                     /* Convert BPF_CLASS(insn->code) == BPF_ALU64 to 32-bit ALU */
> > +                     insn->code = BPF_ALU | BPF_OP(insn->code) | BPF_SRC(insn->code);
>
> Tbh, I think this should be done in do_misc_fixups(),
> mixing it with context handling in convert_ctx_accesses()
> seems a bit confusing.

Good point. Moved.

Thanks a lot for the review!