| Message ID | 20190308132701.133598-3-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: introduce CONFIG_INIT_ALL_MEMORY | expand |
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
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
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 >
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.
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
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.
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 > > >
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
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(+)