remove stack_zero Signed-off-by: Alexei Starovoitov <ast@kernel.org>
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c index 70368fe..07f1abb 100644 --- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c +++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -85,12 +85,9 @@ static bool nfp_bpf_map_update_value_ok(struct bpf_verifier_env *env) const struct bpf_reg_state *reg1 = cur_regs(env) + BPF_REG_1; const struct bpf_reg_state *reg3 = cur_regs(env) + BPF_REG_3; struct bpf_offloaded_map *offmap; - struct bpf_func_state *state; struct nfp_bpf_map *nfp_map; int off, i; - state = env->cur_state->frame[reg3->frameno]; - /* We need to record each time update happens with non-zero words, * in case such word is used in atomic operations. * Implicitly depend on nfp_bpf_stack_arg_ok(reg3) being run before. @@ -101,15 +98,9 @@ static bool nfp_bpf_map_update_value_ok(struct bpf_verifier_env *env) off = reg3->var_off.value; for (i = 0; i < offmap->map.value_size; i++) { - struct bpf_stack_state *stack_entry; - unsigned int soff; - - soff = -(off + i) - 1; - stack_entry = &state->stack[soff / BPF_REG_SIZE]; - if (stack_entry->slot_type[soff % BPF_REG_SIZE] == STACK_ZERO) - continue; - if (nfp_map->use_map[i / 4].type == NFP_MAP_USE_ATOMIC_CNT) { + unsigned int soff = -(off + i) - 1; + pr_vlog(env, "value at offset %d/%d may be non-zero, bpf_map_update_elem() is required to initialize atomic counters to zero to avoid offload endian issues\n", i, soff); return false;
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 090aa26..6028068 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h
@@ -213,7 +213,6 @@ enum bpf_stack_slot_type { STACK_INVALID, /* nothing was stored in this stack slot */ STACK_SPILL, /* register spilled into stack */ STACK_MISC, /* BPF program wrote some data into this slot */ - STACK_ZERO, /* BPF program wrote constant zero */ /* A dynptr is stored in this stack slot. The type of dynptr * is stored in bpf_stack_state->spilled_ptr.dynptr.type */
diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c index 37d72b0..545fe07 100644 --- a/kernel/bpf/log.c +++ b/kernel/bpf/log.c
@@ -539,7 +539,6 @@ static char slot_type_char[] = { [STACK_INVALID] = '?', [STACK_SPILL] = 'r', [STACK_MISC] = 'm', - [STACK_ZERO] = '0', [STACK_DYNPTR] = 'd', [STACK_ITER] = 'i', [STACK_IRQ_FLAG] = 'f' @@ -826,7 +825,6 @@ void print_verifier_state(struct bpf_verifier_env *env, const struct bpf_verifie reg->iter.depth); break; case STACK_MISC: - case STACK_ZERO: default: verbose(env, " fp%d=%s", (-i - 1) * BPF_REG_SIZE, types_buf); break;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index d92cf28..d37893b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c
@@ -1348,7 +1348,6 @@ static bool is_stack_slot_special(const struct bpf_stack_state *stack) return true; case STACK_INVALID: case STACK_MISC: - case STACK_ZERO: return false; default: WARN_ONCE(1, "unknown stack slot type %d\n", type); @@ -1376,9 +1375,7 @@ static bool is_spilled_scalar_reg64(const struct bpf_stack_state *stack) stack->spilled_ptr.type == SCALAR_VALUE; } -/* Mark stack slot as STACK_MISC, unless it is already STACK_INVALID, in which - * case they are equivalent, or it's STACK_ZERO, in which case we preserve - * more precise STACK_ZERO. +/* Mark stack slot as STACK_MISC, unless it is already STACK_INVALID. * Regardless of allow_ptr_leaks setting (i.e., privileged or unprivileged * mode), we won't promote STACK_INVALID to STACK_MISC. In privileged case it is * unnecessary as both are considered equivalent when loading data and pruning, @@ -1387,8 +1384,6 @@ static bool is_spilled_scalar_reg64(const struct bpf_stack_state *stack) */ static void mark_stack_slot_misc(struct bpf_verifier_env *env, u8 *stype) { - if (*stype == STACK_ZERO) - return; if (*stype == STACK_INVALID) return; *stype = STACK_MISC; @@ -5256,7 +5251,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env, for (i = 0; i < size; i++) { u8 type = state->stack[spi].slot_type[i]; - if (type != STACK_MISC && type != STACK_ZERO) { + if (type != STACK_MISC) { sanitize = true; break; } @@ -5325,21 +5320,6 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env, for (i = 0; i < BPF_REG_SIZE; i++) scrub_spilled_slot(&state->stack[spi].slot_type[i]); - /* when we zero initialize stack slots mark them as such */ - if ((reg && register_is_null(reg)) || - (!reg && is_bpf_st_mem(insn) && insn->imm == 0)) { - /* STACK_ZERO case happened because register spill - * wasn't properly aligned at the stack slot boundary, - * so it's not a register spill anymore; force - * originating register to be precise to make - * STACK_ZERO correct for subsequent states - */ - err = mark_chain_precision(env, value_regno); - if (err) - return err; - type = STACK_ZERO; - } - /* Mark slots affected by this stack write. */ for (i = 0; i < size; i++) state->stack[spi].slot_type[(slot - i) % BPF_REG_SIZE] = type; @@ -5377,14 +5357,10 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env, { struct bpf_func_state *cur; /* state of the current function */ int min_off, max_off; - int i, err; + int i, j, err; struct bpf_reg_state *ptr_reg = NULL, *value_reg = NULL; struct bpf_insn *insn = &env->prog->insnsi[insn_idx]; bool writing_zero = false; - /* set if the fact that we're writing a zero is used to let any - * stack slots remain STACK_ZERO - */ - bool zero_used = false; cur = env->cur_state->frame[env->cur_state->curframe]; ptr_reg = &cur->regs[ptr_regno]; @@ -5416,7 +5392,7 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env, stype = &state->stack[spi].slot_type[slot % BPF_REG_SIZE]; mark_stack_slot_scratched(env, spi); - if (!env->allow_ptr_leaks && *stype != STACK_MISC && *stype != STACK_ZERO) { + if (!env->allow_ptr_leaks && *stype != STACK_MISC) { /* Reject the write if range we may write to has not * been initialized beforehand. If we didn't reject * here, the ptr status would be erased below (even @@ -5440,21 +5416,22 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env, is_spilled_scalar_reg(&state->stack[spi])) { struct bpf_reg_state *spill_reg = &state->stack[spi].spilled_ptr; - if (tnum_is_const(spill_reg->var_off) && spill_reg->var_off.value == 0) { - zero_used = true; + if (tnum_is_const(spill_reg->var_off) && spill_reg->var_off.value == 0) continue; - } } /* Erase all other spilled pointers. */ state->stack[spi].spilled_ptr.type = NOT_INIT; + /* Scrub remaining STACK_SPILL bytes in this SPI that are + * outside the write range, to avoid leaving an inconsistent + * state where is_spilled_reg() returns true but spilled_ptr + * is NOT_INIT. + */ + for (j = 0; j < BPF_REG_SIZE; j++) + scrub_spilled_slot(&state->stack[spi].slot_type[j]); /* Update the slot type. */ new_type = STACK_MISC; - if (writing_zero && *stype == STACK_ZERO) { - new_type = STACK_ZERO; - zero_used = true; - } /* If the slot is STACK_INVALID, we check whether it's OK to * pretend that it will be initialized by this write. The slot * might not actually be written to, and so if we mark it as @@ -5470,22 +5447,14 @@ static int check_stack_write_var_off(struct bpf_verifier_env *env, } *stype = new_type; } - if (zero_used) { - /* backtracking doesn't work for STACK_ZERO yet. */ - err = mark_chain_precision(env, value_regno); - if (err) - return err; - } return 0; } /* When register 'dst_regno' is assigned some values from stack[min_off, * max_off), we set the register's type according to the types of the - * respective stack slots. If all the stack values are known to be zeros, then - * so is the destination reg. Otherwise, the register is considered to be - * 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. + * respective stack slots. 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. */ static void mark_reg_stack_read(struct bpf_verifier_env *env, /* func where src register points to */ @@ -5495,27 +5464,13 @@ static void mark_reg_stack_read(struct bpf_verifier_env *env, struct bpf_verifier_state *vstate = env->cur_state; struct bpf_func_state *state = vstate->frame[vstate->curframe]; int i, slot, spi; - u8 *stype; - int zeros = 0; for (i = min_off; i < max_off; i++) { slot = -i - 1; 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 (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]); - } else { - /* have read misc data from the stack */ - mark_reg_unknown(env, state->regs, dst_regno); - } + mark_reg_unknown(env, state->regs, dst_regno); } /* Read the stack at 'off' and put the results into the register indicated by @@ -5587,7 +5542,7 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, if (get_reg_width(reg) > size * BITS_PER_BYTE) state->regs[dst_regno].id = 0; } else { - int spill_cnt = 0, zero_cnt = 0; + int spill_cnt = 0; for (i = 0; i < size; i++) { type = stype[(slot - i) % BPF_REG_SIZE]; @@ -5597,10 +5552,6 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, } if (type == STACK_MISC) continue; - if (type == STACK_ZERO) { - zero_cnt++; - continue; - } if (type == STACK_INVALID && env->allow_uninit_stack) continue; verbose(env, "invalid read from stack off %d+%d size %d\n", @@ -5612,10 +5563,6 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, tnum_is_const(reg->var_off) && reg->var_off.value == 0) { __mark_reg_const_zero(env, &state->regs[dst_regno]); /* this IS register fill, so keep insn_flags */ - } else if (zero_cnt == size) { - /* similarly to mark_reg_stack_read(), preserve zeroes */ - __mark_reg_const_zero(env, &state->regs[dst_regno]); - insn_flags = 0; /* not restoring original register state */ } else { mark_reg_unknown(env, state->regs, dst_regno); insn_flags = 0; /* not restoring original register state */ @@ -5649,8 +5596,6 @@ static int check_stack_read_fixed_off(struct bpf_verifier_env *env, type = stype[(slot - i) % BPF_REG_SIZE]; if (type == STACK_MISC) continue; - if (type == STACK_ZERO) - continue; if (type == STACK_INVALID && env->allow_uninit_stack) continue; verbose(env, "invalid read from stack off %d+%d size %d\n", @@ -8274,8 +8219,7 @@ static int check_stack_range_initialized( stype = &state->stack[spi].slot_type[slot % BPF_REG_SIZE]; if (*stype == STACK_MISC) goto mark; - if ((*stype == STACK_ZERO) || - (*stype == STACK_INVALID && env->allow_uninit_stack)) { + if (*stype == STACK_INVALID && env->allow_uninit_stack) { if (clobber) { /* helper can write anything into the stack */ *stype = STACK_MISC; @@ -9784,7 +9728,6 @@ static int get_constant_map_key(struct bpf_verifier_env *env, struct bpf_reg_state *reg; int slot, spi, off; int spill_size = 0; - int zero_size = 0; int stack_off; int i, err; u8 *stype; @@ -9802,14 +9745,6 @@ static int get_constant_map_key(struct bpf_verifier_env *env, off = slot % BPF_REG_SIZE; stype = state->stack[spi].slot_type; - /* First handle precisely tracked STACK_ZERO */ - for (i = off; i >= 0 && stype[i] == STACK_ZERO; i--) - zero_size++; - if (zero_size >= key_size) { - *value = 0; - return 0; - } - /* Check that stack contains a scalar spill of expected size */ if (!is_spilled_scalar_reg(&state->stack[spi])) return -EOPNOTSUPP; @@ -20030,13 +19965,6 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old, continue; } - /* if old state was safe with misc data in the stack - * it will be safe with zero-initialized stack. - * The opposite is not true - */ - if (old->stack[spi].slot_type[i % BPF_REG_SIZE] == STACK_MISC && - cur->stack[spi].slot_type[i % BPF_REG_SIZE] == STACK_ZERO) - continue; if (old->stack[spi].slot_type[i % BPF_REG_SIZE] != cur->stack[spi].slot_type[i % BPF_REG_SIZE]) /* Ex: old explored (safe) state has STACK_SPILL in @@ -20096,7 +20024,6 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old, return false; break; case STACK_MISC: - case STACK_ZERO: case STACK_INVALID: continue; /* Ensure that new unhandled slot types return false by default */
diff --git a/tools/testing/selftests/bpf/progs/verifier_array_access.c b/tools/testing/selftests/bpf/progs/verifier_array_access.c index 0a187ff..524ae4e 100644 --- a/tools/testing/selftests/bpf/progs/verifier_array_access.c +++ b/tools/testing/selftests/bpf/progs/verifier_array_access.c
@@ -580,12 +580,12 @@ __description("valid map access into an array using natural aligned 32-bit const __success __retval(4) unsigned int an_array_with_a_32bit_constant_0_no_nullness(void) { - /* Unlike the above tests, 32-bit zeroing is precisely tracked even - * if writes are not aligned to BPF_REG_SIZE. This tests that our - * STACK_ZERO handling functions. + /* Use aligned(8) to ensure the compiler spills the constant as + * a full 64-bit STACK_SPILL, enabling constant folding of the + * map lookup key. */ struct test_val *val; - __u32 key = 0; + __u32 __attribute__((aligned(8))) key = 0; val = bpf_map_lookup_elem(&map_array, &key); val->index = offsetof(struct test_val, foo);
diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c index 893d3bb..8a282a5 100644 --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
@@ -464,28 +464,28 @@ l0_%=: r1 >>= 16; \ SEC("raw_tp") __log_level(2) __success -__msg("fp-8=0m??scalar()") -__msg("fp-16=00mm??scalar()") -__msg("fp-24=00mm???scalar()") +__msg("fp-8=mm??scalar()") +__msg("fp-16=mmmm??scalar()") +__msg("fp-24=mmmm???scalar()") __naked void spill_subregs_preserve_stack_zero(void) { asm volatile ( "call %[bpf_get_prandom_u32];" - /* 32-bit subreg spill with ZERO, MISC, and INVALID */ - ".8byte %[fp1_u8_st_zero];" /* ZERO, LLVM-18+: *(u8 *)(r10 -1) = 0; */ + /* 32-bit subreg spill with MISC and INVALID */ + ".8byte %[fp1_u8_st_zero];" /* MISC, LLVM-18+: *(u8 *)(r10 -1) = 0; */ "*(u8 *)(r10 -2) = r0;" /* MISC */ /* fp-3 and fp-4 stay INVALID */ "*(u32 *)(r10 -8) = r0;" - /* 16-bit subreg spill with ZERO, MISC, and INVALID */ - ".8byte %[fp10_u16_st_zero];" /* ZERO, LLVM-18+: *(u16 *)(r10 -10) = 0; */ + /* 16-bit subreg spill with MISC and INVALID */ + ".8byte %[fp10_u16_st_zero];" /* MISC, LLVM-18+: *(u16 *)(r10 -10) = 0; */ "*(u16 *)(r10 -12) = r0;" /* MISC */ /* fp-13 and fp-14 stay INVALID */ "*(u16 *)(r10 -16) = r0;" - /* 8-bit subreg spill with ZERO, MISC, and INVALID */ - ".8byte %[fp18_u16_st_zero];" /* ZERO, LLVM-18+: *(u16 *)(r18 -10) = 0; */ + /* 8-bit subreg spill with MISC and INVALID */ + ".8byte %[fp18_u16_st_zero];" /* MISC, LLVM-18+: *(u16 *)(r18 -10) = 0; */ "*(u16 *)(r10 -20) = r0;" /* MISC */ /* fp-21, fp-22, and fp-23 stay INVALID */ "*(u8 *)(r10 -24) = r0;" @@ -573,7 +573,7 @@ __naked void partial_stack_load_preserves_zeros(void) "r1 += r2;" "*(u8 *)(r1 + 0) = r2;" /* this should be fine */ - /* for completeness, load U64 from STACK_ZERO slot */ + /* for completeness, load U64 from STACK_MISC slot */ "r1 = %[single_byte_buf];" "r2 = *(u64 *)(r10 -8);" "r1 += r2;" @@ -595,13 +595,11 @@ __naked void partial_stack_load_preserves_zeros(void) SEC("raw_tp") __log_level(2) -__success -/* fp-4 is STACK_ZERO */ -__msg("2: (62) *(u32 *)(r10 -4) = 0 ; R10=fp0 fp-8=0000????") -__msg("4: (71) r2 = *(u8 *)(r10 -1) ; R2=0 R10=fp0 fp-8=0000????") -__msg("5: (0f) r1 += r2") -__msg("mark_precise: frame0: last_idx 5 first_idx 0 subseq_idx -1") -__msg("mark_precise: frame0: regs=r2 stack= before 4: (71) r2 = *(u8 *)(r10 -1)") +__failure +/* Without STACK_ZERO, sub-register zero writes are tracked as STACK_MISC. + * Reads back produce unknown scalar, which fails bounds check. + */ +__msg("invalid access to map value") __naked void partial_stack_load_preserves_partial_zeros(void) { asm volatile (