Re: [PATCH bpf-next v4 09/46] bpf: Find the registers that hold a number or a pointer to arena

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: missing propagation of loads_pkt_end to callers in
  bpf_flow_compute_kinds()
--

commit 56842cec67f7566ca6cea05c44634e53a6e064fc
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Find the registers that hold a number or a pointer to arena

This commit adds bpf_flow_compute_kinds() to track which registers and stack
slots hold plain scalars, map value pointers, or the context before each
instruction, and records subprograms that load the packet end pointer in
sub->loads_pkt_end.

> diff --git a/kernel/bpf/flow.c b/kernel/bpf/flow.c
> index 0f5acdf1cb946..f039cc9498542 100644
> --- a/kernel/bpf/flow.c
> +++ b/kernel/bpf/flow.c
> @@ -514,3 +514,317 @@ int bpf_flow_place_guards(struct bpf_verifier_env *env)

[ ... ]

> +/* What @insn loads through the register @base */
> +static enum flow_kind flow_loaded_kind(struct bpf_verifier_env *env, const struct flow_kinds *k,
> +				       const struct bpf_insn *insn, u8 base)
> +{
> +	struct bpf_insn_access_aux info = { .reg_type = SCALAR_VALUE };
> +	int size = bpf_size_to_bytes(BPF_SIZE(insn->code));
> +	bool tstamp = env->prog->tstamp_type_access, valid;
> +
> +	/* a number that is good for a load is an address in arena */
> +	if (test_bit(base, k->is[FLOW_IS_VALUE]))
> +		return FLOW_IS_PLAIN;
> +	if (size == BPF_REG_SIZE && !test_bit(base, k->is[FLOW_IS_CTX]))
> +		return FLOW_IS_OTHER;
> +	valid = env->ops->is_valid_access &&
> +		env->ops->is_valid_access(insn->off, size, BPF_READ, env->prog, &info);
> +	/* it notes what the prog reads, and this is a question */
> +	env->prog->tstamp_type_access = tstamp;
> +	if (valid && info.reg_type == PTR_TO_PACKET_END)
> +		bpf_find_containing_subprog(env, insn - env->prog->insnsi)->loads_pkt_end = true;

[ ... ]

> +	} else if (class == BPF_LD || (class == BPF_JMP && op == BPF_CALL)) {
> +		s = bpf_pseudo_call(insn) ? bpf_find_subprog(env, idx + insn->imm + 1) : -1;
> +		if (s >= 0 && !bpf_subprog_is_global(env, s)) {
> +			lost = flow_kinds_enter(&kinds[sub[s].start], k);
> +			regs = ret[s];
> +		} else if (bpf_is_callx(insn)) {
> +			regs = 0;
> +		}

[Severity: Medium]
As reported by bot+bpf-ci@kernel.org on v3 [1], does loads_pkt_end need to
be propagated from a static callee to its caller?

flow_loaded_kind() sets loads_pkt_end only on the subprog that directly
contains the ctx->data_end load, and neither flow_kinds_step() nor
merge_callee_effects() passes it up the call graph.  In commit "bpf: Verify
static subprogs on their own", func_is_on_its_own() and arg_needs_caller()
check sub->loads_pkt_end on the immediate callee to decide whether a static
subprog that receives a packet pointer must be verified in its caller's
frames.

If a caller passes ctx and a packet pointer to a static subprog that
delegates the ctx->data_end load and bounds check to a nested static
subprog, the outer subprog has loads_pkt_end == false and is verified
without its caller's frame.  find_good_pkt_pointers() in the inner subprog
then never widens the packet pointer in the outer caller's frame, causing
valid packet accesses after the call to be rejected.

[1] https://lore.kernel.org/r/c5cd652a5014ef6157278403838481eb2f4a8c5add5e6de5a2837b3121f30074@mail.kernel.org

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