| Message ID | 20170215125735.17920d58@kaiwan-T460 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 14, 2017 at 11:27 PM, Kaiwan N Billimoria <kaiwan@kaiwantech.com> wrote: > Okay, I've incorporated your suggestions. > Of course, the printk's are temporary. > > Pl see: > - the updated patch, and > - a 'truth table' enumerating some basic testing with these configs, > below: > > --- > diff --git a/init/main.c b/init/main.c > index ef47035..ba44574 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1028,6 +1028,11 @@ static noinline void __init kernel_init_freeable(void) > > do_basic_setup(); > > +#ifdef CONFIG_MEMORY_SANITIZE > + pr_debug("[CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? %s\n", > + page_poisoning_enabled() ? "yes" : "no"); > +#endif > + > /* Open the /dev/console on the rootfs, this should never fail */ > if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0) > pr_err("Warning: unable to open an initial console.\n"); > diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug > index 3e5eada..fbb4290 100644 > --- a/mm/Kconfig.debug > +++ b/mm/Kconfig.debug > @@ -97,3 +97,24 @@ config DEBUG_RODATA_TEST > ---help--- > This option enables a testcase for the setting rodata read-only. > > +config MEMORY_SANITIZE > + bool "Enable memory sanitization features" > + select SLUB_DEBUG > + select PAGE_POISONING > + select PAGE_POISONING_NO_SANITY > + ---help--- > + This option enables memory sanitization features. Particularly, > + when you turn on this option, it auto-enables: > + - SLUB debug > + - page poisoning > + - page poisoning no sanity. > + > + Implication: turning this option on _will_ implicitly enable: > + - the SLUB_DEBUG switch to the equivalent of the kernel command-line > + 'slub_debug=p' ; (where p=PAGE_POISON), > + - page poisoning, equivalent to passing the kernel command-line option > + 'page_poison=on', > + irrespective of whether the options are explicitly passed or not. Hm, we don't want to force this on against other command line settings, so how about what I suggest below... > + > + If unsure, say N. > + > diff --git a/mm/page_poison.c b/mm/page_poison.c > index 2e647c6..8d1e883 100644 > --- a/mm/page_poison.c > +++ b/mm/page_poison.c > @@ -49,6 +49,17 @@ struct page_ext_operations page_poisoning_ops = { > .init = init_page_poisoning, > }; > > +static int __init memory_sanitize_pagepoison_init(void) > +{ > + /* With 'memory sanitize' On, page poisoning Must be turned on */ > + if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) { > + want_page_poisoning = true; > + __page_poisoning_enabled = true; > + } > + return 0; > +} > +early_initcall(memory_sanitize_pagepoison_init); Can this be changed to interact better with the command line arguments, in case someone selects CONFIG_MEMORY_SANITIZE, but boots with page_poison=off? e.g. instead of your init code, do this: static bool want_page_poisoning __read_mostly = IS_ENABLED(CONFIG_MEMORY_SANITIZE); That way the logic for __page_poisoning_enabled is retained, etc. > + > static inline void set_page_poison(struct page *page) > { > struct page_ext *page_ext; > diff --git a/mm/slub.c b/mm/slub.c > index d24e1ce..62de543 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -457,6 +457,17 @@ static int slub_debug; > static char *slub_debug_slabs; > static int disable_higher_order_debug; > > +static int __init memory_sanitize_slubdebug_init(void) > +{ > +/* With 'memory sanitize' On, slub_debug Must be set to 'p' */ > + if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) && > + IS_ENABLED(CONFIG_MEMORY_SANITIZE)) { > + slub_debug |= SLAB_POISON; > + } > + return 0; > +} > +early_initcall(memory_sanitize_slubdebug_init); And instead of this, use: #if defined(CONFIG_SLUB_DEBUG_ON) # define SLUB_DEBUG_INITIAL_FLAGS DEBUG_DEFAULT_FLAGS #elif defined(CONFIG_MEMORY_SANITIZE) # define SLUB_DEBUG_INITIAL_FLAGS SLAB_POISON #else # define SLUB_DEBUG_INITIAL_FLAGS 0 #endif static int slub_debug = SLUB_DEBUG_INITIAL_FLAGS; That way we're just setting the boot-time defaults and it'll safely interact with everything else. (I've added Christoph to CC to see what he thinks of this.) -Kees
> > Hm, we don't want to force this on against other command line > settings, so how about what I suggest below... > Ok, can see the logic in that.. > > Can this be changed to interact better with the command line > arguments, in case someone selects CONFIG_MEMORY_SANITIZE, but boots > with page_poison=off? > > e.g. instead of your init code, do this: > > static bool want_page_poisoning __read_mostly = > IS_ENABLED(CONFIG_MEMORY_SANITIZE); > > That way the logic for __page_poisoning_enabled is retained, etc. > >> + >> static inline void set_page_poison(struct page *page) >> { >> struct page_ext *page_ext; >> diff --git a/mm/slub.c b/mm/slub.c >> index d24e1ce..62de543 100644 >> --- a/mm/slub.c >> +++ b/mm/slub.c >> @@ -457,6 +457,17 @@ static int slub_debug; >> static char *slub_debug_slabs; >> static int disable_higher_order_debug; >> >> +static int __init memory_sanitize_slubdebug_init(void) >> +{ >> +/* With 'memory sanitize' On, slub_debug Must be set to 'p' */ >> + if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) && >> + IS_ENABLED(CONFIG_MEMORY_SANITIZE)) { >> + slub_debug |= SLAB_POISON; >> + } >> + return 0; >> +} >> +early_initcall(memory_sanitize_slubdebug_init); > > And instead of this, use: > > #if defined(CONFIG_SLUB_DEBUG_ON) > # define SLUB_DEBUG_INITIAL_FLAGS DEBUG_DEFAULT_FLAGS > #elif defined(CONFIG_MEMORY_SANITIZE) > # define SLUB_DEBUG_INITIAL_FLAGS SLAB_POISON > #else > # define SLUB_DEBUG_INITIAL_FLAGS 0 > #endif > > static int slub_debug = SLUB_DEBUG_INITIAL_FLAGS; > > That way we're just setting the boot-time defaults and it'll safely > interact with everything else. > > (I've added Christoph to CC to see what he thinks of this.) > What about a kernel command-line param - called 'mem_sanitize'? If set to 'on', it will enforce the forced behaviour (as per the curr implementation). I can think of this (but...): --------------------+--------------------------+--------------------------- mem_sanitize | page_poisoning | slub_debug --------------------+--------------------------+--------------------------- strict | on | |= SLAB_POISON normal | <current default> off | off | off --------------------+--------------------------+--------------------------- But... if you think about it, I guess only the "strict" option is useful in any meaningful way. 'normal' is what we'll get when I re-implement the code as you suggested above. Do we even want an 'off' case? As again, we'd be poking into the page_poisoning and slub_debug values. Of course we can (and will) document all this... Thoughts? -Kaiwan. > -Kees > > -- > Kees Cook > Pixel Security >
On Thu, Feb 16, 2017 at 7:18 PM, Kaiwan N Billimoria <kaiwan@kaiwantech.com> wrote: >> Hm, we don't want to force this on against other command line >> settings, so how about what I suggest below... >> > > Ok, can see the logic in that.. > >> >> Can this be changed to interact better with the command line >> arguments, in case someone selects CONFIG_MEMORY_SANITIZE, but boots >> with page_poison=off? >> >> e.g. instead of your init code, do this: >> >> static bool want_page_poisoning __read_mostly = >> IS_ENABLED(CONFIG_MEMORY_SANITIZE); >> >> That way the logic for __page_poisoning_enabled is retained, etc. >> >>> + >>> static inline void set_page_poison(struct page *page) >>> { >>> struct page_ext *page_ext; >>> diff --git a/mm/slub.c b/mm/slub.c >>> index d24e1ce..62de543 100644 >>> --- a/mm/slub.c >>> +++ b/mm/slub.c >>> @@ -457,6 +457,17 @@ static int slub_debug; >>> static char *slub_debug_slabs; >>> static int disable_higher_order_debug; >>> >>> +static int __init memory_sanitize_slubdebug_init(void) >>> +{ >>> +/* With 'memory sanitize' On, slub_debug Must be set to 'p' */ >>> + if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) && >>> + IS_ENABLED(CONFIG_MEMORY_SANITIZE)) { >>> + slub_debug |= SLAB_POISON; >>> + } >>> + return 0; >>> +} >>> +early_initcall(memory_sanitize_slubdebug_init); >> >> And instead of this, use: >> >> #if defined(CONFIG_SLUB_DEBUG_ON) >> # define SLUB_DEBUG_INITIAL_FLAGS DEBUG_DEFAULT_FLAGS >> #elif defined(CONFIG_MEMORY_SANITIZE) >> # define SLUB_DEBUG_INITIAL_FLAGS SLAB_POISON >> #else >> # define SLUB_DEBUG_INITIAL_FLAGS 0 >> #endif >> >> static int slub_debug = SLUB_DEBUG_INITIAL_FLAGS; >> >> That way we're just setting the boot-time defaults and it'll safely >> interact with everything else. >> >> (I've added Christoph to CC to see what he thinks of this.) >> > > What about a kernel command-line param - called 'mem_sanitize'? > If set to 'on', it will enforce the forced behaviour (as per the curr > implementation). > > I can think of this (but...): > --------------------+--------------------------+--------------------------- > mem_sanitize | page_poisoning | slub_debug > --------------------+--------------------------+--------------------------- > strict | on | |= SLAB_POISON > normal | <current default> > off | off | off > --------------------+--------------------------+--------------------------- > > But... if you think about it, I guess only the "strict" option is useful in any > meaningful way. > 'normal' is what we'll get when I re-implement the code > as you suggested above. > Do we even want an 'off' case? As again, we'd be poking into the page_poisoning > and slub_debug values. > > Of course we can (and will) document all this... > Thoughts? I think mem_sanitize should likely follow the logic used by pax_sanitize_slab. i.e CONFIG_MEMORIZE_SANITIZE as suggested above, then a mem_sanitize= option for "off" and "full". (i.e. CONFIG_MEMORIZE_SANITIZE implies "on") -Kees
diff --git a/init/main.c b/init/main.c index ef47035..ba44574 100644 --- a/init/main.c +++ b/init/main.c @@ -1028,6 +1028,11 @@ static noinline void __init kernel_init_freeable(void) do_basic_setup(); +#ifdef CONFIG_MEMORY_SANITIZE + pr_debug("[CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? %s\n", + page_poisoning_enabled() ? "yes" : "no"); +#endif + /* Open the /dev/console on the rootfs, this should never fail */ if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0) pr_err("Warning: unable to open an initial console.\n"); diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug index 3e5eada..fbb4290 100644 --- a/mm/Kconfig.debug +++ b/mm/Kconfig.debug @@ -97,3 +97,24 @@ config DEBUG_RODATA_TEST ---help--- This option enables a testcase for the setting rodata read-only. +config MEMORY_SANITIZE + bool "Enable memory sanitization features" + select SLUB_DEBUG + select PAGE_POISONING + select PAGE_POISONING_NO_SANITY + ---help--- + This option enables memory sanitization features. Particularly, + when you turn on this option, it auto-enables: + - SLUB debug + - page poisoning + - page poisoning no sanity. + + Implication: turning this option on _will_ implicitly enable: + - the SLUB_DEBUG switch to the equivalent of the kernel command-line + 'slub_debug=p' ; (where p=PAGE_POISON), + - page poisoning, equivalent to passing the kernel command-line option + 'page_poison=on', + irrespective of whether the options are explicitly passed or not. + + If unsure, say N. + diff --git a/mm/page_poison.c b/mm/page_poison.c index 2e647c6..8d1e883 100644 --- a/mm/page_poison.c +++ b/mm/page_poison.c @@ -49,6 +49,17 @@ struct page_ext_operations page_poisoning_ops = { .init = init_page_poisoning, }; +static int __init memory_sanitize_pagepoison_init(void) +{ + /* With 'memory sanitize' On, page poisoning Must be turned on */ + if (IS_ENABLED(CONFIG_MEMORY_SANITIZE)) { + want_page_poisoning = true; + __page_poisoning_enabled = true; + } + return 0; +} +early_initcall(memory_sanitize_pagepoison_init); + static inline void set_page_poison(struct page *page) { struct page_ext *page_ext; diff --git a/mm/slub.c b/mm/slub.c index d24e1ce..62de543 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -457,6 +457,17 @@ static int slub_debug; static char *slub_debug_slabs; static int disable_higher_order_debug; +static int __init memory_sanitize_slubdebug_init(void) +{ +/* With 'memory sanitize' On, slub_debug Must be set to 'p' */ + if (IS_ENABLED(CONFIG_SLUB_DEBUG_ON) && + IS_ENABLED(CONFIG_MEMORY_SANITIZE)) { + slub_debug |= SLAB_POISON; + } + return 0; +} +early_initcall(memory_sanitize_slubdebug_init); + /* * slub is about to manipulate internal object metadata. This memory lies * outside the range of the allocated object, so accessing it would normally @@ -5675,6 +5686,11 @@ static int __init slab_sysfs_init(void) struct kmem_cache *s; int err; +#ifdef CONFIG_MEMORY_SANITIZE + pr_info("[CONFIG_MEMORY_SANITIZE]: slub_debug = P? %s [0x%x]\n", + slub_debug & SLAB_POISON ? "yes" : "no", slub_debug); +#endif + mutex_lock(&slab_mutex); slab_kset = kset_create_and_add("slab", &slab_uevent_ops, kernel_kobj);
Okay, I've incorporated your suggestions. Of course, the printk's are temporary. Pl see: - the updated patch, and - a 'truth table' enumerating some basic testing with these configs, below: --- --- (Minimal) Testing done so far: -------------------------------+----------------------+----------------------- | Kconfig | kernel cmdline | Result* | |--------------+---------------+----------------------+----------------------| SLUB_DEBUG[_ON]|MEMORY_SANITIZE|page_poison|slub_debug|page_poison|slub_debug| |--------------+---------------+----------------------+----------------------| Y | Y | np[1] | np | on | p | Y | Y | np[1] | =p | on | p | Y | Y | np[1] | =fzu | on | p | Y | Y | np[1] | =fzup | on | p | Y | Y | np[1] | = | on | p | |--------------+---------------+----------------------+----------------------| Y | Y | =off | np | on | p | Y | Y | =off | =p | on | p | Y | Y | =off | =fzu | on | p | Y | Y | =off | = | on | p | |--------------+---------------+----------------------+----------------------| Y | Y | =on | np | on | p | Y | Y | =on | =p | on | p | Y | Y | =on | =fzu | on | p | Y | Y | =on | = | on | p | |--------------+---------------+----------------------+----------------------| [1] np = not passed * Result: MEMORY_SANITIZE='y' => page_poison should be 'on' and slub_debug='p' (implying SLAB_POISON bit set). So far, all tests passed.. Thanks, Kaiwan. On Tue, 14 Feb 2017 09:19:27 -0800 Kees Cook <keescook@chromium.org> wrote: > On Mon, Feb 13, 2017 at 7:01 PM, Kaiwan N Billimoria > <kaiwan@kaiwantech.com> wrote: > > Thanks for your response... > >> > >> > >> > >> > +config MEMORY_SANITIZE > >> > + bool "Enable memory sanitization features" > >> > + select SLUB_DEBUG > >> > + select PAGE_POISONING > >> > + select PAGE_POISONING_NO_SANITY if HIBERNATION > >> > + ---help--- > >> > + This option enables ... > >> > >> Good start! Why the "if HIBERNATION" bit? It seems like sanity > >> checks are very expensive, so we'd not want it as part of this > >> config? > > Okay, I wasn't sure. So would it be (more) correct to retain the > > first two configs plus > > PAGE_POISONING_NO_SANITY (without the if)? > > I think so, yes. We may need to tweak it in the future, but I think > that's the correct config for now. > > >> > #if defined(CONFIG_SLUB_DEBUG_ON) > >> > +#if defined(CONFIG_MEMORY_SANITIZE) > >> > +/* With 'memory sanitize' On, slub_debug should be 'P' */ > >> > +static int slub_debug = SLAB_POISON; > >> > +#else > >> > static int slub_debug = DEBUG_DEFAULT_FLAGS; > >> > +#endif /* CONFIG_MEMORY_SANITIZE */ > >> > #else > >> > static int slub_debug; > >> > -#endif > >> > +#endif /* CONFIG_SLUB_DEBUG_ON */ > >> > >> Could the definition of DEBUG_DEFAULT_FLAGS be adjusted instead of > >> doing the ifdefs here in the .c file? Or, perhaps do a slub_debug > >> |= SLAB_POISON in memory_sanitize_init()? > >> > > Yes, the latter sounds good but the init function is in > > mm/page_poison.c and the slub_debug var is a static in mm/slub.c . > > Any suggestions? > > Perhaps add another early_init like you did the page_poison.c? > > -Kees >