Re: [PATCH bpf 1/2] bpf: Fix a kernel verifier crash in stacksafe()

[Yonghong Song <yonghong.song@linux.dev>]

On 8/12/24 10:50 AM, Alexei Starovoitov wrote:
> On Mon, Aug 12, 2024 at 10:47 AM Eduard Zingerman <eddyz87@gmail.com> 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?