Re: [PATCH v13 bpf-next 03/10] bpf: Allow initializing dynptrs in kfuncs

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Joanne Koong <joannelkoong@gmail.com>
Cc: bpf@vger.kernel.org, martin.lau@kernel.org, andrii@kernel.org,
	ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
	toke@kernel.org
Subject: Re: [PATCH v13 bpf-next 03/10] bpf: Allow initializing dynptrs in kfuncs
Date: Mon, 6 Mar 2023 08:36:28 +0100	[thread overview]
Message-ID: <20230306073628.g2kg5vp6lw6vzyya@apollo> (raw)
In-Reply-To: <20230301154953.641654-4-joannelkoong@gmail.com>

On Wed, Mar 01, 2023 at 04:49:46PM CET, Joanne Koong wrote:
> This change allows kfuncs to take in an uninitialized dynptr as a
> parameter. Before this change, only helper functions could successfully
> use uninitialized dynptrs. This change moves the memory access check
> (including stack state growing and slot marking) into
> process_dynptr_func(), which both helpers and kfuncs call into.
>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  kernel/bpf/verifier.c | 67 ++++++++++++++-----------------------------
>  1 file changed, 22 insertions(+), 45 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e0e00509846b..82e39fc5ed05 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -268,7 +268,6 @@ struct bpf_call_arg_meta {
>  	u32 ret_btf_id;
>  	u32 subprogno;
>  	struct btf_field *kptr_field;
> -	u8 uninit_dynptr_regno;
>  };
>
>  struct btf *btf_vmlinux;
> @@ -6225,10 +6224,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno,
>   * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument
>   * type, and declare it as 'const struct bpf_dynptr *' in their prototype.
>   */
> -static int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> -			       enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta)
> +static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx,
> +			       enum bpf_arg_type arg_type)
>  {
>  	struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
> +	int err;
>
>  	/* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an
>  	 * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
> @@ -6254,23 +6254,23 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno,
>  	 *		 to.
>  	 */
>  	if (arg_type & MEM_UNINIT) {
> +		int i;
> +
>  		if (!is_dynptr_reg_valid_uninit(env, reg)) {
>  			verbose(env, "Dynptr has to be an uninitialized dynptr\n");
>  			return -EINVAL;
>  		}
>
> -		/* We only support one dynptr being uninitialized at the moment,
> -		 * which is sufficient for the helper functions we have right now.
> -		 */
> -		if (meta->uninit_dynptr_regno) {
> -			verbose(env, "verifier internal error: multiple uninitialized dynptr args\n");
> -			return -EFAULT;
> +		/* we write BPF_DW bits (8 bytes) at a time */
> +		for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) {
> +			err = check_mem_access(env, insn_idx, regno,
> +					       i, BPF_DW, BPF_WRITE, -1, false);
> +			if (err)
> +				return err;
>  		}

I am not sure moving check_mem_access into process_dynptr_func is the right
thing to do. Not sure if a problem already, but sooner or later it might be.

The side effects of the call should take effect on the current state only after
we have gone through all arguments for the helper/kfunc call. In this case we
will now do stack access while processing the dynptr arg, which may affect the
state of stack we see through other memory arguments coming later.

I think it is better to do it after argument processing is done, similar to
existing meta.access_size handling which is done after check_func_arg loop (for
the same reasons).

> [...]