Re: [PATCH bpf v1 1/5] bpf: Fix kfunc register offset check for PTR_TO_BTF_ID

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Tue, Feb 22, 2022 at 09:51:43AM IST, Alexei Starovoitov wrote:
> On Mon, Feb 21, 2022 at 7:31 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> 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@gmail.com> 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.

Agree with everything, will split and respin. Thanks.

--
Kartikeya