Re: [PATCH bpf-next v3 04/45] bpf: Add join of registers and stack slots

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: join_reg() zeroes dst->delta under explore_alu_limits, failing regsafe()
--

commit 909a8651c00556d259164c0f04d677d2fd83cfc0
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Add join of registers and stack slots

This commit adds the first half of verifier state joining in
kernel/bpf/join.c to merge scalar registers, pointer variable offsets, IDs,
and stack slots across execution paths, along with linear register tracking
in kernel/bpf/scev.c for loop widening.

> diff --git a/kernel/bpf/join.c b/kernel/bpf/join.c
> new file mode 100644
> index 0000000000000..fe513b7a8e884
> --- /dev/null
> +++ b/kernel/bpf/join.c

[ ... ]

> +static bool join_reg(struct bpf_join_state *js, struct bpf_reg_state *dst,
> +		     const struct bpf_reg_state *ra, const struct bpf_reg_state *rb, int pass)
> +{
> +	if (ra->type == NOT_INIT || rb->type == NOT_INIT) {
> +		if (pass == 2 && ra->type != NOT_INIT)
> +			*dst = *rb;
> +		return true;
> +	}
> +
> +	if (ra->type != rb->type)
> +		return false;
> +
> +	switch (base_type(ra->type)) {
> +	case SCALAR_VALUE:
> +		if (js->env->explore_alu_limits && !reg_value_equal(ra, rb))
> +			return false;
> +		if (pass == 1) {
> +			jid_note(js, ra->id, rb->id, scalar_link_consistent(ra, rb));
> +			return true;
> +		}
> +		join_range(js, dst, ra, rb);
> +		/* a link that exists on one side only is dropped */
> +		if (ra->id && !jid_good(js, ra->id, rb->id)) {
> +			dst->id = 0;
> +			dst->delta = 0;
> +		}
> +		return true;

[Severity: Medium]
As reported by bot+bpf-ci@kernel.org on v2 [1], in join_reg() in
kernel/bpf/join.c, can dropping an unmatched or inconsistent scalar ID link
under js->env->explore_alu_limits cause the resulting state to fail the
subsumption check in regsafe()?

When js->env->explore_alu_limits is true and two SCALAR_VALUE registers ra
and rb have identical values prior to id (so reg_value_equal(ra, rb) is true)
with a non-zero constant delta (ra->delta == rb->delta != 0), pass 2 of
join_reg() zeroes dst->id and dst->delta if jid_good(js, ra->id, rb->id)
returns false (for example, when one side has BPF_ADD_CONST64 and the other
has BPF_ADD_CONST32, or when the base ID is paired with a different partner
in another register).

However, in struct bpf_reg_state in include/linux/bpf_verifier.h, the delta
field is located before id:

struct bpf_reg_state {
	enum bpf_reg_type type;
	s32 delta;
	...
	u32 id;
};

When bpf_state_subsumes() checks the joined state via states_equal() ->
regsafe() in kernel/bpf/states.c, the explore_alu_limits path compares all
fields prior to id with memcmp():

	case SCALAR_VALUE:
		if (env->explore_alu_limits) {
			return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&
			       check_scalar_ids(rold->id, rcur->id, idmap);
		}

Because dst->delta was cleared to 0 while ra->delta and rb->delta are
non-zero, memcmp() returns non-zero and regsafe() returns false, triggering a
verifier bug warning and failing verification with -EFAULT.

When js->env->explore_alu_limits is set and ra->delta != 0, should join_reg()
refuse to join the scalars if jid_good(js, ra->id, rb->id) is false?

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

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