linux-kernel - Re: [PATCH 4/5] bug: Provide toggle for BUG on data corruption

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAGXu5j+5i+sdAebvwSnwoDUFCTAHOV4za+jvw9stV5cSAEd_Lg@mail.gmail.com>
Date:	Tue, 16 Aug 2016 17:09:53 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Paul McKenney <paulmck@...ux.vnet.ibm.com>
Cc:	Stephen Boyd <sboyd@...eaurora.org>,
	Daniel Micay <danielmicay@...il.com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Josh Triplett <josh@...htriplett.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Tejun Heo <tj@...nel.org>,
	Michael Ellerman <mpe@...erman.id.au>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dan Williams <dan.j.williams@...el.com>,
	Jan Kara <jack@...e.cz>, Josef Bacik <jbacik@...com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"kernel-hardening@...ts.openwall.com" 
	<kernel-hardening@...ts.openwall.com>,
	Joe Perches <joe@...ches.com>
Subject: Re: [PATCH 4/5] bug: Provide toggle for BUG on data corruption

On Tue, Aug 16, 2016 at 5:01 PM, Paul E. McKenney
<paulmck@...ux.vnet.ibm.com> wrote:
> On Tue, Aug 16, 2016 at 02:42:28PM -0700, Kees Cook wrote:
>> On Tue, Aug 16, 2016 at 2:26 PM, Paul E. McKenney
>> <paulmck@...ux.vnet.ibm.com> wrote:
>> > On Tue, Aug 16, 2016 at 02:11:04PM -0700, Kees Cook wrote:
>> >> The kernel checks for several cases of data structure corruption under
>> >> either normal runtime, or under various CONFIG_DEBUG_* settings. When
>> >> corruption is detected, some systems may want to BUG() immediately instead
>> >> of letting the corruption continue. Many of these manipulation primitives
>> >> can be used by security flaws to gain arbitrary memory write control. This
>> >> provides CONFIG_BUG_ON_CORRUPTION to control newly added BUG() locations.
>> >>
>> >> This is inspired by similar hardening in PaX and Grsecurity, and by
>> >> Stephen Boyd in MSM kernels.
>> >>
>> >> Signed-off-by: Kees Cook <keescook@...omium.org>
>> >
>> > OK, I will bite...  Why both the WARN() and the BUG_ON()?
>>
>> Mostly because not every case of BUG(CORRUPTED_DATA_STRUCTURE) is
>> cleanly paired with a WARN (see the workqueue addition that wants to
>> dump locks too). I could rearrange things a bit, though, and create
>> something like:
>>
>> #ifdef CONFIG_BUG_ON_CORRUPTION
>> # define CORRUPTED(format...) { \
>>     printk(KERN_ERR format); \
>>     BUG(); \
>> }
>> #else
>> # define CORRUPTED(format...) WARN(1, format)
>> #endif
>>
>> What do you think?
>
> First let me see if I understand the rationale...  The idea is to allow
> those in security-irrelevant environments, such as test systems, to
> have the old "complain but soldier on" semantics, while security-conscious
> systems just panic, thereby hopefully converting a more dangerous form
> of attack into a denial-of-service attack.

Right, we don't want to wholesale upgrade all WARNs to BUGs. Just any
security-sensitive conditionals. And based on Laura's feedback, this
is really just about CONFIG_DEBUG_LIST now. We'll likely find some
more to add this to, but for the moment, we'll focus on list.

Here are the rationales as I see it:

- if you've enabled CONFIG_DEBUG_LIST
  - you want to get a report of the corruption
  - you want the kernel to _not operate on the structure_ (this went
missing when s/BUG/WARN/)
- if you've enabled CONFIG_BUG_ON_DATA_CORRUPTION
  - everything from CONFIG_DEBUG_LIST
  - you want the offending process to go away (i.e. BUG instead of WARN)
  - you may want the entire system to dump if you've set the
panic_on_oops sysctl (i.e. BUG)

> An alternative approach would be to make WARN() panic on systems built
> with CONFIG_BUG_ON_CORRUPTION, but we might instead want to choose which
> warnings are fatal on security-conscious systems.
>
> Or am I missing the point?
>
> At a more detailed level, one could argue for something like this:
>
> #define CORRUPTED(format...) \
> do { \
>         if (IS_ENABLED(CONFIG_BUG_ON_CORRUPTION)) { \
>                 printk(KERN_ERR format); \
>                 BUG(); \
>         } else { \
>                 WARN(1, format); \
>         } \
> } while (0)
>
> Some might prefer #ifdef to IS_ENABLED(), but the bare {} needs to
> be do-while in any case.

Yup, this is almost exactly what I've got in the v2. I wanted to
enforce a control-flow side-effect, though, so I've included a
non-optional "return false" as well.

-Kees

-- 
Kees Cook
Nexus Security