On Thu, Apr 21, 2022 at 12:36 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
> > > +
> > > + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
> > > + off_desc->kptr.btf, off_desc->kptr.btf_id))
> > > + goto bad_type;
> >
> > Is full type comparison really needed?
>
> Yes.
>
> > reg->btf should be the same pointer as off_desc->kptr.btf
> > and btf_id should match exactly.
>
> This is not true, it can be vmlinux or module BTF. But if you mean just
> comparing the pointer and btf_id, we still need to handle reg->off.
>
> We want to support cases like:
>
> struct foo {
> struct bar br;
> struct baz bz;
> };
>
> struct foo *v = func(); // PTR_TO_BTF_ID
> map->foo = v; // reg->off is zero, btf and btf_id matches type.
> map->bar = &v->br; // reg->off is still zero, but we need to walk and retry with
> // first member type of struct after comparison fails.
> map->baz = &v->bz; // reg->off is non-zero, so struct needs to be walked to
> // match type.
>
> In the ref case, the argument's offset will always be 0, so third case is not
> going to work, but in the unref case, we want to allow storing pointers to
> structs embedded inside parent struct.
>
> Please let me know if I misunderstood what you meant.
Makes sense.
Please add this comment to the code.
> > Is this a feature proofing for some day when registers with PTR_TO_BTF_ID type
> > will start pointing to prog's btf?
> >
> > > + return 0;
> > > +bad_type:
> > > + verbose(env, "invalid kptr access, R%d type=%s%s ", regno,
> > > + reg_type_str(env, reg->type), reg_name);
> > > + verbose(env, "expected=%s%s\n", reg_type_str(env, PTR_TO_BTF_ID), targ_name);
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static int check_map_kptr_access(struct bpf_verifier_env *env, u32 regno,
> > > + int value_regno, int insn_idx,
> > > + struct bpf_map_value_off_desc *off_desc)
> > > +{
> > > + struct bpf_insn *insn = &env->prog->insnsi[insn_idx];
> > > + int class = BPF_CLASS(insn->code);
> > > + struct bpf_reg_state *val_reg;
> > > +
> > > + /* Things we already checked for in check_map_access and caller:
> > > + * - Reject cases where variable offset may touch kptr
> > > + * - size of access (must be BPF_DW)
> > > + * - tnum_is_const(reg->var_off)
> > > + * - off_desc->offset == off + reg->var_off.value
> > > + */
> > > + /* Only BPF_[LDX,STX,ST] | BPF_MEM | BPF_DW is supported */
> > > + if (BPF_MODE(insn->code) != BPF_MEM) {
> > > + verbose(env, "kptr in map can only be accessed using BPF_MEM instruction mode\n");
> > > + return -EACCES;
> > > + }
> > > +
> > > + if (class == BPF_LDX) {
> > > + val_reg = reg_state(env, value_regno);
> > > + /* We can simply mark the value_regno receiving the pointer
> > > + * value from map as PTR_TO_BTF_ID, with the correct type.
> > > + */
> > > + mark_btf_ld_reg(env, cur_regs(env), value_regno, PTR_TO_BTF_ID, off_desc->kptr.btf,
> > > + off_desc->kptr.btf_id, PTR_MAYBE_NULL);
> > > + val_reg->id = ++env->id_gen;
> >
> > why non zero id this needed?
>
> For mark_ptr_or_null_reg. I'll add a comment.
Ahh. It's because it's not a plain PTR_TO_BTF_ID,
but the one with PTR_MAYBE_NULL.
Makes sense.
