bpf: report verifier bugs as warnings

Three checks for verifier bugs were introduced in commit f4d7e40 ("bpf:
introduce function calls (verification)"). The bugs were reported as incorrect programs instead of kernel warnings as the present patch implements.

Signed-off-by Paul Chaignon <[email protected]>

---
 kernel/bpf/verifier.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

kernel/bpf/verifier.c +7 -7

index 87221fda1321..12499e72b0d5 100644
@@ -1145,9 +1145,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[parent->type],
-				parent->var_off.value, parent->off);
+			WARN_ONCE(1, "verifier bug type %s var_off %lld off %d\n",
+				  reg_type_str[parent->type],
+				  parent->var_off.value, parent->off);
 			return -EFAULT;
 		}
 		/* ... then we depend on parent's value */

@@ -2888,15 +2888,15 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	target_insn = *insn_idx + insn->imm;
 	subprog = find_subprog(env, target_insn + 1);
 	if (subprog < 0) {
-		verbose(env, "verifier bug. No program starts at insn %d\n",
-			target_insn + 1);
+		WARN_ONCE(1, "verifier bug. No program starts at insn %d\n",
+			  target_insn + 1);
 		return -EFAULT;
 	}
 
 	caller = state->frame[state->curframe];
 	if (state->frame[state->curframe + 1]) {
-		verbose(env, "verifier bug. Frame %d already allocated\n",
-			state->curframe + 1);
+		WARN_ONCE(1, "verifier bug. Frame %d already allocated\n",
+			  state->curframe + 1);
 		return -EFAULT;
 	}
 

Quoted 6 lines · Paul Chaignon

Three checks for verifier bugs were introduced in commit f4d7e40 ("bpf:
introduce function calls (verification)"). The bugs were reported as incorrect programs instead of kernel warnings as the present patch implements.

Signed-off-by Paul Chaignon <[email protected]>

Thanks for the patch, I think these WARN_ONCE() are a bit out of context though, meaning it lacks additional information about the program in kernel log once we actually manage to trigger it which we'd otherwise would potentially have had with the verbose() log. And from a program debugging pov, it makes it harder after this patch when verification log would suggest that all is fine. Looks like we already have a few WARN_ONCE() in verifier, they should probably be converted to verbose() as well to be consistent. If we really want to have a kernel warn, then lets add a helper macro verbose_and_warn(...) which will trigger a one-time warning, but keeps the verbose log intact as well.

Quoted 8 lines · Daniel Borkmann

Quoted 6 lines · Paul Chaignon

Three checks for verifier bugs were introduced in commit f4d7e40 ("bpf:
introduce function calls (verification)"). The bugs were reported as incorrect programs instead of kernel warnings as the present patch implements.

Signed-off-by Paul Chaignon <[email protected]>

Quoted 9 lines

Thanks for the patch, I think these WARN_ONCE() are a bit out of context though, meaning it lacks additional information about the program in kernel log once we actually manage to trigger it which we'd otherwise would potentially have had with the verbose() log. And from a program debugging pov, it makes it harder after this patch when verification log would suggest that all is fine. Looks like we already have a few WARN_ONCE() in verifier, they should probably be converted to verbose() as well to be consistent. If we really want to have a kernel warn, then lets add a helper macro verbose_and_warn(...) which will trigger a one-time warning, but keeps the verbose log intact as well.

I think they should stay as verbose() messages and some of the WARN_ON should be converted to verbose().
I don't think there is a need for verbose_and_warn().

If we really want to have a kernel warn, then lets add a
helper macro verbose_and_warn(...) which will trigger a one-time warning, but keeps the verbose log intact as well.

Quoted 3 lines · Daniel Borkmann

If we really want to have a kernel warn, then lets add a
helper macro verbose_and_warn(...) which will trigger a one-time warning, but keeps the verbose log intact as well.

+1

Any time the verifier detects that its internal invariants have been broken, logging a warning is the right thing to do, just like any other part of the kernel.

verbose_and_warn() doesn't fit these two practical cases of warn + verbose. Hence I see no reason to combine warn and verbose into single helper. They're perfectly fine being separate.

Quoted 9 lines · Edward Cree

Quoted 3 lines · Daniel Borkmann

If we really want to have a kernel warn, then lets add a
helper macro verbose_and_warn(...) which will trigger a one-time warning, but keeps the verbose log intact as well.

+1

Any time the verifier detects that its internal invariants have been broken, logging a warning is the right thing to do, just like any other part of the kernel.

It's not black and white.
As I said I don't think verbose_and_warn() is necessary.

Messages like:
verbose(env, "bpf verifier is misconfigured\n");
are technically 'broken internal invariant', but it shouldn't be a warn.

Whereas this:

        if (WARN_ON(regno >= MAX_BPF_REG)) {
                verbose(env, "mark_reg_known_zero(regs, %u)\n", regno);
                /* Something bad happened, let's kill all regs */
                for (regno = 0; regno < MAX_BPF_REG; regno++)
                        __mark_reg_not_init(regs + regno);
                return;
        }
should stay as-is.
It's a warn, and verbose message, and clean of regs.
Similarly:
        if (WARN_ON_ONCE(ptr_reg)) {
                print_verifier_state(env, state);
                verbose(env, "verifier internal error: unexpected ptr_reg\n");
                return -EINVAL;
        }

is a warn and more than just a verbose message.

verbose_and_warn() doesn't fit these two practical cases of warn + verbose. Hence I see no reason to combine warn and verbose into single helper. They're perfectly fine being separate.

Quoted 36 lines · Alexei Starovoitov

Quoted 9 lines · Edward Cree

Quoted 3 lines · Daniel Borkmann

If we really want to have a kernel warn, then lets add a
helper macro verbose_and_warn(...) which will trigger a one-time warning, but keeps the verbose log intact as well.

+1

Any time the verifier detects that its internal invariants have been broken, logging a warning is the right thing to do, just like any other part of the kernel.

It's not black and white.
As I said I don't think verbose_and_warn() is necessary.

Messages like:
verbose(env, "bpf verifier is misconfigured\n");
are technically 'broken internal invariant', but it shouldn't be a warn.

Whereas this:

        if (WARN_ON(regno >= MAX_BPF_REG)) {
                verbose(env, "mark_reg_known_zero(regs, %u)\n", regno);
                /* Something bad happened, let's kill all regs */
                for (regno = 0; regno < MAX_BPF_REG; regno++)
                        __mark_reg_not_init(regs + regno);
                return;
        }
should stay as-is.
It's a warn, and verbose message, and clean of regs.
Similarly:
        if (WARN_ON_ONCE(ptr_reg)) {
                print_verifier_state(env, state);
                verbose(env, "verifier internal error: unexpected ptr_reg\n");
                return -EINVAL;
        }

is a warn and more than just a verbose message.

Quoted 3 lines

verbose_and_warn() doesn't fit these two practical cases of warn + verbose. Hence I see no reason to combine warn and verbose into single helper. They're perfectly fine being separate.

Sure, I think that's okay as well; was mainly thinking to keep some of these WARN wrt broken internal invariant such that tools like syzkaller will actually generate a report w/ reproducer if it ever hits these (as opposed to just ignore them due to ignoring such logs in general).