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

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 4 Apr 2019 10:45:46 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Edward Cree <ecree@...arflare.com>,
        Paul Chaignon <paul.chaignon@...nge.com>,
        Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, Xiao Han <xiao.han@...nge.com>,
        paul.chaignon@...il.com, Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH bpf] bpf: report verifier bugs as warnings

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@...r and include bpf program that triggered it?
Register and stack dumps shouldn't be in the warning.