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

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

Commit Message

Alexander Potapenko March 8, 2019, 1:27 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
---
 mm/page_poison.c         |  5 +++++
 mm/slub.c                |  2 ++
 security/Kconfig.initmem | 11 +++++++++++
 3 files changed, 18 insertions(+)

Comments

Masahiro Yamada April 5, 2019, 11:35 a.m. UTC | #1
On Fri, Mar 8, 2019 at 10:27 PM Alexander Potapenko <glider@google.com> wrote:
>
> diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> index 27aec394365e..5ce49663777a 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 CONFIG_PAGE_POISONING
> +       select CONFIG_PAGE_POISONING_NO_SANITY
> +       select CONFIG_PAGE_POISONING_ZERO
> +       select CONFIG_SLUB_DEBUG

This should like follows (no CONFIG_ prefix):

         select PAGE_POISONING
         select PAGE_POISONING_NO_SANITY
         select PAGE_POISONING_ZERO
         select SLUB_DEBUG

But, again, this causes unmet dependency if SLUB=n





> +       default y
> +       help
> +         Enable page poisoning and slub poisoning by default.
> +
>  config INIT_ALL_STACK
>         bool "Initialize all stack"
>         depends on INIT_ALL_MEMORY
> --
> 2.21.0.360.g471c308f928-goog
>


--
Best Regards
Masahiro Yamada
Alexander Potapenko April 5, 2019, 2 p.m. UTC | #2
On Fri, Apr 5, 2019 at 1:36 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Fri, Mar 8, 2019 at 10:27 PM Alexander Potapenko <glider@google.com> wrote:
> >
> > diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> > index 27aec394365e..5ce49663777a 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 CONFIG_PAGE_POISONING
> > +       select CONFIG_PAGE_POISONING_NO_SANITY
> > +       select CONFIG_PAGE_POISONING_ZERO
> > +       select CONFIG_SLUB_DEBUG
>
> This should like follows (no CONFIG_ prefix):
>
>          select PAGE_POISONING
>          select PAGE_POISONING_NO_SANITY
>          select PAGE_POISONING_ZERO
>          select SLUB_DEBUG
Thanks!
> But, again, this causes unmet dependency if SLUB=n
          select SLUB_DEBUG if SLUB
seems to help. Guess it's better than making CONFIG_INIT_ALL_HEAP
depend on 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
> > --
> > 2.21.0.360.g471c308f928-goog
> >
>
>
> --
> Best Regards
> Masahiro Yamada
Laura Abbott April 8, 2019, 4:43 p.m. UTC | #3
On 3/8/19 5:27 AM, 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
> ---
>   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);
>   
> 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've looked at doing something similar in the past (failing to find
the thread this morning...) and while this will work, it has pretty
serious performance issues. It's not actually the poisoning which
is expensive but that turning on debugging removes the cpu slab
which has significant performance penalties.

I'd rather go back to the proposal of just poisoning the slab
at alloc/free without using SLAB_POISON.

Thanks,
Laura


> diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> index 27aec394365e..5ce49663777a 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 CONFIG_PAGE_POISONING
> +	select CONFIG_PAGE_POISONING_NO_SANITY
> +	select CONFIG_PAGE_POISONING_ZERO
> +	select CONFIG_SLUB_DEBUG
> +	default y
> +	help
> +	  Enable page poisoning and slub poisoning by default.
> +
>   config INIT_ALL_STACK
>   	bool "Initialize all stack"
>   	depends on INIT_ALL_MEMORY
>
Kees Cook April 8, 2019, 5:14 p.m. UTC | #4
On Mon, Apr 8, 2019 at 9:43 AM Laura Abbott <labbott@redhat.com> wrote:
> I've looked at doing something similar in the past (failing to find
> the thread this morning...) and while this will work, it has pretty
> serious performance issues. It's not actually the poisoning which
> is expensive but that turning on debugging removes the cpu slab
> which has significant performance penalties.
>
> I'd rather go back to the proposal of just poisoning the slab
> at alloc/free without using SLAB_POISON.

I still agree this would make the most sense. Fundamentally it's not a
debugging feature.
Alexander Potapenko April 9, 2019, 8:55 a.m. UTC | #5
On Mon, Apr 8, 2019 at 7:14 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Apr 8, 2019 at 9:43 AM Laura Abbott <labbott@redhat.com> wrote:
> > I've looked at doing something similar in the past (failing to find
> > the thread this morning...) and while this will work, it has pretty
> > serious performance issues. It's not actually the poisoning which
> > is expensive but that turning on debugging removes the cpu slab
> > which has significant performance penalties.
> >
> > I'd rather go back to the proposal of just poisoning the slab
> > at alloc/free without using SLAB_POISON.
>
> I still agree this would make the most sense. Fundamentally it's not a
> debugging feature.
Wasn't it you who suggested using SLAB_POISON in the first round of comments? :)

I actually have a working implementation that piggybacks on existing
__GFP_ZERO code to initialize page_alloc() and SLUB allocations:
https://github.com/google/kmsan/commit/4907af529ad525378a0728435c96d3812f71e594
https://github.com/google/kmsan/commit/69618a9668bcf27700cc5da42eebf8ab50d1f56a

I'd better cook a patch based on that.

There's also some overhead when allocations are initialized twice (in
page_alloc() and kmalloc()) or thrice (page_alloc(), kmalloc() and
e.g. sock_alloc_send_pskb())
We can introduce another GFP flag explicitly telling the allocator to
not initialize the memory chunk if we know it'll be initialized by a
higher level allocator
(something along the lines of
https://github.com/google/kmsan/commit/4fc8cff79d868c29688c8a4186e504fda362f6fd)
> --
> Kees Cook
Kees Cook April 9, 2019, 5:01 p.m. UTC | #6
On Tue, Apr 9, 2019 at 1:55 AM Alexander Potapenko <glider@google.com> wrote:
>
> On Mon, Apr 8, 2019 at 7:14 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Apr 8, 2019 at 9:43 AM Laura Abbott <labbott@redhat.com> wrote:
> > > I've looked at doing something similar in the past (failing to find
> > > the thread this morning...) and while this will work, it has pretty
> > > serious performance issues. It's not actually the poisoning which
> > > is expensive but that turning on debugging removes the cpu slab
> > > which has significant performance penalties.
> > >
> > > I'd rather go back to the proposal of just poisoning the slab
> > > at alloc/free without using SLAB_POISON.
> >
> > I still agree this would make the most sense. Fundamentally it's not a
> > debugging feature.
> Wasn't it you who suggested using SLAB_POISON in the first round of comments? :)

Sure, if we want to use what we have right now, that's the way to go.
Optimally, I'd like to see it done the way Laura mentioned, but that's
a long road to convince the heap maintainers, etc.

> I actually have a working implementation that piggybacks on existing
> __GFP_ZERO code to initialize page_alloc() and SLUB allocations:
> https://github.com/google/kmsan/commit/4907af529ad525378a0728435c96d3812f71e594
> https://github.com/google/kmsan/commit/69618a9668bcf27700cc5da42eebf8ab50d1f56a
>
> I'd better cook a patch based on that.

I think it's still better to zero at free (this reduces the lifetime
of the data in memory and should make some use-after-tree bugs stand
out more), but we'll need to do something like what you have here for
doing memory tagging anyway, so we likely need both.

> There's also some overhead when allocations are initialized twice (in
> page_alloc() and kmalloc()) or thrice (page_alloc(), kmalloc() and
> e.g. sock_alloc_send_pskb())
> We can introduce another GFP flag explicitly telling the allocator to
> not initialize the memory chunk if we know it'll be initialized by a
> higher level allocator
> (something along the lines of
> https://github.com/google/kmsan/commit/4fc8cff79d868c29688c8a4186e504fda362f6fd)

Agreed.
Alexander Potapenko April 18, 2019, 1:02 p.m. UTC | #7
On Mon, Apr 8, 2019 at 6:43 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 3/8/19 5:27 AM, 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
> > ---
> >   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);
> >
> > 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've looked at doing something similar in the past (failing to find
> the thread this morning...) and while this will work, it has pretty
> serious performance issues. It's not actually the poisoning which
> is expensive but that turning on debugging removes the cpu slab
> which has significant performance penalties.
>
> I'd rather go back to the proposal of just poisoning the slab
> at alloc/free without using SLAB_POISON.
Hi Laura,

May I wonder what were the performance numbers you were seeing?
I've found this patch:
https://www.openwall.com/lists/kernel-hardening/2016/01/26/1, but
that's around 100% slowdown.
> Thanks,
> Laura
>
>
> > diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> > index 27aec394365e..5ce49663777a 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 CONFIG_PAGE_POISONING
> > +     select CONFIG_PAGE_POISONING_NO_SANITY
> > +     select CONFIG_PAGE_POISONING_ZERO
> > +     select CONFIG_SLUB_DEBUG
> > +     default y
> > +     help
> > +       Enable page poisoning and slub poisoning by default.
> > +
> >   config INIT_ALL_STACK
> >       bool "Initialize all stack"
> >       depends on INIT_ALL_MEMORY
> >
>

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 27aec394365e..5ce49663777a 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 CONFIG_PAGE_POISONING
+	select CONFIG_PAGE_POISONING_NO_SANITY
+	select CONFIG_PAGE_POISONING_ZERO
+	select CONFIG_SLUB_DEBUG
+	default y
+	help
+	  Enable page poisoning and slub poisoning by default.
+
 config INIT_ALL_STACK
 	bool "Initialize all stack"
 	depends on INIT_ALL_MEMORY