Re: [PATCH bpf-next v4 1/4] bpf: Teach vefier to handle const ptrs as args to kfuncs

[Matt Bobrowski <mattbobrowski@google.com>]

On Wed, May 07, 2025 at 08:40:36AM +0200, Viktor Malik wrote:
> When a kfunc takes a const pointer as an argument, the verifier should
> not check that the memory can be accessed for writing as that may lead
> to rejecting safe programs. Extend the verifier to detect such arguments
> and skip the write access check for them.
> 
> The use-case for this change is passing string literals (i.e. read-only
> maps) to read-only string kfuncs.
> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>  include/linux/btf.h   |  5 +++++
>  kernel/bpf/verifier.c | 10 ++++++----
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index ebc0c0c9b944..5cb06c65d91f 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -391,6 +391,11 @@ static inline bool btf_type_is_type_tag(const struct btf_type *t)
>  	return BTF_INFO_KIND(t->info) == BTF_KIND_TYPE_TAG;
>  }
>  
> +static inline bool btf_type_is_const(const struct btf_type *t)
> +{
> +	return BTF_INFO_KIND(t->info) == BTF_KIND_CONST;
> +}

I've seen btf_type_is_* related helpers lean on btf_kind() instead
here, which ultimately just wraps BTF_INFO_KIND() macro.

>  /* union is only a special case of struct:
>   * all its offsetof(member) == 0
>   */
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 54c6953a8b84..e2d74c4d44c1 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8186,7 +8186,7 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
>  }
>  
>  static int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> -			 u32 regno, u32 mem_size)
> +			 u32 regno, u32 mem_size, bool read_only)

Maybe s/read_only/write_mem_access?

>  {
>  	bool may_be_null = type_may_be_null(reg->type);
>  	struct bpf_reg_state saved_reg;
> @@ -8205,7 +8205,8 @@ static int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg
>  	}
>  
>  	err = check_helper_mem_access(env, regno, mem_size, BPF_READ, true, NULL);
> -	err = err ?: check_helper_mem_access(env, regno, mem_size, BPF_WRITE, true, NULL);
> +	if (!read_only)
> +		err = err ?: check_helper_mem_access(env, regno, mem_size, BPF_WRITE, true, NULL);

For clarity, I'd completely get rid of the ternary operator usage
here. You can short circuit the call to check_helper_mem_access() w/
BPF_WRITE by simply checking the error code value from the preceding
call to check_helper_mem_access() w/ BPF_READ in the branch condition
i.e.

```
err = check_helper_mem_access(..., BPF_READ, ...);
if (!err && write_mem_access)
   err = check_helper_mem_acces(..., BPF_WRITE, ...);
```

>  	if (may_be_null)
>  		*reg = saved_reg;
> @@ -10361,7 +10362,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
>  			ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
>  			if (ret < 0)
>  				return ret;
> -			if (check_mem_reg(env, reg, regno, arg->mem_size))
> +			if (check_mem_reg(env, reg, regno, arg->mem_size, false))

For clarity, I'd add: /*write_mem_access=*/false). Same with the below
call to check_mem_reg().

>  				return -EINVAL;
>  			if (!(arg->arg_type & PTR_MAYBE_NULL) && (reg->type & PTR_MAYBE_NULL)) {
>  				bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
> @@ -13252,7 +13253,8 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>  					i, btf_type_str(ref_t), ref_tname, PTR_ERR(resolve_ret));
>  				return -EINVAL;
>  			}
> -			ret = check_mem_reg(env, reg, regno, type_size);
> +			ret = check_mem_reg(env, reg, regno, type_size,
> +					    btf_type_is_const(btf_type_by_id(btf, t->type)));
>  			if (ret < 0)
>  				return ret;
>  			break;
> -- 
> 2.49.0
>