| Message ID | 20200907134055.2878499-7-elver@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KFENCE: A low-overhead sampling-based memory safety error detector | expand |
On Mon, Sep 7, 2020 at 3:41 PM Marco Elver <elver@google.com> wrote: > > From: Alexander Potapenko <glider@google.com> > > We make KFENCE compatible with KASAN for testing KFENCE itself. In > particular, KASAN helps to catch any potential corruptions to KFENCE > state, or other corruptions that may be a result of freepointer > corruptions in the main allocators. > > To indicate that the combination of the two is generally discouraged, > CONFIG_EXPERT=y should be set. It also gives us the nice property that > KFENCE will be build-tested by allyesconfig builds. > > Co-developed-by: Marco Elver <elver@google.com> > Signed-off-by: Marco Elver <elver@google.com> > Signed-off-by: Alexander Potapenko <glider@google.com> > --- > lib/Kconfig.kfence | 2 +- > mm/kasan/common.c | 7 +++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/lib/Kconfig.kfence b/lib/Kconfig.kfence > index 7ac91162edb0..b080e49e15d4 100644 > --- a/lib/Kconfig.kfence > +++ b/lib/Kconfig.kfence > @@ -10,7 +10,7 @@ config HAVE_ARCH_KFENCE_STATIC_POOL > > menuconfig KFENCE > bool "KFENCE: low-overhead sampling-based memory safety error detector" > - depends on HAVE_ARCH_KFENCE && !KASAN && (SLAB || SLUB) > + depends on HAVE_ARCH_KFENCE && (!KASAN || EXPERT) && (SLAB || SLUB) > depends on JUMP_LABEL # To ensure performance, require jump labels > select STACKTRACE > help > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 950fd372a07e..f5c49f0fdeff 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -18,6 +18,7 @@ > #include <linux/init.h> > #include <linux/kasan.h> > #include <linux/kernel.h> > +#include <linux/kfence.h> > #include <linux/kmemleak.h> > #include <linux/linkage.h> > #include <linux/memblock.h> > @@ -396,6 +397,9 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object, > tagged_object = object; > object = reset_tag(object); > > + if (is_kfence_address(object)) > + return false; Is this needed? At least in the slab patch I see that we do : if (kfence_free(objp)) { kmemleak_free_recursive(objp, cachep->flags); return; } before: /* Put the object into the quarantine, don't touch it for now. */ /* Put the object into the quarantine, don't touch it for now. */ if (kasan_slab_free(cachep, objp, _RET_IP_)) if (kasan_slab_free(cachep, objp, _RET_IP_)) return; return; If it's not supposed to be triggered, it can make sense to replace with BUG/WARN. > if (unlikely(nearest_obj(cache, virt_to_head_page(object), object) != > object)) { > kasan_report_invalid_free(tagged_object, ip); > @@ -444,6 +448,9 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object, > if (unlikely(object == NULL)) > return NULL; > > + if (is_kfence_address(object)) > + return (void *)object; > + > redzone_start = round_up((unsigned long)(object + size), > KASAN_SHADOW_SCALE_SIZE); > redzone_end = round_up((unsigned long)object + cache->object_size, > -- > 2.28.0.526.ge36021eeef-goog >
On Fri, 11 Sep 2020 at 09:05, Dmitry Vyukov <dvyukov@google.com> wrote: > > On Mon, Sep 7, 2020 at 3:41 PM Marco Elver <elver@google.com> wrote: > > > > From: Alexander Potapenko <glider@google.com> > > > > We make KFENCE compatible with KASAN for testing KFENCE itself. In > > particular, KASAN helps to catch any potential corruptions to KFENCE > > state, or other corruptions that may be a result of freepointer > > corruptions in the main allocators. > > > > To indicate that the combination of the two is generally discouraged, > > CONFIG_EXPERT=y should be set. It also gives us the nice property that > > KFENCE will be build-tested by allyesconfig builds. > > > > Co-developed-by: Marco Elver <elver@google.com> > > Signed-off-by: Marco Elver <elver@google.com> > > Signed-off-by: Alexander Potapenko <glider@google.com> > > --- > > lib/Kconfig.kfence | 2 +- > > mm/kasan/common.c | 7 +++++++ > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/lib/Kconfig.kfence b/lib/Kconfig.kfence > > index 7ac91162edb0..b080e49e15d4 100644 > > --- a/lib/Kconfig.kfence > > +++ b/lib/Kconfig.kfence > > @@ -10,7 +10,7 @@ config HAVE_ARCH_KFENCE_STATIC_POOL > > > > menuconfig KFENCE > > bool "KFENCE: low-overhead sampling-based memory safety error detector" > > - depends on HAVE_ARCH_KFENCE && !KASAN && (SLAB || SLUB) > > + depends on HAVE_ARCH_KFENCE && (!KASAN || EXPERT) && (SLAB || SLUB) > > depends on JUMP_LABEL # To ensure performance, require jump labels > > select STACKTRACE > > help > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > > index 950fd372a07e..f5c49f0fdeff 100644 > > --- a/mm/kasan/common.c > > +++ b/mm/kasan/common.c > > @@ -18,6 +18,7 @@ > > #include <linux/init.h> > > #include <linux/kasan.h> > > #include <linux/kernel.h> > > +#include <linux/kfence.h> > > #include <linux/kmemleak.h> > > #include <linux/linkage.h> > > #include <linux/memblock.h> > > @@ -396,6 +397,9 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object, > > tagged_object = object; > > object = reset_tag(object); > > > > + if (is_kfence_address(object)) > > + return false; > > Is this needed? > At least in the slab patch I see that we do : > > if (kfence_free(objp)) { > kmemleak_free_recursive(objp, cachep->flags); > return; > } > > before: > > /* Put the object into the quarantine, don't touch it for now. */ /* > Put the object into the quarantine, don't touch it for now. */ > if (kasan_slab_free(cachep, objp, _RET_IP_)) if > (kasan_slab_free(cachep, objp, _RET_IP_)) > return; return; > > > If it's not supposed to be triggered, it can make sense to replace > with BUG/WARN. It is required for SLUB. For SLAB, it seems it might not be necessary. Making the check in kasan/common.c conditional on the allocator seems ugly, so I propose we keep it there. Thanks, -- Marco
diff --git a/lib/Kconfig.kfence b/lib/Kconfig.kfence index 7ac91162edb0..b080e49e15d4 100644 --- a/lib/Kconfig.kfence +++ b/lib/Kconfig.kfence @@ -10,7 +10,7 @@ config HAVE_ARCH_KFENCE_STATIC_POOL menuconfig KFENCE bool "KFENCE: low-overhead sampling-based memory safety error detector" - depends on HAVE_ARCH_KFENCE && !KASAN && (SLAB || SLUB) + depends on HAVE_ARCH_KFENCE && (!KASAN || EXPERT) && (SLAB || SLUB) depends on JUMP_LABEL # To ensure performance, require jump labels select STACKTRACE help diff --git a/mm/kasan/common.c b/mm/kasan/common.c index 950fd372a07e..f5c49f0fdeff 100644 --- a/mm/kasan/common.c +++ b/mm/kasan/common.c @@ -18,6 +18,7 @@ #include <linux/init.h> #include <linux/kasan.h> #include <linux/kernel.h> +#include <linux/kfence.h> #include <linux/kmemleak.h> #include <linux/linkage.h> #include <linux/memblock.h> @@ -396,6 +397,9 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object, tagged_object = object; object = reset_tag(object); + if (is_kfence_address(object)) + return false; + if (unlikely(nearest_obj(cache, virt_to_head_page(object), object) != object)) { kasan_report_invalid_free(tagged_object, ip); @@ -444,6 +448,9 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object, if (unlikely(object == NULL)) return NULL; + if (is_kfence_address(object)) + return (void *)object; + redzone_start = round_up((unsigned long)(object + size), KASAN_SHADOW_SCALE_SIZE); redzone_end = round_up((unsigned long)object + cache->object_size,