Re: [PATCH bpf-next v2 22/43] bpf: add bpf_reg_union()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: division by zero in bpf_reg_union() due to step=0
- [Medium] bpf: bpf_reg_union loses alignment precision by missing optimal common congruence
--

commit f2da61545238e5504547c2ded4955ae49831e34d
Author: Eduard Zingerman <eddyz87@gmail.com>

bpf: add bpf_reg_union()

This commit adds a utility function bpf_reg_union() to merge the scalar values
of two registers. It combines the circular 32-bit and 64-bit bounds, tnums, and
base/step equations, synchronizing the result.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5fd6376da1710..28c16e35b1f36 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17164,6 +17165,33 @@ int bpf_set_reg_range(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
>  	return reg_bounds_sanity_check(env, reg, "bpf_set_reg_range");
>  }
>  
> +/* acc := acc U src, matching types only. Caller must clear acc's scalar ID. */
> +int bpf_reg_union(struct bpf_verifier_env *env, struct bpf_reg_state *acc,
> +		  const struct bpf_reg_state *src)
> +{
> +	u16 base, step;
> +
> +	if (acc->type != src->type) {
> +		verifier_bug(env, "union of registers with different types");
> +		return -EFAULT;
> +	}
> +	acc->r64 = cnum64_union(acc->r64, src->r64);
> +	acc->r32 = cnum32_union(acc->r32, src->r32);
> +	acc->var_off = tnum_union(acc->var_off, src->var_off);
> +
> +	/* Retain a common congruence if the bases agree modulo the gcd. */
> +	step = gcd(acc->step, src->step);
> +	base = acc->base % step;

[Severity: Critical]
Does this code risk a division by zero exception and subsequent kernel panic?

BPF programs can create ANY nodes in Scalar Evolution (SCEV) loops. If a loop
combines two loop-invariant pointers (e.g., PTR_TO_MAP_VALUE from different
map lookups) in a phi-node, they are tracked by SCEV (since
is_widenable_reg_type() permits pointers) and passed to bpf_reg_union().

Because pointers are initialized via kzalloc or mark_reg_known_zero(), their
step field is 0. Calling gcd(0, 0) returns 0, leading to a modulo by zero on
this line.

> +	if (base != src->base % step) {
> +		reg_step_reset(acc);

[Severity: Medium]
Does this condition needlessly discard linear alignment information for valid
congruences?

If bpf_reg_union() merges registers with steps 15/base 10 and step 15/base 5,
it evaluates gcd(15,15)=15, sees 10!=5, and resets the step to 1.

The optimal step would be gcd(gcd(acc->step, src->step), abs(acc->base -
src->base)), which would correctly yield step 5/base 0. This unnecessary
precision loss might cause the verifier to incorrectly reject valid programs
due to a perceived loss of alignment constraints.

> +	} else {
> +		acc->base = base;
> +		acc->step = step;
> +	}
> +	reg_bounds_sync(acc);
> +	return reg_bounds_sanity_check(env, acc, "bpf_reg_union");
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004-scev-minimal-rebase-v2-0-f6254e29fdf2@gmail.com?part=22