* [PATCH v5 1/2] mm, kasan: improve double-free detection
@ 2016-06-07 18:03 Kuthonuzo Luruo
2016-06-09 13:32 ` Alexander Potapenko
2016-06-09 17:00 ` Andrey Ryabinin
0 siblings, 2 replies; 10+ messages in thread
From: Kuthonuzo Luruo @ 2016-06-07 18:03 UTC (permalink / raw)
To: aryabinin, glider, dvyukov, cl, penberg, rientjes, iamjoonsoo.kim, akpm
Cc: kasan-dev, linux-kernel, ynorov, kuthonuzo.luruo
Currently, KASAN may fail to detect concurrent deallocations of the same
object due to a race in kasan_slab_free(). This patch makes double-free
detection more reliable by serializing access to KASAN object metadata.
New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
lock/unlock per-object metadata. Double-free errors are now reported via
kasan_report().
Per-object lock concept from suggestion/observations by Dmitry Vyukov.
Testing:
- Tested with a modified version of the 'slab_test' microbenchmark where
allocs occur on CPU 0; then all other CPUs concurrently attempt to free
the same object.
- Tested with new double-free tests for 'test_kasan' in accompanying patch.
Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
---
Changes in v5:
- Removed redundant print of 'alloc_state' for pr_err in kasan_slab_free.
Changes in v4:
- Takes use of shadow memory in v3 further by storing lock bit in shadow
byte for object header solving the issue of OOB nuking the header lock.
Allocation state is also stored in the shadow header.
- CAS loop using cmpxchg() byte similar to v2 is brought back.
Unfortunately, standard lock primitives cannot be used since there aren't
enough header/redzone shadow bytes for smaller objects. (A generic
bit_spinlock() operating on a byte would have been sweet... :).
- CAS loop with union magic is largely due to suggestions on patch v2 from
Dmitry Vyukov. Thanks.
- preempt enable/disable inside CAS loop to reduce contention is from
review comment on v2 patch from Yury Norov. Thanks.
Changes in v3:
- simplified kasan_meta_lock()/unlock() to use generic bit spinlock apis;
kasan_alloc_meta structure modified accordingly.
- introduced a 'safety valve' for kasan_meta_lock() to prevent a kfree from
getting stuck when a prior out-of-bounds write clobbers the object
header.
- removed potentially unsafe __builtin_return_address(1) from
kasan_report() call per review comment from Yury Norov; callee now passed
into kasan_slab_free().
---
include/linux/kasan.h | 7 ++-
mm/kasan/kasan.c | 125 ++++++++++++++++++++++++++++++++++++++-----------
mm/kasan/kasan.h | 24 +++++++++-
mm/kasan/quarantine.c | 4 +-
mm/kasan/report.c | 24 +++++++++-
mm/slab.c | 3 +-
mm/slub.c | 2 +-
7 files changed, 153 insertions(+), 36 deletions(-)
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 611927f..3db974b 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -53,6 +53,7 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
void kasan_cache_shrink(struct kmem_cache *cache);
void kasan_cache_destroy(struct kmem_cache *cache);
+void kasan_init_object(struct kmem_cache *cache, void *object);
void kasan_poison_slab(struct page *page);
void kasan_unpoison_object_data(struct kmem_cache *cache, void *object);
void kasan_poison_object_data(struct kmem_cache *cache, void *object);
@@ -65,7 +66,7 @@ void kasan_kmalloc(struct kmem_cache *s, const void *object, size_t size,
void kasan_krealloc(const void *object, size_t new_size, gfp_t flags);
void kasan_slab_alloc(struct kmem_cache *s, void *object, gfp_t flags);
-bool kasan_slab_free(struct kmem_cache *s, void *object);
+bool kasan_slab_free(struct kmem_cache *s, void *object, unsigned long caller);
void kasan_poison_slab_free(struct kmem_cache *s, void *object);
struct kasan_cache {
@@ -94,6 +95,7 @@ static inline void kasan_cache_create(struct kmem_cache *cache,
static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
static inline void kasan_cache_destroy(struct kmem_cache *cache) {}
+static inline void kasan_init_object(struct kmem_cache *s, void *object) {}
static inline void kasan_poison_slab(struct page *page) {}
static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
void *object) {}
@@ -110,7 +112,8 @@ static inline void kasan_krealloc(const void *object, size_t new_size,
static inline void kasan_slab_alloc(struct kmem_cache *s, void *object,
gfp_t flags) {}
-static inline bool kasan_slab_free(struct kmem_cache *s, void *object)
+static inline bool kasan_slab_free(struct kmem_cache *s, void *object,
+ unsigned long caller)
{
return false;
}
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 28439ac..daec64b 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -402,6 +402,23 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
cache->object_size +
optimal_redzone(cache->object_size)));
}
+
+static inline union kasan_shadow_meta *get_shadow_meta(
+ struct kasan_alloc_meta *allocp)
+{
+ return (union kasan_shadow_meta *)kasan_mem_to_shadow((void *)allocp);
+}
+
+void kasan_init_object(struct kmem_cache *cache, void *object)
+{
+ if (cache->flags & SLAB_KASAN) {
+ struct kasan_alloc_meta *allocp = get_alloc_info(cache, object);
+ union kasan_shadow_meta *shadow_meta = get_shadow_meta(allocp);
+
+ __memset(allocp, 0, sizeof(*allocp));
+ shadow_meta->data = KASAN_KMALLOC_META;
+ }
+}
#endif
void kasan_cache_shrink(struct kmem_cache *cache)
@@ -431,13 +448,6 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
kasan_poison_shadow(object,
round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
KASAN_KMALLOC_REDZONE);
-#ifdef CONFIG_SLAB
- if (cache->flags & SLAB_KASAN) {
- struct kasan_alloc_meta *alloc_info =
- get_alloc_info(cache, object);
- alloc_info->state = KASAN_STATE_INIT;
- }
-#endif
}
#ifdef CONFIG_SLAB
@@ -501,6 +511,53 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
return (void *)object + cache->kasan_info.free_meta_offset;
}
+
+u8 get_alloc_state(struct kasan_alloc_meta *alloc_info)
+{
+ return get_shadow_meta(alloc_info)->state;
+}
+
+void set_alloc_state(struct kasan_alloc_meta *alloc_info, u8 state)
+{
+ get_shadow_meta(alloc_info)->state = state;
+}
+
+/*
+ * Acquire per-object lock before accessing KASAN metadata. Lock bit is stored
+ * in header shadow memory to protect it from being flipped by out-of-bounds
+ * accesses on object. Standard lock primitives cannot be used since there
+ * aren't enough header shadow bytes.
+ */
+void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
+{
+ union kasan_shadow_meta *shadow_meta = get_shadow_meta(alloc_info);
+ union kasan_shadow_meta old, new;
+
+ for (;;) {
+ old.data = READ_ONCE(shadow_meta->data);
+ if (old.lock) {
+ cpu_relax();
+ continue;
+ }
+ new.data = old.data;
+ new.lock = 1;
+ preempt_disable();
+ if (cmpxchg(&shadow_meta->data, old.data, new.data) == old.data)
+ break;
+ preempt_enable();
+ }
+}
+
+/* Release lock after a kasan_meta_lock(). */
+void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
+{
+ union kasan_shadow_meta *shadow_meta = get_shadow_meta(alloc_info), new;
+
+ new.data = READ_ONCE(shadow_meta->data);
+ new.lock = 0;
+ smp_store_release(&shadow_meta->data, new.data);
+ preempt_enable();
+}
#endif
void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
@@ -520,35 +577,44 @@ void kasan_poison_slab_free(struct kmem_cache *cache, void *object)
kasan_poison_shadow(object, rounded_up_size, KASAN_KMALLOC_FREE);
}
-bool kasan_slab_free(struct kmem_cache *cache, void *object)
+bool kasan_slab_free(struct kmem_cache *cache, void *object,
+ unsigned long caller)
{
#ifdef CONFIG_SLAB
+ struct kasan_alloc_meta *alloc_info;
+ struct kasan_free_meta *free_info;
+ u8 alloc_state;
+
/* RCU slabs could be legally used after free within the RCU period */
if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
return false;
- if (likely(cache->flags & SLAB_KASAN)) {
- struct kasan_alloc_meta *alloc_info =
- get_alloc_info(cache, object);
- struct kasan_free_meta *free_info =
- get_free_info(cache, object);
-
- switch (alloc_info->state) {
- case KASAN_STATE_ALLOC:
- alloc_info->state = KASAN_STATE_QUARANTINE;
- quarantine_put(free_info, cache);
- set_track(&free_info->track, GFP_NOWAIT);
- kasan_poison_slab_free(cache, object);
- return true;
+ if (unlikely(!(cache->flags & SLAB_KASAN)))
+ return false;
+
+ alloc_info = get_alloc_info(cache, object);
+ kasan_meta_lock(alloc_info);
+ alloc_state = get_alloc_state(alloc_info);
+ if (alloc_state == KASAN_STATE_ALLOC) {
+ free_info = get_free_info(cache, object);
+ quarantine_put(free_info, cache);
+ set_track(&free_info->track, GFP_NOWAIT);
+ kasan_poison_slab_free(cache, object);
+ set_alloc_state(alloc_info, KASAN_STATE_QUARANTINE);
+ kasan_meta_unlock(alloc_info);
+ return true;
+ }
+ switch (alloc_state) {
case KASAN_STATE_QUARANTINE:
case KASAN_STATE_FREE:
- pr_err("Double free");
- dump_stack();
- break;
+ kasan_report((unsigned long)object, 0, false, caller);
+ kasan_meta_unlock(alloc_info);
+ return true;
default:
+ pr_err("invalid allocation state!\n");
break;
- }
}
+ kasan_meta_unlock(alloc_info);
return false;
#else
kasan_poison_slab_free(cache, object);
@@ -580,10 +646,15 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
if (cache->flags & SLAB_KASAN) {
struct kasan_alloc_meta *alloc_info =
get_alloc_info(cache, object);
+ unsigned long flags;
- alloc_info->state = KASAN_STATE_ALLOC;
+ local_irq_save(flags);
+ kasan_meta_lock(alloc_info);
+ set_alloc_state(alloc_info, KASAN_STATE_ALLOC);
alloc_info->alloc_size = size;
set_track(&alloc_info->track, flags);
+ kasan_meta_unlock(alloc_info);
+ local_irq_restore(flags);
}
#endif
}
@@ -636,7 +707,7 @@ void kasan_kfree(void *ptr)
kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page),
KASAN_FREE_PAGE);
else
- kasan_slab_free(page->slab_cache, ptr);
+ kasan_slab_free(page->slab_cache, ptr, _RET_IP_);
}
void kasan_kfree_large(const void *ptr)
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index fb87923..76bdadd 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -12,6 +12,8 @@
#define KASAN_KMALLOC_REDZONE 0xFC /* redzone inside slub object */
#define KASAN_KMALLOC_FREE 0xFB /* object was freed (kmem_cache_free/kfree) */
#define KASAN_GLOBAL_REDZONE 0xFA /* redzone for global variable */
+#define KASAN_KMALLOC_META 0xE0 /* slab object header shadow; see union kasan_shadow_meta */
+#define KASAN_KMALLOC_META_MAX 0xEF /* end of header shadow code */
/*
* Stack redzone shadow values
@@ -73,10 +75,24 @@ struct kasan_track {
depot_stack_handle_t stack;
};
+/*
+ * Object header lock bit and allocation state are stored in shadow memory to
+ * prevent out-of-bounds accesses on object from overwriting them.
+ */
+union kasan_shadow_meta {
+ u8 data;
+ struct {
+ u8 lock : 1; /* lock bit */
+ u8 state : 2; /* enum kasan_state */
+ u8 unused : 1;
+ u8 poison : 4; /* poison constant; do not use */
+ };
+};
+
struct kasan_alloc_meta {
+ u32 alloc_size : 24;
+ u32 unused : 8;
struct kasan_track track;
- u32 state : 2; /* enum kasan_state */
- u32 alloc_size : 30;
};
struct qlist_node {
@@ -114,6 +130,10 @@ void kasan_report(unsigned long addr, size_t size,
void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);
void quarantine_reduce(void);
void quarantine_remove_cache(struct kmem_cache *cache);
+void kasan_meta_lock(struct kasan_alloc_meta *alloc_info);
+void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info);
+u8 get_alloc_state(struct kasan_alloc_meta *alloc_info);
+void set_alloc_state(struct kasan_alloc_meta *alloc_info, u8 state);
#else
static inline void quarantine_put(struct kasan_free_meta *info,
struct kmem_cache *cache) { }
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 4973505..ce9c913 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -148,7 +148,9 @@ static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
unsigned long flags;
local_irq_save(flags);
- alloc_info->state = KASAN_STATE_FREE;
+ kasan_meta_lock(alloc_info);
+ set_alloc_state(alloc_info, KASAN_STATE_FREE);
+ kasan_meta_unlock(alloc_info);
___cache_free(cache, object, _THIS_IP_);
local_irq_restore(flags);
}
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index b3c122d..48c9c60 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -53,6 +53,17 @@ static void print_error_description(struct kasan_access_info *info)
const char *bug_type = "unknown-crash";
u8 *shadow_addr;
+#ifdef CONFIG_SLAB
+ if (!info->access_size) {
+ bug_type = "double-free";
+ pr_err("BUG: KASAN: %s attempt in %pS on object at addr %p\n",
+ bug_type, (void *)info->ip, info->access_addr);
+ pr_err("%s by task %s/%d\n", bug_type, current->comm,
+ task_pid_nr(current));
+ info->first_bad_addr = info->access_addr;
+ return;
+ }
+#endif
info->first_bad_addr = find_first_bad_addr(info->access_addr,
info->access_size);
@@ -75,6 +86,7 @@ static void print_error_description(struct kasan_access_info *info)
break;
case KASAN_PAGE_REDZONE:
case KASAN_KMALLOC_REDZONE:
+ case KASAN_KMALLOC_META ... KASAN_KMALLOC_META_MAX:
bug_type = "slab-out-of-bounds";
break;
case KASAN_GLOBAL_REDZONE:
@@ -131,7 +143,7 @@ static void print_track(struct kasan_track *track)
}
static void object_err(struct kmem_cache *cache, struct page *page,
- void *object, char *unused_reason)
+ void *object, struct kasan_access_info *info)
{
struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
struct kasan_free_meta *free_info;
@@ -140,7 +152,9 @@ static void object_err(struct kmem_cache *cache, struct page *page,
pr_err("Object at %p, in cache %s\n", object, cache->name);
if (!(cache->flags & SLAB_KASAN))
return;
- switch (alloc_info->state) {
+ if (info->access_size)
+ kasan_meta_lock(alloc_info);
+ switch (get_alloc_state(alloc_info)) {
case KASAN_STATE_INIT:
pr_err("Object not allocated yet\n");
break;
@@ -161,6 +175,8 @@ static void object_err(struct kmem_cache *cache, struct page *page,
print_track(&free_info->track);
break;
}
+ if (info->access_size)
+ kasan_meta_unlock(alloc_info);
}
#endif
@@ -177,8 +193,12 @@ static void print_address_description(struct kasan_access_info *info)
struct kmem_cache *cache = page->slab_cache;
object = nearest_obj(cache, page,
(void *)info->access_addr);
+#ifdef CONFIG_SLAB
+ object_err(cache, page, object, info);
+#else
object_err(cache, page, object,
"kasan: bad access detected");
+#endif
return;
}
dump_page(page, "kasan: bad access detected");
diff --git a/mm/slab.c b/mm/slab.c
index 763096a..b8c51a6 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2611,6 +2611,7 @@ static void cache_init_objs(struct kmem_cache *cachep,
cachep->ctor(objp);
kasan_poison_object_data(cachep, objp);
}
+ kasan_init_object(cachep, index_to_obj(cachep, page, i));
if (!shuffled)
set_free_obj(page, i, i);
@@ -3508,7 +3509,7 @@ static inline void __cache_free(struct kmem_cache *cachep, void *objp,
unsigned long caller)
{
/* Put the object into the quarantine, don't touch it for now. */
- if (kasan_slab_free(cachep, objp))
+ if (kasan_slab_free(cachep, objp, _RET_IP_))
return;
___cache_free(cachep, objp, caller);
diff --git a/mm/slub.c b/mm/slub.c
index 5beeeb2..f25c0c2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1344,7 +1344,7 @@ static inline void slab_free_hook(struct kmem_cache *s, void *x)
if (!(s->flags & SLAB_DEBUG_OBJECTS))
debug_check_no_obj_freed(x, s->object_size);
- kasan_slab_free(s, x);
+ kasan_slab_free(s, x, _RET_IP_);
}
static inline void slab_free_freelist_hook(struct kmem_cache *s,
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] mm, kasan: improve double-free detection
2016-06-07 18:03 [PATCH v5 1/2] mm, kasan: improve double-free detection Kuthonuzo Luruo
@ 2016-06-09 13:32 ` Alexander Potapenko
2016-06-09 16:54 ` Luruo, Kuthonuzo
2016-06-09 17:00 ` Andrey Ryabinin
1 sibling, 1 reply; 10+ messages in thread
From: Alexander Potapenko @ 2016-06-09 13:32 UTC (permalink / raw)
To: Kuthonuzo Luruo
Cc: Andrey Ryabinin, Dmitriy Vyukov, Christoph Lameter, penberg,
David Rientjes, Joonsoo Kim, Andrew Morton, kasan-dev, LKML,
ynorov
On Tue, Jun 7, 2016 at 8:03 PM, Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com> wrote:
> Currently, KASAN may fail to detect concurrent deallocations of the same
> object due to a race in kasan_slab_free(). This patch makes double-free
> detection more reliable by serializing access to KASAN object metadata.
> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> lock/unlock per-object metadata. Double-free errors are now reported via
> kasan_report().
>
> Per-object lock concept from suggestion/observations by Dmitry Vyukov.
Note I've sent out a patch that enables stackdepot support in SLUB.
I'll probably need to wait till you patch lands and add locking to SLUB as well.
> Testing:
> - Tested with a modified version of the 'slab_test' microbenchmark where
> allocs occur on CPU 0; then all other CPUs concurrently attempt to free
> the same object.
> - Tested with new double-free tests for 'test_kasan' in accompanying patch.
>
> Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
> ---
>
> Changes in v5:
> - Removed redundant print of 'alloc_state' for pr_err in kasan_slab_free.
>
> Changes in v4:
> - Takes use of shadow memory in v3 further by storing lock bit in shadow
> byte for object header solving the issue of OOB nuking the header lock.
> Allocation state is also stored in the shadow header.
> - CAS loop using cmpxchg() byte similar to v2 is brought back.
> Unfortunately, standard lock primitives cannot be used since there aren't
> enough header/redzone shadow bytes for smaller objects. (A generic
> bit_spinlock() operating on a byte would have been sweet... :).
> - CAS loop with union magic is largely due to suggestions on patch v2 from
> Dmitry Vyukov. Thanks.
> - preempt enable/disable inside CAS loop to reduce contention is from
> review comment on v2 patch from Yury Norov. Thanks.
>
> Changes in v3:
> - simplified kasan_meta_lock()/unlock() to use generic bit spinlock apis;
> kasan_alloc_meta structure modified accordingly.
> - introduced a 'safety valve' for kasan_meta_lock() to prevent a kfree from
> getting stuck when a prior out-of-bounds write clobbers the object
> header.
> - removed potentially unsafe __builtin_return_address(1) from
> kasan_report() call per review comment from Yury Norov; callee now passed
> into kasan_slab_free().
>
> ---
>
> include/linux/kasan.h | 7 ++-
> mm/kasan/kasan.c | 125 ++++++++++++++++++++++++++++++++++++++-----------
> mm/kasan/kasan.h | 24 +++++++++-
> mm/kasan/quarantine.c | 4 +-
> mm/kasan/report.c | 24 +++++++++-
> mm/slab.c | 3 +-
> mm/slub.c | 2 +-
> 7 files changed, 153 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 611927f..3db974b 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -53,6 +53,7 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
> void kasan_cache_shrink(struct kmem_cache *cache);
> void kasan_cache_destroy(struct kmem_cache *cache);
>
> +void kasan_init_object(struct kmem_cache *cache, void *object);
> void kasan_poison_slab(struct page *page);
> void kasan_unpoison_object_data(struct kmem_cache *cache, void *object);
> void kasan_poison_object_data(struct kmem_cache *cache, void *object);
> @@ -65,7 +66,7 @@ void kasan_kmalloc(struct kmem_cache *s, const void *object, size_t size,
> void kasan_krealloc(const void *object, size_t new_size, gfp_t flags);
>
> void kasan_slab_alloc(struct kmem_cache *s, void *object, gfp_t flags);
> -bool kasan_slab_free(struct kmem_cache *s, void *object);
> +bool kasan_slab_free(struct kmem_cache *s, void *object, unsigned long caller);
> void kasan_poison_slab_free(struct kmem_cache *s, void *object);
>
> struct kasan_cache {
> @@ -94,6 +95,7 @@ static inline void kasan_cache_create(struct kmem_cache *cache,
> static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
> static inline void kasan_cache_destroy(struct kmem_cache *cache) {}
>
> +static inline void kasan_init_object(struct kmem_cache *s, void *object) {}
> static inline void kasan_poison_slab(struct page *page) {}
> static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
> void *object) {}
> @@ -110,7 +112,8 @@ static inline void kasan_krealloc(const void *object, size_t new_size,
>
> static inline void kasan_slab_alloc(struct kmem_cache *s, void *object,
> gfp_t flags) {}
> -static inline bool kasan_slab_free(struct kmem_cache *s, void *object)
> +static inline bool kasan_slab_free(struct kmem_cache *s, void *object,
> + unsigned long caller)
> {
> return false;
> }
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 28439ac..daec64b 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -402,6 +402,23 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
> cache->object_size +
> optimal_redzone(cache->object_size)));
> }
> +
> +static inline union kasan_shadow_meta *get_shadow_meta(
> + struct kasan_alloc_meta *allocp)
> +{
> + return (union kasan_shadow_meta *)kasan_mem_to_shadow((void *)allocp);
> +}
> +
> +void kasan_init_object(struct kmem_cache *cache, void *object)
> +{
> + if (cache->flags & SLAB_KASAN) {
> + struct kasan_alloc_meta *allocp = get_alloc_info(cache, object);
> + union kasan_shadow_meta *shadow_meta = get_shadow_meta(allocp);
> +
> + __memset(allocp, 0, sizeof(*allocp));
I think we need initialize the lock first, then lock it in order to
touch *allocp.
> + shadow_meta->data = KASAN_KMALLOC_META;
Shouldn't this be a release store?
> + }
> +}
> #endif
>
> void kasan_cache_shrink(struct kmem_cache *cache)
> @@ -431,13 +448,6 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
> kasan_poison_shadow(object,
> round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
> KASAN_KMALLOC_REDZONE);
> -#ifdef CONFIG_SLAB
> - if (cache->flags & SLAB_KASAN) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - alloc_info->state = KASAN_STATE_INIT;
> - }
> -#endif
> }
>
> #ifdef CONFIG_SLAB
> @@ -501,6 +511,53 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
> BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
> return (void *)object + cache->kasan_info.free_meta_offset;
> }
> +
> +u8 get_alloc_state(struct kasan_alloc_meta *alloc_info)
> +{
> + return get_shadow_meta(alloc_info)->state;
> +}
> +
> +void set_alloc_state(struct kasan_alloc_meta *alloc_info, u8 state)
> +{
> + get_shadow_meta(alloc_info)->state = state;
> +}
> +
> +/*
> + * Acquire per-object lock before accessing KASAN metadata. Lock bit is stored
> + * in header shadow memory to protect it from being flipped by out-of-bounds
> + * accesses on object. Standard lock primitives cannot be used since there
> + * aren't enough header shadow bytes.
> + */
> +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
> +{
> + union kasan_shadow_meta *shadow_meta = get_shadow_meta(alloc_info);
> + union kasan_shadow_meta old, new;
> +
> + for (;;) {
> + old.data = READ_ONCE(shadow_meta->data);
> + if (old.lock) {
> + cpu_relax();
> + continue;
> + }
> + new.data = old.data;
> + new.lock = 1;
> + preempt_disable();
> + if (cmpxchg(&shadow_meta->data, old.data, new.data) == old.data)
> + break;
> + preempt_enable();
> + }
> +}
> +
> +/* Release lock after a kasan_meta_lock(). */
> +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
> +{
> + union kasan_shadow_meta *shadow_meta = get_shadow_meta(alloc_info), new;
> +
> + new.data = READ_ONCE(shadow_meta->data);
> + new.lock = 0;
> + smp_store_release(&shadow_meta->data, new.data);
> + preempt_enable();
> +}
> #endif
>
> void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
> @@ -520,35 +577,44 @@ void kasan_poison_slab_free(struct kmem_cache *cache, void *object)
> kasan_poison_shadow(object, rounded_up_size, KASAN_KMALLOC_FREE);
> }
>
> -bool kasan_slab_free(struct kmem_cache *cache, void *object)
> +bool kasan_slab_free(struct kmem_cache *cache, void *object,
> + unsigned long caller)
> {
> #ifdef CONFIG_SLAB
> + struct kasan_alloc_meta *alloc_info;
> + struct kasan_free_meta *free_info;
> + u8 alloc_state;
> +
> /* RCU slabs could be legally used after free within the RCU period */
> if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
> return false;
>
> - if (likely(cache->flags & SLAB_KASAN)) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - struct kasan_free_meta *free_info =
> - get_free_info(cache, object);
> -
> - switch (alloc_info->state) {
> - case KASAN_STATE_ALLOC:
> - alloc_info->state = KASAN_STATE_QUARANTINE;
> - quarantine_put(free_info, cache);
> - set_track(&free_info->track, GFP_NOWAIT);
> - kasan_poison_slab_free(cache, object);
> - return true;
> + if (unlikely(!(cache->flags & SLAB_KASAN)))
> + return false;
> +
> + alloc_info = get_alloc_info(cache, object);
> + kasan_meta_lock(alloc_info);
> + alloc_state = get_alloc_state(alloc_info);
> + if (alloc_state == KASAN_STATE_ALLOC) {
> + free_info = get_free_info(cache, object);
> + quarantine_put(free_info, cache);
> + set_track(&free_info->track, GFP_NOWAIT);
> + kasan_poison_slab_free(cache, object);
> + set_alloc_state(alloc_info, KASAN_STATE_QUARANTINE);
> + kasan_meta_unlock(alloc_info);
> + return true;
> + }
> + switch (alloc_state) {
> case KASAN_STATE_QUARANTINE:
> case KASAN_STATE_FREE:
> - pr_err("Double free");
> - dump_stack();
> - break;
> + kasan_report((unsigned long)object, 0, false, caller);
> + kasan_meta_unlock(alloc_info);
> + return true;
> default:
> + pr_err("invalid allocation state!\n");
I suggest you also print the object pointer here.
> break;
> - }
> }
> + kasan_meta_unlock(alloc_info);
> return false;
> #else
> kasan_poison_slab_free(cache, object);
> @@ -580,10 +646,15 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
> if (cache->flags & SLAB_KASAN) {
> struct kasan_alloc_meta *alloc_info =
> get_alloc_info(cache, object);
> + unsigned long flags;
>
> - alloc_info->state = KASAN_STATE_ALLOC;
> + local_irq_save(flags);
> + kasan_meta_lock(alloc_info);
> + set_alloc_state(alloc_info, KASAN_STATE_ALLOC);
> alloc_info->alloc_size = size;
> set_track(&alloc_info->track, flags);
> + kasan_meta_unlock(alloc_info);
> + local_irq_restore(flags);
> }
> #endif
> }
> @@ -636,7 +707,7 @@ void kasan_kfree(void *ptr)
> kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page),
> KASAN_FREE_PAGE);
> else
> - kasan_slab_free(page->slab_cache, ptr);
> + kasan_slab_free(page->slab_cache, ptr, _RET_IP_);
> }
>
> void kasan_kfree_large(const void *ptr)
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index fb87923..76bdadd 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -12,6 +12,8 @@
> #define KASAN_KMALLOC_REDZONE 0xFC /* redzone inside slub object */
> #define KASAN_KMALLOC_FREE 0xFB /* object was freed (kmem_cache_free/kfree) */
> #define KASAN_GLOBAL_REDZONE 0xFA /* redzone for global variable */
> +#define KASAN_KMALLOC_META 0xE0 /* slab object header shadow; see union kasan_shadow_meta */
> +#define KASAN_KMALLOC_META_MAX 0xEF /* end of header shadow code */
>
> /*
> * Stack redzone shadow values
> @@ -73,10 +75,24 @@ struct kasan_track {
> depot_stack_handle_t stack;
> };
>
> +/*
> + * Object header lock bit and allocation state are stored in shadow memory to
> + * prevent out-of-bounds accesses on object from overwriting them.
> + */
> +union kasan_shadow_meta {
> + u8 data;
> + struct {
> + u8 lock : 1; /* lock bit */
> + u8 state : 2; /* enum kasan_state */
> + u8 unused : 1;
> + u8 poison : 4; /* poison constant; do not use */
> + };
> +};
> +
> struct kasan_alloc_meta {
> + u32 alloc_size : 24;
Why reduce the alloc size?
> + u32 unused : 8;
> struct kasan_track track;
> - u32 state : 2; /* enum kasan_state */
> - u32 alloc_size : 30;
> };
>
> struct qlist_node {
> @@ -114,6 +130,10 @@ void kasan_report(unsigned long addr, size_t size,
> void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);
> void quarantine_reduce(void);
> void quarantine_remove_cache(struct kmem_cache *cache);
> +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info);
> +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info);
> +u8 get_alloc_state(struct kasan_alloc_meta *alloc_info);
> +void set_alloc_state(struct kasan_alloc_meta *alloc_info, u8 state);
> #else
> static inline void quarantine_put(struct kasan_free_meta *info,
> struct kmem_cache *cache) { }
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 4973505..ce9c913 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -148,7 +148,9 @@ static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
> unsigned long flags;
>
> local_irq_save(flags);
> - alloc_info->state = KASAN_STATE_FREE;
> + kasan_meta_lock(alloc_info);
> + set_alloc_state(alloc_info, KASAN_STATE_FREE);
> + kasan_meta_unlock(alloc_info);
> ___cache_free(cache, object, _THIS_IP_);
> local_irq_restore(flags);
> }
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index b3c122d..48c9c60 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -53,6 +53,17 @@ static void print_error_description(struct kasan_access_info *info)
> const char *bug_type = "unknown-crash";
> u8 *shadow_addr;
>
> +#ifdef CONFIG_SLAB
> + if (!info->access_size) {
> + bug_type = "double-free";
> + pr_err("BUG: KASAN: %s attempt in %pS on object at addr %p\n",
> + bug_type, (void *)info->ip, info->access_addr);
> + pr_err("%s by task %s/%d\n", bug_type, current->comm,
> + task_pid_nr(current));
> + info->first_bad_addr = info->access_addr;
> + return;
> + }
> +#endif
> info->first_bad_addr = find_first_bad_addr(info->access_addr,
> info->access_size);
>
> @@ -75,6 +86,7 @@ static void print_error_description(struct kasan_access_info *info)
> break;
> case KASAN_PAGE_REDZONE:
> case KASAN_KMALLOC_REDZONE:
> + case KASAN_KMALLOC_META ... KASAN_KMALLOC_META_MAX:
> bug_type = "slab-out-of-bounds";
> break;
> case KASAN_GLOBAL_REDZONE:
> @@ -131,7 +143,7 @@ static void print_track(struct kasan_track *track)
> }
>
> static void object_err(struct kmem_cache *cache, struct page *page,
> - void *object, char *unused_reason)
> + void *object, struct kasan_access_info *info)
> {
> struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
> struct kasan_free_meta *free_info;
> @@ -140,7 +152,9 @@ static void object_err(struct kmem_cache *cache, struct page *page,
> pr_err("Object at %p, in cache %s\n", object, cache->name);
> if (!(cache->flags & SLAB_KASAN))
> return;
> - switch (alloc_info->state) {
> + if (info->access_size)
> + kasan_meta_lock(alloc_info);
In which case can info->access_size be zero? Guess even in that case
we need to lock the metadata.
> + switch (get_alloc_state(alloc_info)) {
> case KASAN_STATE_INIT:
> pr_err("Object not allocated yet\n");
> break;
> @@ -161,6 +175,8 @@ static void object_err(struct kmem_cache *cache, struct page *page,
> print_track(&free_info->track);
> break;
> }
> + if (info->access_size)
> + kasan_meta_unlock(alloc_info);
> }
> #endif
>
> @@ -177,8 +193,12 @@ static void print_address_description(struct kasan_access_info *info)
> struct kmem_cache *cache = page->slab_cache;
> object = nearest_obj(cache, page,
> (void *)info->access_addr);
> +#ifdef CONFIG_SLAB
> + object_err(cache, page, object, info);
> +#else
> object_err(cache, page, object,
> "kasan: bad access detected");
> +#endif
> return;
> }
> dump_page(page, "kasan: bad access detected");
> diff --git a/mm/slab.c b/mm/slab.c
> index 763096a..b8c51a6 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2611,6 +2611,7 @@ static void cache_init_objs(struct kmem_cache *cachep,
> cachep->ctor(objp);
> kasan_poison_object_data(cachep, objp);
> }
> + kasan_init_object(cachep, index_to_obj(cachep, page, i));
>
> if (!shuffled)
> set_free_obj(page, i, i);
> @@ -3508,7 +3509,7 @@ static inline void __cache_free(struct kmem_cache *cachep, void *objp,
> unsigned long caller)
> {
> /* Put the object into the quarantine, don't touch it for now. */
> - if (kasan_slab_free(cachep, objp))
> + if (kasan_slab_free(cachep, objp, _RET_IP_))
> return;
>
> ___cache_free(cachep, objp, caller);
> diff --git a/mm/slub.c b/mm/slub.c
> index 5beeeb2..f25c0c2 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1344,7 +1344,7 @@ static inline void slab_free_hook(struct kmem_cache *s, void *x)
> if (!(s->flags & SLAB_DEBUG_OBJECTS))
> debug_check_no_obj_freed(x, s->object_size);
>
> - kasan_slab_free(s, x);
> + kasan_slab_free(s, x, _RET_IP_);
> }
>
> static inline void slab_free_freelist_hook(struct kmem_cache *s,
> --
> 1.7.1
>
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v5 1/2] mm, kasan: improve double-free detection
2016-06-09 13:32 ` Alexander Potapenko
@ 2016-06-09 16:54 ` Luruo, Kuthonuzo
0 siblings, 0 replies; 10+ messages in thread
From: Luruo, Kuthonuzo @ 2016-06-09 16:54 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrey Ryabinin, Dmitriy Vyukov, Christoph Lameter, penberg,
David Rientjes, Joonsoo Kim, Andrew Morton, kasan-dev, LKML,
ynorov
> > Currently, KASAN may fail to detect concurrent deallocations of the same
> > object due to a race in kasan_slab_free(). This patch makes double-free
> > detection more reliable by serializing access to KASAN object metadata.
> > New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> > lock/unlock per-object metadata. Double-free errors are now reported via
> > kasan_report().
> >
> > Per-object lock concept from suggestion/observations by Dmitry Vyukov.
> Note I've sent out a patch that enables stackdepot support in SLUB.
> I'll probably need to wait till you patch lands and add locking to SLUB as well.
My patch can wait; It can be rebased and resent for consideration by
maintainers/reviewers after your patch has been reviewed.
> > +void kasan_init_object(struct kmem_cache *cache, void *object)
> > +{
> > + if (cache->flags & SLAB_KASAN) {
> > + struct kasan_alloc_meta *allocp = get_alloc_info(cache, object);
> > + union kasan_shadow_meta *shadow_meta =
> get_shadow_meta(allocp);
> > +
> > + __memset(allocp, 0, sizeof(*allocp));
> I think we need initialize the lock first, then lock it in order to
> touch *allocp.
> > + shadow_meta->data = KASAN_KMALLOC_META;
> Shouldn't this be a release store?
Object at this point is being initialized by SLAB off a newly allocated page/slab.
Concurrent access to the embryonic object is unlikely/not expected. I don't
think locking here is of any benefit...
> > default:
> > + pr_err("invalid allocation state!\n");
> I suggest you also print the object pointer here.
ok.
> > +
> > struct kasan_alloc_meta {
> > + u32 alloc_size : 24;
> Why reduce the alloc size?
Thought to save some bits while still accounting for max object size
instrumented by KASAN. But now, with the spectre of OOB writes,
header real estate suddenly seems a lot less valuable ;-). A reset to
u32 won’t be inappropriate.
> > if (!(cache->flags & SLAB_KASAN))
> > return;
> > - switch (alloc_info->state) {
> > + if (info->access_size)
> > + kasan_meta_lock(alloc_info);
> In which case can info->access_size be zero? Guess even in that case
> we need to lock the metadata.
For a double-free error, access size is zero and lock is already held.
Thank you very much for reviewing the patch!
Kuthonuzo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] mm, kasan: improve double-free detection
2016-06-07 18:03 [PATCH v5 1/2] mm, kasan: improve double-free detection Kuthonuzo Luruo
2016-06-09 13:32 ` Alexander Potapenko
@ 2016-06-09 17:00 ` Andrey Ryabinin
2016-06-10 17:03 ` Andrey Ryabinin
1 sibling, 1 reply; 10+ messages in thread
From: Andrey Ryabinin @ 2016-06-09 17:00 UTC (permalink / raw)
To: Kuthonuzo Luruo, glider, dvyukov, cl, penberg, rientjes,
iamjoonsoo.kim, akpm
Cc: kasan-dev, linux-kernel, ynorov
On 06/07/2016 09:03 PM, Kuthonuzo Luruo wrote:
Next time, when/if you send patch series, send patches in one thread, i.e. patches should be replies to the cover letter.
Your patches are not linked together, which makes them harder to track.
> Currently, KASAN may fail to detect concurrent deallocations of the same
> object due to a race in kasan_slab_free(). This patch makes double-free
> detection more reliable by serializing access to KASAN object metadata.
> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> lock/unlock per-object metadata. Double-free errors are now reported via
> kasan_report().
>
> Per-object lock concept from suggestion/observations by Dmitry Vyukov.
>
So, I still don't like this, this too way hacky and complex.
I have some thoughts about how to make this lockless and robust enough.
I'll try to sort this out tomorrow.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] mm, kasan: improve double-free detection
2016-06-09 17:00 ` Andrey Ryabinin
@ 2016-06-10 17:03 ` Andrey Ryabinin
2016-06-10 17:09 ` Dmitry Vyukov
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Andrey Ryabinin @ 2016-06-10 17:03 UTC (permalink / raw)
To: Kuthonuzo Luruo, glider, dvyukov, cl, penberg, rientjes,
iamjoonsoo.kim, akpm
Cc: kasan-dev, linux-kernel, ynorov
On 06/09/2016 08:00 PM, Andrey Ryabinin wrote:
> On 06/07/2016 09:03 PM, Kuthonuzo Luruo wrote:
>
> Next time, when/if you send patch series, send patches in one thread, i.e. patches should be replies to the cover letter.
> Your patches are not linked together, which makes them harder to track.
>
>
>> Currently, KASAN may fail to detect concurrent deallocations of the same
>> object due to a race in kasan_slab_free(). This patch makes double-free
>> detection more reliable by serializing access to KASAN object metadata.
>> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
>> lock/unlock per-object metadata. Double-free errors are now reported via
>> kasan_report().
>>
>> Per-object lock concept from suggestion/observations by Dmitry Vyukov.
>>
>
>
> So, I still don't like this, this too way hacky and complex.
> I have some thoughts about how to make this lockless and robust enough.
> I'll try to sort this out tomorrow.
>
So, I something like this should work.
Tested very briefly.
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index ac4b3c4..8691142 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -75,6 +75,8 @@ struct kasan_cache {
int kasan_module_alloc(void *addr, size_t size);
void kasan_free_shadow(const struct vm_struct *vm);
+void kasan_init_slab_obj(struct kmem_cache *cache, const void *object);
+
size_t ksize(const void *);
static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
@@ -102,6 +104,9 @@ static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
static inline void kasan_poison_object_data(struct kmem_cache *cache,
void *object) {}
+static inline void kasan_init_slab_obj(struct kmem_cache *cache,
+ const void *object) { }
+
static inline void kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags) {}
static inline void kasan_kfree_large(const void *ptr) {}
static inline void kasan_poison_kfree(void *ptr) {}
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 6845f92..ab0fded 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -388,11 +388,9 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
*size += sizeof(struct kasan_alloc_meta);
/* Add free meta. */
- if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
- cache->object_size < sizeof(struct kasan_free_meta)) {
- cache->kasan_info.free_meta_offset = *size;
- *size += sizeof(struct kasan_free_meta);
- }
+ cache->kasan_info.free_meta_offset = *size;
+ *size += sizeof(struct kasan_free_meta);
+
redzone_adjust = optimal_redzone(cache->object_size) -
(*size - cache->object_size);
if (redzone_adjust > 0)
@@ -431,13 +429,6 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
kasan_poison_shadow(object,
round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
KASAN_KMALLOC_REDZONE);
-#ifdef CONFIG_SLAB
- if (cache->flags & SLAB_KASAN) {
- struct kasan_alloc_meta *alloc_info =
- get_alloc_info(cache, object);
- alloc_info->state = KASAN_STATE_INIT;
- }
-#endif
}
#ifdef CONFIG_SLAB
@@ -501,6 +492,20 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
return (void *)object + cache->kasan_info.free_meta_offset;
}
+
+void kasan_init_slab_obj(struct kmem_cache *cache, const void *object)
+{
+ struct kasan_alloc_meta *alloc_info;
+ struct kasan_free_meta *free_info;
+
+ if (!(cache->flags & SLAB_KASAN))
+ return;
+
+ alloc_info = get_alloc_info(cache, object);
+ free_info = get_free_info(cache, object);
+ __memset(alloc_info, 0, sizeof(*alloc_info));
+ __memset(free_info, 0, sizeof(*free_info));
+}
#endif
void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
@@ -523,37 +528,47 @@ static void kasan_poison_slab_free(struct kmem_cache *cache, void *object)
bool kasan_slab_free(struct kmem_cache *cache, void *object)
{
#ifdef CONFIG_SLAB
+ struct kasan_free_meta *free_info = get_free_info(cache, object);
+ struct kasan_track new_free_stack, old_free_stack;
+ s8 old_shadow;
+
/* RCU slabs could be legally used after free within the RCU period */
if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
return false;
- if (likely(cache->flags & SLAB_KASAN)) {
- struct kasan_alloc_meta *alloc_info =
- get_alloc_info(cache, object);
- struct kasan_free_meta *free_info =
- get_free_info(cache, object);
-
- switch (alloc_info->state) {
- case KASAN_STATE_ALLOC:
- alloc_info->state = KASAN_STATE_QUARANTINE;
- quarantine_put(free_info, cache);
- set_track(&free_info->track, GFP_NOWAIT);
- kasan_poison_slab_free(cache, object);
- return true;
- case KASAN_STATE_QUARANTINE:
- case KASAN_STATE_FREE:
- pr_err("Double free");
- dump_stack();
- break;
- default:
- break;
- }
+ if (unlikely(!(cache->flags & SLAB_KASAN)))
+ return false;
+
+ set_track(&new_free_stack, GFP_NOWAIT);
+ old_free_stack = xchg(&free_info->track, new_free_stack);
+ old_shadow = xchg((s8 *)kasan_mem_to_shadow(object),
+ KASAN_KMALLOC_FREE);
+
+ if (old_shadow < 0 || old_shadow >= KASAN_SHADOW_SCALE_SIZE) {
+ struct kasan_track free_stack;
+
+ /* Paired with xchg() above */
+ free_stack = smp_load_acquire(&free_info->track);
+
+ /*
+ * We didn't raced with another instance of kasan_slab_free()
+ * so the previous free stack supposed to be in old_free_stack.
+ * Otherwise, free_stack will contain stack trace of another
+ * kfree() call.
+ */
+ if (free_stack.id == new_free_stack.id)
+ free_stack = old_free_stack;
+
+ kasan_report_double_free(cache, object,
+ free_stack, old_shadow);
+ return false;
}
- return false;
-#else
kasan_poison_slab_free(cache, object);
- return false;
+ return true;
+
#endif
+ kasan_poison_slab_free(cache, object);
+ return false;
}
void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
@@ -581,7 +596,6 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
struct kasan_alloc_meta *alloc_info =
get_alloc_info(cache, object);
- alloc_info->state = KASAN_STATE_ALLOC;
alloc_info->alloc_size = size;
set_track(&alloc_info->track, flags);
}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index fb87923..9b46d2e 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -59,24 +59,21 @@ struct kasan_global {
* Structures to keep alloc and free tracks *
*/
-enum kasan_state {
- KASAN_STATE_INIT,
- KASAN_STATE_ALLOC,
- KASAN_STATE_QUARANTINE,
- KASAN_STATE_FREE
-};
-
#define KASAN_STACK_DEPTH 64
struct kasan_track {
- u32 pid;
- depot_stack_handle_t stack;
+union {
+ struct {
+ u32 pid;
+ depot_stack_handle_t stack;
+ };
+ u64 id;
+};
};
struct kasan_alloc_meta {
struct kasan_track track;
- u32 state : 2; /* enum kasan_state */
- u32 alloc_size : 30;
+ u32 alloc_size;
};
struct qlist_node {
@@ -109,6 +106,9 @@ static inline bool kasan_report_enabled(void)
void kasan_report(unsigned long addr, size_t size,
bool is_write, unsigned long ip);
+void kasan_report_double_free(struct kmem_cache *cache, void *object,
+ struct kasan_track free_stack, s8 shadow);
+
#ifdef CONFIG_SLAB
void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 4973505..3ec039c 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -144,11 +144,9 @@ static void *qlink_to_object(struct qlist_node *qlink, struct kmem_cache *cache)
static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
{
void *object = qlink_to_object(qlink, cache);
- struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
unsigned long flags;
local_irq_save(flags);
- alloc_info->state = KASAN_STATE_FREE;
___cache_free(cache, object, _THIS_IP_);
local_irq_restore(flags);
}
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index b3c122d..a0f4519 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -140,28 +140,13 @@ static void object_err(struct kmem_cache *cache, struct page *page,
pr_err("Object at %p, in cache %s\n", object, cache->name);
if (!(cache->flags & SLAB_KASAN))
return;
- switch (alloc_info->state) {
- case KASAN_STATE_INIT:
- pr_err("Object not allocated yet\n");
- break;
- case KASAN_STATE_ALLOC:
- pr_err("Object allocated with size %u bytes.\n",
- alloc_info->alloc_size);
- pr_err("Allocation:\n");
- print_track(&alloc_info->track);
- break;
- case KASAN_STATE_FREE:
- case KASAN_STATE_QUARANTINE:
- pr_err("Object freed, allocated with size %u bytes\n",
- alloc_info->alloc_size);
- free_info = get_free_info(cache, object);
- pr_err("Allocation:\n");
- print_track(&alloc_info->track);
- pr_err("Deallocation:\n");
- print_track(&free_info->track);
- break;
- }
+ free_info = get_free_info(cache, object);
+ pr_err("Allocation:\n");
+ print_track(&alloc_info->track);
+ pr_err("Deallocation:\n");
+ print_track(&free_info->track);
}
+
#endif
static void print_address_description(struct kasan_access_info *info)
@@ -245,17 +230,31 @@ static void print_shadow_for_address(const void *addr)
static DEFINE_SPINLOCK(report_lock);
-static void kasan_report_error(struct kasan_access_info *info)
+static void kasan_start_report(unsigned long *flags)
{
- unsigned long flags;
- const char *bug_type;
-
/*
* Make sure we don't end up in loop.
*/
kasan_disable_current();
- spin_lock_irqsave(&report_lock, flags);
+ spin_lock_irqsave(&report_lock, *flags);
pr_err("==================================================================\n");
+}
+
+static void kasan_end_report(unsigned long *flags)
+{
+ pr_err("==================================================================\n");
+ add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
+ spin_unlock_irqrestore(&report_lock, *flags);
+ kasan_enable_current();
+}
+
+static void kasan_report_error(struct kasan_access_info *info)
+{
+ unsigned long flags;
+ const char *bug_type;
+
+ kasan_start_report(&flags);
+
if (info->access_addr <
kasan_shadow_to_mem((void *)KASAN_SHADOW_START)) {
if ((unsigned long)info->access_addr < PAGE_SIZE)
@@ -276,10 +275,29 @@ static void kasan_report_error(struct kasan_access_info *info)
print_address_description(info);
print_shadow_for_address(info->first_bad_addr);
}
- pr_err("==================================================================\n");
- add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
- spin_unlock_irqrestore(&report_lock, flags);
- kasan_enable_current();
+
+ kasan_end_report(&flags);
+}
+
+void kasan_report_double_free(struct kmem_cache *cache, void *object,
+ struct kasan_track free_stack, s8 shadow)
+{
+ unsigned long flags;
+
+ kasan_start_report(&flags);
+
+ pr_err("BUG: Double free or corrupt pointer\n");
+ pr_err("Unexpected shadow byte: 0x%hhX\n", shadow);
+
+ dump_stack();
+ pr_err("Object at %p, in cache %s\n", object, cache->name);
+ get_alloc_info(cache, object);
+ pr_err("Allocation:\n");
+ print_track(&get_alloc_info(cache, object)->track);
+ pr_err("Deallocation:\n");
+ print_track(&free_stack);
+
+ kasan_end_report(&flags);
}
void kasan_report(unsigned long addr, size_t size,
diff --git a/mm/slab.c b/mm/slab.c
index 763096a..65c942b 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2604,9 +2604,11 @@ static void cache_init_objs(struct kmem_cache *cachep,
}
for (i = 0; i < cachep->num; i++) {
+ objp = index_to_obj(cachep, page, i);
+ kasan_init_slab_obj(cachep, objp);
+
/* constructor could break poison info */
if (DEBUG == 0 && cachep->ctor) {
- objp = index_to_obj(cachep, page, i);
kasan_unpoison_object_data(cachep, objp);
cachep->ctor(objp);
kasan_poison_object_data(cachep, objp);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] mm, kasan: improve double-free detection
2016-06-10 17:03 ` Andrey Ryabinin
@ 2016-06-10 17:09 ` Dmitry Vyukov
2016-06-15 16:18 ` Andrey Ryabinin
2016-06-13 11:56 ` Alexander Potapenko
2016-06-14 6:46 ` Luruo, Kuthonuzo
2 siblings, 1 reply; 10+ messages in thread
From: Dmitry Vyukov @ 2016-06-10 17:09 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Kuthonuzo Luruo, Alexander Potapenko, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
kasan-dev, LKML, Yury Norov
On Fri, Jun 10, 2016 at 7:03 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 06/09/2016 08:00 PM, Andrey Ryabinin wrote:
>> On 06/07/2016 09:03 PM, Kuthonuzo Luruo wrote:
>>
>> Next time, when/if you send patch series, send patches in one thread, i.e. patches should be replies to the cover letter.
>> Your patches are not linked together, which makes them harder to track.
>>
>>
>>> Currently, KASAN may fail to detect concurrent deallocations of the same
>>> object due to a race in kasan_slab_free(). This patch makes double-free
>>> detection more reliable by serializing access to KASAN object metadata.
>>> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
>>> lock/unlock per-object metadata. Double-free errors are now reported via
>>> kasan_report().
>>>
>>> Per-object lock concept from suggestion/observations by Dmitry Vyukov.
>>>
>>
>>
>> So, I still don't like this, this too way hacky and complex.
>> I have some thoughts about how to make this lockless and robust enough.
>> I'll try to sort this out tomorrow.
>>
>
>
> So, I something like this should work.
> Tested very briefly.
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index ac4b3c4..8691142 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -75,6 +75,8 @@ struct kasan_cache {
> int kasan_module_alloc(void *addr, size_t size);
> void kasan_free_shadow(const struct vm_struct *vm);
>
> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object);
> +
> size_t ksize(const void *);
> static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
>
> @@ -102,6 +104,9 @@ static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
> static inline void kasan_poison_object_data(struct kmem_cache *cache,
> void *object) {}
>
> +static inline void kasan_init_slab_obj(struct kmem_cache *cache,
> + const void *object) { }
> +
> static inline void kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags) {}
> static inline void kasan_kfree_large(const void *ptr) {}
> static inline void kasan_poison_kfree(void *ptr) {}
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 6845f92..ab0fded 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -388,11 +388,9 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
> *size += sizeof(struct kasan_alloc_meta);
>
> /* Add free meta. */
> - if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
> - cache->object_size < sizeof(struct kasan_free_meta)) {
> - cache->kasan_info.free_meta_offset = *size;
> - *size += sizeof(struct kasan_free_meta);
> - }
> + cache->kasan_info.free_meta_offset = *size;
> + *size += sizeof(struct kasan_free_meta);
> +
Why?!
Please don't worsen runtime characteristics of KASAN. We run real
systems with it.
Most objects are small. This can lead to significant memory consumption.
> redzone_adjust = optimal_redzone(cache->object_size) -
> (*size - cache->object_size);
> if (redzone_adjust > 0)
> @@ -431,13 +429,6 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
> kasan_poison_shadow(object,
> round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
> KASAN_KMALLOC_REDZONE);
> -#ifdef CONFIG_SLAB
> - if (cache->flags & SLAB_KASAN) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - alloc_info->state = KASAN_STATE_INIT;
> - }
> -#endif
> }
>
> #ifdef CONFIG_SLAB
> @@ -501,6 +492,20 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
> BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
> return (void *)object + cache->kasan_info.free_meta_offset;
> }
> +
> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object)
> +{
> + struct kasan_alloc_meta *alloc_info;
> + struct kasan_free_meta *free_info;
> +
> + if (!(cache->flags & SLAB_KASAN))
> + return;
> +
> + alloc_info = get_alloc_info(cache, object);
> + free_info = get_free_info(cache, object);
> + __memset(alloc_info, 0, sizeof(*alloc_info));
> + __memset(free_info, 0, sizeof(*free_info));
> +}
> #endif
>
> void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
> @@ -523,37 +528,47 @@ static void kasan_poison_slab_free(struct kmem_cache *cache, void *object)
> bool kasan_slab_free(struct kmem_cache *cache, void *object)
> {
> #ifdef CONFIG_SLAB
> + struct kasan_free_meta *free_info = get_free_info(cache, object);
> + struct kasan_track new_free_stack, old_free_stack;
> + s8 old_shadow;
> +
> /* RCU slabs could be legally used after free within the RCU period */
> if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
> return false;
>
> - if (likely(cache->flags & SLAB_KASAN)) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - struct kasan_free_meta *free_info =
> - get_free_info(cache, object);
> -
> - switch (alloc_info->state) {
> - case KASAN_STATE_ALLOC:
> - alloc_info->state = KASAN_STATE_QUARANTINE;
> - quarantine_put(free_info, cache);
> - set_track(&free_info->track, GFP_NOWAIT);
> - kasan_poison_slab_free(cache, object);
> - return true;
> - case KASAN_STATE_QUARANTINE:
> - case KASAN_STATE_FREE:
> - pr_err("Double free");
> - dump_stack();
> - break;
> - default:
> - break;
> - }
> + if (unlikely(!(cache->flags & SLAB_KASAN)))
> + return false;
> +
> + set_track(&new_free_stack, GFP_NOWAIT);
> + old_free_stack = xchg(&free_info->track, new_free_stack);
> + old_shadow = xchg((s8 *)kasan_mem_to_shadow(object),
> + KASAN_KMALLOC_FREE);
> +
> + if (old_shadow < 0 || old_shadow >= KASAN_SHADOW_SCALE_SIZE) {
> + struct kasan_track free_stack;
> +
> + /* Paired with xchg() above */
> + free_stack = smp_load_acquire(&free_info->track);
> +
> + /*
> + * We didn't raced with another instance of kasan_slab_free()
> + * so the previous free stack supposed to be in old_free_stack.
> + * Otherwise, free_stack will contain stack trace of another
> + * kfree() call.
> + */
> + if (free_stack.id == new_free_stack.id)
> + free_stack = old_free_stack;
> +
> + kasan_report_double_free(cache, object,
> + free_stack, old_shadow);
> + return false;
> }
> - return false;
> -#else
> kasan_poison_slab_free(cache, object);
> - return false;
> + return true;
> +
> #endif
> + kasan_poison_slab_free(cache, object);
> + return false;
> }
>
> void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
> @@ -581,7 +596,6 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
> struct kasan_alloc_meta *alloc_info =
> get_alloc_info(cache, object);
>
> - alloc_info->state = KASAN_STATE_ALLOC;
> alloc_info->alloc_size = size;
> set_track(&alloc_info->track, flags);
> }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index fb87923..9b46d2e 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -59,24 +59,21 @@ struct kasan_global {
> * Structures to keep alloc and free tracks *
> */
>
> -enum kasan_state {
> - KASAN_STATE_INIT,
> - KASAN_STATE_ALLOC,
> - KASAN_STATE_QUARANTINE,
> - KASAN_STATE_FREE
> -};
> -
> #define KASAN_STACK_DEPTH 64
>
> struct kasan_track {
> - u32 pid;
> - depot_stack_handle_t stack;
> +union {
> + struct {
> + u32 pid;
> + depot_stack_handle_t stack;
> + };
> + u64 id;
> +};
> };
>
> struct kasan_alloc_meta {
> struct kasan_track track;
> - u32 state : 2; /* enum kasan_state */
> - u32 alloc_size : 30;
> + u32 alloc_size;
> };
>
> struct qlist_node {
> @@ -109,6 +106,9 @@ static inline bool kasan_report_enabled(void)
>
> void kasan_report(unsigned long addr, size_t size,
> bool is_write, unsigned long ip);
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> + struct kasan_track free_stack, s8 shadow);
> +
>
> #ifdef CONFIG_SLAB
> void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 4973505..3ec039c 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -144,11 +144,9 @@ static void *qlink_to_object(struct qlist_node *qlink, struct kmem_cache *cache)
> static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
> {
> void *object = qlink_to_object(qlink, cache);
> - struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
> unsigned long flags;
>
> local_irq_save(flags);
> - alloc_info->state = KASAN_STATE_FREE;
> ___cache_free(cache, object, _THIS_IP_);
> local_irq_restore(flags);
> }
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index b3c122d..a0f4519 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,28 +140,13 @@ static void object_err(struct kmem_cache *cache, struct page *page,
> pr_err("Object at %p, in cache %s\n", object, cache->name);
> if (!(cache->flags & SLAB_KASAN))
> return;
> - switch (alloc_info->state) {
> - case KASAN_STATE_INIT:
> - pr_err("Object not allocated yet\n");
> - break;
> - case KASAN_STATE_ALLOC:
> - pr_err("Object allocated with size %u bytes.\n",
> - alloc_info->alloc_size);
> - pr_err("Allocation:\n");
> - print_track(&alloc_info->track);
> - break;
> - case KASAN_STATE_FREE:
> - case KASAN_STATE_QUARANTINE:
> - pr_err("Object freed, allocated with size %u bytes\n",
> - alloc_info->alloc_size);
> - free_info = get_free_info(cache, object);
> - pr_err("Allocation:\n");
> - print_track(&alloc_info->track);
> - pr_err("Deallocation:\n");
> - print_track(&free_info->track);
> - break;
> - }
> + free_info = get_free_info(cache, object);
> + pr_err("Allocation:\n");
> + print_track(&alloc_info->track);
> + pr_err("Deallocation:\n");
> + print_track(&free_info->track);
> }
> +
> #endif
>
> static void print_address_description(struct kasan_access_info *info)
> @@ -245,17 +230,31 @@ static void print_shadow_for_address(const void *addr)
>
> static DEFINE_SPINLOCK(report_lock);
>
> -static void kasan_report_error(struct kasan_access_info *info)
> +static void kasan_start_report(unsigned long *flags)
> {
> - unsigned long flags;
> - const char *bug_type;
> -
> /*
> * Make sure we don't end up in loop.
> */
> kasan_disable_current();
> - spin_lock_irqsave(&report_lock, flags);
> + spin_lock_irqsave(&report_lock, *flags);
> pr_err("==================================================================\n");
> +}
> +
> +static void kasan_end_report(unsigned long *flags)
> +{
> + pr_err("==================================================================\n");
> + add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> + spin_unlock_irqrestore(&report_lock, *flags);
> + kasan_enable_current();
> +}
> +
> +static void kasan_report_error(struct kasan_access_info *info)
> +{
> + unsigned long flags;
> + const char *bug_type;
> +
> + kasan_start_report(&flags);
> +
> if (info->access_addr <
> kasan_shadow_to_mem((void *)KASAN_SHADOW_START)) {
> if ((unsigned long)info->access_addr < PAGE_SIZE)
> @@ -276,10 +275,29 @@ static void kasan_report_error(struct kasan_access_info *info)
> print_address_description(info);
> print_shadow_for_address(info->first_bad_addr);
> }
> - pr_err("==================================================================\n");
> - add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> - spin_unlock_irqrestore(&report_lock, flags);
> - kasan_enable_current();
> +
> + kasan_end_report(&flags);
> +}
> +
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> + struct kasan_track free_stack, s8 shadow)
> +{
> + unsigned long flags;
> +
> + kasan_start_report(&flags);
> +
> + pr_err("BUG: Double free or corrupt pointer\n");
> + pr_err("Unexpected shadow byte: 0x%hhX\n", shadow);
> +
> + dump_stack();
> + pr_err("Object at %p, in cache %s\n", object, cache->name);
> + get_alloc_info(cache, object);
> + pr_err("Allocation:\n");
> + print_track(&get_alloc_info(cache, object)->track);
> + pr_err("Deallocation:\n");
> + print_track(&free_stack);
> +
> + kasan_end_report(&flags);
> }
>
> void kasan_report(unsigned long addr, size_t size,
> diff --git a/mm/slab.c b/mm/slab.c
> index 763096a..65c942b 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2604,9 +2604,11 @@ static void cache_init_objs(struct kmem_cache *cachep,
> }
>
> for (i = 0; i < cachep->num; i++) {
> + objp = index_to_obj(cachep, page, i);
> + kasan_init_slab_obj(cachep, objp);
> +
> /* constructor could break poison info */
> if (DEBUG == 0 && cachep->ctor) {
> - objp = index_to_obj(cachep, page, i);
> kasan_unpoison_object_data(cachep, objp);
> cachep->ctor(objp);
> kasan_poison_object_data(cachep, objp);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] mm, kasan: improve double-free detection
2016-06-10 17:03 ` Andrey Ryabinin
2016-06-10 17:09 ` Dmitry Vyukov
@ 2016-06-13 11:56 ` Alexander Potapenko
2016-06-14 6:46 ` Luruo, Kuthonuzo
2 siblings, 0 replies; 10+ messages in thread
From: Alexander Potapenko @ 2016-06-13 11:56 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Kuthonuzo Luruo, Dmitriy Vyukov, Christoph Lameter, penberg,
David Rientjes, Joonsoo Kim, Andrew Morton, kasan-dev, LKML,
ynorov
On Fri, Jun 10, 2016 at 7:03 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 06/09/2016 08:00 PM, Andrey Ryabinin wrote:
>> On 06/07/2016 09:03 PM, Kuthonuzo Luruo wrote:
>>
>> Next time, when/if you send patch series, send patches in one thread, i.e. patches should be replies to the cover letter.
>> Your patches are not linked together, which makes them harder to track.
>>
>>
>>> Currently, KASAN may fail to detect concurrent deallocations of the same
>>> object due to a race in kasan_slab_free(). This patch makes double-free
>>> detection more reliable by serializing access to KASAN object metadata.
>>> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
>>> lock/unlock per-object metadata. Double-free errors are now reported via
>>> kasan_report().
>>>
>>> Per-object lock concept from suggestion/observations by Dmitry Vyukov.
>>>
>>
>>
>> So, I still don't like this, this too way hacky and complex.
>> I have some thoughts about how to make this lockless and robust enough.
>> I'll try to sort this out tomorrow.
>>
>
>
> So, I something like this should work.
> Tested very briefly.
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index ac4b3c4..8691142 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -75,6 +75,8 @@ struct kasan_cache {
> int kasan_module_alloc(void *addr, size_t size);
> void kasan_free_shadow(const struct vm_struct *vm);
>
> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object);
> +
> size_t ksize(const void *);
> static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
>
> @@ -102,6 +104,9 @@ static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
> static inline void kasan_poison_object_data(struct kmem_cache *cache,
> void *object) {}
>
> +static inline void kasan_init_slab_obj(struct kmem_cache *cache,
> + const void *object) { }
> +
> static inline void kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags) {}
> static inline void kasan_kfree_large(const void *ptr) {}
> static inline void kasan_poison_kfree(void *ptr) {}
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 6845f92..ab0fded 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -388,11 +388,9 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
> *size += sizeof(struct kasan_alloc_meta);
>
> /* Add free meta. */
> - if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
> - cache->object_size < sizeof(struct kasan_free_meta)) {
> - cache->kasan_info.free_meta_offset = *size;
> - *size += sizeof(struct kasan_free_meta);
> - }
> + cache->kasan_info.free_meta_offset = *size;
> + *size += sizeof(struct kasan_free_meta);
> +
> redzone_adjust = optimal_redzone(cache->object_size) -
> (*size - cache->object_size);
> if (redzone_adjust > 0)
> @@ -431,13 +429,6 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
> kasan_poison_shadow(object,
> round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
> KASAN_KMALLOC_REDZONE);
> -#ifdef CONFIG_SLAB
> - if (cache->flags & SLAB_KASAN) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - alloc_info->state = KASAN_STATE_INIT;
> - }
> -#endif
> }
>
> #ifdef CONFIG_SLAB
> @@ -501,6 +492,20 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
> BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
> return (void *)object + cache->kasan_info.free_meta_offset;
> }
> +
> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object)
> +{
> + struct kasan_alloc_meta *alloc_info;
> + struct kasan_free_meta *free_info;
> +
> + if (!(cache->flags & SLAB_KASAN))
> + return;
> +
> + alloc_info = get_alloc_info(cache, object);
> + free_info = get_free_info(cache, object);
> + __memset(alloc_info, 0, sizeof(*alloc_info));
> + __memset(free_info, 0, sizeof(*free_info));
> +}
> #endif
>
> void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
> @@ -523,37 +528,47 @@ static void kasan_poison_slab_free(struct kmem_cache *cache, void *object)
> bool kasan_slab_free(struct kmem_cache *cache, void *object)
> {
> #ifdef CONFIG_SLAB
> + struct kasan_free_meta *free_info = get_free_info(cache, object);
> + struct kasan_track new_free_stack, old_free_stack;
> + s8 old_shadow;
> +
> /* RCU slabs could be legally used after free within the RCU period */
> if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
> return false;
>
> - if (likely(cache->flags & SLAB_KASAN)) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - struct kasan_free_meta *free_info =
> - get_free_info(cache, object);
> -
> - switch (alloc_info->state) {
> - case KASAN_STATE_ALLOC:
> - alloc_info->state = KASAN_STATE_QUARANTINE;
> - quarantine_put(free_info, cache);
> - set_track(&free_info->track, GFP_NOWAIT);
> - kasan_poison_slab_free(cache, object);
> - return true;
> - case KASAN_STATE_QUARANTINE:
> - case KASAN_STATE_FREE:
> - pr_err("Double free");
> - dump_stack();
> - break;
> - default:
> - break;
> - }
> + if (unlikely(!(cache->flags & SLAB_KASAN)))
> + return false;
> +
> + set_track(&new_free_stack, GFP_NOWAIT);
> + old_free_stack = xchg(&free_info->track, new_free_stack);
> + old_shadow = xchg((s8 *)kasan_mem_to_shadow(object),
> + KASAN_KMALLOC_FREE);
Indeed, is the use of two xchg operations better than what Kuthonuzo suggests?
On a related note, I wonder whether false sharing becomes a problem
when we perform atomic operations on the shadow (or spin on it, like
in the original suggestion).
> +
> + if (old_shadow < 0 || old_shadow >= KASAN_SHADOW_SCALE_SIZE) {
> + struct kasan_track free_stack;
> +
> + /* Paired with xchg() above */
> + free_stack = smp_load_acquire(&free_info->track);
> +
> + /*
> + * We didn't raced with another instance of kasan_slab_free()
> + * so the previous free stack supposed to be in old_free_stack.
> + * Otherwise, free_stack will contain stack trace of another
> + * kfree() call.
> + */
> + if (free_stack.id == new_free_stack.id)
> + free_stack = old_free_stack;
> +
> + kasan_report_double_free(cache, object,
> + free_stack, old_shadow);
> + return false;
> }
> - return false;
> -#else
> kasan_poison_slab_free(cache, object);
> - return false;
> + return true;
> +
> #endif
> + kasan_poison_slab_free(cache, object);
> + return false;
> }
>
> void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
> @@ -581,7 +596,6 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
> struct kasan_alloc_meta *alloc_info =
> get_alloc_info(cache, object);
>
> - alloc_info->state = KASAN_STATE_ALLOC;
> alloc_info->alloc_size = size;
> set_track(&alloc_info->track, flags);
> }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index fb87923..9b46d2e 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -59,24 +59,21 @@ struct kasan_global {
> * Structures to keep alloc and free tracks *
> */
>
> -enum kasan_state {
> - KASAN_STATE_INIT,
> - KASAN_STATE_ALLOC,
> - KASAN_STATE_QUARANTINE,
> - KASAN_STATE_FREE
> -};
> -
> #define KASAN_STACK_DEPTH 64
>
> struct kasan_track {
> - u32 pid;
> - depot_stack_handle_t stack;
> +union {
> + struct {
> + u32 pid;
> + depot_stack_handle_t stack;
> + };
> + u64 id;
> +};
> };
>
> struct kasan_alloc_meta {
> struct kasan_track track;
> - u32 state : 2; /* enum kasan_state */
> - u32 alloc_size : 30;
> + u32 alloc_size;
> };
>
> struct qlist_node {
> @@ -109,6 +106,9 @@ static inline bool kasan_report_enabled(void)
>
> void kasan_report(unsigned long addr, size_t size,
> bool is_write, unsigned long ip);
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> + struct kasan_track free_stack, s8 shadow);
> +
>
> #ifdef CONFIG_SLAB
> void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 4973505..3ec039c 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -144,11 +144,9 @@ static void *qlink_to_object(struct qlist_node *qlink, struct kmem_cache *cache)
> static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
> {
> void *object = qlink_to_object(qlink, cache);
> - struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
> unsigned long flags;
>
> local_irq_save(flags);
> - alloc_info->state = KASAN_STATE_FREE;
> ___cache_free(cache, object, _THIS_IP_);
> local_irq_restore(flags);
> }
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index b3c122d..a0f4519 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,28 +140,13 @@ static void object_err(struct kmem_cache *cache, struct page *page,
> pr_err("Object at %p, in cache %s\n", object, cache->name);
> if (!(cache->flags & SLAB_KASAN))
> return;
> - switch (alloc_info->state) {
> - case KASAN_STATE_INIT:
> - pr_err("Object not allocated yet\n");
> - break;
> - case KASAN_STATE_ALLOC:
> - pr_err("Object allocated with size %u bytes.\n",
> - alloc_info->alloc_size);
> - pr_err("Allocation:\n");
> - print_track(&alloc_info->track);
> - break;
> - case KASAN_STATE_FREE:
> - case KASAN_STATE_QUARANTINE:
> - pr_err("Object freed, allocated with size %u bytes\n",
> - alloc_info->alloc_size);
> - free_info = get_free_info(cache, object);
> - pr_err("Allocation:\n");
> - print_track(&alloc_info->track);
> - pr_err("Deallocation:\n");
> - print_track(&free_info->track);
> - break;
> - }
> + free_info = get_free_info(cache, object);
> + pr_err("Allocation:\n");
> + print_track(&alloc_info->track);
> + pr_err("Deallocation:\n");
> + print_track(&free_info->track);
> }
> +
> #endif
>
> static void print_address_description(struct kasan_access_info *info)
> @@ -245,17 +230,31 @@ static void print_shadow_for_address(const void *addr)
>
> static DEFINE_SPINLOCK(report_lock);
>
> -static void kasan_report_error(struct kasan_access_info *info)
> +static void kasan_start_report(unsigned long *flags)
> {
> - unsigned long flags;
> - const char *bug_type;
> -
> /*
> * Make sure we don't end up in loop.
> */
> kasan_disable_current();
> - spin_lock_irqsave(&report_lock, flags);
> + spin_lock_irqsave(&report_lock, *flags);
> pr_err("==================================================================\n");
> +}
> +
> +static void kasan_end_report(unsigned long *flags)
> +{
> + pr_err("==================================================================\n");
> + add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> + spin_unlock_irqrestore(&report_lock, *flags);
> + kasan_enable_current();
> +}
> +
> +static void kasan_report_error(struct kasan_access_info *info)
> +{
> + unsigned long flags;
> + const char *bug_type;
> +
> + kasan_start_report(&flags);
> +
> if (info->access_addr <
> kasan_shadow_to_mem((void *)KASAN_SHADOW_START)) {
> if ((unsigned long)info->access_addr < PAGE_SIZE)
> @@ -276,10 +275,29 @@ static void kasan_report_error(struct kasan_access_info *info)
> print_address_description(info);
> print_shadow_for_address(info->first_bad_addr);
> }
> - pr_err("==================================================================\n");
> - add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> - spin_unlock_irqrestore(&report_lock, flags);
> - kasan_enable_current();
> +
> + kasan_end_report(&flags);
> +}
> +
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> + struct kasan_track free_stack, s8 shadow)
> +{
> + unsigned long flags;
> +
> + kasan_start_report(&flags);
> +
> + pr_err("BUG: Double free or corrupt pointer\n");
> + pr_err("Unexpected shadow byte: 0x%hhX\n", shadow);
> +
> + dump_stack();
> + pr_err("Object at %p, in cache %s\n", object, cache->name);
> + get_alloc_info(cache, object);
> + pr_err("Allocation:\n");
> + print_track(&get_alloc_info(cache, object)->track);
> + pr_err("Deallocation:\n");
> + print_track(&free_stack);
> +
> + kasan_end_report(&flags);
> }
>
> void kasan_report(unsigned long addr, size_t size,
> diff --git a/mm/slab.c b/mm/slab.c
> index 763096a..65c942b 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2604,9 +2604,11 @@ static void cache_init_objs(struct kmem_cache *cachep,
> }
>
> for (i = 0; i < cachep->num; i++) {
> + objp = index_to_obj(cachep, page, i);
> + kasan_init_slab_obj(cachep, objp);
> +
> /* constructor could break poison info */
> if (DEBUG == 0 && cachep->ctor) {
> - objp = index_to_obj(cachep, page, i);
> kasan_unpoison_object_data(cachep, objp);
> cachep->ctor(objp);
> kasan_poison_object_data(cachep, objp);
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v5 1/2] mm, kasan: improve double-free detection
2016-06-10 17:03 ` Andrey Ryabinin
2016-06-10 17:09 ` Dmitry Vyukov
2016-06-13 11:56 ` Alexander Potapenko
@ 2016-06-14 6:46 ` Luruo, Kuthonuzo
2016-06-15 16:38 ` Andrey Ryabinin
2 siblings, 1 reply; 10+ messages in thread
From: Luruo, Kuthonuzo @ 2016-06-14 6:46 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: glider, dvyukov, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
kasan-dev, linux-kernel, ynorov
> > Next time, when/if you send patch series, send patches in one thread, i.e.
> > patches should be replies to the cover letter.
> > Your patches are not linked together, which makes them harder to track.
Thanks for the tip; but doesn't this conflict with the advice in
https://www.kernel.org/doc/Documentation/SubmittingPatches, specifically the
use of the "summary phrase"...
> >
> >
> >> Currently, KASAN may fail to detect concurrent deallocations of the same
> >> object due to a race in kasan_slab_free(). This patch makes double-free
> >> detection more reliable by serializing access to KASAN object metadata.
> >> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
> >> lock/unlock per-object metadata. Double-free errors are now reported via
> >> kasan_report().
> >>
> >> Per-object lock concept from suggestion/observations by Dmitry Vyukov.
> >>
> >
> >
> > So, I still don't like this, this too way hacky and complex.
I don't think patch is particularly complex; but respect your judgment.
> > I have some thoughts about how to make this lockless and robust enough.
> > I'll try to sort this out tomorrow.
> >
>
>
> So, I something like this should work.
> Tested very briefly.
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index ac4b3c4..8691142 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -75,6 +75,8 @@ struct kasan_cache {
> int kasan_module_alloc(void *addr, size_t size);
> void kasan_free_shadow(const struct vm_struct *vm);
>
> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object);
> +
> size_t ksize(const void *);
> static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
>
> @@ -102,6 +104,9 @@ static inline void kasan_unpoison_object_data(struct
> kmem_cache *cache,
> static inline void kasan_poison_object_data(struct kmem_cache *cache,
> void *object) {}
>
> +static inline void kasan_init_slab_obj(struct kmem_cache *cache,
> + const void *object) { }
> +
> static inline void kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags) {}
> static inline void kasan_kfree_large(const void *ptr) {}
> static inline void kasan_poison_kfree(void *ptr) {}
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 6845f92..ab0fded 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -388,11 +388,9 @@ void kasan_cache_create(struct kmem_cache *cache,
> size_t *size,
> *size += sizeof(struct kasan_alloc_meta);
>
> /* Add free meta. */
> - if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
> - cache->object_size < sizeof(struct kasan_free_meta)) {
> - cache->kasan_info.free_meta_offset = *size;
> - *size += sizeof(struct kasan_free_meta);
> - }
> + cache->kasan_info.free_meta_offset = *size;
> + *size += sizeof(struct kasan_free_meta);
> +
> redzone_adjust = optimal_redzone(cache->object_size) -
> (*size - cache->object_size);
> if (redzone_adjust > 0)
> @@ -431,13 +429,6 @@ void kasan_poison_object_data(struct kmem_cache
> *cache, void *object)
> kasan_poison_shadow(object,
> round_up(cache->object_size,
> KASAN_SHADOW_SCALE_SIZE),
> KASAN_KMALLOC_REDZONE);
> -#ifdef CONFIG_SLAB
> - if (cache->flags & SLAB_KASAN) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - alloc_info->state = KASAN_STATE_INIT;
> - }
> -#endif
> }
>
> #ifdef CONFIG_SLAB
> @@ -501,6 +492,20 @@ struct kasan_free_meta *get_free_info(struct
> kmem_cache *cache,
> BUILD_BUG_ON(sizeof(struct kasan_free_meta) > 32);
> return (void *)object + cache->kasan_info.free_meta_offset;
> }
> +
> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object)
> +{
> + struct kasan_alloc_meta *alloc_info;
> + struct kasan_free_meta *free_info;
> +
> + if (!(cache->flags & SLAB_KASAN))
> + return;
> +
> + alloc_info = get_alloc_info(cache, object);
> + free_info = get_free_info(cache, object);
> + __memset(alloc_info, 0, sizeof(*alloc_info));
> + __memset(free_info, 0, sizeof(*free_info));
> +}
> #endif
>
> void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
> @@ -523,37 +528,47 @@ static void kasan_poison_slab_free(struct
> kmem_cache *cache, void *object)
> bool kasan_slab_free(struct kmem_cache *cache, void *object)
> {
> #ifdef CONFIG_SLAB
> + struct kasan_free_meta *free_info = get_free_info(cache, object);
> + struct kasan_track new_free_stack, old_free_stack;
> + s8 old_shadow;
> +
> /* RCU slabs could be legally used after free within the RCU period */
> if (unlikely(cache->flags & SLAB_DESTROY_BY_RCU))
> return false;
>
> - if (likely(cache->flags & SLAB_KASAN)) {
> - struct kasan_alloc_meta *alloc_info =
> - get_alloc_info(cache, object);
> - struct kasan_free_meta *free_info =
> - get_free_info(cache, object);
> -
> - switch (alloc_info->state) {
> - case KASAN_STATE_ALLOC:
> - alloc_info->state = KASAN_STATE_QUARANTINE;
> - quarantine_put(free_info, cache);
> - set_track(&free_info->track, GFP_NOWAIT);
> - kasan_poison_slab_free(cache, object);
> - return true;
> - case KASAN_STATE_QUARANTINE:
> - case KASAN_STATE_FREE:
> - pr_err("Double free");
> - dump_stack();
> - break;
> - default:
> - break;
> - }
> + if (unlikely(!(cache->flags & SLAB_KASAN)))
> + return false;
> +
> + set_track(&new_free_stack, GFP_NOWAIT);
> + old_free_stack = xchg(&free_info->track, new_free_stack);
> + old_shadow = xchg((s8 *)kasan_mem_to_shadow(object),
> + KASAN_KMALLOC_FREE);
> +
> + if (old_shadow < 0 || old_shadow >= KASAN_SHADOW_SCALE_SIZE) {
> + struct kasan_track free_stack;
> +
> + /* Paired with xchg() above */
> + free_stack = smp_load_acquire(&free_info->track);
> +
> + /*
> + * We didn't raced with another instance of kasan_slab_free()
> + * so the previous free stack supposed to be in old_free_stack.
> + * Otherwise, free_stack will contain stack trace of another
> + * kfree() call.
> + */
> + if (free_stack.id == new_free_stack.id)
> + free_stack = old_free_stack;
> +
> + kasan_report_double_free(cache, object,
> + free_stack, old_shadow);
> + return false;
> }
> - return false;
> -#else
> kasan_poison_slab_free(cache, object);
> - return false;
> + return true;
> +
> #endif
> + kasan_poison_slab_free(cache, object);
> + return false;
> }
>
> void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
> @@ -581,7 +596,6 @@ void kasan_kmalloc(struct kmem_cache *cache, const
> void *object, size_t size,
> struct kasan_alloc_meta *alloc_info =
> get_alloc_info(cache, object);
>
> - alloc_info->state = KASAN_STATE_ALLOC;
> alloc_info->alloc_size = size;
> set_track(&alloc_info->track, flags);
> }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index fb87923..9b46d2e 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -59,24 +59,21 @@ struct kasan_global {
> * Structures to keep alloc and free tracks *
> */
>
> -enum kasan_state {
> - KASAN_STATE_INIT,
> - KASAN_STATE_ALLOC,
> - KASAN_STATE_QUARANTINE,
> - KASAN_STATE_FREE
> -};
> -
> #define KASAN_STACK_DEPTH 64
>
> struct kasan_track {
> - u32 pid;
> - depot_stack_handle_t stack;
> +union {
> + struct {
> + u32 pid;
> + depot_stack_handle_t stack;
> + };
> + u64 id;
> +};
> };
>
> struct kasan_alloc_meta {
> struct kasan_track track;
> - u32 state : 2; /* enum kasan_state */
> - u32 alloc_size : 30;
> + u32 alloc_size;
> };
>
> struct qlist_node {
> @@ -109,6 +106,9 @@ static inline bool kasan_report_enabled(void)
>
> void kasan_report(unsigned long addr, size_t size,
> bool is_write, unsigned long ip);
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> + struct kasan_track free_stack, s8 shadow);
> +
>
> #ifdef CONFIG_SLAB
> void quarantine_put(struct kasan_free_meta *info, struct kmem_cache
> *cache);
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 4973505..3ec039c 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -144,11 +144,9 @@ static void *qlink_to_object(struct qlist_node *qlink,
> struct kmem_cache *cache)
> static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
> {
> void *object = qlink_to_object(qlink, cache);
> - struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
> unsigned long flags;
>
> local_irq_save(flags);
> - alloc_info->state = KASAN_STATE_FREE;
> ___cache_free(cache, object, _THIS_IP_);
> local_irq_restore(flags);
> }
> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> index b3c122d..a0f4519 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,28 +140,13 @@ static void object_err(struct kmem_cache *cache,
> struct page *page,
> pr_err("Object at %p, in cache %s\n", object, cache->name);
> if (!(cache->flags & SLAB_KASAN))
> return;
> - switch (alloc_info->state) {
> - case KASAN_STATE_INIT:
> - pr_err("Object not allocated yet\n");
> - break;
> - case KASAN_STATE_ALLOC:
> - pr_err("Object allocated with size %u bytes.\n",
> - alloc_info->alloc_size);
> - pr_err("Allocation:\n");
> - print_track(&alloc_info->track);
> - break;
> - case KASAN_STATE_FREE:
> - case KASAN_STATE_QUARANTINE:
> - pr_err("Object freed, allocated with size %u bytes\n",
> - alloc_info->alloc_size);
> - free_info = get_free_info(cache, object);
> - pr_err("Allocation:\n");
> - print_track(&alloc_info->track);
> - pr_err("Deallocation:\n");
> - print_track(&free_info->track);
> - break;
> - }
> + free_info = get_free_info(cache, object);
> + pr_err("Allocation:\n");
> + print_track(&alloc_info->track);
> + pr_err("Deallocation:\n");
> + print_track(&free_info->track);
> }
> +
> #endif
>
> static void print_address_description(struct kasan_access_info *info)
> @@ -245,17 +230,31 @@ static void print_shadow_for_address(const void
> *addr)
>
> static DEFINE_SPINLOCK(report_lock);
>
> -static void kasan_report_error(struct kasan_access_info *info)
> +static void kasan_start_report(unsigned long *flags)
> {
> - unsigned long flags;
> - const char *bug_type;
> -
> /*
> * Make sure we don't end up in loop.
> */
> kasan_disable_current();
> - spin_lock_irqsave(&report_lock, flags);
> + spin_lock_irqsave(&report_lock, *flags);
>
> pr_err("====================================================
> ==============\n");
> +}
> +
> +static void kasan_end_report(unsigned long *flags)
> +{
> +
> pr_err("====================================================
> ==============\n");
> + add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> + spin_unlock_irqrestore(&report_lock, *flags);
> + kasan_enable_current();
> +}
> +
> +static void kasan_report_error(struct kasan_access_info *info)
> +{
> + unsigned long flags;
> + const char *bug_type;
> +
> + kasan_start_report(&flags);
> +
> if (info->access_addr <
> kasan_shadow_to_mem((void
> *)KASAN_SHADOW_START)) {
> if ((unsigned long)info->access_addr < PAGE_SIZE)
> @@ -276,10 +275,29 @@ static void kasan_report_error(struct
> kasan_access_info *info)
> print_address_description(info);
> print_shadow_for_address(info->first_bad_addr);
> }
> -
> pr_err("====================================================
> ==============\n");
> - add_taint(TAINT_BAD_PAGE, LOCKDEP_NOW_UNRELIABLE);
> - spin_unlock_irqrestore(&report_lock, flags);
> - kasan_enable_current();
> +
> + kasan_end_report(&flags);
> +}
> +
> +void kasan_report_double_free(struct kmem_cache *cache, void *object,
> + struct kasan_track free_stack, s8 shadow)
> +{
> + unsigned long flags;
> +
> + kasan_start_report(&flags);
> +
> + pr_err("BUG: Double free or corrupt pointer\n");
> + pr_err("Unexpected shadow byte: 0x%hhX\n", shadow);
> +
> + dump_stack();
> + pr_err("Object at %p, in cache %s\n", object, cache->name);
> + get_alloc_info(cache, object);
> + pr_err("Allocation:\n");
> + print_track(&get_alloc_info(cache, object)->track);
> + pr_err("Deallocation:\n");
> + print_track(&free_stack);
> +
> + kasan_end_report(&flags);
> }
>
> void kasan_report(unsigned long addr, size_t size,
> diff --git a/mm/slab.c b/mm/slab.c
> index 763096a..65c942b 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2604,9 +2604,11 @@ static void cache_init_objs(struct kmem_cache
> *cachep,
> }
>
> for (i = 0; i < cachep->num; i++) {
> + objp = index_to_obj(cachep, page, i);
> + kasan_init_slab_obj(cachep, objp);
> +
> /* constructor could break poison info */
> if (DEBUG == 0 && cachep->ctor) {
> - objp = index_to_obj(cachep, page, i);
> kasan_unpoison_object_data(cachep, objp);
> cachep->ctor(objp);
> kasan_poison_object_data(cachep, objp);
Nice hack & novel approach. It does have the flaw that subsequent error reports
for object will have a bogus deallocation track.
Kuthonuzo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] mm, kasan: improve double-free detection
2016-06-10 17:09 ` Dmitry Vyukov
@ 2016-06-15 16:18 ` Andrey Ryabinin
0 siblings, 0 replies; 10+ messages in thread
From: Andrey Ryabinin @ 2016-06-15 16:18 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Kuthonuzo Luruo, Alexander Potapenko, Christoph Lameter,
Pekka Enberg, David Rientjes, Joonsoo Kim, Andrew Morton,
kasan-dev, LKML, Yury Norov
On 06/10/2016 08:09 PM, Dmitry Vyukov wrote:
> On Fri, Jun 10, 2016 at 7:03 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>>
>>
>> On 06/09/2016 08:00 PM, Andrey Ryabinin wrote:
>>> On 06/07/2016 09:03 PM, Kuthonuzo Luruo wrote:
>>>
>>> Next time, when/if you send patch series, send patches in one thread, i.e. patches should be replies to the cover letter.
>>> Your patches are not linked together, which makes them harder to track.
>>>
>>>
>>>> Currently, KASAN may fail to detect concurrent deallocations of the same
>>>> object due to a race in kasan_slab_free(). This patch makes double-free
>>>> detection more reliable by serializing access to KASAN object metadata.
>>>> New functions kasan_meta_lock() and kasan_meta_unlock() are provided to
>>>> lock/unlock per-object metadata. Double-free errors are now reported via
>>>> kasan_report().
>>>>
>>>> Per-object lock concept from suggestion/observations by Dmitry Vyukov.
>>>>
>>>
>>>
>>> So, I still don't like this, this too way hacky and complex.
>>> I have some thoughts about how to make this lockless and robust enough.
>>> I'll try to sort this out tomorrow.
>>>
>>
>>
>> So, I something like this should work.
>> Tested very briefly.
>>
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index ac4b3c4..8691142 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -75,6 +75,8 @@ struct kasan_cache {
>> int kasan_module_alloc(void *addr, size_t size);
>> void kasan_free_shadow(const struct vm_struct *vm);
>>
>> +void kasan_init_slab_obj(struct kmem_cache *cache, const void *object);
>> +
>> size_t ksize(const void *);
>> static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
>>
>> @@ -102,6 +104,9 @@ static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
>> static inline void kasan_poison_object_data(struct kmem_cache *cache,
>> void *object) {}
>>
>> +static inline void kasan_init_slab_obj(struct kmem_cache *cache,
>> + const void *object) { }
>> +
>> static inline void kasan_kmalloc_large(void *ptr, size_t size, gfp_t flags) {}
>> static inline void kasan_kfree_large(const void *ptr) {}
>> static inline void kasan_poison_kfree(void *ptr) {}
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index 6845f92..ab0fded 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -388,11 +388,9 @@ void kasan_cache_create(struct kmem_cache *cache, size_t *size,
>> *size += sizeof(struct kasan_alloc_meta);
>>
>> /* Add free meta. */
>> - if (cache->flags & SLAB_DESTROY_BY_RCU || cache->ctor ||
>> - cache->object_size < sizeof(struct kasan_free_meta)) {
>> - cache->kasan_info.free_meta_offset = *size;
>> - *size += sizeof(struct kasan_free_meta);
>> - }
>> + cache->kasan_info.free_meta_offset = *size;
>> + *size += sizeof(struct kasan_free_meta);
>> +
>
>
> Why?!
> Please don't worsen runtime characteristics of KASAN. We run real
> systems with it.
> Most objects are small. This can lead to significant memory consumption.
>
Yeah, this is a temp hack actually, because I didn't finish this part yet.
Basically, I want to make free stack always available (i.e. always save it in redzone),
because the is always better to have more information. Also this makes bug
report code a bit easier.
Of course, increasing memory usage is not what we want, so my plan is to make this:
- remove alloc_size, because we already now object size. I mean cache->object_size.
For kmalloc()'ed objects object_size is rounded up size, but exact size of allocation
usually is not valuable information (Personally, I can't remember it being useful).
- Unify allocation stack and free stack and keep them both in redzone. This is exactly 16-bytes, so this won't
increase memory usage. So only quarantine pointer may be stored in freed object.
Proposed changes will actually decrease memory usage, because 8-byte objects will occupy less space.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/2] mm, kasan: improve double-free detection
2016-06-14 6:46 ` Luruo, Kuthonuzo
@ 2016-06-15 16:38 ` Andrey Ryabinin
0 siblings, 0 replies; 10+ messages in thread
From: Andrey Ryabinin @ 2016-06-15 16:38 UTC (permalink / raw)
To: Luruo, Kuthonuzo
Cc: glider, dvyukov, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
kasan-dev, linux-kernel, ynorov
On 06/14/2016 09:46 AM, Luruo, Kuthonuzo wrote:
>>> Next time, when/if you send patch series, send patches in one thread, i.e.
>>> patches should be replies to the cover letter.
>>> Your patches are not linked together, which makes them harder to track.
>
> Thanks for the tip; but doesn't this conflict with the advice in
> https://www.kernel.org/doc/Documentation/SubmittingPatches, specifically the
> use of the "summary phrase"...
>
No, it doesn't. 'summary phrase' is about "Subject:" line. My point is about setting 'In-Reaply-To' which section 15) of SubmittingPatches
For example, this one is good case:
http://thread.gmane.org/gmane.linux.kernel/2244577
All patches are replies to the cover letter, so they are in one thread.
And this is yours:
http://thread.gmane.org/gmane.linux.kernel/2237786
http://thread.gmane.org/gmane.linux.kernel/2237787
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-06-15 16:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07 18:03 [PATCH v5 1/2] mm, kasan: improve double-free detection Kuthonuzo Luruo
2016-06-09 13:32 ` Alexander Potapenko
2016-06-09 16:54 ` Luruo, Kuthonuzo
2016-06-09 17:00 ` Andrey Ryabinin
2016-06-10 17:03 ` Andrey Ryabinin
2016-06-10 17:09 ` Dmitry Vyukov
2016-06-15 16:18 ` Andrey Ryabinin
2016-06-13 11:56 ` Alexander Potapenko
2016-06-14 6:46 ` Luruo, Kuthonuzo
2016-06-15 16:38 ` Andrey Ryabinin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).