Re: [PATCH bpf-next 12/26] bpf: Refine registers linked by scalar evolution at conditional jumps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] bpf: false verifier_bug_if() in check_special_kfunc() for
  slice_slots
- [Medium] bpf: missing tnum_overlap() check in refine_linked_regs()
--

commit 99b81d82029ad28c10234130c05d5b0dee306874
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Refine registers linked by scalar evolution at conditional jumps

This commit uses scalar evolution to find live registers and stack slots
that hold a tested register plus a constant offset on every path to a
conditional jump, and refines their bounds in the main verifier pass.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 83249ecbdfb1d..d980e2c608050 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -16054,6 +16060,12 @@ static int check_special_kfunc(struct bpf_verifier_env *env, struct bpf_call_arg
>  		regs[BPF_REG_0].buf = meta->slice_buf;
>  		if (meta->slice_buf.size)
>  			env->seen_stack_slice = true;
> +		if (verifier_bug_if(meta->slice_buf.size &&
> +				    !type_is_rdonly_mem(regs[BPF_REG_0].type) &&
> +				    !bpf_scev_in_slice_slots(env, meta->slice_buf.off,
> +							     meta->slice_buf.size),
> +				    env, "slice buffer is not in scev slice_slots"))
> +			return -EFAULT;
>  	} else {
>  		return 0;
>  	}

[Severity: Medium]
Can bpf_scev_in_slice_slots() return false here and trigger a false
verifier_bug_if() when bpf_dynptr_slice_rdwr() is called with a
variable-offset PTR_TO_STACK buffer?

bpf_scev_in_slice_slots() requires every slot in
[meta->slice_buf.off, meta->slice_buf.off + meta->slice_buf.size) to be
set in env->scev->slice_slots, which find_slice_slots() populates from
bpf_may_write_mask():

kernel/bpf/verifier.c:check_kfunc_call() {
    ...
		if (buf->type == PTR_TO_STACK) {
			meta.slice_buf.frameno = buf->frameno;
			meta.slice_buf.off = reg_smin(buf);
			meta.slice_buf.size = reg_smax(buf) - reg_smin(buf) +
					      meta.arg_constant.value;
    ...
}

When arg_track tracks discrete stack offsets (such as fp-128 and fp-96)
that merge into a range in the main pass, record_stack_access() only sets
may_write_mask for those discrete slots, leaving the intermediate gap
slots (fp-120..fp-104) unset; unlike check_may_write_mask() with
fixed_off == false, bpf_scev_in_slice_slots() rejects the gap slots and
fails verification with -EFAULT.

Similarly, if buffer__szk (meta.arg_constant.value) is 0 and buf has
reg_smax(buf) > reg_smin(buf), record_arg_access() returns early on
!info.size without setting any bits in may_write_mask, while
meta.slice_buf.size is still non-zero.

> @@ -19730,6 +19742,82 @@ static void sync_linked_regs(struct bpf_verifier_env *env, struct bpf_verifier_s
[ ... ]
> +static void refine_linked_regs(struct bpf_verifier_env *env, struct bpf_verifier_state *vstate,
> +			       int idx, u32 regno)
> +{
> +	struct bpf_func_state *frame = vstate->frame[vstate->curframe];
> +	struct bpf_reg_state *reg, implied, saved;
> +	u32 pos, linked_regno, fill_off, bits;
> +	s64 delta;
> +
> +	if (frame->regs[regno].type != SCALAR_VALUE)
> +		return;
> +	for (pos = 0;
> +	     bpf_scev_next_linked_reg(env, idx, regno, &pos, &linked_regno, &fill_off, &delta,
> +				      &bits);
> +	     pos++) {
> +		reg = scev_reg_for_fill(frame, linked_regno, fill_off, bits);
> +		if (!reg || reg->type != SCALAR_VALUE)
> +			continue;
> +
> +		implied = frame->regs[regno];
> +		add_scalar_to_reg(&implied, delta);
> +		if (regs_fit_in_bits(reg, &implied, bits))
> +			bits = 64;
> +		if (bits < 32)
> +			continue;
> +		saved = *reg;
> +		regs_refine_cond_op(reg, &implied, BPF_JEQ, bits == 32);
> +		reg_bounds_sync(reg);
> +		if (reg_is_empty(reg) || const_tnum_range_mismatch(reg) ||
> +		    const_tnum_range_mismatch_32(reg)) {
> +			*reg = saved;
> +			continue;
> +		}

[Severity: Medium]
Does refine_linked_regs() need to check tnum_overlap() before calling
regs_refine_cond_op(reg, &implied, BPF_JEQ, bits == 32)?

tnum_intersect() assumes its operands overlap and ORs conflicting known 0
and known 1 bits into known 1 bits rather than producing an empty state.
On an infeasible branch where reg and implied have overlapping cnum
ranges but disjoint var_off tnums (for instance, r1 = (u8)r0 refined to
even values in [0, 4] while r0 is refined to odd values in [1, 5] on the
taken branch of 'if r0 & 1'), regs_refine_cond_op() turns r1 into odd
values in [1, 3] without triggering reg_is_empty() or
const_tnum_range_mismatch*(), corrupting reg instead of restoring saved.

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