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

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>
Subject: Re: [PATCH bpf v1 1/5] bpf: Fix kfunc register offset check for PTR_TO_BTF_ID
Date: Tue, 22 Feb 2022 09:01:32 +0530	[thread overview]
Message-ID: <20220222033132.2ooqxlvld7xxrghm@apollo.legion> (raw)
In-Reply-To: <CAADnVQJ-1D8f36EF-mQk_B_UmGyDbHZnEtYC_mNqt_yDncOCNg@mail.gmail.com>

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.

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.

--
Kartikeya