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.
> 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 ?
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.
