On 8/12/24 10:50 AM, Alexei Starovoitov wrote:
On Mon, Aug 12, 2024 at 10:47 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
On Mon, 2024-08-12 at 10:44 -0700, Alexei Starovoitov wrote:
[...]
Should we move the check up instead?
if (i >= cur->allocated_stack)
return false;
Checking it twice looks odd.
A few checks before that, namely:
if (!(old->stack[spi].spilled_ptr.live & REG_LIVE_READ)
&& exact == NOT_EXACT) {
i += BPF_REG_SIZE - 1;
/* explored state didn't use this */
continue;
}
if (old->stack[spi].slot_type[i % BPF_REG_SIZE] == STACK_INVALID)
continue;
if (env->allow_uninit_stack &&
old->stack[spi].slot_type[i % BPF_REG_SIZE] == STACK_MISC)
continue;
Should be done regardless cur->allocated_stack.
Right, but then let's sink old->slot_type != cur->slot_type down?
We could do the following to avoid double comparison: diff --git
a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index
df3be12096cf..1906798f1a3d 100644 --- a/kernel/bpf/verifier.c +++
b/kernel/bpf/verifier.c @@ -17338,10 +17338,13 @@ static bool
stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old, */
for (i = 0; i < old->allocated_stack; i++) { struct bpf_reg_state
*old_reg, *cur_reg; + bool cur_exceed_bound; spi = i / BPF_REG_SIZE; -
if (exact != NOT_EXACT && + cur_exceed_bound = i >=
cur->allocated_stack; + + if (exact != NOT_EXACT && !cur_exceed_bound &&
old->stack[spi].slot_type[i % BPF_REG_SIZE] !=
cur->stack[spi].slot_type[i % BPF_REG_SIZE]) return false; @@ -17363,7
+17366,7 @@ static bool stacksafe(struct bpf_verifier_env *env, struct
bpf_func_state *old, /* explored stack has more populated slots than
current stack * and these slots were used */ - if (i >=
cur->allocated_stack) + if (cur_exceed_bound) return false; /* 64-bit
scalar spill vs all slots MISC and vice versa. WDYT?
