Re: [PATCH bpf-next 09/26] bpf: Check writes to the stack against bpf_may_write_mask()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] bpf: false verifier_bug_if() in check_may_write_mask() on
  widened size_reg
- [Medium] bpf: missing ancestor frames in arg_track_deref() on
  ARG_IMPRECISE fill
- [Low] selftests/bpf: store_to_one_of_two_buffers never tests
  !fixed_off path
--

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

bpf: Check writes to the stack against bpf_may_write_mask()

This commit adds check_may_write_mask() to verify during the main verification
pass that stack writes in check_stack_write() and
check_stack_range_initialized() fall within bpf_may_write_mask(), along with a
selftest for variable-offset stack stores.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5bffc1e288f60..ae3b751b99a1c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4327,6 +4327,31 @@ static void invalidate_stack_slices(struct bpf_verifier_env *env, u32 frameno,
>  	}));
>  }
>  
> +/*
> + * Scalar evolution takes the may-defs of stack slots from bpf_may_write_mask(): a slot that is not
> + * in the mask holds the same value before and after the insn. The iteration count of a loop depends
> + * on that, and nothing detects a wrong one later. Here an insn, a helper or a kfunc writes to
> + * [min_off, max_off) of frame @frameno. For a frame of a caller the insn is the callsite. Without
> + * @fixed_off the range can be wider than the mask: two offsets become a range at a join point. Then
> + * the write is somewhere in the range.
> + */
> +static int check_may_write_mask(struct bpf_verifier_env *env, u32 frameno, int min_off, int max_off,
> +				bool fixed_off)
> +{
> +	struct bpf_verifier_state *st = env->cur_state;
> +	int idx = frameno == st->curframe ? env->insn_idx : st->frame[frameno + 1]->callsite;
> +	int spi, first = bpf_get_spi(max_off - 1), last = bpf_get_spi(min_off);
> +	const unsigned long *mask = bpf_may_write_mask(env, idx);
> +
> +	spi = fixed_off ? find_next_zero_bit(mask, last + 1, first) : last + 1;
> +	if (!fixed_off && find_next_bit(mask, last + 1, first) > last)
> +		spi = first;
> +	if (verifier_bug_if(spi <= last, env, "insn %d writes to fp%d outside of may_write_mask",
> +			    idx, -(spi + 1) * BPF_REG_SIZE))
> +		return -EFAULT;
> +	return 0;
> +}

[Severity: Medium]
Can check_may_write_mask() hit a false verifier_bug_if() when a subprogram
fills a caller stack pointer from an ARG_IMPRECISE pointer and writes through
it?

A subprogram at frame F > 0 can spill a stack pointer belonging to an older
ancestor frame (such as frame 0) into frame F's own stack.  When
arg_track_deref() dereferences a pointer with a single known ancestor frame
and an unknown offset, fill_from_stack() sets .mask = GENMASK(ptr->frame, 0)
to cover any older frame pointer spilled there.  However, for an
ARG_IMPRECISE pointer, arg_track_deref() returns *ptr unchanged:

kernel/bpf/liveness.c:arg_track_deref() {
	...
	if (ptr->frame >= 0 && ptr->frame < depth) {
		struct spill_snapshot *parent = env->callsite_at_stack[callsites[ptr->frame]];

		return fill_from_stack(insn, at_out, reg, parent->at, parent->slots, ptr->frame);
	}
	if (ptr->frame == ARG_IMPRECISE && !(ptr->mask & BIT(depth)) && ptr->mask)
		return *ptr;
}

If frame 1 spills a pointer to frame 0's stack into frame 1's stack, a
pointer to frame 1's stack is joined with a pointer to frame 2's stack
(producing ARG_IMPRECISE with mask = BIT(1) | BIT(2)), and frame 3 loads the
spilled frame 0 pointer from that joined pointer and writes through it,
record_imprecise() only marks FM_MAY_WRITE on frames 1 and 2.  Frame 0's
may_write_mask remains clear at its callsite, so check_may_write_mask()
triggers verifier_bug_if() and rejects the program with -EFAULT.

Should arg_track_deref() widen ptr->mask to GENMASK(fls(ptr->mask) - 1, 0) in
that case?

[ ... ]

> @@ -8032,6 +8061,13 @@ static int check_stack_range_initialized(
>  		max_off = reg_smax(reg) + off;
>  	}
>  
> +	if (clobber && access_size) {
> +		err = check_may_write_mask(env, state->frameno, min_off, max_off + access_size,
> +					   tnum_is_const(reg->var_off));
> +		if (err)
> +			return err;
> +	}

[Severity: Medium]
Can this check_may_write_mask() call trigger a false verifier_bug_if() when
the helper or kfunc size register is constant in const_reg_mask but widened
in do_check()?

When aux->const_reg_mask has the size register marked constant,
bpf_helper_stack_access_bytes() and bpf_kfunc_stack_access_bytes() bound
bpf_may_write_mask() using aux->const_reg_vals[size_reg] (for example, 8
bytes at fp-16).  During do_check(), sanitize_speculative_path() can call
mark_reg_unknown() on size_reg at a conditional jump on a speculative path,
and size_reg can then be bounded to a larger range such as [0, 16] before the
call.  check_mem_size_reg() then calls check_stack_range_initialized() with
access_size = reg_umax(size_reg) (16) and tnum_is_const(reg->var_off) true, so
check_may_write_mask() requires both fp-16 and fp-8 to be set in
may_write_mask and hits verifier_bug_if() on fp-8.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_raw_stack.c b/tools/testing/selftests/bpf/progs/verifier_raw_stack.c
> index 0fe631411b9cc..d0975a28a2435 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_raw_stack.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_raw_stack.c
> @@ -125,6 +125,41 @@ __naked void stack_skb_load_bytes_init(void)
>  	: __clobber_all);
>  }
>  
> +/* The pointer is fp-128 or fp-96. At the loop header the two offsets are joined to a range. */
> +SEC("tc")
> +__description("raw_stack: store to one of two buffers")
> +__success __retval(0)
> +__naked void store_to_one_of_two_buffers(void)
> +{
> +	asm volatile ("					\
> +	call %[bpf_get_prandom_u32];			\
> +	r8 = r0;					\
> +	r1 = 0;						\
> +	*(u64*)(r10 - 144) = r1;			\
> +	*(u64*)(r10 - 120) = r1;			\
> +	*(u64*)(r10 - 88) = r1;				\
> +	r3 = r10;					\
> +	r3 += -128;					\
> +	*(u64*)(r10 - 136) = r3;			\
> +l0_%=:	r8 *= 8;					\
> +	if w8 s> 100 goto l1_%=;			\
> +	r3 = r10;					\
> +	r3 += -96;					\
> +	*(u64*)(r10 - 136) = r3;			\
> +	r4 = *(u64*)(r10 - 144);			\
> +	r4 += 1;					\
> +	*(u64*)(r10 - 144) = r4;			\
> +	if r4 < 3 goto l0_%=;				\
> +l1_%=:	r3 = *(u64*)(r10 - 136);			\
> +	r1 = 7;						\
> +	*(u64*)(r3 + 8) = r1;				\
> +	r0 = 0;						\
> +	exit;						\
> +"	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}

[Severity: Low]
Does store_to_one_of_two_buffers() actually reach the !fixed_off branch in
check_may_write_mask()?

Because the loop has max_header_count = 3 and r8 *= 8 is not linear,
bpf_flow_loop_enter() sets entry->apart = true and walks all 3 iterations
separately without widening at the loop header; moreover, join_reg() rejects
joining two PTR_TO_STACK registers with different offsets (fp-128 and fp-96).
As a result, r3 at *(u64 *)(r3 + 8) = r1 always has a constant var_off and
fixed_off is always true.

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