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. 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.
> The patch seems to be doing several things at once. Please split.
--
Kartikeya
