[PATCH bpf-next] bpf: Always WARN_ONCE on verifier bugs

[PATCH bpf-next] bpf: Always WARN_ONCE on verifier bugs

[Paul Chaignon]

Throughout the verifier's logic, there are multiple checks for
inconsistent states that should never happen and would indicate a
verifier bug. These bugs are typically logged in the verifier logs and
sometimes preceded by a WARN_ONCE.

This patch reworks these checks to consistently emit a verifier log AND
a warning. The consistent use of WARN_ONCE should help fuzzers (ex.
syzkaller) expose any situation where they are actually able to reach
one of those buggy verifier states.

Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
---
 kernel/bpf/verifier.c | 63 +++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 99aa2c890e7b..521acbba2fda 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -196,6 +196,12 @@ struct bpf_verifier_stack_elem {
 
 #define BPF_PRIV_STACK_MIN_SIZE		64
 
+#define verifier_bug(env, fmt, args...)				\
+	do {							\
+		WARN_ONCE(1, "verifier bug: " fmt, ##args);	\
+		verbose(env, "verifier bug: " fmt, ##args);	\
+	} while (0)
+
 static int acquire_reference(struct bpf_verifier_env *env, int insn_idx);
 static int release_reference_nomark(struct bpf_verifier_state *state, int ref_obj_id);
 static int release_reference(struct bpf_verifier_env *env, int ref_obj_id);
@@ -1924,8 +1930,7 @@ static struct bpf_verifier_state *get_loop_entry(struct bpf_verifier_env *env,
 
 	while (topmost && topmost->loop_entry) {
 		if (steps++ > st->dfs_depth) {
-			WARN_ONCE(true, "verifier bug: infinite loop in get_loop_entry\n");
-			verbose(env, "verifier bug: infinite loop in get_loop_entry()\n");
+			verifier_bug(env, "infinite loop in get_loop_entry\n");
 			return ERR_PTR(-EFAULT);
 		}
 		topmost = topmost->loop_entry;
@@ -3460,9 +3465,9 @@ static int mark_reg_read(struct bpf_verifier_env *env,
 		if (writes && state->live & REG_LIVE_WRITTEN)
 			break;
 		if (parent->live & REG_LIVE_DONE) {
-			verbose(env, "verifier BUG type %s var_off %lld off %d\n",
-				reg_type_str(env, parent->type),
-				parent->var_off.value, parent->off);
+			verifier_bug(env, "type %s var_off %lld off %d\n",
+				     reg_type_str(env, parent->type),
+				     parent->var_off.value, parent->off);
 			return -EFAULT;
 		}
 		/* The first condition is more likely to be true than the
@@ -6562,13 +6567,13 @@ static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx,
 		next_insn = i + insn[i].imm + 1;
 		sidx = find_subprog(env, next_insn);
 		if (sidx < 0) {
-			WARN_ONCE(1, "verifier bug. No program starts at insn %d\n",
-				  next_insn);
+			verifier_bug(env, "No program starts at insn %d\n",
+				     next_insn);
 			return -EFAULT;
 		}
 		if (subprog[sidx].is_async_cb) {
 			if (subprog[sidx].has_tail_call) {
-				verbose(env, "verifier bug. subprog has tail_call and async cb\n");
+				verifier_bug(env, "subprog has tail_call and async cb\n");
 				return -EFAULT;
 			}
 			/* async callbacks don't increase bpf prog stack size unless called directly */
@@ -6676,8 +6681,8 @@ static int get_callee_stack_depth(struct bpf_verifier_env *env,
 
 	subprog = find_subprog(env, start);
 	if (subprog < 0) {
-		WARN_ONCE(1, "verifier bug. No program starts at insn %d\n",
-			  start);
+		verifier_bug(env, "No program starts at insn %d\n",
+			     start);
 		return -EFAULT;
 	}
 	return env->subprog_info[subprog].stack_depth;
@@ -7984,7 +7989,7 @@ static int check_stack_range_initialized(
 		slot = -i - 1;
 		spi = slot / BPF_REG_SIZE;
 		if (state->allocated_stack <= slot) {
-			verbose(env, "verifier bug: allocated_stack too small\n");
+			verbose(env, "allocated_stack too small\n");
 			return -EFAULT;
 		}
 
@@ -8413,7 +8418,7 @@ static int process_timer_func(struct bpf_verifier_env *env, int regno,
 		return -EINVAL;
 	}
 	if (meta->map_ptr) {
-		verbose(env, "verifier bug. Two map pointers in a timer helper\n");
+		verifier_bug(env, "Two map pointers in a timer helper\n");
 		return -EFAULT;
 	}
 	meta->map_uid = reg->map_uid;
@@ -10285,8 +10290,8 @@ static int setup_func_entry(struct bpf_verifier_env *env, int subprog, int calls
 	}
 
 	if (state->frame[state->curframe + 1]) {
-		verbose(env, "verifier bug. Frame %d already allocated\n",
-			state->curframe + 1);
+		verifier_bug(env, "Frame %d already allocated\n",
+			     state->curframe + 1);
 		return -EFAULT;
 	}
 
@@ -10400,8 +10405,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 			if (err)
 				return err;
 		} else {
-			bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
-				i, arg->arg_type);
+			verifier_bug(env, "unrecognized arg#%d type %d\n",
+				     i, arg->arg_type);
 			return -EFAULT;
 		}
 	}
@@ -10464,13 +10469,13 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
 	env->subprog_info[subprog].is_cb = true;
 	if (bpf_pseudo_kfunc_call(insn) &&
 	    !is_callback_calling_kfunc(insn->imm)) {
-		verbose(env, "verifier bug: kfunc %s#%d not marked as callback-calling\n",
-			func_id_name(insn->imm), insn->imm);
+		verifier_bug(env, "kfunc %s#%d not marked as callback-calling\n",
+			     func_id_name(insn->imm), insn->imm);
 		return -EFAULT;
 	} else if (!bpf_pseudo_kfunc_call(insn) &&
 		   !is_callback_calling_function(insn->imm)) { /* helper */
-		verbose(env, "verifier bug: helper %s#%d not marked as callback-calling\n",
-			func_id_name(insn->imm), insn->imm);
+		verifier_bug(env, "helper %s#%d not marked as callback-calling\n",
+			     func_id_name(insn->imm), insn->imm);
 		return -EFAULT;
 	}
 
@@ -10523,7 +10528,7 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	target_insn = *insn_idx + insn->imm + 1;
 	subprog = find_subprog(env, target_insn);
 	if (subprog < 0) {
-		verbose(env, "verifier bug. No program starts at insn %d\n", target_insn);
+		verifier_bug(env, "No program starts at insn %d\n", target_insn);
 		return -EFAULT;
 	}
 
@@ -11124,7 +11129,7 @@ static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
 	err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr,
 						  fmt_map_off);
 	if (err) {
-		verbose(env, "verifier bug\n");
+		verbose(env, "failed to retrieve map value address\n");
 		return -EFAULT;
 	}
 	fmt = (char *)(long)fmt_addr + fmt_map_off;
@@ -19689,8 +19694,8 @@ static int do_check(struct bpf_verifier_env *env)
 						return err;
 					break;
 				} else {
-					if (WARN_ON_ONCE(env->cur_state->loop_entry)) {
-						verbose(env, "verifier bug: env->cur_state->loop_entry != NULL\n");
+					if (unlikely(env->cur_state->loop_entry)) {
+						verifier_bug(env, "env->cur_state->loop_entry != NULL\n");
 						return -EFAULT;
 					}
 					do_print_state = true;
@@ -20750,8 +20755,8 @@ static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
 		if (bpf_pseudo_kfunc_call(&insn))
 			continue;
 
-		if (WARN_ON(load_reg == -1)) {
-			verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
+		if (unlikely(load_reg == -1)) {
+			verifier_bug(env, "zext_dst is set, but no reg is defined\n");
 			return -EFAULT;
 		}
 
@@ -21071,8 +21076,8 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 		 */
 		subprog = find_subprog(env, i + insn->imm + 1);
 		if (subprog < 0) {
-			WARN_ONCE(1, "verifier bug. No program starts at insn %d\n",
-				  i + insn->imm + 1);
+			verifier_bug(env, "No program starts at insn %d\n",
+				     i + insn->imm + 1);
 			return -EFAULT;
 		}
 		/* temporarily remember subprog id inside insn instead of
@@ -22433,7 +22438,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 			continue;
 		/* We need two slots in case timed may_goto is supported. */
 		if (stack_slots > slots) {
-			verbose(env, "verifier bug: stack_slots supports may_goto only\n");
+			verifier_bug(env, "stack_slots supports may_goto only\n");
 			return -EFAULT;
 		}
 
-- 
2.43.0

Re: [PATCH bpf-next] bpf: Always WARN_ONCE on verifier bugs

[Alexei Starovoitov]

On Thu, May 8, 2025 at 1:40 PM Paul Chaignon <paul.chaignon@gmail.com> wrote:
>
> Throughout the verifier's logic, there are multiple checks for
> inconsistent states that should never happen and would indicate a
> verifier bug. These bugs are typically logged in the verifier logs and
> sometimes preceded by a WARN_ONCE.
>
> This patch reworks these checks to consistently emit a verifier log AND
> a warning. The consistent use of WARN_ONCE should help fuzzers (ex.
> syzkaller) expose any situation where they are actually able to reach
> one of those buggy verifier states.

No. We cannot do it.
WARN_ONCE is for kernel level issues.
In some configs use panic_on_warn=1 too.
Whereas a verifier bug is contained within a verifier.
It will not bring the kernel down.
We should remove most of the existing WARN_ONCE instead.
Potentially replace them with pr_info_once().

pw-bot: cr

Re: [PATCH bpf-next] bpf: Always WARN_ONCE on verifier bugs

[Daniel Borkmann]

On 5/9/25 12:31 AM, Alexei Starovoitov wrote:
> On Thu, May 8, 2025 at 1:40 PM Paul Chaignon <paul.chaignon@gmail.com> wrote:
>>
>> Throughout the verifier's logic, there are multiple checks for
>> inconsistent states that should never happen and would indicate a
>> verifier bug. These bugs are typically logged in the verifier logs and
>> sometimes preceded by a WARN_ONCE.
>>
>> This patch reworks these checks to consistently emit a verifier log AND
>> a warning. The consistent use of WARN_ONCE should help fuzzers (ex.
>> syzkaller) expose any situation where they are actually able to reach
>> one of those buggy verifier states.
> 
> No. We cannot do it.
> WARN_ONCE is for kernel level issues.
> In some configs use panic_on_warn=1 too.
> Whereas a verifier bug is contained within a verifier.
> It will not bring the kernel down.

Agree.

> We should remove most of the existing WARN_ONCE instead.
> Potentially replace them with pr_info_once().

Just a thought, maybe one potential avenue could be to have an equivalent of
CONFIG_DEBUG_NET which we make a hard dependency of CONFIG_DEBUG_KERNEL so that
/noone/ enables this anywhere in production, and then fuzzers could use it
to their advantage. The default case for a DEBUG_BPF_WARN_ONCE would then fall
to BUILD_BUG_ON_INVALID() which lets compiler check validity but not generate
code.

Cheers,
Daniel

Re: [PATCH bpf-next] bpf: Always WARN_ONCE on verifier bugs

[Alexei Starovoitov]

On Fri, May 9, 2025 at 1:26 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 5/9/25 12:31 AM, Alexei Starovoitov wrote:
> > On Thu, May 8, 2025 at 1:40 PM Paul Chaignon <paul.chaignon@gmail.com> wrote:
> >>
> >> Throughout the verifier's logic, there are multiple checks for
> >> inconsistent states that should never happen and would indicate a
> >> verifier bug. These bugs are typically logged in the verifier logs and
> >> sometimes preceded by a WARN_ONCE.
> >>
> >> This patch reworks these checks to consistently emit a verifier log AND
> >> a warning. The consistent use of WARN_ONCE should help fuzzers (ex.
> >> syzkaller) expose any situation where they are actually able to reach
> >> one of those buggy verifier states.
> >
> > No. We cannot do it.
> > WARN_ONCE is for kernel level issues.
> > In some configs use panic_on_warn=1 too.
> > Whereas a verifier bug is contained within a verifier.
> > It will not bring the kernel down.
>
> Agree.
>
> > We should remove most of the existing WARN_ONCE instead.
> > Potentially replace them with pr_info_once().
>
> Just a thought, maybe one potential avenue could be to have an equivalent of
> CONFIG_DEBUG_NET which we make a hard dependency of CONFIG_DEBUG_KERNEL so that
> /noone/ enables this anywhere in production, and then fuzzers could use it
> to their advantage. The default case for a DEBUG_BPF_WARN_ONCE would then fall
> to BUILD_BUG_ON_INVALID() which lets compiler check validity but not generate
> code.

Good idea.
but I'm not a fan of the new kconfig.
I'd rather combine some of them.
Like make CONFIG_BPF_JIT depend on CONFIG_BPF_SYSCALL.
That would remove a ton of empty static inline helpers.

For this case doing WARN_ON only when IS_ENABLED(CONFIG_DEBUG_KERNEL)
should be fine.

I guess we can introduce BPF_WARN_ONCE() and friends family of macros
and use them everywhere in kernel/bpf/ where the warning is not
harmful to the kernel ?

Re: [PATCH bpf-next] bpf: Always WARN_ONCE on verifier bugs

[Paul Chaignon]

On Fri, May 09, 2025 at 10:58:00AM -0700, Alexei Starovoitov wrote:
> On Fri, May 9, 2025 at 1:26 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 5/9/25 12:31 AM, Alexei Starovoitov wrote:
> > > On Thu, May 8, 2025 at 1:40 PM Paul Chaignon <paul.chaignon@gmail.com> wrote:
> > >>
> > >> Throughout the verifier's logic, there are multiple checks for
> > >> inconsistent states that should never happen and would indicate a
> > >> verifier bug. These bugs are typically logged in the verifier logs and
> > >> sometimes preceded by a WARN_ONCE.
> > >>
> > >> This patch reworks these checks to consistently emit a verifier log AND
> > >> a warning. The consistent use of WARN_ONCE should help fuzzers (ex.
> > >> syzkaller) expose any situation where they are actually able to reach
> > >> one of those buggy verifier states.
> > >
> > > No. We cannot do it.
> > > WARN_ONCE is for kernel level issues.
> > > In some configs use panic_on_warn=1 too.
> > > Whereas a verifier bug is contained within a verifier.
> > > It will not bring the kernel down.
> >
> > Agree.
> >
> > > We should remove most of the existing WARN_ONCE instead.
> > > Potentially replace them with pr_info_once().
> >
> > Just a thought, maybe one potential avenue could be to have an equivalent of
> > CONFIG_DEBUG_NET which we make a hard dependency of CONFIG_DEBUG_KERNEL so that
> > /noone/ enables this anywhere in production, and then fuzzers could use it
> > to their advantage. The default case for a DEBUG_BPF_WARN_ONCE would then fall
> > to BUILD_BUG_ON_INVALID() which lets compiler check validity but not generate
> > code.
> 
> Good idea.
> but I'm not a fan of the new kconfig.
> I'd rather combine some of them.
> Like make CONFIG_BPF_JIT depend on CONFIG_BPF_SYSCALL.
> That would remove a ton of empty static inline helpers.
> 
> For this case doing WARN_ON only when IS_ENABLED(CONFIG_DEBUG_KERNEL)
> should be fine.
> 
> I guess we can introduce BPF_WARN_ONCE() and friends family of macros
> and use them everywhere in kernel/bpf/ where the warning is not
> harmful to the kernel ?

Thanks for the reviews! I've sent a v2 that introduces BPF_WARN_ONCE and
uses it in verifier_bug() to emit a warning iff CONFIG_DEBUG_KERNEL is
enabled.

I've refrained from replacing all WARN_ONCE at once as I'm not sure I
have all the context yet to know when BPF_WARN_ONCE is more appropriate.
That said, we should be able to use it for other cases and introduce
sibling macros as we go.