Re: [PATCH bpf-next v3 05/13] bpf: 4-byte precise clean_verifier_state

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Verifier bypass leading to out-of-bounds memory access due to precision loss when 8-byte scalar spills are partially read.
--

--- Patch [5]: [PATCH bpf-next v3 05/13] bpf: 4-byte precise clean_verifier_state ---
commit 720b66feb217d6b05897c14d4beceb7ff588cf92
Author: Eduard Zingerman <[email protected]>

bpf: 4-byte precise clean_verifier_state

Migrate clean_verifier_state() and its liveness queries from 8-byte
SPI granularity to 4-byte half-slot granularity.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ee3945a919e3a..0731e99aa5412 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -20075,26 +20074,83 @@ static void clean_func_state(struct bpf_verifier_env *env,
[ ... ]
> +			/*
> +			 * Only destroy spilled_ptr when hi half is dead.
> +			 * If hi half is still live with STACK_SPILL, the
> +			 * spilled_ptr metadata is needed for correct state
> +			 * comparison in stacksafe().
> +			 * is_spilled_reg() is using slot_type[7], but
> +			 * is_spilled_scalar_after() check either slot_type[0] or [4]
> +			 */
> +			if (!hi_live) {
> +				struct bpf_reg_state *spill = &st->stack[i].spilled_ptr;
> +
> +				if (lo_live && stype == STACK_SPILL) {
> +					u8 val = STACK_MISC;
> +
> +					/*
> +					 * 8 byte spill of scalar 0 where half slot is dead
> +					 * should become STACK_ZERO in lo 4 bytes.
> +					 */
> +					if (register_is_null(spill))
> +						val = STACK_ZERO;
> +					for (j = 0; j < 4; j++) {
> +						u8 *t = &st->stack[i].slot_type[j];
> +
> +						if (*t == STACK_SPILL)
> +							*t = val;
> +					}
> +				}
> +				__mark_reg_not_init(env, spill);
> +			}

[Severity: Critical]
Can this lead to a verifier bypass and out-of-bounds access due to precision
loss when 8-byte scalar spills are partially read?

If a code path spills a non-zero precise scalar (e.g., an array index) to the
stack as an 8-byte write, and then reads only the upper 4 bytes, `lo_live`
would be true and `hi_live` would be false.

This code then destroys the `spilled_ptr` via `__mark_reg_not_init()`, which
loses the exact value and precision metadata. It downgrades the live half to
`STACK_MISC`, which represents an unbounded scalar.

If a different path reaches the same point with an out-of-bounds precise
scalar, wouldn't `stacksafe()` compare the new state against the cached state
containing `STACK_MISC`?

Since an imprecise unbounded scalar is evaluated as a superset of any precise
scalar in `regsafe()`, wouldn't the out-of-bounds path be incorrectly pruned,
allowing an out-of-bounds memory access to execute at runtime?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5