linux-kernel - Re: [PATCH bpf-next v2 2/3] bpf: Fix aux usage after do_check

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 for Android: free password hash cracker in your pocket

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

Message-ID: <877c0rdl8m.fsf@fau.de>
Date: Wed, 02 Jul 2025 10:56:25 +0200
From: Luis Gerhorst <luis.gerhorst@....de>
To: Eduard Zingerman <eddyz87@...il.com>
Cc: Andrii Nakryiko <andrii@...nel.org>,  Mykola Lysenko <mykolal@...com>,
  Alexei Starovoitov <ast@...nel.org>,  Daniel Borkmann
 <daniel@...earbox.net>,  Martin KaFai Lau <martin.lau@...ux.dev>,  Song
 Liu <song@...nel.org>,  Yonghong Song <yonghong.song@...ux.dev>,  John
 Fastabend <john.fastabend@...il.com>,  KP Singh <kpsingh@...nel.org>,
  Stanislav Fomichev <sdf@...ichev.me>,  Hao Luo <haoluo@...gle.com>,  Jiri
 Olsa <jolsa@...nel.org>,  Shuah Khan <shuah@...nel.org>,  Kumar Kartikeya
 Dwivedi <memxor@...il.com>,  Peilin Ye <yepeilin@...gle.com>,  Jiayuan
 Chen <mrpre@....com>,  Saket Kumar Bhaskar <skb99@...ux.ibm.com>,  Ihor
 Solodrai <isolodrai@...a.com>,  Daniel Xu <dxu@...uu.xyz>,
  bpf@...r.kernel.org,  linux-kselftest@...r.kernel.org,
  linux-kernel@...r.kernel.org,  Paul Chaignon <paul.chaignon@...il.com>,
  syzbot+dc27c5fb8388e38d2d37@...kaller.appspotmail.com
Subject: Re: [PATCH bpf-next v2 2/3] bpf: Fix aux usage after do_check_insn()

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

> On Sat, 2025-06-28 at 16:50 +0200, Luis Gerhorst wrote:
>
> [...]
>
>> @@ -19955,11 +19960,11 @@ static int do_check(struct bpf_verifier_env *env)
>>  			/* Prevent this speculative path from ever reaching the
>>  			 * insn that would have been unsafe to execute.
>>  			 */
>> -			cur_aux(env)->nospec = true;
>> +			prev_aux(env)->nospec = true;
>
> I don't like the prev_aux() call in this position, as one needs to
> understand that after do_check_insn() call what was current became
> previous. This at-least requires a comment. Implementation with a
> temporary variable (as at the bottom of this email), imo, is less
> cognitive load.

I think I agree. I will send a v3 with the variable.

>>  			/* IF it was an ADD/SUB insn, potentially remove any
>>  			 * markings for alu sanitization.
>>  			 */
>> -			cur_aux(env)->alu_state = 0;
>> +			prev_aux(env)->alu_state = 0;
>>  			goto process_bpf_exit;
>>  		} else if (err < 0) {
>>  			return err;
>
> [...]
>
> ---
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a136d9b1b25f..a923614b7104 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19953,6 +19953,7 @@ static int do_check(struct bpf_verifier_env *env)
>  	bool pop_log = !(env->log.level & BPF_LOG_LEVEL2);
>  	struct bpf_verifier_state *state = env->cur_state;
>  	struct bpf_insn *insns = env->prog->insnsi;
> +	struct bpf_insn_aux_data *insn_aux;
>  	int insn_cnt = env->prog->len;
>  	bool do_print_state = false;
>  	int prev_insn_idx = -1;
> @@ -19972,6 +19973,7 @@ static int do_check(struct bpf_verifier_env *env)
>  		}
>  
>  		insn = &insns[env->insn_idx];
> +		insn_aux = &env->insn_aux_data[env->insn_idx];
>  
>  		if (++env->insn_processed > BPF_COMPLEXITY_LIMIT_INSNS) {
>  			verbose(env,
> @@ -20048,7 +20050,7 @@ static int do_check(struct bpf_verifier_env *env)
>  		/* Reduce verification complexity by stopping speculative path
>  		 * verification when a nospec is encountered.
>  		 */
> -		if (state->speculative && cur_aux(env)->nospec)
> +		if (state->speculative && insn_aux->nospec)
>  			goto process_bpf_exit;
>  
>  		err = do_check_insn(env, &do_print_state);
> @@ -20056,11 +20058,11 @@ static int do_check(struct bpf_verifier_env *env)
>  			/* Prevent this speculative path from ever reaching the
>  			 * insn that would have been unsafe to execute.
>  			 */
> -			cur_aux(env)->nospec = true;
> +			insn_aux->nospec = true;
>  			/* If it was an ADD/SUB insn, potentially remove any
>  			 * markings for alu sanitization.
>  			 */
> -			cur_aux(env)->alu_state = 0;
> +			insn_aux->alu_state = 0;
>  			goto process_bpf_exit;
>  		} else if (err < 0) {
>  			return err;
> @@ -20069,7 +20071,7 @@ static int do_check(struct bpf_verifier_env *env)
>  		}
>  		WARN_ON_ONCE(err);
>  
> -		if (state->speculative && cur_aux(env)->nospec_result) {
> +		if (state->speculative && insn_aux->nospec_result) {
>  			/* If we are on a path that performed a jump-op, this
>  			 * may skip a nospec patched-in after the jump. This can
>  			 * currently never happen because nospec_result is only