Re: [RFC PATCH bpf-next 2/7] bpf: Verify calls of kfuncs with a body through the body

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] bpf: uninitialized data leak and state mismatch for void kfuncs
- [Critical] bpf: argument checking bypass for nested kfunc calls
- [High] bpf: value truncation of 64-bit constant arguments in kfunc_bind
--

commit 0eafd0ff2fd1aa892e8a507b878aae0d4eedd33b
Author: Yusheng Zheng <yunwei356@gmail.com>

bpf: Verify calls of kfuncs with a body through the body

This commit allows the BPF verifier to inline the bodies of certain kfuncs
for context-aware analysis. It updates the CFG to replace kfunc calls with
their respective bodies before verification, tracks constant arguments, and
attempts to natively execute these inlined kfuncs if supported by the JIT.

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h

[ ... ]

> +/* A call of a kfunc with a body, which the verifier replaced by the body */
> +struct bpf_kfunc_inline {
> +	struct bpf_insn call;
> +	const struct bpf_kfunc_body *body;
> +	unsigned long addr;		/* of the compiled kfunc */
> +	u8 *image;			/* native code for the JIT */
> +	u32 start;
> +	/* the BPF registers that R0-R5 are bound to, and the constant arguments */
> +	u8 reg[MAX_BPF_FUNC_REG_ARGS + 1];
> +	s32 imm[MAX_BPF_FUNC_REG_ARGS + 1];

[Severity: High]
Is it intentional to use s32 for the constant arguments here? Since BPF
registers hold 64-bit values, could this lead to truncating 64-bit constants
if a kfunc expects a u64 or s64?

[ ... ]

> diff --git a/kernel/bpf/kfunc_inline.c b/kernel/bpf/kfunc_inline.c
> --- a/kernel/bpf/kfunc_inline.c
> +++ b/kernel/bpf/kfunc_inline.c

[ ... ]

> +int bpf_inline_kfunc_bodies(struct bpf_verifier_env *env)
> +{

[ ... ]

> +		prog = bpf_patch_insn_data(env, i, r->body->insns, r->body->len);
> +		if (!prog)
> +			return -ENOMEM;
> +		env->prog = prog;
> +		for (j = i; j < i + r->body->len; j++)
> +			env->insn_aux_data[j].kfunc_body = 1;
> +		env->insn_aux_data[i].kfunc_body_entry = 1;
> +		i += r->body->len - 1;

[Severity: Critical]
Since bpf_inline_kfunc_bodies() skips over the newly inlined instructions, it
appears nested kfunc calls inside the inlined body are not inlined themselves
and remain as standard BPF call instructions. 

Combined with the skipped prototype initialization in bpf_add_kfunc_call(),
could this allow a user to pass arbitrary unverified arguments to the nested
kfunc if do_check_main() later evaluates it using a zeroed descriptor?

> +	}
> +	return 0;
> +}
> +
> +/*
> + * The body of a kfunc gets only the arguments of the call and leaves R1-R5
> + * like it. The constant (__k) arguments must be known, and native code may
> + * use their values.
> + */
> +int bpf_mark_kfunc_body_regs(struct bpf_verifier_env *env, int prev_insn_idx,
> +			     const struct bpf_insn_aux_data *aux)
> +{
> +	struct bpf_kfunc_inline *r = env->kfunc_inlines;
> +	struct bpf_reg_state *regs = cur_regs(env);
> +	u32 clobber = 0;
> +	int i, err;
> +
> +	/* leaving the body */
> +	if (prev_insn_idx >= 0 && env->insn_aux_data[prev_insn_idx].kfunc_body &&
> +	    (!aux->kfunc_body || aux->kfunc_body_entry))
> +		clobber |= GENMASK(BPF_REG_5, BPF_REG_1);

[Severity: Critical]
Should this also clobber R0 when leaving a void kfunc?

If the kfunc does not return a value, R0 appears to be left uninitialized.
If the JIT falls back to the compiled C function, it might leave R0 with
uninitialized garbage or a user-controlled value. Could the BPF program then
dereference R0, leading to arbitrary memory reads or privilege escalation?

> +	if (aux->kfunc_body_entry) {
> +		while (r->start != env->insn_idx)
> +			r++;
> +		for (i = BPF_REG_1; i <= BPF_REG_5 && !env->cur_state->speculative; i++) {
> +			if (!(r->imm_mask & BIT(i)))
> +				continue;

[ ... ]

> +			err = mark_chain_precision(env, i);
> +			if (err)
> +				return err;
> +			/* emitted code needs the same constants on every path */
> +			if (r->entered && r->imm[i] != (s32)regs[i].var_off.value)
> +				r->copy = true;
> +			r->imm[i] = regs[i].var_off.value;

[Severity: High]
Will this safely handle 64-bit constants?

If multiple paths call the same kfunc with 64-bit constants that share the
same lower 32 bits, the verifier might incorrectly assume they are identical
due to the s32 cast. Could this cause the JIT to emit native code with the
wrong constant?

> +		}
> +		r->entered = true;
> +		clobber |= BIT(BPF_REG_0) | (GENMASK(BPF_REG_5, BPF_REG_0) &
> +					     ~GENMASK(r->nargs, BPF_REG_0));
> +	}

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -2990,7 +2991,10 @@ int bpf_add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, u16 offset)
>  	desc = &tab->descs[tab->nr_descs];
>  	memset(desc, 0, sizeof(*desc));
>  
> -	err = gen_kfunc_arg_proto(env, &meta, &func_model, &desc->proto);
> +	/* the body of a kfunc that the program may call checks its arguments */
> +	body = kfunc.flags && btf_kfunc_is_allowed(kfunc.btf, func_id, env->prog) ?
> +	       btf_find_kfunc_body(kfunc.btf, func_id) : NULL;
> +	err = body ? 0 : gen_kfunc_arg_proto(env, &meta, &func_model, &desc->proto);

[Severity: Critical]
If gen_kfunc_arg_proto() is skipped for kfuncs with bodies, desc->proto
remains zero-initialized. Could this bypass argument validation for nested
kfunc calls that do not get inlined?

>  	if (err)
>  		return err;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005142219.33451-1-yunwei356@gmail.com?part=2