[v3,2/2] initmem: introduce CONFIG_INIT_ALL_HEAP
diff mbox series

Message ID 20190408170418.148554-3-glider@google.com
State New
Headers show
Series
  • RFC: introduce CONFIG_INIT_ALL_MEMORY
Related show

Commit Message

Alexander Potapenko April 8, 2019, 5:04 p.m. UTC
This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
without the need to pass any boot parameters.

No performance optimizations are done at the moment to reduce double
initialization of memory regions.

Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: linux-security-module@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
 v3:
  - addressed comments by Masahiro Yamada (Kconfig fixes)
---
 mm/page_poison.c         |  5 +++++
 mm/slub.c                |  2 ++
 security/Kconfig.initmem | 11 +++++++++++
 3 files changed, 18 insertions(+)

Comments

Jann Horn April 8, 2019, 5:39 p.m. UTC | #1
On Mon, Apr 8, 2019 at 7:20 PM Alexander Potapenko <glider@google.com> wrote:
> This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
> without the need to pass any boot parameters.
>
> No performance optimizations are done at the moment to reduce double
> initialization of memory regions.
[...]
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 21d4f97cb49b..a1985f33f635 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -12,9 +12,14 @@ static bool want_page_poisoning __read_mostly;
>
>  static int __init early_page_poison_param(char *buf)
>  {
> +#ifdef CONFIG_INIT_ALL_HEAP
> +       want_page_poisoning = true;
> +       return 0;
> +#else
>         if (!buf)
>                 return -EINVAL;
>         return strtobool(buf, &want_page_poisoning);
> +#endif
>  }
>  early_param("page_poison", early_page_poison_param);
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 1b08fbcb7e61..00e0197d3f35 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1287,6 +1287,8 @@ static int __init setup_slub_debug(char *str)
>         if (*str == ',')
>                 slub_debug_slabs = str + 1;
>  out:
> +       if (IS_ENABLED(CONFIG_INIT_ALL_HEAP))
> +               slub_debug |= SLAB_POISON;
>         return 1;
>  }

I don't understand how this is supposed to work. As far as I can tell,
the "slub_debug |= SLAB_POISON;" only happens if you actually pass in
a "slub_debug" boot parameter? Same thing for "want_page_poisoning =
true;".

Also, didn't Laura suggest in
https://www.openwall.com/lists/kernel-hardening/2019/04/08/4 that a
different approach might be more sensible to reduce the performance
hit?
Mark Rutland April 9, 2019, 9:32 a.m. UTC | #2
On Mon, Apr 08, 2019 at 07:04:18PM +0200, Alexander Potapenko wrote:
> This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
> without the need to pass any boot parameters.
> 
> No performance optimizations are done at the moment to reduce double
> initialization of memory regions.
> 
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Kostya Serebryany <kcc@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Sandeep Patil <sspatil@android.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-kbuild@vger.kernel.org
> Cc: kernel-hardening@lists.openwall.com
> ---
>  v3:
>   - addressed comments by Masahiro Yamada (Kconfig fixes)
> ---
>  mm/page_poison.c         |  5 +++++
>  mm/slub.c                |  2 ++
>  security/Kconfig.initmem | 11 +++++++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 21d4f97cb49b..a1985f33f635 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -12,9 +12,14 @@ static bool want_page_poisoning __read_mostly;
>  
>  static int __init early_page_poison_param(char *buf)
>  {
> +#ifdef CONFIG_INIT_ALL_HEAP
> +	want_page_poisoning = true;
> +	return 0;
> +#else
>  	if (!buf)
>  		return -EINVAL;
>  	return strtobool(buf, &want_page_poisoning);
> +#endif
>  }
>  early_param("page_poison", early_page_poison_param);

IIUC this is only called if page_poison is passed on the command line,
so want_page_poisoning won't be set automatically unless that's passed.

Presumably you want to initialize it at definition with:

static bool want_page_poisoning __read_mostly = IS_ENABLED(CONFIG_INIT_ALL_HEAP);

with:

#ifndef CONFIG_INIT_ALL_HEAP
static int __init early_page_poison_param(char *buf)
{
	...
}
early_param("page_poison", early_page_poison_param);
#endif

... so that it can't be disabled?

Thanks,
Mark.
Alexander Potapenko April 9, 2019, 9:53 a.m. UTC | #3
On Tue, Apr 9, 2019 at 11:32 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Mon, Apr 08, 2019 at 07:04:18PM +0200, Alexander Potapenko wrote:
> > This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
> > without the need to pass any boot parameters.
> >
> > No performance optimizations are done at the moment to reduce double
> > initialization of memory regions.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Kostya Serebryany <kcc@google.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Sandeep Patil <sspatil@android.com>
> > Cc: linux-security-module@vger.kernel.org
> > Cc: linux-kbuild@vger.kernel.org
> > Cc: kernel-hardening@lists.openwall.com
> > ---
> >  v3:
> >   - addressed comments by Masahiro Yamada (Kconfig fixes)
> > ---
> >  mm/page_poison.c         |  5 +++++
> >  mm/slub.c                |  2 ++
> >  security/Kconfig.initmem | 11 +++++++++++
> >  3 files changed, 18 insertions(+)
> >
> > diff --git a/mm/page_poison.c b/mm/page_poison.c
> > index 21d4f97cb49b..a1985f33f635 100644
> > --- a/mm/page_poison.c
> > +++ b/mm/page_poison.c
> > @@ -12,9 +12,14 @@ static bool want_page_poisoning __read_mostly;
> >
> >  static int __init early_page_poison_param(char *buf)
> >  {
> > +#ifdef CONFIG_INIT_ALL_HEAP
> > +     want_page_poisoning = true;
> > +     return 0;
> > +#else
> >       if (!buf)
> >               return -EINVAL;
> >       return strtobool(buf, &want_page_poisoning);
> > +#endif
> >  }
> >  early_param("page_poison", early_page_poison_param);
>
> IIUC this is only called if page_poison is passed on the command line,
> so want_page_poisoning won't be set automatically unless that's passed.
>
> Presumably you want to initialize it at definition with:
>
> static bool want_page_poisoning __read_mostly = IS_ENABLED(CONFIG_INIT_ALL_HEAP);
Yes, I've misunderstood how boot parameters work.
Thanks for the suggestions!
We'd better look into wiping allocations without POISON_SLAB though.
> with:
>
> #ifndef CONFIG_INIT_ALL_HEAP
> static int __init early_page_poison_param(char *buf)
> {
>         ...
> }
> early_param("page_poison", early_page_poison_param);
> #endif
>
> ... so that it can't be disabled?
>
> Thanks,
> Mark.

Patch
diff mbox series

diff --git a/mm/page_poison.c b/mm/page_poison.c
index 21d4f97cb49b..a1985f33f635 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -12,9 +12,14 @@  static bool want_page_poisoning __read_mostly;
 
 static int __init early_page_poison_param(char *buf)
 {
+#ifdef CONFIG_INIT_ALL_HEAP
+	want_page_poisoning = true;
+	return 0;
+#else
 	if (!buf)
 		return -EINVAL;
 	return strtobool(buf, &want_page_poisoning);
+#endif
 }
 early_param("page_poison", early_page_poison_param);
 
diff --git a/mm/slub.c b/mm/slub.c
index 1b08fbcb7e61..00e0197d3f35 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1287,6 +1287,8 @@  static int __init setup_slub_debug(char *str)
 	if (*str == ',')
 		slub_debug_slabs = str + 1;
 out:
+	if (IS_ENABLED(CONFIG_INIT_ALL_HEAP))
+		slub_debug |= SLAB_POISON;
 	return 1;
 }
 
diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
index 5e49a55382ad..37cc10a2eeb5 100644
--- a/security/Kconfig.initmem
+++ b/security/Kconfig.initmem
@@ -13,6 +13,17 @@  config INIT_ALL_MEMORY
 
 if INIT_ALL_MEMORY
 
+config INIT_ALL_HEAP
+	bool "Initialize all heap"
+	depends on INIT_ALL_MEMORY
+	select PAGE_POISONING
+	select PAGE_POISONING_NO_SANITY
+	select PAGE_POISONING_ZERO
+	select SLUB_DEBUG if SLUB
+	default y
+	help
+	  Enable page poisoning and SLUB poisoning by default.
+
 config INIT_ALL_STACK
 	bool "Initialize all stack"
 	depends on INIT_ALL_MEMORY