On Fri, Dec 18, 2020 at 11:52:20AM -0800, Andrii Nakryiko wrote:
> >
> >
> > >
> > > > + const struct bpf_reg_state saved_reg = *reg;
> > >
> > > this saving and restoring of the original state due to
> > > mark_ptr_not_null_reg() is a bit ugly. Maybe it's better to refactor
> > > mark_ptr_not_null_reg to just return a new register type on success or
> > > 0 (NOT_INIT) on failure? Then you won't have to do this.
> >
> > It is not enough just to convert register's type - e.g. we also
> > want to change map_ptr to map->inner_map_meta for a case of
> > PTR_TO_MAP_VALUE_OR_NULL and inner_map_meta because it may be
> > used in check_helper_mem_access() -> check_map_access().
>
>
> Yep, missed that part in patch #1. But thinking about this more, I'm
> now missing the point of saving and restoring the register state. A
> comment would be welcome here, if it's really needed. I.e., if
> mark_ptr_not_null_reg fails, it doesn't change the state of the
> register. If check_helper_mem_access fails and changes the sate, then
> you have a similar problem few lines below anyway. So what's the case
> when check_helper_mem_access() succeeds and changes register state,
> but you still need to restore the register?
This saving is required because btf_check_func_arg_match()
happens in a callee context and we don't want to modify the
register state as it may be possible that the register will be
used later in the callee.
If any of the checks fail - the verifier mustn't accept a
program. If both of the checks succeed - we want to keep the
register state as it was before the call.
> > > > +}
> > > > +
> > > > /* Implementation details:
> > > > * bpf_map_lookup returns PTR_TO_MAP_VALUE_OR_NULL
> > > > * Two bpf_map_lookups (even with the same key) will have different reg->id.
> > > > @@ -11435,6 +11458,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> > > > mark_reg_known_zero(env, regs, i);
> > > > else if (regs[i].type == SCALAR_VALUE)
> > > > mark_reg_unknown(env, regs, i);
> > > > + else if (regs[i].type == PTR_TO_MEM_OR_NULL) {
> > > > + const u32 mem_size = regs[i].mem_size;
> > > > +
> > > > + mark_reg_known_zero(env, regs, i);
> > > > + regs[i].mem_size = mem_size;
> > > > + regs[i].id = i;
> > >
> > > I don't think we need to set id, we don't use that for PTR_TO_MEM registers.
> >
> > If we don't set id then in check_cond_jump_id() ->
> > mark_ptr_or_null_regs() -> mark_ptr_or_null_reg() we don't
> > transform register type either to SCALAR(NULL case) or
> > PTR_TO_MEM(value case):
> > ...
> > if (reg_type_may_be_null(reg->type) && reg->id == id &&
> > ...
> >
> > The end result is that the verifier mem access checks fail for a
> > PTR_TO_MEM_OR_NULL register.
>
> Hm... I see now. I was looking at check_helper_call() and handling of
> RET_PTR_TO_ALLOC_MEM_OR_NULL return result for bpf_ringbuf_reserve().
> It didn't seem to set id at all and yet works just fine. But now I see
> extra
>
> if (reg_type_may_be_null(regs[BPF_REG_0].type))
> regs[BPF_REG_0].id = ++env->id_gen;
>
> after the big if/else if block there, so it makes sense. Thanks.
>
>
> regs[i].id = i; might not be wrong, but is unconventional, so let's
> stick with `++env->id_gen`?
>
Agreed.
--
Dmitrii Banshchikov
