On Mon, 2026-07-13 at 21:22 +0000, Eduard Zingerman wrote:
> Sorry, could you please explain again what's wrong with the original check?
> Here it is:
>
> if (bpf_is_spilled_reg(&state->stack[spi]) &&
> (state->stack[spi].spilled_ptr.type == SCALAR_VALUE ||
> env->allow_ptr_leaks)) {
> ... accepted ...
> }
>
> And it says: if there is a register spill at 'spi':
> - always allow access if the spilled register is scalar
> - when allow_ptr_leaks is true, allow access when spilled
> register is either scalar or pointer.
>
> The goal here is to avoid leaking pointers, and pointers are always 8-bytes.
You're right that this block is a pointer-leak check, and a scalar spill
can't leak a pointer -- so the fix isn't justified on those grounds. The
invariant it targets is the other one this function enforces: rejecting
reads of uninitialized stack when !allow_uninit_stack.
The block just above already gates that:
/* verifier.c:6780 */
if ((*stype == STACK_ZERO) ||
(*stype == STACK_INVALID && env->allow_uninit_stack)) { ... goto mark; }
For a CAP_BPF-only load allow_uninit_stack is false, so a STACK_INVALID
byte should reach the -EACCES below. allow_ptr_leaks and allow_uninit_stack
are both keyed on CAP_PERFMON today but are separate flags; reads-of-invalid
is independent of pointer leaks. The existing fix for the related
producer-side bug, 69772f509e08 ("bpf: Don't mark STACK_INVALID as
STACK_MISC in mark_stack_slot_misc"), frames it the same way:
"we should not upgrade STACK_INVALID to STACK_MISC when allow_ptr_leaks
is false, since invalid contents shouldn't be read unless the program
has the relevant capabilities."
After a 1-byte scalar spill to fp-8, save_register_state() marks only the
covered byte STACK_SPILL; the sibling fp-7 goes to mark_stack_slot_misc(),
which (verifier.c:1234) won't promote STACK_INVALID. So slot_type[7]
(fp-8) is STACK_SPILL and slot_type[6] (fp-7) is STACK_INVALID. When the
helper walk reaches fp-7, the uninit gate above fails correctly, but the
spill block accepts it anyway -- bpf_is_spilled_reg() tests
slot_type[BPF_REG_SIZE-1], the fp-8 sibling, not the fp-7 byte being checked.
The patch makes the fast path cover only bytes it actually marked, so the
invalid sibling falls through to the -EACCES the upper gate chose for it.
It's the consumer-side counterpart to 69772f509e08 (producer side) and its
direct-read selftest f513c3635078: the direct read was already covered, the
helper-argument path was not.
Runtime: for a CAP_BPF-only load, passing fp-7 as the value to
bpf_map_update_elem() after the spill to fp-8 exports a byte the program
never wrote. A direct read of fp-7 rejects; only the helper path accepts.
The commit message should say this rather than just restating the mechanism
-- I'll send a v2. Happy to move the fix elsewhere if you'd prefer.
