Re: [PATCHv2 bpf-next 4/8] selftests/bpf: Test const kfunc memory arguments

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: kfunc_call_test_const_mem() fails verifier load with MEM_WRITE
--

commit d0fe747011b126dd39ad19108026e34fdd9f18ce
Author: Jiri Olsa <jolsa@kernel.org>

selftests/bpf: Test const kfunc memory arguments

This commit adds BPF selftests and bpf_testmod kfuncs to test passing
read-only global data, stack data, and const scalar structs to const-qualified
kfunc memory arguments, as well as rejecting read-only data for writable
memory arguments.

> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> index 8e6560c31e781..4f4a6713aa6e5 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> @@ -5,6 +5,9 @@
>  #include "bpf_misc.h"
>  #include "../test_kmods/bpf_testmod_kfunc.h"
>  
> +static const char kfunc_const_data[] = "read-only source";
> +static const struct prog_test_pass1 kfunc_const_struct = { .x0 = 1 };
> +
>  static struct bpf_spin_lock kfunc_call_lock SEC(".data.A");
>  
>  SEC("tc")
> @@ -208,6 +211,17 @@ int kfunc_call_test_pass(struct __sk_buff *skb)
>  	return 0;
>  }
>  
> +SEC("tc")
> +int kfunc_call_test_const_mem(struct __sk_buff *skb)
> +{
> +	char stack_data[] = "read-write";
> +
> +	bpf_kfunc_call_test_const_mem(kfunc_const_data, sizeof(kfunc_const_data));
> +	bpf_kfunc_call_test_const_mem(stack_data, sizeof(stack_data));
> +	bpf_kfunc_call_test_const_struct(&kfunc_const_struct);
> +	return 0;
> +}

[Severity: Medium]
Does kfunc_call_test_const_mem() pass the verifier without a corresponding
verifier change to preserve const qualifiers on kfunc memory arguments?

In kernel/bpf/verifier.c, get_kfunc_arg_type() strips all type modifiers
including BTF_KIND_CONST from the pointed-to type via
btf_type_skip_modifiers(), advances ref_id_ptr past them, and unconditionally
sets MEM_WRITE on sized memory arguments:

kernel/bpf/verifier.c:get_kfunc_arg_type() {
	...
	ref_id_ptr = &t->type;
	ref_t = btf_type_skip_modifiers(meta->btf, *ref_id_ptr, &ref_id);
	while (*ref_id_ptr != ref_id)
		ref_id_ptr = &btf_type_by_id(meta->btf, *ref_id_ptr)->type;
	...
	} else if (arg + 1 < nargs &&
		 (is_kfunc_arg_mem_size(meta->btf, &args[arg + 1]) ||
		  is_kfunc_arg_const_mem_size(meta->btf, &args[arg + 1]))) {
		...
		arg_type = ARG_PTR_TO_MEM | MEM_WRITE;
	}
	...
	if (base_type(arg_type) == ARG_PTR_TO_BTF_ID) {
		if (is_kfunc_arg_map(meta->btf, &args[arg]))
			proto->arg_btf_id[arg] = reg2btf_ids[CONST_PTR_TO_MAP];
		else
			proto->arg_btf_id[arg] = ref_id_ptr;
}

Similarly, for bpf_kfunc_call_test_const_struct(), when &kfunc_const_struct
(a PTR_TO_MAP_VALUE in .rodata) is passed, resolve_func_arg_type() converts
the scalar-struct pointer argument to fixed-size memory with MEM_WRITE set:

kernel/bpf/verifier.c:resolve_func_arg_type() {
	...
	if (!btf_type_is_scalar_struct(env, btf, ref_t))
		return 0;
	...
	*arg_type = ARG_PTR_TO_MEM | MEM_FIXED_SIZE | MEM_WRITE |
		    (*arg_type & (PTR_MAYBE_NULL | MEM_UNINIT));
}

When check_func_arg() validates kfunc_const_data and &kfunc_const_struct,
func_arg_access_type() returns BPF_READ | BPF_WRITE because MEM_WRITE is set,
causing check_helper_mem_access() -> check_map_access_type() to reject the
read-only .rodata map with -EACCES ("write into map forbidden").

Because kfunc_call_test_const_mem is an autoloaded SEC("tc") program in
kfunc_call_test.c, wouldn't this cause kfunc_call_test__open_and_load() and
kfunc_call_test_lskel__open_and_load() in verify_success() to fail and break
the kfunc_call test suite?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103075916.100050-5-jolsa@kernel.org?part=1