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: <b6931bd0dd72327c55287862f821ca6c4c3eb69a.camel@gmail.com>
Date: Wed, 11 Jun 2025 10:20:40 -0700
From: Eduard Zingerman <eddyz87@...il.com>
To: Luis Gerhorst <luis.gerhorst@....de>
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

On Wed, 2025-06-11 at 16:03 +0200, Luis Gerhorst wrote:
> Eduard Zingerman <eddyz87@...il.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.

[...]