Re: [syzbot] [bpf?] KASAN: slab-use-after-free Read in do_check

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2025-06-11 at 16:03 +0200, Luis Gerhorst wrote:
> Eduard Zingerman <eddyz87@gmail.com> writes:
> 
> > Accessed memory is freed at an error path in push_stack():
> > 
> >   static struct bpf_verifier_state *push_stack(...)
> >   {
> >   	...
> >   err:
> >   	free_verifier_state(env->cur_state, true); // <-- KASAN points here
> >   	...
> >   }
> > 
> > And is accessed after being freed here:
> > 
> >   static int do_check(struct bpf_verifier_env *env)
> >   {
> >   	...
> > 		err = do_check_insn(env, &do_print_state);
> > KASAN -->	if (state->speculative && error_recoverable_with_nospec(err)) ...
> >   	...
> >   }
> >   
> > [...]
> > 
> > Either 'state = env->cur_state' is needed after 'do_check_insn()' or
> > error path should not free env->cur_state (seems logical).
> 
> Sorry, this was my error from [1]. Thanks for the pointer.
> 
> Yes, I think the former makes sense (with the respective `state &&`
> added to the if).
> 
> The latter might also be possible, but I guess it would require more
> significant changes.

do_check_common() has the following logic:

   out:
         /* check for NULL is necessary, since cur_state can be freed inside                                                                                                                                                                                                                           
          * do_check() under memory pressure.                                                                                                                                                                                                                                                          
          */
         if (env->cur_state) {
                 free_verifier_state(state: env->cur_state, free_self: true);
                 env->cur_state = NULL;
         }
         while (!pop_stack(env, prev_insn_idx: NULL, insn_idx: NULL, pop_log: false));
         if (!ret && pop_log)
                 bpf_vlog_reset(log: &env->log, new_pos: 0);
         free_states(env);
         return ret;

Same cleanup cycles are done in push_stack() and push_async_cb(),
both functions are only reachable from do_check_common() via
do_check() -> do_check_insn().

Hence, I think that cur state should not be freed in push_*()
functions and pop_stack() loop there is not needed.

> state->speculative does not make sense if the error path of push_stack()
> ran. In that case, `state->speculative &&
> error_recoverable_with_nospec(err)` as a whole should already never
> evaluate to true (because all cases where push_stack() fails also return
> a non-recoverable error -ENOMEM/-EFAULT).
> 
> Alternatively to adding `state = env->cur_state` and `state &&`, turning
> the check around would avoid the use-after-free. However, I think your
> idea is better because it is more explicit compared to this:
> 
> 	if (error_recoverable_with_nospec(err) && state->speculative) ...
> 
> Does this make sense to you? If yes I can send the fix later today.

I think this flip makes perfect sense and should be done.

[...]