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

[Luis Gerhorst]

Eduard Zingerman <eddyz87@xxxxxxxxx> writes:

> On Wed, 2025-06-11 at 16:03 +0200, Luis Gerhorst wrote:
>> Eduard Zingerman <eddyz87@xxxxxxxxx> writes:
>> 
>> > Either 'state = env->cur_state' is needed after 'do_check_insn()' or
>> > error path should not free env->cur_state (seems logical).

[...]

>> 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.

Ah, yes I agree. I sent a patch separate from the fix [2].

>> 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).

I noticed the was not really true yet, I had to fix the call for
sanitize_ptr_alu() to return -ENOMEM while [3] is not landed yet.

>> 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.

I sent the fix [1], let me know if it is as desired.

[1] https://lore.kernel.org/all/20250611210728.266563-1-luis.gerhorst@xxxxxx/
[2] https://lore.kernel.org/all/20250611211431.275731-1-luis.gerhorst@xxxxxx/
[3] https://lore.kernel.org/all/20250603213232.339242-1-luis.gerhorst@xxxxxx/