[PATCH bpf] bpf: report verifier bugs as warnings

[PATCH bpf] bpf: report verifier bugs as warnings

[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 <paul.chaignon@orange.com>
---
 kernel/bpf/verifier.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 87221fda1321..12499e72b0d5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -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;
 	}
 
-- 
2.17.1

Re: [PATCH bpf] bpf: report verifier bugs as warnings

[Daniel Borkmann]

On 04/02/2019 01:58 PM, Paul Chaignon wrote:
> 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 <paul.chaignon@orange.com>

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.

> ---
>  kernel/bpf/verifier.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 87221fda1321..12499e72b0d5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -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;
>  	}
>  
> 

Re: [PATCH bpf] bpf: report verifier bugs as warnings

[Alexei Starovoitov]

On Tue, Apr 02, 2019 at 04:37:19PM +0200, Daniel Borkmann wrote:
> On 04/02/2019 01:58 PM, Paul Chaignon wrote:
> > 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 <paul.chaignon@orange.com>
> 
> 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().

Re: [PATCH bpf] bpf: report verifier bugs as warnings

[Paul Chaignon]

On Tue, Apr 02, 2019 at 10:39:29AM -0700, Alexei Starovoitov wrote:
> On Tue, Apr 02, 2019 at 04:37:19PM +0200, Daniel Borkmann wrote:
> > On 04/02/2019 01:58 PM, Paul Chaignon wrote:
> > > 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 <paul.chaignon@orange.com>
> > 
> > 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().
> 

Agreed for the verbose messages; it makes sense.  I'm a bit surprised you
don't think the verifier should warn on verifier bugs though.  It already
warns for other internal bugs such as 'regno >= MAX_BPF_REG'.  Or should
these be converted to verbose() as well?

Re: [PATCH bpf] bpf: report verifier bugs as warnings

[Edward Cree]

On 02/04/2019 15:37, Daniel Borkmann wrote:
> 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.

Re: [PATCH bpf] bpf: report verifier bugs as warnings

[Alexei Starovoitov]

On Wed, Apr 03, 2019 at 04:52:40PM +0100, Edward Cree wrote:
> On 02/04/2019 15:37, Daniel Borkmann wrote:
> > 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.

Re: [PATCH bpf] bpf: report verifier bugs as warnings

[Edward Cree]

On 03/04/2019 18:30, Alexei Starovoitov wrote:
> 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.
Makes sense.

So presumably the right patch would be to add more appropriate (concise
 and less context-dependent) WARN messages alongside the verbose()s that
 Paul's patch changed?

Re: [PATCH bpf] bpf: report verifier bugs as warnings

[Daniel Borkmann]

On 04/03/2019 07:30 PM, Alexei Starovoitov wrote:
> On Wed, Apr 03, 2019 at 04:52:40PM +0100, Edward Cree wrote:
>> On 02/04/2019 15:37, Daniel Borkmann wrote:
>>> 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.

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).

Re: [PATCH bpf] bpf: report verifier bugs as warnings

[Alexei Starovoitov]

On Thu, Apr 04, 2019 at 12:41:32AM +0200, Daniel Borkmann wrote:
> On 04/03/2019 07:30 PM, Alexei Starovoitov wrote:
> > On Wed, Apr 03, 2019 at 04:52:40PM +0100, Edward Cree wrote:
> >> On 02/04/2019 15:37, Daniel Borkmann wrote:
> >>> 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.
> 
> 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).

That's a good point. People and bots react to kernel warnings.
My concern with generic WARN though that it adds unnecessary taint,
module, stack, register dumps that are useless to debug the verifier issue.
Also some folks use panic_on_warn and imo that is complete overkill
to panic the box when integrity of the kernel is sound.
When verifier hits such corner case it rejects the program and completes
cleanly. Worst case there will be memory leak, though unlikely.

I think we need special verifier_warn() helper that will do
pr_warn("WARNING: CPU: ..."); or whatever else necessary to capture
syzbot and human attention plus a message to ask folks to report bugs
to bpf@vger and include bpf program that triggered it?
Register and stack dumps shouldn't be in the warning.