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

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2024-08-12 at 10:50 -0700, 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?

It does not seem correct to swap the order for these two checks:

		if (exact != NOT_EXACT && i < cur->allocated_stack &&
		    old->stack[spi].slot_type[i % BPF_REG_SIZE] !=
		    cur->stack[spi].slot_type[i % BPF_REG_SIZE])
			return false;

		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 we do, 'slot_type' won't be checked for 'cur' when 'old' register is not marked live.