Re: [PATCH bpf-next v10 07/24] bpf: Verify an unwind through landing pads and epilogues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: skipped exception pad verification for global subprogs that throw and unwind
- [Medium] bpf: Soft lockup in mark_callx_might_unwind() due to O(N^2) instruction traversal
--

commit 54596ff7029b639349bb84341497b649e38e027e
Author: Yonghong Song <yonghong.song@linux.dev>

bpf: Verify an unwind through landing pads and epilogues

This commit adds verifier support for tracking control flow through
unwind landing pads and epilogues. It calculates whether a subprogram
might unwind by propagating the might_unwind flag along the CFG,
and integrates backtracking through these new paths.

> diff --git a/kernel/bpf/cfg.c b/kernel/bpf/cfg.c
> --- a/kernel/bpf/cfg.c
> +++ b/kernel/bpf/cfg.c
> @@ -716,6 +753,82 @@

[ ... ]

> +static void mark_callx_might_unwind(struct bpf_verifier_env *env)
> +{
> +	struct bpf_insn *insns = env->prog->insnsi;
> +	struct bpf_subprog_info *caller, *callee;
> +	int i, j, len = env->prog->len;
> +	struct bpf_func_ptr *ptrs;
> +	bool changed;
> +	u32 cnt;
> +
> +	if (!addr_taken_subprog_might_unwind(env))
> +		return;
> +
> +	for (i = 0; i < len; i++)
> +		if (bpf_is_callx(&insns[i]))
> +			bpf_find_containing_subprog(env, i)->might_unwind = true;
> +
> +	do {
> +		changed = false;
> +		for (i = 0; i < len; i++) {
> +			caller = bpf_find_containing_subprog(env, i);
> +			if (caller->might_unwind)
> +				continue;

[Severity: Medium]
Could this loop cause a CPU soft lockup for very large BPF programs?

The loop iterates up to len times (which can be up to 1 million) and
repeatedly calls bpf_find_containing_subprog(), which performs a binary
search. Because this propagates upward through the call graph, the do-while
loop could execute many times (e.g., up to 256 for the maximum call depth).

Without a cond_resched(), this might stall the CPU in kernel space for an
extended period.

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19464,8 +19602,14 @@ static int do_check_insn(struct bpf_verifier_env *env, bool *do_print_state)
>  				cur_func(env)->no_stack_arg_load = true;
>  			if (bpf_is_callx(insn))
>  				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);
> +			if (insn->src_reg == BPF_PSEUDO_CALL) {
> +				int call_idx = env->insn_idx;
> +
> +				err = check_func_call(env, insn, &env->insn_idx);
> +				if (err)
> +					return err;

[Severity: High]
If a global subprogram has the might_throw flag set, check_func_call()
simulates the exception exit and returns PROCESS_BPF_EXIT (which is 1).

Because err is positive, does the check here mistakenly return early,
bypassing the subsequent call to process_global_call_unwind()? 

If the subprogram can both throw and unwind, its landing pad might never be
visited by the verifier, potentially leading to a dead code sanitization trap
at runtime.

> +				return process_global_call_unwind(env, call_idx, do_print_state);
> +			}
>  			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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009041447.17092-1-yonghong.song@linux.dev?part=7