mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: "Emil Tsalapatis" <emil@etsalapatis.com>
To: "Woojin Ji" <random6.xyz@gmail.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"John Fastabend" <john.fastabend@gmail.com>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Martin KaFai Lau" <martin.lau@linux.dev>,
	"Eduard Zingerman" <eddyz87@gmail.com>,
	"Kumar Kartikeya Dwivedi" <memxor@gmail.com>,
	"Song Liu" <song@kernel.org>,
	"Yonghong Song" <yonghong.song@linux.dev>,
	"Jiri Olsa" <jolsa@kernel.org>, "Shuah Khan" <shuah@kernel.org>
Cc: <bpf@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH bpf] bpf: Track spilled zero scalars for var-off stack reads
Date: Thu, 11 Jun 2026 19:43:12 -0400	[thread overview]
Message-ID: <DJ6MABD6AW71.4585Y3MKTIGC@etsalapatis.com> (raw)
In-Reply-To: <20260611-bpf-stack-var-off-zero-v1-v1-1-0ec407376147@gmail.com>

On Thu Jun 11, 2026 at 6:11 AM EDT, Woojin Ji wrote:
> mark_reg_stack_read() currently treats a variable-offset stack read as
> known-zero only when every byte in the read range has STACK_ZERO type.
> This loses precision for stack bytes that are represented as STACK_SPILL
> but come from a spilled scalar const zero.
>
> Fixed-offset stack reads already preserve such partial reads from a
> spilled scalar zero as const zero. Variable-offset stack writes also keep
> a spilled scalar zero intact when a zero write overlaps it. The read side
> is therefore inconsistent and can reject otherwise valid programs: a byte
> read from a spilled zero becomes an unknown u8 and may then make a
> stack access through that value appear out of bounds.
>
> Treat STACK_SPILL bytes backed by a spilled scalar const zero as zero
> bytes in mark_reg_stack_read(). When such a spilled scalar is used to
> prove the destination register is const zero, mark every contributing
> source stack slot precise before state pruning can use an explored
> zero-spill path for a later non-zero spill path.
>
> This has to be done eagerly for variable-offset loads. Fixed-offset stack
> fills can record one source stack slot in the jump history and propagate
> destination precision back to that slot later, but a variable-offset load
> may source bytes from multiple stack slots. Seed precision backtracking
> with every zero-spill slot that contributes to the const-zero
> classification instead.
>
> Add verifier selftests for both sides: accepted programs that read a byte
> through a variable stack offset from spilled zero scalars, including a
> multi-slot range, and a rejected program that would be accepted unsafely
> by a naive implementation that promotes spilled zero bytes to const zero
> without tracking the source spilled slot precisely. Use
> BPF_F_TEST_STATE_FREQ for the rejected test so it does not depend on the
> current verifier checkpoint heuristic thresholds.
>
> Tested:
>   ./test_progs -t verifier_var_off
>
> Assisted-by: opencode:gpt-5.5
> Signed-off-by: Woojin Ji <random6.xyz@gmail.com>

Correctness of the patch aside, is this a problem ever see in real
programs? Unless I'm missing something this is extra complexity in the
verifier to address a theoretical possibility.

> ---
>  kernel/bpf/verifier.c                              | 52 ++++++++++---
>  .../testing/selftests/bpf/progs/verifier_var_off.c | 88 ++++++++++++++++++++++
>  2 files changed, 130 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7fb88e1cd..c4b89fbd9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4058,14 +4058,21 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env,
>   * SCALAR. This function does not deal with register filling; the caller must
>   * ensure that all spilled registers in the stack range have been marked as
>   * read.
> + *
> + * If the const-zero classification depends on spilled scalar zeroes, mark the
> + * contributing stack slots precise so pruning cannot reuse a zero-spill state
> + * for a later path containing a different spilled scalar.
> + *
> + * Returns an error if precision backtracking fails.
>   */
> -static void mark_reg_stack_read(struct bpf_verifier_env *env,
> -				/* func where src register points to */
> -				struct bpf_func_state *ptr_state,
> -				int min_off, int max_off, int dst_regno)
> +static int mark_reg_stack_read(struct bpf_verifier_env *env,
> +			       /* func where src register points to */
> +			       struct bpf_func_state *ptr_state,
> +			       int min_off, int max_off, int dst_regno)
>  {
>  	struct bpf_verifier_state *vstate = env->cur_state;
>  	struct bpf_func_state *state = vstate->frame[vstate->curframe];
> +	u64 zero_spill_mask = 0;
>  	int i, slot, spi;
>  	u8 *stype;
>  	int zeros = 0;
> @@ -4075,19 +4082,38 @@ static void mark_reg_stack_read(struct bpf_verifier_env *env,
>  		spi = slot / BPF_REG_SIZE;
>  		mark_stack_slot_scratched(env, spi);
>  		stype = ptr_state->stack[spi].slot_type;
> -		if (stype[slot % BPF_REG_SIZE] != STACK_ZERO)
> -			break;
> -		zeros++;
> +		if (stype[slot % BPF_REG_SIZE] == STACK_ZERO) {
> +			zeros++;
> +			continue;
> +		}
> +		if (stype[slot % BPF_REG_SIZE] == STACK_SPILL &&
> +		    bpf_is_spilled_scalar_reg(&ptr_state->stack[spi]) &&
> +		    tnum_is_const(ptr_state->stack[spi].spilled_ptr.var_off) &&
> +		    ptr_state->stack[spi].spilled_ptr.var_off.value == 0) {
> +			zero_spill_mask |= 1ull << spi;
> +			zeros++;
> +			continue;
> +		}
> +		break;
>  	}
>  	if (zeros == max_off - min_off) {
>  		/* Any access_size read into register is zero extended,
>  		 * so the whole register == const_zero.
>  		 */
>  		__mark_reg_const_zero(env, &state->regs[dst_regno]);
> +		if (zero_spill_mask) {
> +			for (spi = 0; spi < MAX_BPF_STACK / BPF_REG_SIZE; spi++) {
> +				if (zero_spill_mask & (1ull << spi))
> +					bpf_bt_set_frame_slot(&env->bt, ptr_state->frameno, spi);
> +			}
> +			return mark_chain_precision_batch(env, env->cur_state);
> +		}
>  	} else {
>  		/* have read misc data from the stack */
>  		mark_reg_unknown(env, state->regs, dst_regno);
>  	}
> +
> +	return 0;
>  }
>  
>  /* Read the stack at 'off' and put the results into the register indicated by
> @@ -4109,6 +4135,7 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
>  	int i, slot = -off - 1, spi = slot / BPF_REG_SIZE;
>  	struct bpf_reg_state *reg;
>  	u8 *stype, type;
> +	int err;
>  	int insn_flags = insn_stack_access_flags(reg_state->frameno, spi);
>  
>  	stype = reg_state->stack[spi].slot_type;
> @@ -4235,8 +4262,11 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env,
>  			}
>  			return -EACCES;
>  		}
> -		if (dst_regno >= 0)
> -			mark_reg_stack_read(env, reg_state, off, off + size, dst_regno);
> +		if (dst_regno >= 0) {
> +			err = mark_reg_stack_read(env, reg_state, off, off + size, dst_regno);
> +			if (err)
> +				return err;
> +		}
>  		insn_flags = 0; /* we are not restoring spilled register */
>  	}
>  	if (insn_flags)
> @@ -4291,7 +4321,9 @@ static int check_stack_read_var_off(struct bpf_verifier_env *env,
>  
>  	min_off = reg->smin_value + off;
>  	max_off = reg->smax_value + off;
> -	mark_reg_stack_read(env, ptr_state, min_off, max_off + size, dst_regno);
> +	err = mark_reg_stack_read(env, ptr_state, min_off, max_off + size, dst_regno);
> +	if (err)
> +		return err;
>  	check_fastcall_stack_contract(env, ptr_state, env->insn_idx, min_off);
>  	return 0;
>  }
> diff --git a/tools/testing/selftests/bpf/progs/verifier_var_off.c b/tools/testing/selftests/bpf/progs/verifier_var_off.c
> index f345466bc..2d4878270 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_var_off.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_var_off.c
> @@ -59,6 +59,94 @@ __naked void stack_read_priv_vs_unpriv(void)
>  "	::: __clobber_all);
>  }
>  
> +SEC("cgroup/skb")
> +__description("variable-offset stack read preserves spilled zero")
> +__success
> +__failure_unpriv __msg_unpriv("R2 variable stack access prohibited for !root")
> +__retval(0)
> +__naked void stack_read_var_off_preserves_spilled_zero(void)
> +{
> +	asm volatile (
> +	"r0 = 0; "
> +	" *(u64 *)(r10 - 8) = r0; "
> +	"r2 = *(u32 *)(r1 + 0); "
> +	"r2 &= 7; "
> +	"r2 -= 8; "
> +	"r2 += r10; "
> +	"r3 = *(u8 *)(r2 + 0); "
> +	"r1 = r10; "
> +	"r1 += -1; "
> +	"r1 += r3; "
> +	" *(u8 *)(r1 + 0) = r3; "
> +	"r0 = 0; "
> +	"exit; "
> +	::
> +	: __clobber_all);
> +}
> +
> +SEC("cgroup/skb")
> +__description("variable-offset stack read preserves spilled zero across slots")
> +__success
> +__failure_unpriv __msg_unpriv("R2 variable stack access prohibited for !root")
> +__retval(0)
> +__naked void stack_read_var_off_preserves_spilled_zero_across_slots(void)
> +{
> +	asm volatile (
> +	"r0 = 0; "
> +	" *(u64 *)(r10 - 8) = r0; "
> +	" *(u64 *)(r10 - 16) = r0; "
> +	"r2 = *(u32 *)(r1 + 0); "
> +	"r2 &= 15; "
> +	"r2 -= 16; "
> +	"r2 += r10; "
> +	"r3 = *(u8 *)(r2 + 0); "
> +	"r1 = r10; "
> +	"r1 += -1; "
> +	"r1 += r3; "
> +	" *(u8 *)(r1 + 0) = r3; "
> +	"r0 = 0; "
> +	"exit; "
> +	::
> +	: __clobber_all);
> +}
> +
> +SEC("cgroup/skb")
> +__description("variable-offset stack read tracks spilled zero precisely")
> +__failure
> +__flag(BPF_F_TEST_STATE_FREQ)
> +__msg("invalid variable-offset write to stack R1")
> +__failure_unpriv __msg_unpriv("R2 variable stack access prohibited for !root")
> +__naked void stack_read_var_off_tracks_spilled_zero_precisely(void)
> +{
> +	asm volatile (
> +	"r6 = *(u32 *)(r1 + 0); "
> +	"r6 &= 1; "
> +	"r0 = 0; "
> +	"if r6 != 0 goto "
> +	"+"
> +	"2; "
> +	" *(u64 *)(r10 - 8) = r0; "
> +	"goto "
> +	"+"
> +	"2; "
> +	"r0 = 1; "
> +	" *(u64 *)(r10 - 8) = r0; "
> +	"r0 = 0; "
> +	"r2 = *(u32 *)(r1 + 4); "
> +	"r2 &= 7; "
> +	"r2 -= 8; "
> +	"r2 += r10; "
> +	"r3 = *(u8 *)(r2 + 0); "
> +	"r1 = r10; "
> +	"r1 += -1; "
> +	"r1 += r3; "
> +	" *(u8 *)(r1 + 0) = 0; "
> +	"r0 = 0; "
> +	"exit; "
> +	::
> +	: __clobber_all);
> +}
> +
>  SEC("cgroup/skb")
>  __description("variable-offset stack read, uninitialized")
>  __success
>
> ---
> base-commit: ddd664bbff63e09e7a7f9acae9c43605d4cf185f
> change-id: 20260610-bpf-stack-var-off-zero-v1-34ad1bc3b533
>
> Best regards,
> --  
> Woojin Ji <random6.xyz@gmail.com>


  reply	other threads:[~2026-06-11 23:43 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-11 10:11 Woojin Ji
2026-06-11 23:43 ` Emil Tsalapatis [this message]
2026-06-12 22:27 ` Eduard Zingerman
2026-06-13  0:13 ` Eduard Zingerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DJ6MABD6AW71.4585Y3MKTIGC@etsalapatis.com \
    --to=emil@etsalapatis.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=eddyz87@gmail.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=martin.lau@linux.dev \
    --cc=memxor@gmail.com \
    --cc=random6.xyz@gmail.com \
    --cc=shuah@kernel.org \
    --cc=song@kernel.org \
    --cc=yonghong.song@linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®