On 5/8/25 11:08, Matt Bobrowski wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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.
This function is using the same style as 7 btf_type_is_* helpers
directly above it so I don't think that it'd be doing something
non-standard here.
>> /* 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, ...);
> ```
That's a nice idea, thanks! I'll definitely use it in some form
(depending on how we end up representing the access type param).
>
>> 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
>>
>
