Re: [RFC bpf-next 1/9] bpf: Add __srcptr kfunc argument suffix

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: __srcptr ignored for scalar-struct pointers in resolve_func_arg_type()
- [Low] bpf: docs: short section underline for __srcptr in kfuncs.rst
--

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

bpf: Add __srcptr kfunc argument suffix

This patch adds the __srcptr suffix annotation for BPF kfunc pointer
arguments so that the verifier checks them with MEM_RDONLY instead of
MEM_WRITE. It also documents the new annotation in
Documentation/bpf/kfuncs.rst.

> diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> index 73578a4c3b1fb..6466b76fd721e 100644
> --- a/Documentation/bpf/kfuncs.rst
> +++ b/Documentation/bpf/kfuncs.rst
> @@ -351,6 +351,29 @@ In this case, a program attached to the ``inode_init_security`` LSM hook
>  can pass the hook's own ``xattr_count`` argument through to the kfunc,
>  which claims xattr slots by writing through it.
>  
> +2.3.10 __srcptr Annotation
> +-------------------------

[Severity: Low]
The section underline in Documentation/bpf/kfuncs.rst is 25 dashes while the
heading is 26 characters long, which causes Sphinx to emit a "Title underline
too short" warning when building the documentation. Should one more dash be
added here?

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5601d20562186..7dd7b172a2971 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -14137,7 +14151,7 @@ get_kfunc_arg_type(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
>  				reg_arg_name(env, argno), btf_type_str(ref_t), ref_tname);
>  			return -EINVAL;
>  		}
> -		arg_type = ARG_PTR_TO_MEM | MEM_WRITE;
> +		arg_type = ARG_PTR_TO_MEM | mem_prot(meta->btf, &args[arg]);
>  	} else if (btf_type_is_struct(ref_t))
>  		/* A pointer to a struct without a size argument is classified as ARG_PTR_TO_BTF_ID */
>  		arg_type = ARG_PTR_TO_BTF_ID;
> @@ -14162,7 +14176,7 @@ get_kfunc_arg_type(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
>  			return -EINVAL;
>  		}
>  		proto->arg_size[arg] = type_size;
> -		arg_type = ARG_PTR_TO_MEM | MEM_FIXED_SIZE | MEM_WRITE;
> +		arg_type = ARG_PTR_TO_MEM | MEM_FIXED_SIZE | mem_prot(meta->btf, &args[arg]);
>  	}

[Severity: Medium]
Should resolve_func_arg_type() in kernel/bpf/verifier.c also use
mem_prot(meta->btf, &args[arg]) instead of hardcoding MEM_WRITE?

In get_kfunc_arg_type(), a pointer to a struct or union without a following
__sz or __szk argument matches btf_type_is_struct(ref_t) and is classified as
ARG_PTR_TO_BTF_ID, bypassing the final else branch above.

Later, when a BPF program passes a non-BTF-ID memory pointer (such as
read-only map values from .rodata, PTR_TO_MEM | MEM_RDONLY, map keys, or
stack memory) to a kfunc parameter pointing to a fixed-size struct or union
composed of scalars, check_func_arg() -> resolve_func_arg_type() converts the
argument type to fixed-size memory, but still unconditionally sets MEM_WRITE:

kernel/bpf/verifier.c:resolve_func_arg_type() {
	...
	if (!btf_type_is_scalar_struct(env, btf, ref_t))
		return 0;

	resolve_ret = btf_resolve_size(btf, ref_t, arg_size);
	if (IS_ERR(resolve_ret)) {
		...
	}
	*arg_type = ARG_PTR_TO_MEM | MEM_FIXED_SIZE | MEM_WRITE |
		    (*arg_type & (PTR_MAYBE_NULL | MEM_UNINIT));

	return 0;
}

Because MEM_WRITE remains set for scalar-struct pointers annotated with
__srcptr, won't the verifier still reject read-only memory arguments and
clobber spilled stack slots?

>  
>  	if (is_kfunc_arg_uninit(meta->btf, &args[arg]))

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