Re: [PATCH bpf-next v3 15/18] bpf: Classify kfunc pointer arguments from BTF, resolve type against the register

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Allowing PTR_TO_BTF_ID as a KF_ARG_PTR_TO_MEM argument is either broken or fundamentally unsafe due to unvalidated access flags.
--

--- Patch [15]: [PATCH bpf-next v3 15/18] bpf: Classify kfunc pointer arguments from BTF, resolve type against the register ---
commit d5adf1f80d7ac5827091492c5099bcb0c05e15d1
Author: Amery Hung <[email protected]>

bpf: Classify kfunc pointer arguments from BTF, resolve type against the register

This commit changes kfunc pointer argument classification to rely strictly
on BTF rather than register state. It introduces support for passing a
PTR_TO_BTF_ID as a fixed-size memory buffer when pointing to a scalar struct.

>     The new design now accepts one previously rejected case: passing
>     PTR_TO_BTF_ID to a pointer to scalar w/o a following __sz/__szk. The
>     argument will be classified as KF_ARG_PTR_TO_MEM | MEM_FIXED_SIZE. The
>     PTR_TO_BTF_ID register will go through check_mem_reg() ->
>     check_helper_mem_access() -> check_ptr_to_btf_access(), and by default only
>     read is allowed.

[Severity: Medium]
Will this newly accepted case always fail verification?

While the commit message states that "by default only read is allowed,"
the call to check_mem_reg() for KF_ARG_PTR_TO_MEM arguments
unconditionally demands BPF_READ | BPF_WRITE access. This causes
check_ptr_to_btf_access() to reject standard kernel pointers with -EACCES.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1ccf3b764c515..9045369ba5694 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11405,37 +11414,37 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
[ ... ]
> -	/* This is the catch all argument type of register types supported by
> -	 * check_helper_mem_access. However, we only allow when argument type is
> -	 * pointer to scalar, or struct composed (recursively) of scalars. When
> -	 * arg_mem_size is true, the pointer can be void *.
> +	/* A pointer to a struct without a size argument is classified as KF_ARG_PTR_TO_BTF_ID */
> +	if (btf_type_is_struct(ref_t))
> +		return KF_ARG_PTR_TO_BTF_ID;
> +
> +	/*
> +	 * Otherwise this is a fixed-size memory buffer supported by
> +	 * check_helper_mem_access(): a pointer to a scalar or a struct of
> +	 * scalars. The access size is derived from the pointed-to BTF type.
>  	 */
> -	if (!btf_type_is_scalar(ref_t) && !__btf_type_is_scalar_struct(env, meta->btf, ref_t, 0) &&
> -	    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
> -		verbose(env, "%s pointer type %s %s must point to %sscalar, or struct with scalar\n",
> -			reg_arg_name(env, argno),
> -			btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
> +	if (!btf_type_is_scalar(ref_t) &&
> +	    !__btf_type_is_scalar_struct(env, meta->btf, ref_t, 0)) {
> +		verbose(env, "%s pointer type %s %s must point to scalar, or struct with scalar\n",
> +			reg_arg_name(env, argno), btf_type_str(ref_t), ref_tname);
>  		return -EINVAL;
>  	}
> -	return arg_mem_size ? KF_ARG_PTR_TO_MEM : KF_ARG_PTR_TO_MEM | MEM_FIXED_SIZE;
> +	return KF_ARG_PTR_TO_MEM | MEM_FIXED_SIZE;
>  }

[Severity: Medium]
When get_kfunc_ptr_arg_type() returns KF_ARG_PTR_TO_MEM | MEM_FIXED_SIZE for
a PTR_TO_BTF_ID, or when check_kfunc_args() falls through to KF_ARG_PTR_TO_MEM,
it verifies the argument as a memory buffer:

kernel/bpf/verifier.c:check_kfunc_args() {
    ...
		case KF_ARG_PTR_TO_MEM:
			if (kf_arg_type & MEM_FIXED_SIZE) {
				resolve_ret = btf_resolve_size(btf, ref_t, &type_size);
                ...
				ret = check_mem_reg(env, reg, argno, type_size, BPF_READ | BPF_WRITE, meta);
    ...
}

Because check_mem_reg() is passed BPF_READ | BPF_WRITE, it requires write
access. When this goes through check_helper_mem_access() and reaches
check_ptr_to_btf_access(), it will be rejected because atype != BPF_READ:

kernel/bpf/verifier.c:check_ptr_to_btf_access() {
    ...
		if (atype != BPF_READ && !type_is_ptr_alloc_obj(reg->type)) {
			verbose(env, "only read is supported\n");
			return -EACCES;
		}
    ...
}

Could this unconditionally break the newly introduced feature for standard
(non-allocated) kernel pointers? Also, if the BPF_READ | BPF_WRITE flag were
simply changed to BPF_READ to pass verification, could it introduce a
vulnerability since the verifier cannot guarantee that the underlying kfunc
will not write to the buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15