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@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: Sun, 20 Feb 2022 08:19:15 +0530	[thread overview]
Message-ID: <20220220024915.nohjpzvsn5bu2opo@apollo.legion> (raw)
In-Reply-To: <20220220022409.r5y2bovtgz3r2n47@ast-mbp.dhcp.thefacebook.com>

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