| Message ID | 20170225003232.GA123380@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2017-02-24 at 16:32 -0800, Kees Cook wrote: > This enables page and slab poisoning by default under > CONFIG_MEMORY_SANITIZE. > Based on work by Kaiwan N Billimoria. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > This is more what I had in mind. This is based on the latest patch, > but > handles slab and slub. I tested slub with lkdtm's READ_AFTER_FREE and > READ_BUDDY_AFTER_FREE which both trip the poisoning tests. > > While doing this, I noticed one major issue with slub/slab poisoning: > it performs poisoning both on alloc and free, which is a rather large > performance hit, so the next step is likely to find a way to split the > poisoning into alloc and free halves so that CONFIG_MEMORY_SANITIZE > poisoning will only happen on the free. It also still fills with a value that will make pointers point to userspace, right? Rather than a better value or zeroing (and relying on mmap_min_addr, although that's pretty small and easy to index past). It also verifies the poisoning is still there, which can be wanted but is more expensive. It increases allocations by the size of a pointer which PaX doesn't do anymore, and there's the whole losing the fast path which seems to mean losing CPU caching too since it's just hitting the global stuff. The debug infrastructure could also be a risk, that would not be present in normal builds up to this point. I would like to be able to use upstream features but I think case it seems that anyone not willing to unnecessarily lose performance and security properties will need to keep carrying ~15 line of code out of tree or using the PaX features. Is there a path to doing it properly from here? I really don't see the point in it being upstreaming if it's such a hack. It's frustrating... Not to mention that real allocator hardening like not having inline free lists and being able to reliably detect double-free would be nice. Maybe there should just be a security-oriented slab allocator, freeing it from needing to avoid stepping on other people's toes. It just needs to pick a performance target and then the design can shift away from the current one within that target over time. *shrug*
On Sat, Feb 25, 2017 at 12:36 AM, Daniel Micay <danielmicay@gmail.com> wrote: > On Fri, 2017-02-24 at 16:32 -0800, Kees Cook wrote: >> This enables page and slab poisoning by default under >> CONFIG_MEMORY_SANITIZE. >> Based on work by Kaiwan N Billimoria. >> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> This is more what I had in mind. This is based on the latest patch, >> but >> handles slab and slub. I tested slub with lkdtm's READ_AFTER_FREE and >> READ_BUDDY_AFTER_FREE which both trip the poisoning tests. >> >> While doing this, I noticed one major issue with slub/slab poisoning: >> it performs poisoning both on alloc and free, which is a rather large >> performance hit, so the next step is likely to find a way to split the >> poisoning into alloc and free halves so that CONFIG_MEMORY_SANITIZE >> poisoning will only happen on the free. > > It also still fills with a value that will make pointers point to > userspace, right? Rather than a better value or zeroing (and relying on > mmap_min_addr, although that's pretty small and easy to index past). Correct. We have to get there in pieces, though I continue to think it would still be easier to make the sanitization feature separate from debugging. In PaX, the poison value on x86_64 is 0xfe (to make addresses aim into the noncanonical range), otherwise 0xff (to point at the top of memory). > It also verifies the poisoning is still there, which can be wanted but > is more expensive. It increases allocations by the size of a pointer > which PaX doesn't do anymore, and there's the whole losing the fast path > which seems to mean losing CPU caching too since it's just hitting the > global stuff. The debug infrastructure could also be a risk, that would > not be present in normal builds up to this point. Gathering the reasons against this being part of the debug path would be a nice thing to collect; it might be convincing to the allocator maintainers. > I would like to be able to use upstream features but I think case it > seems that anyone not willing to unnecessarily lose performance and > security properties will need to keep carrying ~15 line of code out of > tree or using the PaX features. The simple form (without the SLAB_NO_SANITIZE) is alarmingly small. :) > Is there a path to doing it properly from here? I really don't see the > point in it being upstreaming if it's such a hack. It's frustrating... > > Not to mention that real allocator hardening like not having inline free > lists and being able to reliably detect double-free would be nice. Maybe > there should just be a security-oriented slab allocator, freeing it from > needing to avoid stepping on other people's toes. It just needs to pick > a performance target and then the design can shift away from the current > one within that target over time. *shrug* I'd love to see someone step up and create this for upstream. I think it'd make a lot of sense instead of trying to shoe-horn things into SLUB... -Kees
Hi Kees, > I'd love to see someone step up and create this for upstream. I think > it'd make a lot of sense instead of trying to shoe-horn things into > SLUB... > Ok, am unsure if I clearly understand all the issues involved; but of course it's always better to make a start and then evolve. So, how exactly can this be tackled? Do we go down the "new SLUB for security" path? And, if yes, then how exactly does one get started? I'll need some pointers pl... Regards, Kaiwan. On Thu, Mar 2, 2017 at 12:46 AM, Kees Cook <keescook@chromium.org> wrote: > On Sat, Feb 25, 2017 at 12:36 AM, Daniel Micay <danielmicay@gmail.com> wrote: >> On Fri, 2017-02-24 at 16:32 -0800, Kees Cook wrote: >>> This enables page and slab poisoning by default under >>> CONFIG_MEMORY_SANITIZE. >>> Based on work by Kaiwan N Billimoria. >>> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> This is more what I had in mind. This is based on the latest patch, >>> but >>> handles slab and slub. I tested slub with lkdtm's READ_AFTER_FREE and >>> READ_BUDDY_AFTER_FREE which both trip the poisoning tests. >>> >>> While doing this, I noticed one major issue with slub/slab poisoning: >>> it performs poisoning both on alloc and free, which is a rather large >>> performance hit, so the next step is likely to find a way to split the >>> poisoning into alloc and free halves so that CONFIG_MEMORY_SANITIZE >>> poisoning will only happen on the free. >> >> It also still fills with a value that will make pointers point to >> userspace, right? Rather than a better value or zeroing (and relying on >> mmap_min_addr, although that's pretty small and easy to index past). > > Correct. We have to get there in pieces, though I continue to think it > would still be easier to make the sanitization feature separate from > debugging. > > In PaX, the poison value on x86_64 is 0xfe (to make addresses aim into > the noncanonical range), otherwise 0xff (to point at the top of > memory). > >> It also verifies the poisoning is still there, which can be wanted but >> is more expensive. It increases allocations by the size of a pointer >> which PaX doesn't do anymore, and there's the whole losing the fast path >> which seems to mean losing CPU caching too since it's just hitting the >> global stuff. The debug infrastructure could also be a risk, that would >> not be present in normal builds up to this point. > > Gathering the reasons against this being part of the debug path would > be a nice thing to collect; it might be convincing to the allocator > maintainers. > >> I would like to be able to use upstream features but I think case it >> seems that anyone not willing to unnecessarily lose performance and >> security properties will need to keep carrying ~15 line of code out of >> tree or using the PaX features. > > The simple form (without the SLAB_NO_SANITIZE) is alarmingly small. :) > >> Is there a path to doing it properly from here? I really don't see the >> point in it being upstreaming if it's such a hack. It's frustrating... >> >> Not to mention that real allocator hardening like not having inline free >> lists and being able to reliably detect double-free would be nice. Maybe >> there should just be a security-oriented slab allocator, freeing it from >> needing to avoid stepping on other people's toes. It just needs to pick >> a performance target and then the design can shift away from the current >> one within that target over time. *shrug* > > I'd love to see someone step up and create this for upstream. I think > it'd make a lot of sense instead of trying to shoe-horn things into > SLUB... > > -Kees > > -- > Kees Cook > Pixel Security >
On Mon, Mar 27, 2017 at 3:54 AM, Kaiwan N Billimoria <kaiwan@kaiwantech.com> wrote: > On Thu, Mar 2, 2017 at 12:46 AM, Kees Cook <keescook@chromium.org> wrote: >> I'd love to see someone step up and create this for upstream. I think >> it'd make a lot of sense instead of trying to shoe-horn things into >> SLUB... >> > Ok, am unsure if I clearly understand all the issues involved; but of > course it's always better to make a start and then evolve. So, how > exactly can this be tackled? Do we go down the "new SLUB for security" > path? And, if yes, then how exactly does one get started? I'll need > some pointers pl... Well, mainly it would need someone dedicated to creating a whole new slab allocator for the kernel, and prioritizing security for it. Daniel has a bunch of ideas on this, but I don't know enough currently to make suggestions for what the design should look like. Making sanity-checks fast would be a driving principle, though. :) -Kees
On Fri, Feb 24, 2017 at 4:32 PM, Kees Cook <keescook@chromium.org> wrote: > This enables page and slab poisoning by default under CONFIG_MEMORY_SANITIZE. > Based on work by Kaiwan N Billimoria. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > This is more what I had in mind. This is based on the latest patch, but > handles slab and slub. I tested slub with lkdtm's READ_AFTER_FREE and > READ_BUDDY_AFTER_FREE which both trip the poisoning tests. > > While doing this, I noticed one major issue with slub/slab poisoning: > it performs poisoning both on alloc and free, which is a rather large > performance hit, so the next step is likely to find a way to split the > poisoning into alloc and free halves so that CONFIG_MEMORY_SANITIZE > poisoning will only happen on the free. While this has performance flaws (to say the least), what do people think of this general approach to dealing with having a single CONFIG to control this at least? Should I (or someone) see what the slab cache maintainers think of this? -Kees > --- > init/main.c | 5 +++++ > mm/Kconfig.debug | 23 +++++++++++++++++++++++ > mm/page_poison.c | 3 ++- > mm/slab.c | 4 ++++ > mm/slub.c | 7 +++++++ > 5 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/init/main.c b/init/main.c > index 24ea48745061..e5f571bfe56f 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1030,6 +1030,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 afcc550877ff..910a7a359b96 100644 > --- a/mm/Kconfig.debug > +++ b/mm/Kconfig.debug > @@ -90,3 +90,26 @@ config DEBUG_PAGE_REF > careful when enabling this feature because it adds about 30 KB to the > kernel code. However the runtime performance overhead is virtually > nil until the tracepoints are actually enabled. > + > +config MEMORY_SANITIZE > + bool "Enable memory sanitization features" > + depends on SLUB || SLAB > + select SLUB_DEBUG if SLUB > + 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: > + - page poisoning (without sanity checking) > + - kmem_cache poisoning > + > + 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=SLAB_POISON). > + - page poisoning, equivalent to passing the kernel command-line > + option 'page_poison=on'. > + > + Of course, kernel command-line options 'page_poison' and 'slub_debug' > + are still honored. > + > + If unsure, say N. > diff --git a/mm/page_poison.c b/mm/page_poison.c > index 2e647c65916b..6f7e37c8ac2f 100644 > --- a/mm/page_poison.c > +++ b/mm/page_poison.c > @@ -7,7 +7,8 @@ > #include <linux/ratelimit.h> > > static bool __page_poisoning_enabled __read_mostly; > -static bool want_page_poisoning __read_mostly; > +static bool want_page_poisoning __read_mostly = > + IS_ENABLED(CONFIG_MEMORY_SANITIZE); > > static int early_page_poison_param(char *buf) > { > diff --git a/mm/slab.c b/mm/slab.c > index bd63450a9b16..1462b0b8b0a0 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -1894,6 +1894,10 @@ unsigned long kmem_cache_flags(unsigned long object_size, > unsigned long flags, const char *name, > void (*ctor)(void *)) > { > +#ifdef CONFIG_MEMORY_SANITIZE > + flags |= SLAB_POISON; > +#endif > + > return flags; > } > > diff --git a/mm/slub.c b/mm/slub.c > index 7f4bc7027ed5..5041f42c942b 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -452,6 +452,8 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p) > */ > #if defined(CONFIG_SLUB_DEBUG_ON) > static int slub_debug = DEBUG_DEFAULT_FLAGS; > +#elif defined(CONFIG_MEMORY_SANITIZE) > +static int slub_debug = SLAB_POISON; > #else > static int slub_debug; > #endif > @@ -5755,6 +5757,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); > -- > 2.7.4 > > > -- > Kees Cook > Pixel Security
diff --git a/init/main.c b/init/main.c index 24ea48745061..e5f571bfe56f 100644 --- a/init/main.c +++ b/init/main.c @@ -1030,6 +1030,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 afcc550877ff..910a7a359b96 100644 --- a/mm/Kconfig.debug +++ b/mm/Kconfig.debug @@ -90,3 +90,26 @@ config DEBUG_PAGE_REF careful when enabling this feature because it adds about 30 KB to the kernel code. However the runtime performance overhead is virtually nil until the tracepoints are actually enabled. + +config MEMORY_SANITIZE + bool "Enable memory sanitization features" + depends on SLUB || SLAB + select SLUB_DEBUG if SLUB + 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: + - page poisoning (without sanity checking) + - kmem_cache poisoning + + 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=SLAB_POISON). + - page poisoning, equivalent to passing the kernel command-line + option 'page_poison=on'. + + Of course, kernel command-line options 'page_poison' and 'slub_debug' + are still honored. + + If unsure, say N. diff --git a/mm/page_poison.c b/mm/page_poison.c index 2e647c65916b..6f7e37c8ac2f 100644 --- a/mm/page_poison.c +++ b/mm/page_poison.c @@ -7,7 +7,8 @@ #include <linux/ratelimit.h> static bool __page_poisoning_enabled __read_mostly; -static bool want_page_poisoning __read_mostly; +static bool want_page_poisoning __read_mostly = + IS_ENABLED(CONFIG_MEMORY_SANITIZE); static int early_page_poison_param(char *buf) { diff --git a/mm/slab.c b/mm/slab.c index bd63450a9b16..1462b0b8b0a0 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -1894,6 +1894,10 @@ unsigned long kmem_cache_flags(unsigned long object_size, unsigned long flags, const char *name, void (*ctor)(void *)) { +#ifdef CONFIG_MEMORY_SANITIZE + flags |= SLAB_POISON; +#endif + return flags; } diff --git a/mm/slub.c b/mm/slub.c index 7f4bc7027ed5..5041f42c942b 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -452,6 +452,8 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p) */ #if defined(CONFIG_SLUB_DEBUG_ON) static int slub_debug = DEBUG_DEFAULT_FLAGS; +#elif defined(CONFIG_MEMORY_SANITIZE) +static int slub_debug = SLAB_POISON; #else static int slub_debug; #endif @@ -5755,6 +5757,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);
This enables page and slab poisoning by default under CONFIG_MEMORY_SANITIZE. Based on work by Kaiwan N Billimoria. Signed-off-by: Kees Cook <keescook@chromium.org> --- This is more what I had in mind. This is based on the latest patch, but handles slab and slub. I tested slub with lkdtm's READ_AFTER_FREE and READ_BUDDY_AFTER_FREE which both trip the poisoning tests. While doing this, I noticed one major issue with slub/slab poisoning: it performs poisoning both on alloc and free, which is a rather large performance hit, so the next step is likely to find a way to split the poisoning into alloc and free halves so that CONFIG_MEMORY_SANITIZE poisoning will only happen on the free. --- init/main.c | 5 +++++ mm/Kconfig.debug | 23 +++++++++++++++++++++++ mm/page_poison.c | 3 ++- mm/slab.c | 4 ++++ mm/slub.c | 7 +++++++ 5 files changed, 41 insertions(+), 1 deletion(-)