Re: [PATCH bpf-next v1 01/13] bpf: Refactor ARG_PTR_TO_DYNPTR checks into process_dynptr_func

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: David Vernet <void@manifault.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <martin.lau@kernel.org>,
	Joanne Koong <joannelkoong@gmail.com>
Subject: Re: [PATCH bpf-next v1 01/13] bpf: Refactor ARG_PTR_TO_DYNPTR checks into process_dynptr_func
Date: Wed, 19 Oct 2022 11:34:12 +0530	[thread overview]
Message-ID: <20221019060412.dqvq2e23cuh3jw52@apollo> (raw)
In-Reply-To: <Y08CYWzGTvKHQXvy@maniforge.dhcp.thefacebook.com>

On Wed, Oct 19, 2022 at 01:15:37AM IST, David Vernet wrote:
> On Tue, Oct 18, 2022 at 07:29:08PM +0530, Kumar Kartikeya Dwivedi wrote:
>
> Hey Kumar, thanks for looking at this stuff.
>
> > ARG_PTR_TO_DYNPTR is akin to ARG_PTR_TO_TIMER, ARG_PTR_TO_KPTR, where
> > the underlying register type is subjected to more special checks to
> > determine the type of object represented by the pointer and its state
> > consistency.
> >
> > Move dynptr checks to their own 'process_dynptr_func' function so that
> > is consistent and in-line with existing code. This also makes it easier
> > to reuse this code for kfunc handling.
>
> Just out of curiosity, do you have a specific use case for when you'd envision
> a kfunc taking a dynptr? I'm not saying there are none, just curious if you
> have any specifically that you've considered.
>

There is already a kfunc that takes dynptrs, bpf_verify_pkcs7_signature. I am
sure we'll get more in the future.

> > To this end, remove the dependency on bpf_call_arg_meta parameter by
> > instead taking the uninit_dynptr_regno by pointer. This is only needed
> > to be set to a valid pointer when arg_type has MEM_UNINIT.
> >
> > Then, reuse this consolidated function in kfunc dynptr handling too.
> > Note that for kfuncs, the arg_type constraint of DYNPTR_TYPE_LOCAL has
> > been lifted.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf_verifier.h                  |   8 +-
> >  kernel/bpf/btf.c                              |  17 +--
> >  kernel/bpf/verifier.c                         | 115 ++++++++++--------
> >  .../bpf/prog_tests/kfunc_dynptr_param.c       |   5 +-
> >  .../bpf/progs/test_kfunc_dynptr_param.c       |  12 --
> >  5 files changed, 69 insertions(+), 88 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 9e1e6965f407..a33683e0618b 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -593,11 +593,9 @@ int check_kfunc_mem_size_reg(struct bpf_verifier_env *env, struct bpf_reg_state
> >  			     u32 regno);
> >  int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> >  		   u32 regno, u32 mem_size);
> > -bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> > -			      struct bpf_reg_state *reg);
> > -bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> > -			     struct bpf_reg_state *reg,
> > -			     enum bpf_arg_type arg_type);
> > +int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> > +			enum bpf_arg_type arg_type, int argno,
> > +			u8 *uninit_dynptr_regno);
> >
> >  /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */
> >  static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog,
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index eba603cec2c5..1827d889e08a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6486,23 +6486,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >  						return -EINVAL;
> >  					}
> >
> > -					if (!is_dynptr_reg_valid_init(env, reg)) {
> > -						bpf_log(log,
> > -							"arg#%d pointer type %s %s must be valid and initialized\n",
> > -							i, btf_type_str(ref_t),
> > -							ref_tname);
> > +					if (process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR, i, NULL))
> >  						return -EINVAL;
> > -					}
>
> Could you please clarify why you're removing the DYNPTR_TYPE_LOCAL constraint
> for kfuncs?
>
> You seemed to have removed the following negative selftest:
>
> > -SEC("?lsm.s/bpf")
> > -int BPF_PROG(dynptr_type_not_supp, int cmd, union bpf_attr *attr,
> > -	     unsigned int size)
> > -{
> > -	char write_data[64] = "hello there, world!!";
> > -	struct bpf_dynptr ptr;
> > -
> > -	bpf_ringbuf_reserve_dynptr(&ringbuf, sizeof(write_data), 0, &ptr);
> > -
> > -	return bpf_verify_pkcs7_signature(&ptr, &ptr, NULL);
> > -}
> > -
>
> But it was clearly the intention of the test validate that we can't pass a
> dynptr to a ringbuf region to this kfunc, so I'm curious what's changed since
> that test was added.
>

There was no inherent limitation for just accepting local dynptrs, it's that
when this was added I suggested sticking to one kind back then, because of the
code divergence between kfunc argument checking and helper argument checking.

Now that both share the same code, it's easier to handle everything one place
and make it work everywhere the same way.

Also, next patch adds a very clear distinction between argument type which only
operates on the dynamically sized memory slice and ones which may also modify
dynptr, which also makes it easier to support things for kfuncs by setting
MEM_RDONLY.

> > -
> > -					if (!is_dynptr_type_expected(env, reg,
> > -							ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL)) {
> > -						bpf_log(log,
> > -							"arg#%d pointer type %s %s points to unsupported dynamic pointer type\n",
> > -							i, btf_type_str(ref_t),
> > -							ref_tname);
> > -						return -EINVAL;
> > -					}
> > -
> >  					continue;
> >  				}
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 6f6d2d511c06..31c0c999448e 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -782,8 +782,7 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> >  	return true;
> >  }
> >
> > -bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> > -			      struct bpf_reg_state *reg)
> > +static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> >  {
> >  	struct bpf_func_state *state = func(env, reg);
> >  	int spi = get_spi(reg->off);
> > @@ -802,9 +801,8 @@ bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> >  	return true;
> >  }
> >
> > -bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> > -			     struct bpf_reg_state *reg,
> > -			     enum bpf_arg_type arg_type)
> > +static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > +				    enum bpf_arg_type arg_type)
> >  {
> >  	struct bpf_func_state *state = func(env, reg);
> >  	enum bpf_dynptr_type dynptr_type;
> > @@ -5573,6 +5571,65 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno,
> >  	return 0;
> >  }
> >
> > +int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> > +			enum bpf_arg_type arg_type, int argno,
> > +			u8 *uninit_dynptr_regno)
> > +{
>
> IMO 'process' is a bit too generic of a term. If we decide to go with this,
> what do you think about changing the name to check_func_dynptr_arg(), or just
> check_dynptr_arg()?
>

While I agree, then it would be different from the existing ones and look a bit
odd in the list (e.g. process_spin_lock, process_kptr_func, etc.). So I am not
very sure, but if you still feel it's better I don't mind.

> [...]