Re: [PATCH bpf 1/2] bpf: fork state when comparing sign crossing ranges with zero

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Inconsistent verifier state and bypassed linked register synchronization.
- [Medium] Missing scalar type check applies zero-comparison workaround to pointer offsets.
--

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

bpf: fork state when comparing sign crossing ranges with zero

This commit introduces a workaround for the verifier when evaluating a
comparison of a sign-crossing scalar against zero. It forks the verifier state
to process negative and non-negative branches separately, restoring precision
lost in the cnum-based scalar representation.

Link: https://lore.kernel.org/bpf/96c4a1aa4333d10b882a9b5093d2d982f9f106e3.camel@gmail.com/T/

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c8d980fdd7099..7998e8da5e554 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -15965,6 +15965,73 @@ static void sync_linked_regs(struct bpf_verifier_env *env, struct bpf_verifier_s
> +static int maybe_fork_cmp_with_zero(struct bpf_verifier_env *env,
> +				    struct bpf_insn *insn,
> +				    struct bpf_reg_state *src_reg,
> +				    struct bpf_reg_state *dst_reg)
> +{
[ ... ]
> +	if (!is_reg_const(src_reg, is_jmp32) || reg_const_value(src_reg, is_jmp32) != 0)
> +		return 0;
> +

[Severity: Medium]
Does this code apply the scalar workaround to pointer types?

It appears that dst_reg is not checked for SCALAR_VALUE. If dst_reg is a 
valid pointer with a variable offset (e.g., PTR_TO_MAP_VALUE) that happens 
to cross zero, and the program compares it against zero (if (ptr == 0)), 
the verifier might artificially restrict the pointer's offset and fork the state.

Since valid pointers are not zero, both forks will follow the fall-through 
path, meaning this doesn't create a safety vulnerability but could lead to 
unnecessary state exploration and semantically incorrect behavior. Should we 
verify dst_reg->type == SCALAR_VALUE here?

> +	bool cross_sign32 = is_jmp32 &&
> +		dst_reg->r32.size < S32_MAX &&
> +		cnum32_smin(dst_reg->r32) < 0 && cnum32_smax(dst_reg->r32) > 0;
> +	bool cross_sign64 = !is_jmp32 &&
> +		dst_reg->r64.size < S64_MAX &&
> +		cnum64_smin(dst_reg->r64) < 0 && cnum64_smax(dst_reg->r64) > 0;
> +	if (!cross_sign32 && !cross_sign64)
> +		return 0;
> +
> +	fork = push_stack(env, env->insn_idx, env->insn_idx, cur->speculative);
> +	if (!fork)
> +		return -ENOMEM;
> +
> +	fork_dst_regno = swapped ? insn->src_reg : insn->dst_reg;
> +	fork_dst_reg = &fork->frame[fork->curframe]->regs[fork_dst_regno];
> +	if (is_jmp32) {
> +		cnum32_intersect_with_srange(&dst_reg->r32, S32_MIN, -1);
> +		cnum32_intersect_with_srange(&fork_dst_reg->r32, 0, S32_MAX);
> +	} else {
> +		cnum64_intersect_with_srange(&dst_reg->r64, S64_MIN, -1);
> +		cnum64_intersect_with_srange(&fork_dst_reg->r64, 0, S64_MAX);
> +	}

[Severity: Medium]
Are we leaving dst_reg and fork_dst_reg in an inconsistent state by not
calling reg_bounds_sync()?

Because dst_reg->r32 or r64 are modified directly here without syncing, 
the bounds are narrowed but var_off and the other bounds remain wide.

Additionally, because the dst_reg bounds are forced to be strictly negative 
here, is_branch_taken() later in check_cond_jmp_op() might evaluate 
the branch statically (pred == 0). If it does, check_cond_jmp_op() will 
return early and completely bypass sync_linked_regs() for the current branch.

While this makes the verifier strictly more conservative for the linked
registers (since wider bounds fail faster), it could cause linked registers
to lose precision and lead to potential spurious rejections.

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-cnum-split-at-zero-v1-0-986c03752226@gmail.com?part=1