diff mbox

Merge in PAX_MEMORY_SANITIZE work from grsec to linux-next

Message ID 20170209090714.63ce6333@kaiwan-T460 (mailing list archive)
State New, archived
Headers show

Commit Message

Kaiwan N Billimoria Feb. 9, 2017, 3:37 a.m. UTC
Hi Kees,

> I think CONFIG_MEMORY_SANITIZE would enable:
> 
> CONFIG_SLUB_DEBUG=y
> CONFIG_PAGE_POISONING=y
> CONFIG_PAGE_POISONING_NO_SANITY=y
> 
> but it would _also_ need to set these kernel command-line variables as
> if they had been set:
> 
> page_poison=1
> slub_debug=P
> 
> No, the first step would be for the config to only provide the above
> changes.

Have made a first cut (below). Pl take a look and let me know if okay 
and how I should proceed.

Note-
 - the printks' are just for checking that options are enabled as required 
(will get rid of them later)
 - this is on linux-next (x86_64)
 - dmesg filtered output when booted with this kernel (no page_posion or slub_debug 
cmdline options passed):

$ dmesg |grep "MEMORY_SANITIZE"
kern  :info  : [  +0.000387] [CONFIG_MEMORY_SANITIZE]: slub_debug = P? yes [0x800]
kern  :debug : [  +0.000000] [CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? yes
$

===

Regards,
Kaiwan.

> Then, we'd want to add the poison value defaults as you mention:
> 
> > -Would include the 'new' poison / sanitize values of 0xfe and 0xff
> > for slab (64 and 32-bit resp), etc etc.  
> 
> And then finally add the exceptions for the "frequently freed" slub
> caches, as identified by PaX already. This would need the flag
> defined, the poisoning logic adjusted to check the flag, and for the
> new kernel command-line options for changing the whether or not the
> flag is respected (like PaX's "pax_sanitize_slab").
> 
> > Even if this is (somewhat) correct, my thinking is - correct me I'm
> > totally wrong here - that the whole purpose of the exercise is to
> > improve _security_; hence, tying this code into the "debug
> > features" of the kernel isn't really what we want, yes?  
> 
> In the discussions Laura had with the mm folks, the only realistic
> path to landing this in the upstream kernel is through these debug
> features.
> 
> > Most folks would only use debug during development, if at all -
> > given all the concerns regarding performance. Here, the objective
> > is to enable a powerful security feature set. Hence, the config
> > directives should come under the 'Security Options' menu.  
> 
> We're not close to having a "Kernel Security" menu, so for now, I've
> wanted to focus on getting the features, and then making the Kconfig
> menus pretty later.
> 
> Hopefully that all makes sense! :)
> 
> -Kees
>

Comments

Kaiwan N Billimoria Feb. 13, 2017, 11:42 a.m. UTC | #1
<Nudge>, kindly check out the patch, thanks.


Regards,
Kaiwan.

Kaiwan N Billimoria
✉ kaiwan@kaiwantech.com
✉ kaiwan.billimoria@gmail.com

kaiwanTECH
http://kaiwantech.in
Do visit our enhanced website.

4931, 11th Floor, Highpoint IV, 45 Palace Road, Bangalore 560 001.
☎ +91.9845016788 (M)
☎ TeleFax: +91.80.22389396 | Alt. Tel: +91.80.64500257

LinkedIn: https://in.linkedin.com/in/kaiwanbillimoria

I blog here:
Tech : http://kaiwantech.wordpress.com  |
Running : http://kaiwanbill.blogspot.in/

"Don't be afraid that your life will end,
be afraid that it will never begin."

On Thu, Feb 9, 2017 at 9:07 AM, Kaiwan N Billimoria <kaiwan@kaiwantech.com>
wrote:

> Hi Kees,
>
> > I think CONFIG_MEMORY_SANITIZE would enable:
> >
> > CONFIG_SLUB_DEBUG=y
> > CONFIG_PAGE_POISONING=y
> > CONFIG_PAGE_POISONING_NO_SANITY=y
> >
> > but it would _also_ need to set these kernel command-line variables as
> > if they had been set:
> >
> > page_poison=1
> > slub_debug=P
> >
> > No, the first step would be for the config to only provide the above
> > changes.
>
> Have made a first cut (below). Pl take a look and let me know if okay
> and how I should proceed.
>
> Note-
>  - the printks' are just for checking that options are enabled as required
> (will get rid of them later)
>  - this is on linux-next (x86_64)
>  - dmesg filtered output when booted with this kernel (no page_posion or
> slub_debug
> cmdline options passed):
>
> $ dmesg |grep "MEMORY_SANITIZE"
> kern  :info  : [  +0.000387] [CONFIG_MEMORY_SANITIZE]: slub_debug = P? yes
> [0x800]
> kern  :debug : [  +0.000000] [CONFIG_MEMORY_SANITIZE]:
> page_poisoning_enabled? yes
> $
>
> ===
> 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..8eed6ca 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -97,3 +97,11 @@ 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 if HIBERNATION
> +       ---help---
> +       This option enables ...
> +
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 2e647c6..b45bc0a 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
>          .init = init_page_poisoning,
>  };
>
> +#ifdef CONFIG_MEMORY_SANITIZE
> +static int __init memory_sanitize_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_init);
> +#endif
> +
>  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..ed26b10 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -449,10 +449,15 @@ static inline void *restore_red_left(struct
> kmem_cache *s, void *p)
>   * Debug settings:
>   */
>  #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 */
>
>  static char *slub_debug_slabs;
>  static int disable_higher_order_debug;
> @@ -5675,6 +5680,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);
> ===
>
> Regards,
> Kaiwan.
>
> > Then, we'd want to add the poison value defaults as you mention:
> >
> > > -Would include the 'new' poison / sanitize values of 0xfe and 0xff
> > > for slab (64 and 32-bit resp), etc etc.
> >
> > And then finally add the exceptions for the "frequently freed" slub
> > caches, as identified by PaX already. This would need the flag
> > defined, the poisoning logic adjusted to check the flag, and for the
> > new kernel command-line options for changing the whether or not the
> > flag is respected (like PaX's "pax_sanitize_slab").
> >
> > > Even if this is (somewhat) correct, my thinking is - correct me I'm
> > > totally wrong here - that the whole purpose of the exercise is to
> > > improve _security_; hence, tying this code into the "debug
> > > features" of the kernel isn't really what we want, yes?
> >
> > In the discussions Laura had with the mm folks, the only realistic
> > path to landing this in the upstream kernel is through these debug
> > features.
> >
> > > Most folks would only use debug during development, if at all -
> > > given all the concerns regarding performance. Here, the objective
> > > is to enable a powerful security feature set. Hence, the config
> > > directives should come under the 'Security Options' menu.
> >
> > We're not close to having a "Kernel Security" menu, so for now, I've
> > wanted to focus on getting the features, and then making the Kconfig
> > menus pretty later.
> >
> > Hopefully that all makes sense! :)
> >
> > -Kees
> >
>
>
>
Kees Cook Feb. 13, 2017, 5:28 p.m. UTC | #2
On Wed, Feb 8, 2017 at 7:37 PM, Kaiwan N Billimoria
<kaiwan@kaiwantech.com> wrote:
> Hi Kees,
>
>> I think CONFIG_MEMORY_SANITIZE would enable:
>>
>> CONFIG_SLUB_DEBUG=y
>> CONFIG_PAGE_POISONING=y
>> CONFIG_PAGE_POISONING_NO_SANITY=y
>>
>> but it would _also_ need to set these kernel command-line variables as
>> if they had been set:
>>
>> page_poison=1
>> slub_debug=P
>>
>> No, the first step would be for the config to only provide the above
>> changes.
>
> Have made a first cut (below). Pl take a look and let me know if okay
> and how I should proceed.
>
> Note-
>  - the printks' are just for checking that options are enabled as required
> (will get rid of them later)
>  - this is on linux-next (x86_64)
>  - dmesg filtered output when booted with this kernel (no page_posion or slub_debug
> cmdline options passed):
>
> $ dmesg |grep "MEMORY_SANITIZE"
> kern  :info  : [  +0.000387] [CONFIG_MEMORY_SANITIZE]: slub_debug = P? yes [0x800]
> kern  :debug : [  +0.000000] [CONFIG_MEMORY_SANITIZE]: page_poisoning_enabled? yes
> $
>
> ===
> 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..8eed6ca 100644
> --- a/mm/Kconfig.debug
> +++ b/mm/Kconfig.debug
> @@ -97,3 +97,11 @@ 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 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?

> +
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 2e647c6..b45bc0a 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
>          .init = init_page_poisoning,
>  };
>
> +#ifdef CONFIG_MEMORY_SANITIZE
> +static int __init memory_sanitize_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_init);
> +#endif

The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.

> +
>  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..ed26b10 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -449,10 +449,15 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p)
>   * Debug settings:
>   */
>  #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()?

>
>  static char *slub_debug_slabs;
>  static int disable_higher_order_debug;
> @@ -5675,6 +5680,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);

Good first step to getting the CONFIG to do something meaningful, thanks!

-Kees
Kaiwan N Billimoria Feb. 14, 2017, 3:01 a.m. UTC | #3
​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)?

>
> > +#ifdef CONFIG_MEMORY_SANITIZE
> > +static int __init memory_sanitize_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_init);
> > +#endif
>
> The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.
>
> ​Agree on the redundancy, will do.


> >  #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?
​

>
> > +#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);
>
> Good first step to getting the CONFIG to do something meaningful, thanks!
>
> -Kees
>
> ​Thanks, happy to help!
- Kaiwan.​

> --
> Kees Cook
> Pixel Security
>
>
Kaiwan N Billimoria Feb. 14, 2017, 8:04 a.m. UTC | #4
>> diff --git a/mm/page_poison.c b/mm/page_poison.c
>> index 2e647c6..b45bc0a 100644
>> --- a/mm/page_poison.c
>> +++ b/mm/page_poison.c
>> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
>>          .init = init_page_poisoning,
>>  };
>>
>> +#ifdef CONFIG_MEMORY_SANITIZE
>> +static int __init memory_sanitize_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_init);
>> +#endif
>
> The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.

An academic question perhaps- am not clear as to why the IS_ENABLED()
is preferable to an #ifdef. I thought eliminating a runtime 'if'
condition (by using an ifdef) is more optimal than the "if
IS_ENABLED(foo)"? Or (probably) does the compiler optimize it out
regardless (assuming that CONFIG_MEMORY_SANITIZE is not selected of
course)?

-Thanks, kaiwan.
Mark Rutland Feb. 14, 2017, 4:19 p.m. UTC | #5
On Tue, Feb 14, 2017 at 01:34:38PM +0530, Kaiwan N Billimoria wrote:
> >> diff --git a/mm/page_poison.c b/mm/page_poison.c
> >> index 2e647c6..b45bc0a 100644
> >> --- a/mm/page_poison.c
> >> +++ b/mm/page_poison.c
> >> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
> >>          .init = init_page_poisoning,
> >>  };
> >>
> >> +#ifdef CONFIG_MEMORY_SANITIZE
> >> +static int __init memory_sanitize_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_init);
> >> +#endif
> >
> > The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.
> 
> An academic question perhaps- am not clear as to why the IS_ENABLED()
> is preferable to an #ifdef. I thought eliminating a runtime 'if'
> condition (by using an ifdef) is more optimal than the "if
> IS_ENABLED(foo)"? Or (probably) does the compiler optimize it out
> regardless (assuming that CONFIG_MEMORY_SANITIZE is not selected of
> course)?

We expect the compiler to optimize the code out, since IS_ENABLED(x) is
a compile-time constant.

As for why it's preferable, it has several benefits.

For one thing, using if (IS_ENABLED(x)) means that we always get build
coverage of the code predicated on the config option, so it's harder to
accidentally break some build configurations.

For another, it composes more nicely with other conditionals, which is
useful when you have a condition that depends on several config options,
or config options and runtime expressions.

e.g. something like:

	if (IS_ENABLED(FOO) && IS_ENABLED(BAR) && runtime_option)
		do_combination_thing();
	else if (IS_ENABLED(BAR))
		do_bar_only_thing();
	else
		do_other_thing();

... is much more painful to write using ifdefs.

Generally, it's nicer for people to deal with than ifdeffery. It's
easier for people to match braces in well-formatted code than it is to
match #ifdef #endif pairs.

Thanks,
Mark.
Kaiwan N Billimoria Feb. 14, 2017, 4:33 p.m. UTC | #6
Thanks for your succinct explanation Mark! Makes sense.
Will incorporate it, as Kees suggests..

On Tue, 14 Feb 2017, 9:49 pm Mark Rutland, <mark.rutland@arm.com> wrote:

> On Tue, Feb 14, 2017 at 01:34:38PM +0530, Kaiwan N Billimoria wrote:
> > >> diff --git a/mm/page_poison.c b/mm/page_poison.c
> > >> index 2e647c6..b45bc0a 100644
> > >> --- a/mm/page_poison.c
> > >> +++ b/mm/page_poison.c
> > >> @@ -49,6 +49,19 @@ struct page_ext_operations page_poisoning_ops = {
> > >>          .init = init_page_poisoning,
> > >>  };
> > >>
> > >> +#ifdef CONFIG_MEMORY_SANITIZE
> > >> +static int __init memory_sanitize_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_init);
> > >> +#endif
> > >
> > > The ifdef and the IS_ENABLED seem redundant to me. I'd drop the ifdef.
> >
> > An academic question perhaps- am not clear as to why the IS_ENABLED()
> > is preferable to an #ifdef. I thought eliminating a runtime 'if'
> > condition (by using an ifdef) is more optimal than the "if
> > IS_ENABLED(foo)"? Or (probably) does the compiler optimize it out
> > regardless (assuming that CONFIG_MEMORY_SANITIZE is not selected of
> > course)?
>
> We expect the compiler to optimize the code out, since IS_ENABLED(x) is
> a compile-time constant.
>
> As for why it's preferable, it has several benefits.
>
> For one thing, using if (IS_ENABLED(x)) means that we always get build
> coverage of the code predicated on the config option, so it's harder to
> accidentally break some build configurations.
>
> For another, it composes more nicely with other conditionals, which is
> useful when you have a condition that depends on several config options,
> or config options and runtime expressions.
>
> e.g. something like:
>
>         if (IS_ENABLED(FOO) && IS_ENABLED(BAR) && runtime_option)
>                 do_combination_thing();
>         else if (IS_ENABLED(BAR))
>                 do_bar_only_thing();
>         else
>                 do_other_thing();
>
> ... is much more painful to write using ifdefs.
>
> Generally, it's nicer for people to deal with than ifdeffery. It's
> easier for people to match braces in well-formatted code than it is to
> match #ifdef #endif pairs.
>
> Thanks,
> Mark.
>
>
Kees Cook Feb. 14, 2017, 5:19 p.m. UTC | #7
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
diff mbox

Patch

===
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..8eed6ca 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -97,3 +97,11 @@  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 if HIBERNATION
+	---help---
+   	This option enables ...
+
diff --git a/mm/page_poison.c b/mm/page_poison.c
index 2e647c6..b45bc0a 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -49,6 +49,19 @@  struct page_ext_operations page_poisoning_ops = {
	 .init = init_page_poisoning,
 };
 
+#ifdef CONFIG_MEMORY_SANITIZE
+static int __init memory_sanitize_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_init);
+#endif
+
 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..ed26b10 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -449,10 +449,15 @@  static inline void *restore_red_left(struct kmem_cache *s, void *p)
  * Debug settings:
  */
 #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 */
 
 static char *slub_debug_slabs;
 static int disable_higher_order_debug;
@@ -5675,6 +5680,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);