linux-kernel - Re: [syzbot] [bpf?] KASAN: slab-use-after-free Read in do

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87plfa3qxi.fsf@fau.de>
Date: Wed, 11 Jun 2025 23:32:25 +0200
From: Luis Gerhorst <luis.gerhorst@....de>
To: Eduard Zingerman <eddyz87@...il.com>
Cc: andrii@...nel.org,  ast@...nel.org,  bpf@...r.kernel.org,
  daniel@...earbox.net,  haoluo@...gle.com,  john.fastabend@...il.com,
  jolsa@...nel.org,  kpsingh@...nel.org,  linux-kernel@...r.kernel.org,
  martin.lau@...ux.dev,  sdf@...ichev.me,  song@...nel.org,
  syzkaller-bugs@...glegroups.com,  yonghong.song@...ux.dev
Subject: Re: [syzbot] [bpf?] KASAN: slab-use-after-free Read in do_check

Eduard Zingerman <eddyz87@...il.com> writes:

> On Wed, 2025-06-11 at 16:03 +0200, Luis Gerhorst wrote:
>> Eduard Zingerman <eddyz87@...il.com> 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@fau.de/
[2] https://lore.kernel.org/all/20250611211431.275731-1-luis.gerhorst@fau.de/
[3] https://lore.kernel.org/all/20250603213232.339242-1-luis.gerhorst@fau.de/