Re: [PATCH bpf-next v12 06/25] bpf: Verify an unwind through landing pads and epilogues

[Yonghong Song <yonghong.song@linux.dev>]

On 10/16/26 8:27 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] bpf: missing might_unwind propagation in mark_callx_might_unwind()
> --
>
> commit c4002615f0bb137c4f685915b07888fb1dc5bbea
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> bpf: Verify an unwind through landing pads and epilogues
>
> This commit teaches the BPF verifier to follow stack unwinds from
> bpf_unwind(), bpf_unwind_resume(), and unwinding global subprograms
> through caller landing pads and epilogues, including CFG effect
> propagation and precision backtracking.
>
>> diff --git a/kernel/bpf/cfg.c b/kernel/bpf/cfg.c
>> index ad7ea08f3b15e..9608293bdc069 100644
>> --- a/kernel/bpf/cfg.c
>> +++ b/kernel/bpf/cfg.c
> [ ... ]
>
>> @@ -91,6 +99,7 @@ static void merge_callee_effects(struct bpf_verifier_env *env, int t, int w)
>>   	caller->changes_pkt_data |= callee->changes_pkt_data;
>>   	caller->might_sleep |= callee->might_sleep;
>>   	caller->might_throw |= callee->might_throw;
>> +	caller->might_unwind |= callee->might_unwind;
>>   }
>>   
>>   enum {
> [ ... ]
>
>> @@ -705,6 +716,82 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
>>   	}
>>   }
>>   
>> +static bool addr_taken_subprog_might_unwind(struct bpf_verifier_env *env)
>> +{
>> +	struct bpf_insn *insns = env->prog->insnsi;
>> +	struct bpf_subprog_info *callee;
>> +	struct bpf_func_ptr *ptrs;
>> +	u32 cnt, j;
>> +	int i;
>> +
>> +	for (i = 0; i < env->prog->len; i++) {
>> +		if (bpf_pseudo_func(&insns[i])) {
>> +			callee = bpf_find_containing_subprog(env, i + insns[i].imm + 1);
>> +			if (callee->might_unwind)
>> +				return true;
>> +			continue;
>> +		}
>> +		ptrs = insn_func_ptrs(env, i, &cnt);
>> +		for (j = 0; j < cnt; j++) {
>> +			callee = bpf_find_containing_subprog(env, ptrs[j].xlated_off);
>> +			if (callee->might_unwind)
>> +				return true;
>> +		}
>> +	}
>> +	return false;
>> +}
>> +
>> +/*
>> + * merge_callee_effects() does not carry might_unwind to a subprog that calls
>> + * through a pointer it was handed. So if any subprog a callx can reach may
>> + * unwind, mark every subprog with a callx, and its callers.
>> + *
>> + * This is conservative: a subprog with a callx is marked even when nothing it
>> + * calls unwinds. The main walk knows each callx's target and could tell those
>> + * apart.
>> + */
>> +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;
> [Severity: High]
> Can addr_taken_subprog_might_unwind() return false prematurely here when an
> address-taken subprogram calls back into an unwinding subprogram whose
> entry instruction is still in the DISCOVERED state during the CFG walk?
>
> In bpf_check_cfg(), visit_insn() traverses insn_func_ptrs() (for example,
> an ld_imm64 referencing a read-only map such as .rodata that holds function
> pointers) and bpf_pseudo_func() as CFG edges. Because neither
> sort_subprogs_topo() nor check_max_stack_depth_subprog() rejects cycles
> through insn_func_ptrs() or async callback bpf_pseudo_func() edges, a
> subprogram cb whose address is in .rodata or taken via bpf_pseudo_func()
> can call a subprogram G via BPF_PSEUDO_CALL while G references .rodata or
> &cb on one branch and calls bpf_unwind() on a second branch.
>
> If bpf_check_cfg() explores G before cb:
>
> 1. G's entry instruction is marked DISCOVERED and pushes branch 1 onto
>     insn_stack while G->might_unwind is still false.
> 2. Branch 1's insn_func_ptrs() or bpf_pseudo_func() edge pushes cb onto
>     insn_stack.
> 3. cb's BPF_PSEUDO_CALL to G runs merge_callee_effects() while
>     G->might_unwind is still false, and push_insn() sees G's entry is
>     already DISCOVERED and returns DONE_EXPLORING, so cb finishes exploration
>     with cb->might_unwind == false.
> 4. bpf_check_cfg() later explores branch 2 of G and sets
>     G->might_unwind = true.
>
> When mark_callx_might_unwind() then runs, addr_taken_subprog_might_unwind()
> sees cb->might_unwind == false and returns early before the fixed-point loop
> below can propagate G->might_unwind to cb and its callers.
>
> As a result, a global subprogram calling cb (via callx or BPF_PSEUDO_CALL)
> will have might_unwind == false, causing process_global_call_unwind() to
> skip verifying the unwind path at the caller and allowing acquired
> references or locks to leak on unwind.

I think you are right, inside mark_callx_might_unwind(), before
addr_taken_subprog_might_unwind(), we indeed need to do one pass
for the below do loop as it may actually mark might_unwind as true.

>
>> +
>> +	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;
>> +			if (bpf_pseudo_call(&insns[i]) || bpf_pseudo_func(&insns[i])) {
>> +				callee = bpf_find_containing_subprog(env, i + insns[i].imm + 1);
>> +				if (!callee->might_unwind)
>> +					continue;
>> +				caller->might_unwind = true;
>> +				changed = true;
>> +				continue;
>> +			}
>> +			ptrs = insn_func_ptrs(env, i, &cnt);
>> +			for (j = 0; j < cnt; j++) {
>> +				callee = bpf_find_containing_subprog(env, ptrs[j].xlated_off);
>> +				if (!callee->might_unwind)
>> +					continue;
>> +				caller->might_unwind = true;
>> +				changed = true;
>> +			}
>> +		}
>> +	} while (changed);
>> +}
> [ ... ]
>