Re: [PATCH bpf-next v2 16/29] bpf: Recompute a pointer after the jump that bounds its index

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] bpf: missing BPF_MAX_VAR_OFF offset bounds check in
  recompute_ptr()
--

commit 5f6f31940968145851bf6579a0206e69bee69836
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Recompute a pointer after the jump that bounds its index

This commit extends linked-register refinement at conditional jumps to
recompute a pointer register p = x + i once a conditional jump bounds the
index i.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 138581a759bbb..c2a3f74d441fe 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19741,6 +19741,43 @@ static struct bpf_reg_state *scev_reg_for_fill(struct bpf_func_state *frame, u32
>  	return &ss->spilled_ptr;
>  }
>  
> +/*
> + *	p = buf + i; if (i > 100) return 0; *p = 0;
> + * p is @dst_reg, x is @ptr_reg, i is @off_reg. Scalar evolution says that p == x + i. So the insns
> + * 'p = x; p += i' are a no-op at run time, and a state that is sound before them is sound after
> + * them: apply their transfer function. Type, object and offset of p are taken from x as it is now;
> + * how p was computed does not matter.
> + * The old p is replaced, not intersected: a state does not distinguish two values of one hash map,
> + * and the intersection of an offset into the one with an offset into the other holds for neither.
> + * It is replaced only if the new range is a subset of the old one. An empty range is a subset too:
> + * the path is infeasible, p is left as it is.
> + */
> +static bool recompute_ptr(struct bpf_verifier_env *env, struct bpf_reg_state *dst_reg,
> +			  const struct bpf_reg_state *ptr_reg, const struct bpf_reg_state *off_reg)
> +{
> +	struct bpf_reg_state sum = *ptr_reg;
> +
> +	/* the cpu can speculate past the jump with an out-of-bounds index */
> +	if (!env->bypass_spec_v1)
> +		return false;
> +	/* 'p += i' assigns a new id to a packet pointer */
> +	if (ptr_reg->type == SCALAR_VALUE || type_is_pkt_pointer(ptr_reg->type) ||
> +	    !bpf_is_widenable_reg_type(ptr_reg))
> +		return false;
> +	__mark_reg32_unbounded(&sum);
> +	scalar_step_add(&sum, ptr_reg, off_reg);
> +	sum.r64 = cnum64_add(ptr_reg->r64, off_reg->r64);
> +	sum.var_off = tnum_add(ptr_reg->var_off, off_reg->var_off);
> +	reg_bounds_sync(&sum);
> +	if (reg_is_empty(&sum) || const_tnum_range_mismatch(&sum) ||
> +	    const_tnum_range_mismatch_32(&sum))
> +		return false;
> +	if (!bpf_reg_union_compatible(dst_reg, &sum) || !cnum64_is_subset(dst_reg->r64, sum.r64))
> +		return false;
> +	*dst_reg = sum;
> +	return true;
> +}

[Severity: High]
Can recompute_ptr() overwrite *dst_reg with a pointer offset outside
(-BPF_MAX_VAR_OFF, BPF_MAX_VAR_OFF)?

When adjust_ptr_min_max_vals() verifies the earlier p = x + i while i is
not yet constant, check_reg_sane_offset_scalar() and
check_reg_sane_offset_ptr() only check known constant offsets and smin,
so dst_reg->r64 can have an smax >= BPF_MAX_VAR_OFF (for example
[-16, 0xfffffff0ULL] on PTR_TO_STACK).  If a subsequent conditional jump
narrows i to the constant 0xfffffff0ULL, cnum64_is_subset(dst_reg->r64,
sum.r64) succeeds and recompute_ptr() sets dst_reg->var_off.value to
0xfffffff0ULL without calling check_reg_sane_offset_scalar() or
check_reg_sane_offset_ptr().

Downstream callers that assume a constant var_off.value fits in 32 bits
then truncate it, allowing out-of-bounds kernel memory access at runtime:

kernel/bpf/verifier.c:stack_slot_obj_get_spi() {
	...
	off = reg->var_off.value;
	if (off >= 0 || off % BPF_REG_SIZE) {
	...
}

Here 0xfffffff0ULL is truncated to int off = -16, validating stack slot
fp-16 while passing r10 + 0xfffffff0ULL to the helper or kfunc.

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