Re: [PATCH bpf-next v3 1/2] bpf: Enforce gotox targets against subprog bounds

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/06/28 02:24PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] The patch adds a runtime bounds check to prevent `gotox` targets from jumping outside the current subprogram, but fails to ensure that dynamically observed targets were actually modeled during Control Flow Graph (CFG) construction. This allows an attacker to follow unmodeled control-flow edges and bypass static liveness-based state pruning.
> --
> 
> commit 5e0bdd5b4a7e2ce073913482349669d9387d6a32
> Author: Nuoqi Gui <gnq25@mails.tsinghua.edu.cn>
> 
> bpf: Enforce gotox targets against subprog bounds
> 
> During CFG construction, the verifier records the modeled gotox target set
> in insn_aux_data->jt. This patch adds a check to reject targets outside the
> calling subprogram bounds when check_indirect_jump() follows targets from
> the runtime PTR_TO_INSN register's actual INSN_ARRAY map.
> 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> [ ... ]
> > @@ -17188,6 +17190,23 @@ static int check_indirect_jump(struct bpf_verifier_env *env, struct bpf_insn *in
> >  		return -EINVAL;
> >  	}
> >  
> > +	subprog = bpf_find_containing_subprog(env, env->insn_idx);
> > +	if (verifier_bug_if(!subprog, env,
> > +			    "gotox insn %d is outside subprog bounds\n",
> > +			    env->insn_idx))
> > +		return -EFAULT;
> > +	subprog_start = subprog->start;
> > +	subprog_end = (subprog + 1)->start;
> > +
> > +	for (i = 0; i < n; i++) {
> > +		u32 target = env->gotox_tmp_buf->items[i];
> > +
> > +		if (target < subprog_start || target >= subprog_end) {
> 
> [Severity: Critical]
> This is a pre-existing issue, but does this bounds check alone fully protect
> against unmodeled control-flow edges?
> 
> During CFG construction in kernel/bpf/cfg.c:jt_from_subprog(), if an
> INSN_ARRAY map's minimum offset falls outside the subprogram, the heuristic
> skips the map entirely:
> 
>     if (jt_cur->items[0] >= subprog_start && jt_cur->items[0] < subprog_end) {
> 
> If an attacker creates an INSN_ARRAY map containing both an out-of-bounds
> offset (e.g., 0) and a valid in-bounds target Y, the CFG construction drops
> Y completely because items[0] is less than subprog_start.
> 
> Later at runtime, if the attacker restricts the bounds to index exactly Y,
> this check passes because Y is within subprog_start and subprog_end. The
> verifier then explores the gotox to Y edge that was never modeled in the
> CFG jump tables.

The robot is right here, the check should verify that the map was
actually checked during the config stage. I think the easiest way
to check this is do the same check: verify that the whole map fits
within the subprogram. (An optimization would be to cache this info
somehow.  Every jump table map can only lie within one subprog, so
subprog index cached is enough.)

Could you please address this and add a corresponding selftest?

You also have lost some acks for this patch from v2, please
don't forget to add them the next time.

> Because liveness analysis relies strictly on the statically modeled CFG,
> could traversing this unmodeled edge cause liveness to incorrectly assume
> registers are dead, allowing an unsound state pruning bypass?
> 
> Should this logic verify that the dynamic target is actually present in
> env->insn_aux_data[env->insn_idx].jt, rather than only verifying the target
> is within subprogram boundaries?
> 
> > +			verbose(env, "gotox target %u outside subprog\n", target);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260628-f01-03-gotox-bpf-next-v3-0-b744432e1361@mails.tsinghua.edu.cn?part=1