|
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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.