linux-kernel - Re: [PATCH bpf-next v2] bpf: Remove redundant free_verifier_state()/pop

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: <a4fbe41d6f4c25c3d1edd42905eb556541857327.camel@gmail.com>
Date: Fri, 13 Jun 2025 14:17:46 -0700
From: Eduard Zingerman <eddyz87@...il.com>
To: Luis Gerhorst <luis.gerhorst@....de>, Alexei Starovoitov
 <ast@...nel.org>,  Daniel Borkmann <daniel@...earbox.net>, John Fastabend
 <john.fastabend@...il.com>, Andrii Nakryiko	 <andrii@...nel.org>, Martin
 KaFai Lau <martin.lau@...ux.dev>, Song Liu	 <song@...nel.org>, Yonghong
 Song <yonghong.song@...ux.dev>, KP Singh	 <kpsingh@...nel.org>, Stanislav
 Fomichev <sdf@...ichev.me>, Hao Luo	 <haoluo@...gle.com>, Jiri Olsa
 <jolsa@...nel.org>, bpf@...r.kernel.org, 	linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2] bpf: Remove redundant
 free_verifier_state()/pop_stack()

On Fri, 2025-06-13 at 11:01 +0200, Luis Gerhorst wrote:
> This patch removes duplicated code.
> 
> Eduard points out [1]:
> 
>     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.
> 
> This would also fix the 'symptom' for [2], but the issue also has a
> simpler fix which was sent separately. This fix also makes sure the
> push_*() callers always return an error for which
> error_recoverable_with_nospec(err) is false. This is required because
> otherwise we try to recover and access the stale `state`.
> 
> Moving free_verifier_state() and pop_stack(..., pop_log=false) to happen
> after the bpf_vlog_reset() call in do_check_common() is fine because the
> pop_stack() call that is moved does not call bpf_vlog_reset() with the
> pop_log=false parameter.
> 
> [1] https://lore.kernel.org/all/b6931bd0dd72327c55287862f821ca6c4c3eb69a.camel@gmail.com/
> [2] https://lore.kernel.org/all/68497853.050a0220.33aa0e.036a.GAE@google.com/
> 
> Reported-by: Eduard Zingerman <eddyz87@...il.com>
> Link: https://lore.kernel.org/all/b6931bd0dd72327c55287862f821ca6c4c3eb69a.camel@gmail.com/
> Acked-by: Eduard Zingerman <eddyz87@...il.com>
> Signed-off-by: Luis Gerhorst <luis.gerhorst@....de>
> ---

Tried v2, all looks good.

[...]

> @@ -22934,6 +22922,11 @@ static void free_states(struct bpf_verifier_env *env)
>  	struct bpf_scc_info *info;
>  	int i, j;
>  
> +	WARN_ON_ONCE(!env->cur_state);

Tbh I woudn't do this a warning, just an 'if (env->cur_state) ...',
but that's immaterial. Given current way do_check_common() is written
env->cur_state != NULL at this point, so the patch is safe to land.

> +	free_verifier_state(env->cur_state, true);
> +	env->cur_state = NULL;
> +	while (!pop_stack(env, NULL, NULL, false));
> +
>  	list_for_each_safe(pos, tmp, &env->free_list) {
>  		sl = container_of(pos, struct bpf_verifier_state_list, node);
>  		free_verifier_state(&sl->state, false);