Re: [PATCH bpf-next v4 27/46] bpf: Track a pointer to one of a set of functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: unconditional R1-R5 split in do_check_insn() causes
  state explosion
--

commit 5fe8c15ac1cfee1e78fee4f0e12cfb386fffa7e1
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Track a pointer to one of a set of functions

This commit defers splitting verifier states when loading function pointers
from a read-only map table by representing the loaded register as a set of
possible functions until a callx or callback helper/kfunc requires a single
target.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7769b4bd14673..a5c3e86f3f87f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -21009,6 +21125,12 @@ static int do_check_insn(struct bpf_verifier_env *env, bool *do_print_state)
>  				return check_func_callx(env, insn, &env->insn_idx);
>  			if (insn->src_reg == BPF_PSEUDO_CALL)
>  				return check_func_call(env, insn, &env->insn_idx);
> +			/* a callback is one function */
> +			for (i = BPF_REG_1; i <= BPF_REG_5; i++) {
> +				err = split_func_set(env, i);
> +				if (err)
> +					return err;
> +			}
>  			if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
>  				return check_kfunc_call(env, insn, &env->insn_idx);
>  			return check_helper_call(env, insn, &env->insn_idx);

[Severity: Medium]
Can splitting all of R1 through R5 before every helper and kfunc call cause
an N * N state explosion or unnecessary state cloning?

When a function set of size N is loaded into R3 and copied to R2 before
calling bpf_timer_set_callback(), both R2 and R3 are marked live in
live_regs_before because bpf_timer_set_callback_proto declares a third
ARG_PTR_TO_PROG_AUX argument.  Splitting R2 and R3 independently produces
N * N combinations at the call instruction instead of the N states produced
before this patch, exceeding BPF_FLOW_MAX_STATES (1024) for N >= 33 and
rejecting the program with -E2BIG.

In addition, non-sync-callback helper and kfunc calls inside a basic block
are not flow points, so dead caller-saved registers in R1 through R5 have
not been scrubbed by bpf_clean_verifier_state() when do_check_insn() runs.
If a dead register in R1 through R5 still holds a func_set of size N,
split_func_set() pushes N - 1 states onto env->head at env->insn_idx,
inflating env->total_states and re-verifying the call and the rest of the
basic block.

Splitting only when checking an argument that expects a function pointer (such
as in process_kf_arg_ptr_to_func() and the ARG_PTR_TO_FUNC path in
check_func_arg()) would avoid splitting unrelated or dead registers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107033758.1547725-1-alexei.starovoitov@gmail.com?part=27