Re: [PATCH bpf-next v2 09/35] 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 3 potential issue(s) to consider:
- [Medium] bpf: misclassified 32-bit context loads in flow_loaded_kind()
- [Medium] bpf: misclassified void * special kfuncs in flow_returned_kind()
- [Medium] bpf: stack pointer spill via BPF_STORE_REL lost in flow_kinds_step()
--

commit d616e924eec822ed1af91bad97267316408cf00d
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() in kernel/bpf/flow.c to perform a
forward dataflow pass before the main verification pass, identifying which
registers and stack slots hold plain scalars/arena pointers, scalar-only map
value pointers, or context pointers on every path reaching each instruction.

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

[ ... ]

> +/* What holds on both ways to an insn. Returns true if that is less than before. */
> +static bool flow_kinds_meet(struct flow_kinds *to, const struct flow_kinds *from)
> +{
> +	unsigned long lost = 0;
> +	int i, j;
> +
> +	for (i = 0; i < FLOW_KINDS; i++) {
> +		for (j = 0; j < ARRAY_SIZE(to->is[i]); j++) {
> +			lost |= to->is[i][j] & ~from->is[i][j];
> +			to->is[i][j] &= from->is[i][j];
> +		}
> +	}
> +	return lost;
> +}

[ ... ]

> +/* 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;
> +	/* only the context has pointers that are shorter than a register */
> +	if (!test_bit(base, k->is[FLOW_IS_CTX]))
> +		return size < BPF_REG_SIZE ? FLOW_IS_PLAIN : FLOW_IS_OTHER;

[Severity: Medium]
Can flow_loaded_kind() in kernel/bpf/flow.c misclassify 32-bit context
pointer loads (such as skb->data or xdp_md->data) as FLOW_IS_PLAIN when a
register holds the context pointer on one incoming path and a non-context
value on another?

In bpf_flow_compute_kinds(), all bits in struct flow_kinds start as 1 and are
intersected across predecessors in flow_kinds_meet() via bitwise AND:

	to->is[i][j] &= from->is[i][j];

As a result, test_bit(base, k->is[FLOW_IS_CTX]) is a must-analysis bit that
is 1 only when base holds the unmodified context pointer on every path
reaching the instruction, and becomes 0 whenever base may hold a non-context
value on at least one path (for example, after a branch or across static
subprogram callsites).

However, flow_loaded_kind() checks !test_bit(base, k->is[FLOW_IS_CTX]) and
returns FLOW_IS_PLAIN for size < BPF_REG_SIZE, treating a cleared bit in
FLOW_IS_CTX as meaning that base cannot be a context pointer on any path.
When a 32-bit context pointer field is loaded through such a register,
flow_loaded_kind() marks the resulting packet pointer as FLOW_IS_PLAIN,
allowing the main pass to join states with distinct packet offsets and reject
valid programs. In addition, when k->is[FLOW_IS_CTX] drops from 1 to 0 during
the fixpoint loop, flow_loaded_kind() transitions its result for sub-8-byte
non-scalar context loads from FLOW_IS_OTHER (0) back up to FLOW_IS_PLAIN (1),
making the transfer function non-monotone.

> +	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;
> +	return valid && info.reg_type == SCALAR_VALUE ? FLOW_IS_PLAIN : FLOW_IS_OTHER;
> +}
> +
> +/* What a helper or a kfunc returns */
> +static enum flow_kind flow_returned_kind(struct bpf_verifier_env *env, int idx)
> +{
> +	struct bpf_insn_aux_data *aux = &env->insn_aux_data[idx];
> +	struct bpf_insn *insn = &env->prog->insnsi[idx];
> +	const struct bpf_func_proto *fn;
> +	struct bpf_call_arg_meta meta;
> +	const struct btf_type *t;
> +
> +	if (bpf_pseudo_kfunc_call(insn)) {
> +		if (bpf_fetch_kfunc_arg_meta(env, insn->imm, insn->off, &meta) < 0)
> +			return FLOW_IS_OTHER;
> +		t = btf_type_skip_modifiers(meta.btf, meta.func_proto->type, NULL);
> +		if (!btf_type_is_ptr(t))
> +			return FLOW_IS_PLAIN;
> +		/* void * is a number, unless the verifier knows the kfunc. Such kfunc may fail. */
> +		t = btf_type_skip_modifiers(meta.btf, t->type, NULL);
> +		return btf_type_is_void(t) && !(meta.kfunc_flags & KF_RET_NULL) ? FLOW_IS_PLAIN :
> +										  FLOW_IS_OTHER;

[Severity: Medium]
Does flow_returned_kind() in kernel/bpf/flow.c misclassify the return value
of bpf_cast_to_kern_ctx() and bpf_rdonly_cast() as FLOW_IS_PLAIN?

Both kfuncs have a void * return type in BTF, but neither has KF_RET_NULL set
in common_btf_ids in kernel/bpf/helpers.c:

	BTF_ID_FLAGS(func, bpf_cast_to_kern_ctx, KF_FASTCALL)
	BTF_ID_FLAGS(func, bpf_rdonly_cast, KF_FASTCALL | KF_PERFMON)

In kernel/bpf/verifier.c, check_special_kfunc() sets R0 for these calls to
PTR_TO_BTF_ID (or PTR_TO_MEM):

	} else if (is_kfunc_call(meta, special_kfunc_list[KF_bpf_cast_to_kern_ctx])) {
		mark_reg_known_zero(env, regs, BPF_REG_0);
		regs[BPF_REG_0].type = PTR_TO_BTF_ID | PTR_TRUSTED;
		regs[BPF_REG_0].btf = desc_btf;
		regs[BPF_REG_0].btf_id = meta->ret_btf_id;
	} else if (is_kfunc_call(meta, special_kfunc_list[KF_bpf_rdonly_cast])) {

Because KF_RET_NULL is not set on either kfunc, flow_returned_kind() returns
FLOW_IS_PLAIN, which sets both FLOW_IS_PLAIN and FLOW_IS_VALUE on R0 and
causes the returned pointer and any pointers loaded through it (via
flow_loaded_kind() checking FLOW_IS_VALUE) to be treated as FLOW_IS_PLAIN,
allowing states with different constant offsets to be joined.

> +	}

[ ... ]

> +static bool flow_kinds_step(struct bpf_verifier_env *env, struct flow_kinds *kinds, u16 *ret,
> +			    struct flow_kinds *k, int idx)
> +{
> +	struct bpf_subprog_info *sub = env->subprog_info;
> +	struct bpf_insn *insn = &env->prog->insnsi[idx];
> +	u8 class = BPF_CLASS(insn->code), op = BPF_OP(insn->code);
> +	u8 dst = insn->dst_reg, src = insn->src_reg;
> +	bool wide = BPF_SIZE(insn->code) == BPF_DW && BPF_MODE(insn->code) == BPF_MEM &&
> +		    !(insn->off % BPF_REG_SIZE);
> +	bool lost = false;
> +	u16 regs = U16_MAX;
> +	int i, s;

[ ... ]

> +	} else if (class == BPF_LDX) {
> +		s = flow_mem_place(insn, src);
> +		if (wide && s >= 0)
> +			flow_kind_copy(k, dst, k, s);
> +		else
> +			flow_kind_set(k, dst, flow_loaded_kind(env, k, insn, src));
> +	} else if (class == BPF_STX || class == BPF_ST) {
> +		i = bpf_atomic_load_reg(insn);
> +		if (i >= 0)
> +			flow_kind_set(k, i, flow_loaded_kind(env, k, insn,
> +						insn->imm == BPF_LOAD_ACQ ? src : dst));
> +		s = flow_mem_place(insn, dst);
> +		if (class == BPF_STX && wide && s >= 0)
> +			flow_kind_copy(k, s, k, src);
> +		else if (s >= 0)
> +			flow_kind_set(k, s, FLOW_IS_PLAIN);

[Severity: Medium]
What happens in flow_kinds_step() in kernel/bpf/flow.c if a 64-bit pointer is
spilled to a stack slot using a store-release instruction (BPF_STX |
BPF_ATOMIC | BPF_DW with imm == BPF_STORE_REL)?

In kernel/bpf/verifier.c, check_atomic_store() handles BPF_STORE_REL by
calling check_store_reg(env, insn, true), which spills a pointer register to
the stack on an aligned 8-byte write.

In flow_kinds_step(), wide requires BPF_MODE(insn->code) == BPF_MEM, which is
false for BPF_ATOMIC instructions. When a 64-bit BPF_STORE_REL writes a
non-arena pointer to a tracked stack slot (s >= 0), wide is false, so
flow_kinds_step() takes the else branch and sets slot s to FLOW_IS_PLAIN
instead of copying the kind from src. A subsequent 64-bit fill from that
stack slot will then classify the reloaded pointer register as FLOW_IS_PLAIN
(and similarly a 64-bit BPF_LOAD_ACQ from a stack slot will not copy the
stack slot kind).

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