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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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.