Date:   Wed, 10 Apr 2019 15:07:53 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     Sinan Kaya <Okaya@...nel.org>, Kees Cook <keescook@...omium.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Nicholas Piggin <npiggin@...il.com>, gor <gor@...ux.ibm.com>,
        Adrian Reber <adrian@...as.de>,
        Richard Guy Briggs <rgb@...hat.com>
Subject: Re: [PATCH v1] init: Do not select DEBUG_KERNEL by default

On Wed, Apr 10, 2019 at 3:04 PM Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
>
> ----- On Apr 10, 2019, at 5:53 PM, Sinan Kaya Okaya@...nel.org wrote:
>
> > On 4/10/2019 5:45 PM, Kees Cook wrote:
> >> On Wed, Apr 10, 2019 at 2:26 PM Sinan Kaya <okaya@...nel.org> wrote:
> >>>
> >>> We can't seem to have a kernel with CONFIG_EXPERT set but
> >>> CONFIG_DEBUG_KERNEL unset these days.
> >>>
> >>> While some of the features under the CONFIG_EXPERT require
> >>> CONFIG_DEBUG_KERNEL, it doesn't apply for all features.
> >>>
> >>> The meaning of CONFIG_EXPERT and CONFIG_DEBUG_KERNEL has been
> >>> mixed here.
> >>
> >> I don't agree: the point of EXPERT is to show _everything_, which
> >> means DEBUG_KERNEL should be selected to show those options as well. I
> >> think this is fine as-is. What is the problem you want to solve?
> >>
> >> I think of it as low (nothing selected) medium (DEBUG_KERNEL) and high
> >> (EXPERT and DEBUG_KERNEL). So EXPERT enables DEBUG_KERNEL too.
> >>
> >
> > Sure, let's see if there is a better option.
> >
> > I don't want any of the debug features in my kernel but still
> > need all the expert features. My kernel is considered a production
> > kernel. I don't really want to ship all the good debug enables.
> >
> > On the other hand, I need the features under CONFIG_EXPERT to have
> > a functional system.
> >
> > Let's take "multiple users" as an example.
> >
> > What's the point of having a kernel without multiple users? :)
> >
> > I don't see the relationship between CONFIG_DEBUG and CONFIG_EXPERT
> > as none of the features except KALLSYMS depend on it. If there was
> > a compile time dependency, I'd say move it to the things that need
> > it as this patch suggests.
> >
> > P.S. I found a circular dependency now. I can respin the patch based
> > on feedback.
>
> I think part of the issue here is that a few .c/.S files use CONFIG_DEBUG_KERNEL
> as #ifdef directly, which I'm not sure was meant to be. For instance:
>
> arch/powerpc/kernel/sysfs.c:
>
> #ifdef CONFIG_DEBUG_KERNEL
> SYSFS_SPRSETUP(hid0, SPRN_HID0);
> SYSFS_SPRSETUP(hid1, SPRN_HID1);
> SYSFS_SPRSETUP(hid4, SPRN_HID4);
> SYSFS_SPRSETUP(hid5, SPRN_HID5);
> SYSFS_SPRSETUP(ima0, SPRN_PA6T_IMA0);
> SYSFS_SPRSETUP(ima1, SPRN_PA6T_IMA1);
> SYSFS_SPRSETUP(ima2, SPRN_PA6T_IMA2);
> SYSFS_SPRSETUP(ima3, SPRN_PA6T_IMA3);
> SYSFS_SPRSETUP(ima4, SPRN_PA6T_IMA4);
> SYSFS_SPRSETUP(ima5, SPRN_PA6T_IMA5);
> SYSFS_SPRSETUP(ima6, SPRN_PA6T_IMA6);
> SYSFS_SPRSETUP(ima7, SPRN_PA6T_IMA7);
> SYSFS_SPRSETUP(ima8, SPRN_PA6T_IMA8);
> SYSFS_SPRSETUP(ima9, SPRN_PA6T_IMA9);
> SYSFS_SPRSETUP(imaat, SPRN_PA6T_IMAAT);
> SYSFS_SPRSETUP(btcr, SPRN_PA6T_BTCR);
> SYSFS_SPRSETUP(pccr, SPRN_PA6T_PCCR);
> SYSFS_SPRSETUP(rpccr, SPRN_PA6T_RPCCR);
> SYSFS_SPRSETUP(der, SPRN_PA6T_DER);
> SYSFS_SPRSETUP(mer, SPRN_PA6T_MER);
> SYSFS_SPRSETUP(ber, SPRN_PA6T_BER);
> SYSFS_SPRSETUP(ier, SPRN_PA6T_IER);
> SYSFS_SPRSETUP(sier, SPRN_PA6T_SIER);
> SYSFS_SPRSETUP(siar, SPRN_PA6T_SIAR);
> SYSFS_SPRSETUP(tsr0, SPRN_PA6T_TSR0);
> SYSFS_SPRSETUP(tsr1, SPRN_PA6T_TSR1);
> SYSFS_SPRSETUP(tsr2, SPRN_PA6T_TSR2);
> SYSFS_SPRSETUP(tsr3, SPRN_PA6T_TSR3);
> #endif /* CONFIG_DEBUG_KERNEL */
>
>
> arch/mips/kernel/setup.c:
>
> #if defined(CONFIG_DEBUG_KERNEL) && defined(CONFIG_DEBUG_INFO)
>                 /*
>                  * This information is necessary when debugging the kernel
>                  * But is a security vulnerability otherwise!
>                  */
>                 show_kernel_relocation(KERN_INFO);
> #endif

This one is unfortunate for sure. :P

> net/netfilter/core.c:
>
> static void hooks_validate(const struct nf_hook_entries *hooks)
> {
> #ifdef CONFIG_DEBUG_KERNEL
>         struct nf_hook_ops **orig_ops;
>         int prio = INT_MIN;
>         size_t i = 0;
>
>         orig_ops = nf_hook_entries_get_hook_ops(hooks);
>
>         for (i = 0; i < hooks->num_hook_entries; i++) {
>                 if (orig_ops[i] == &dummy_ops)
>                         continue;
>
>                 WARN_ON(orig_ops[i]->priority < prio);
>
>                 if (orig_ops[i]->priority > prio)
>                         prio = orig_ops[i]->priority;
>         }
> #endif
> }

This seems best to just always enable, neither caller appears to be fast-path.

>
> and also:
> arch/xtensa/kernel/smp.c
> arch/xtensa/kernel/entry.S
>
> I was under the impression that config DEBUG_KERNEL was only making a
> "group" of menu entries visible without any direct impact on the code,
> but it does not appear to be the case for a few exceptions. Perhaps this
> is the actual issue ? (and lack of documentation of this Kconfig entry)

Yeah, that's certainly not how it was intended. But under EXPERT, I
think there is still an argument to made that it's the right thing to
do.

-- 
Kees Cook

Powered by blists - more mailing lists