On Fri, Feb 9, 2024 at 5:13 PM Eduard Zingerman <eddyz87@xxxxxxxxx> 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 = ®s[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!
