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/
