On Mon, Feb 21, 2022 at 7:31 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Tue, Feb 22, 2022 at 02:06:15AM IST, Alexei Starovoitov wrote:
> > On Sat, Feb 19, 2022 at 6:49 PM Kumar Kartikeya Dwivedi
> > <memxor@xxxxxxxxx> wrote:
> > >
> > > On Sun, Feb 20, 2022 at 07:54:09AM IST, Alexei Starovoitov wrote:
> > > > On Sat, Feb 19, 2022 at 05:07:40PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > >
> > > > > +/* Caller ensures reg->type does not have PTR_MAYBE_NULL */
> > > > > +int check_func_arg_reg_off(struct bpf_verifier_env *env,
> > > > > + const struct bpf_reg_state *reg, int regno,
> > > > > + bool arg_alloc_mem)
> > > > > +{
> > > > > + enum bpf_reg_type type = reg->type;
> > > > > + int err;
> > > > > +
> > > > > + WARN_ON_ONCE(type & PTR_MAYBE_NULL);
> > > >
> > > > So the warn was added and made things more difficult and check had to be moved
> > > > into check_mem_reg to clear that flag?
> > > > Why add that warn in the first place then?
> > > > The logic get convoluted because of that.
> > > >
> > >
> > > Ok, will drop.
> > >
> > > > > + if (reg->off < 0) {
> > > > > + verbose(env, "negative offset %s ptr R%d off=%d disallowed\n",
> > > > > + reg_type_str(env, reg->type), regno, reg->off);
> > > > > + return -EACCES;
> > > > > + }
> > > >
> > > > Out of the whole patch this part is useful. The rest seems to dealing
> > > > with self inflicted pain.
> > > > Just call check_ptr_off_reg() for kfunc ?
> > >
> > > I still think we should call a common helper.
> >
> > What is the point of "common helper" when types
> > with explicit allow of reg offset like PTR_TO_PACKET cannot
> > be passed into kfuncs?
> > A common helper would mislead the reader that such checks are necessary.
> >
>
> PTR_TO_PACKET is certainly allowed to be passed to kfunc, and not just that,
> PTR_TO_STACK, PTR_TO_BUF, PTR_TO_MEM, PTR_TO_MAP_VALUE, PTR_TO_MAP_KEY, all are
> permited after we set ptr_to_mem_ok = true for kfunc. And these can have fixed
> off and sometimes var_off to be set. They are also allowed for global functions.
Ahh. Right. The whole check inside check_mem_reg dance confused me.
> Which is why I thought having a single list in the entire verifier would be more
> easier to maintain, then we can update it in one place and ensure both BPF
> helpers and kfunc are covered by the same checks and expectations for fixed and
> variable offsets. It isn't 'misleading' because all those types are also
> permitted for kfuncs.
>
> > > For kfunc there are also reg->type
> > > PTR_TO_SOCK etc., for them fixed offset should be rejected. So we can either
> > > have a common helper like this for both kfunc and BPF helpers, or exposing
> > > fixed_off_ok parameter in check_ptr_off_reg. Your wish.
> >
> > Are you saying that we should allow PTR_TO_SOCKET+fixed_off ?
>
> No, I said we need to allow fixed off for PTR_TO_BTF_ID, but also prevent
> var_off for it, but just using check_ptr_off_reg would not help because it
> prevents fixed_off, and using __check_ptr_off_reg with fixed_off_ok == true
> would be wrong for PTR_TO_SOCKET etc. Hence some refactoring is needed.
>
> And using check_ptr_off_reg ultimately (through the common check or directly)
> also rejects var_off for PTR_TO_BTF_ID, which was the actual problem that
> started this whole patch.
>
> > I guess than it's better to convert this line
> > err = __check_ptr_off_reg(env, reg, regno,
> > type == PTR_TO_BTF_ID);
> > into a helper.
> > And convert this line:
> > reg->type == PTR_TO_BTF_ID ||
> > (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type))
> >
> > into another helper.
> > Like:
> > static inline bool is_ptr_to_btf_id(type)
> > {
> > return type == PTR_TO_BTF_ID ||
> > (reg2btf_ids[base_type(type)] && !type_flag(type));
> > }
> > and
> > int check_ptr_off_reg(struct bpf_verifier_env *env,
> > const struct bpf_reg_state *reg, int regno)
> > {
> > return __check_ptr_off_reg(env, reg, regno, is_ptr_to_btf_id(reg->type));
> > }
> >
> > and call check_ptr_off_reg() directly from check_func_arg()
> > instead of __check_ptr_off_reg.
> >
> > and call check_ptr_off_reg() from btf_check_func_arg_match() too.
Thoughts about the above proposal?
In addition to above we can have check_func_arg_reg_off()
and call it early and once in btf_check_func_arg_match()
instead of hiding deep in a call chain.
I don't like 'bool arg_alloc_mem' part though
and would like to get rid of 'bool ptr_to_mem_ok' eventually as well.
Such flags make the code harder to follow,
since the action on the flag value is done outside
of the main part of the code.
For example reading btf_check_func_arg_match() on its own is complicated.
The developer has to examine all call sites to see how they pass that flag.
Same thing with 'bool arg_alloc_mem'.
Please pass arg_type instead.
This patch should be split into three.
p1 - refactor into check_func_arg_reg_off helper.
p2 - call it form btf_check_func_arg_match
p3 - add off < 0 check.
If you're adding "while at it" to the commit log it means that
it shouldn't be a single patch.
