[PATCH v2 1/2] mm, kasan: improve double-free detection

[PATCH v2 1/2] mm, kasan: improve double-free detection

[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().

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 'test_kasan' kasan_double_free() test in accompanying
  patch.

Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
---

Changes in v2:
- Incorporated suggestions from Dmitry Vyukov. New per-object metadata
  lock/unlock functions; kasan_alloc_meta modified to add new state while
  using fewer bits overall.
- Double-free pr_err promoted to kasan_report().
- kasan_init_object() introduced to initialize KASAN object metadata
  during slab creation. KASAN_STATE_INIT initialization removed from
  kasan_poison_object_data().
 
---
 include/linux/kasan.h |    8 +++
 mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++-------------
 mm/kasan/kasan.h      |   15 +++++-
 mm/kasan/quarantine.c |    7 +++-
 mm/kasan/report.c     |   31 +++++++++++--
 mm/slab.c             |    1 +
 6 files changed, 142 insertions(+), 38 deletions(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 645c280..c7bf625 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -78,6 +78,10 @@ struct kasan_cache {
 int kasan_module_alloc(void *addr, size_t size);
 void kasan_free_shadow(const struct vm_struct *vm);
 
+#ifdef CONFIG_SLAB
+void kasan_init_object(struct kmem_cache *cache, void *object);
+#endif
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
@@ -124,6 +128,10 @@ static inline void kasan_poison_slab_free(struct kmem_cache *s, void *object) {}
 static inline int kasan_module_alloc(void *addr, size_t size) { return 0; }
 static inline void kasan_free_shadow(const struct vm_struct *vm) {}
 
+#ifdef CONFIG_SLAB
+static inline void kasan_init_object(struct kmem_cache *cache, void *object) {}
+#endif
+
 #endif /* CONFIG_KASAN */
 
 #endif /* LINUX_KASAN_H */
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index ef2e87b..a840b49 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -34,6 +34,7 @@
 #include <linux/string.h>
 #include <linux/types.h>
 #include <linux/vmalloc.h>
+#include <linux/atomic.h>
 
 #include "kasan.h"
 #include "../slab.h"
@@ -419,13 +420,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
@@ -470,6 +464,18 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
 	return depot_save_stack(&trace, flags);
 }
 
+void kasan_init_object(struct kmem_cache *cache, void *object)
+{
+	struct kasan_alloc_meta *alloc_info;
+
+	if (cache->flags & SLAB_KASAN) {
+		kasan_unpoison_object_data(cache, object);
+		alloc_info = get_alloc_info(cache, object);
+		__memset(alloc_info, 0, sizeof(*alloc_info));
+		kasan_poison_object_data(cache, object);
+	}
+}
+
 static inline void set_track(struct kasan_track *track, gfp_t flags)
 {
 	track->pid = current->pid;
@@ -489,6 +495,39 @@ 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;
 }
+
+/* acquire per-object lock for access to KASAN metadata. */
+void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
+{
+	union kasan_alloc_data old, new;
+
+	preempt_disable();
+	for (;;) {
+		old.packed = READ_ONCE(alloc_info->data);
+		if (unlikely(old.lock)) {
+			cpu_relax();
+			continue;
+		}
+		new.packed = old.packed;
+		new.lock = 1;
+		if (likely(cmpxchg(&alloc_info->data, old.packed, new.packed)
+					== old.packed))
+			break;
+	}
+}
+
+/* release lock after a kasan_meta_lock(). */
+void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
+{
+	union kasan_alloc_data alloc_data;
+
+	alloc_data.packed = READ_ONCE(alloc_info->data);
+	alloc_data.lock = 0;
+	if (unlikely(xchg(&alloc_info->data, alloc_data.packed) !=
+				(alloc_data.packed | 0x1U)))
+		WARN_ONCE(1, "%s: lock not held!\n", __func__);
+	preempt_enable();
+}
 #endif
 
 void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
@@ -511,32 +550,41 @@ 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_alloc_meta *alloc_info;
+	struct kasan_free_meta *free_info;
+	union kasan_alloc_data alloc_data;
+
 	/* 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;
+
+	alloc_info = get_alloc_info(cache, object);
+	kasan_meta_lock(alloc_info);
+	alloc_data.packed = alloc_info->data;
+	if (alloc_data.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);
+		alloc_data.state = KASAN_STATE_QUARANTINE;
+		alloc_info->data = alloc_data.packed;
+		kasan_meta_unlock(alloc_info);
+		return true;
 	}
+	switch (alloc_data.state) {
+	case KASAN_STATE_QUARANTINE:
+	case KASAN_STATE_FREE:
+		kasan_report((unsigned long)object, 0, false,
+				(unsigned long)__builtin_return_address(1));
+		kasan_meta_unlock(alloc_info);
+		return true;
+	default:
+		break;
+	}
+	kasan_meta_unlock(alloc_info);
 	return false;
 #else
 	kasan_poison_slab_free(cache, object);
@@ -568,12 +616,20 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
 		KASAN_KMALLOC_REDZONE);
 #ifdef CONFIG_SLAB
 	if (cache->flags & SLAB_KASAN) {
+		union kasan_alloc_data alloc_data;
 		struct kasan_alloc_meta *alloc_info =
 			get_alloc_info(cache, object);
-
-		alloc_info->state = KASAN_STATE_ALLOC;
-		alloc_info->alloc_size = size;
+		unsigned long flags;
+
+		local_irq_save(flags);
+		kasan_meta_lock(alloc_info);
+		alloc_data.packed = alloc_info->data;
+		alloc_data.state = KASAN_STATE_ALLOC;
+		alloc_data.size_delta = cache->object_size - size;
+		alloc_info->data = alloc_data.packed;
 		set_track(&alloc_info->track, flags);
+		kasan_meta_unlock(alloc_info);
+		local_irq_restore(flags);
 	}
 #endif
 }
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 7da78a6..df2724d 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -73,10 +73,19 @@ struct kasan_track {
 	depot_stack_handle_t stack;
 };
 
+union kasan_alloc_data {
+	struct {
+		u32 lock : 1;
+		u32 state : 2;		/* enum kasan_state */
+		u32 size_delta : 24;	/* object_size - alloc size */
+		u32 unused : 5;
+	};
+	u32 packed;
+};
+
 struct kasan_alloc_meta {
 	struct kasan_track track;
-	u32 state : 2;	/* enum kasan_state */
-	u32 alloc_size : 30;
+	u32 data;	/* encoded as union kasan_alloc_data */
 	u32 reserved;
 };
 
@@ -112,4 +121,6 @@ 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);
 #endif
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 40159a6..d59a569 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -144,10 +144,15 @@ static void qlink_free(void **qlink, struct kmem_cache *cache)
 {
 	void *object = qlink_to_object(qlink, cache);
 	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
+	union kasan_alloc_data alloc_data;
 	unsigned long flags;
 
 	local_irq_save(flags);
-	alloc_info->state = KASAN_STATE_FREE;
+	kasan_meta_lock(alloc_info);
+	alloc_data.packed = alloc_info->data;
+	alloc_data.state = KASAN_STATE_FREE;
+	alloc_info->data = alloc_data.packed;
+	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..cecf2fa 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) {
+		pr_err("BUG: KASAN: double-free attempt in %pS on object at addr %p\n",
+				(void *)info->ip, info->access_addr);
+		pr_err("Double free by task %s/%d\n",
+				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);
 
@@ -131,29 +142,34 @@ 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, char *unused_reason,
+		struct kasan_access_info *info)
 {
 	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
 	struct kasan_free_meta *free_info;
+	union kasan_alloc_data alloc_data;
 
 	dump_stack();
 	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);
+	alloc_data.packed = alloc_info->data;
+	switch (alloc_data.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);
+				(cache->object_size - alloc_data.size_delta));
 		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);
+				(cache->object_size - alloc_data.size_delta));
 		free_info = get_free_info(cache, object);
 		pr_err("Allocation:\n");
 		print_track(&alloc_info->track);
@@ -161,6 +177,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 +195,13 @@ 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,
+					"kasan: bad access detected", 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 3f20800..110d586 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2651,6 +2651,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);
-- 
1.7.1

[PATCH v2 1/2] mm, kasan: improve double-free detection

[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().

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 'test_kasan' kasan_double_free() test in accompanying
  patch.

Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
---

Changes in v2:
- Incorporated suggestions from Dmitry Vyukov. New per-object metadata
  lock/unlock functions; kasan_alloc_meta modified to add new state while
  using fewer bits overall.
- Double-free pr_err promoted to kasan_report().
- kasan_init_object() introduced to initialize KASAN object metadata
  during slab creation. KASAN_STATE_INIT initialization removed from
  kasan_poison_object_data().
 
---
 include/linux/kasan.h |    8 +++
 mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++-------------
 mm/kasan/kasan.h      |   15 +++++-
 mm/kasan/quarantine.c |    7 +++-
 mm/kasan/report.c     |   31 +++++++++++--
 mm/slab.c             |    1 +
 6 files changed, 142 insertions(+), 38 deletions(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 645c280..c7bf625 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -78,6 +78,10 @@ struct kasan_cache {
 int kasan_module_alloc(void *addr, size_t size);
 void kasan_free_shadow(const struct vm_struct *vm);
 
+#ifdef CONFIG_SLAB
+void kasan_init_object(struct kmem_cache *cache, void *object);
+#endif
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
@@ -124,6 +128,10 @@ static inline void kasan_poison_slab_free(struct kmem_cache *s, void *object) {}
 static inline int kasan_module_alloc(void *addr, size_t size) { return 0; }
 static inline void kasan_free_shadow(const struct vm_struct *vm) {}
 
+#ifdef CONFIG_SLAB
+static inline void kasan_init_object(struct kmem_cache *cache, void *object) {}
+#endif
+
 #endif /* CONFIG_KASAN */
 
 #endif /* LINUX_KASAN_H */
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index ef2e87b..a840b49 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -34,6 +34,7 @@
 #include <linux/string.h>
 #include <linux/types.h>
 #include <linux/vmalloc.h>
+#include <linux/atomic.h>
 
 #include "kasan.h"
 #include "../slab.h"
@@ -419,13 +420,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
@@ -470,6 +464,18 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
 	return depot_save_stack(&trace, flags);
 }
 
+void kasan_init_object(struct kmem_cache *cache, void *object)
+{
+	struct kasan_alloc_meta *alloc_info;
+
+	if (cache->flags & SLAB_KASAN) {
+		kasan_unpoison_object_data(cache, object);
+		alloc_info = get_alloc_info(cache, object);
+		__memset(alloc_info, 0, sizeof(*alloc_info));
+		kasan_poison_object_data(cache, object);
+	}
+}
+
 static inline void set_track(struct kasan_track *track, gfp_t flags)
 {
 	track->pid = current->pid;
@@ -489,6 +495,39 @@ 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;
 }
+
+/* acquire per-object lock for access to KASAN metadata. */
+void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
+{
+	union kasan_alloc_data old, new;
+
+	preempt_disable();
+	for (;;) {
+		old.packed = READ_ONCE(alloc_info->data);
+		if (unlikely(old.lock)) {
+			cpu_relax();
+			continue;
+		}
+		new.packed = old.packed;
+		new.lock = 1;
+		if (likely(cmpxchg(&alloc_info->data, old.packed, new.packed)
+					== old.packed))
+			break;
+	}
+}
+
+/* release lock after a kasan_meta_lock(). */
+void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
+{
+	union kasan_alloc_data alloc_data;
+
+	alloc_data.packed = READ_ONCE(alloc_info->data);
+	alloc_data.lock = 0;
+	if (unlikely(xchg(&alloc_info->data, alloc_data.packed) !=
+				(alloc_data.packed | 0x1U)))
+		WARN_ONCE(1, "%s: lock not held!\n", __func__);
+	preempt_enable();
+}
 #endif
 
 void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
@@ -511,32 +550,41 @@ 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_alloc_meta *alloc_info;
+	struct kasan_free_meta *free_info;
+	union kasan_alloc_data alloc_data;
+
 	/* 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;
+
+	alloc_info = get_alloc_info(cache, object);
+	kasan_meta_lock(alloc_info);
+	alloc_data.packed = alloc_info->data;
+	if (alloc_data.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);
+		alloc_data.state = KASAN_STATE_QUARANTINE;
+		alloc_info->data = alloc_data.packed;
+		kasan_meta_unlock(alloc_info);
+		return true;
 	}
+	switch (alloc_data.state) {
+	case KASAN_STATE_QUARANTINE:
+	case KASAN_STATE_FREE:
+		kasan_report((unsigned long)object, 0, false,
+				(unsigned long)__builtin_return_address(1));
+		kasan_meta_unlock(alloc_info);
+		return true;
+	default:
+		break;
+	}
+	kasan_meta_unlock(alloc_info);
 	return false;
 #else
 	kasan_poison_slab_free(cache, object);
@@ -568,12 +616,20 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
 		KASAN_KMALLOC_REDZONE);
 #ifdef CONFIG_SLAB
 	if (cache->flags & SLAB_KASAN) {
+		union kasan_alloc_data alloc_data;
 		struct kasan_alloc_meta *alloc_info =
 			get_alloc_info(cache, object);
-
-		alloc_info->state = KASAN_STATE_ALLOC;
-		alloc_info->alloc_size = size;
+		unsigned long flags;
+
+		local_irq_save(flags);
+		kasan_meta_lock(alloc_info);
+		alloc_data.packed = alloc_info->data;
+		alloc_data.state = KASAN_STATE_ALLOC;
+		alloc_data.size_delta = cache->object_size - size;
+		alloc_info->data = alloc_data.packed;
 		set_track(&alloc_info->track, flags);
+		kasan_meta_unlock(alloc_info);
+		local_irq_restore(flags);
 	}
 #endif
 }
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 7da78a6..df2724d 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -73,10 +73,19 @@ struct kasan_track {
 	depot_stack_handle_t stack;
 };
 
+union kasan_alloc_data {
+	struct {
+		u32 lock : 1;
+		u32 state : 2;		/* enum kasan_state */
+		u32 size_delta : 24;	/* object_size - alloc size */
+		u32 unused : 5;
+	};
+	u32 packed;
+};
+
 struct kasan_alloc_meta {
 	struct kasan_track track;
-	u32 state : 2;	/* enum kasan_state */
-	u32 alloc_size : 30;
+	u32 data;	/* encoded as union kasan_alloc_data */
 	u32 reserved;
 };
 
@@ -112,4 +121,6 @@ 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);
 #endif
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 40159a6..d59a569 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -144,10 +144,15 @@ static void qlink_free(void **qlink, struct kmem_cache *cache)
 {
 	void *object = qlink_to_object(qlink, cache);
 	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
+	union kasan_alloc_data alloc_data;
 	unsigned long flags;
 
 	local_irq_save(flags);
-	alloc_info->state = KASAN_STATE_FREE;
+	kasan_meta_lock(alloc_info);
+	alloc_data.packed = alloc_info->data;
+	alloc_data.state = KASAN_STATE_FREE;
+	alloc_info->data = alloc_data.packed;
+	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..cecf2fa 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) {
+		pr_err("BUG: KASAN: double-free attempt in %pS on object at addr %p\n",
+				(void *)info->ip, info->access_addr);
+		pr_err("Double free by task %s/%d\n",
+				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);
 
@@ -131,29 +142,34 @@ 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, char *unused_reason,
+		struct kasan_access_info *info)
 {
 	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
 	struct kasan_free_meta *free_info;
+	union kasan_alloc_data alloc_data;
 
 	dump_stack();
 	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);
+	alloc_data.packed = alloc_info->data;
+	switch (alloc_data.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);
+				(cache->object_size - alloc_data.size_delta));
 		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);
+				(cache->object_size - alloc_data.size_delta));
 		free_info = get_free_info(cache, object);
 		pr_err("Allocation:\n");
 		print_track(&alloc_info->track);
@@ -161,6 +177,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 +195,13 @@ 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,
+					"kasan: bad access detected", 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 3f20800..110d586 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2651,6 +2651,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);
-- 
1.7.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Yury Norov]

On Fri, May 06, 2016 at 05:17:27PM +0530, Kuthonuzo Luruo 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().
> 
> 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 'test_kasan' kasan_double_free() test in accompanying
>   patch.
> 
> Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
> ---
> 
> Changes in v2:
> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>   using fewer bits overall.
> - Double-free pr_err promoted to kasan_report().
> - kasan_init_object() introduced to initialize KASAN object metadata
>   during slab creation. KASAN_STATE_INIT initialization removed from
>   kasan_poison_object_data().
>  
> ---
>  include/linux/kasan.h |    8 +++
>  mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++-------------
>  mm/kasan/kasan.h      |   15 +++++-
>  mm/kasan/quarantine.c |    7 +++-
>  mm/kasan/report.c     |   31 +++++++++++--
>  mm/slab.c             |    1 +
>  6 files changed, 142 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 645c280..c7bf625 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -78,6 +78,10 @@ struct kasan_cache {
>  int kasan_module_alloc(void *addr, size_t size);
>  void kasan_free_shadow(const struct vm_struct *vm);
>  
> +#ifdef CONFIG_SLAB
> +void kasan_init_object(struct kmem_cache *cache, void *object);
> +#endif
> +
>  #else /* CONFIG_KASAN */
>  
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -124,6 +128,10 @@ static inline void kasan_poison_slab_free(struct kmem_cache *s, void *object) {}
>  static inline int kasan_module_alloc(void *addr, size_t size) { return 0; }
>  static inline void kasan_free_shadow(const struct vm_struct *vm) {}
>  
> +#ifdef CONFIG_SLAB
> +static inline void kasan_init_object(struct kmem_cache *cache, void *object) {}
> +#endif
> +
>  #endif /* CONFIG_KASAN */
>  
>  #endif /* LINUX_KASAN_H */
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..a840b49 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -34,6 +34,7 @@
>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/vmalloc.h>
> +#include <linux/atomic.h>
>  
>  #include "kasan.h"
>  #include "../slab.h"
> @@ -419,13 +420,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
> @@ -470,6 +464,18 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
>  	return depot_save_stack(&trace, flags);
>  }
>  
> +void kasan_init_object(struct kmem_cache *cache, void *object)
> +{
> +	struct kasan_alloc_meta *alloc_info;
> +
> +	if (cache->flags & SLAB_KASAN) {
> +		kasan_unpoison_object_data(cache, object);
> +		alloc_info = get_alloc_info(cache, object);
> +		__memset(alloc_info, 0, sizeof(*alloc_info));
> +		kasan_poison_object_data(cache, object);
> +	}
> +}
> +
>  static inline void set_track(struct kasan_track *track, gfp_t flags)
>  {
>  	track->pid = current->pid;
> @@ -489,6 +495,39 @@ 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;
>  }
> +
> +/* acquire per-object lock for access to KASAN metadata. */

I believe there's strong reason not to use standard spin_lock() or
similar. I think it's proper place to explain it.

> +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
> +{
> +	union kasan_alloc_data old, new;
> +
> +	preempt_disable();

It's better to disable and enable preemption inside the loop
on each iteration, to decrease contention.

> +	for (;;) {
> +		old.packed = READ_ONCE(alloc_info->data);
> +		if (unlikely(old.lock)) {
> +			cpu_relax();
> +			continue;
> +		}
> +		new.packed = old.packed;
> +		new.lock = 1;
> +		if (likely(cmpxchg(&alloc_info->data, old.packed, new.packed)
> +					== old.packed))
> +			break;
> +	}
> +}
> +
> +/* release lock after a kasan_meta_lock(). */
> +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
> +{
> +	union kasan_alloc_data alloc_data;
> +
> +	alloc_data.packed = READ_ONCE(alloc_info->data);
> +	alloc_data.lock = 0;
> +	if (unlikely(xchg(&alloc_info->data, alloc_data.packed) !=
> +				(alloc_data.packed | 0x1U)))
> +		WARN_ONCE(1, "%s: lock not held!\n", __func__);

Nitpick. It never happens in normal case, correct?. Why don't you place it under
some developer config, or even leave at dev branch? The function will
be twice shorter without it.

> +	preempt_enable();
> +}
>  #endif
>  
>  void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
> @@ -511,32 +550,41 @@ 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_alloc_meta *alloc_info;
> +	struct kasan_free_meta *free_info;
> +	union kasan_alloc_data alloc_data;
> +
>  	/* 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;
> +
> +	alloc_info = get_alloc_info(cache, object);
> +	kasan_meta_lock(alloc_info);
> +	alloc_data.packed = alloc_info->data;
> +	if (alloc_data.state == KASAN_STATE_ALLOC) {
> +		free_info = get_free_info(cache, object);
> +		quarantine_put(free_info, cache);

I just pulled master and didn't find this function. If your patchset
is based on other branch, please notice it.

> +		set_track(&free_info->track, GFP_NOWAIT);

It may fail for many reasons. Is it OK to ignore it? If OK, I think it
should be explained.

> +		kasan_poison_slab_free(cache, object);
> +		alloc_data.state = KASAN_STATE_QUARANTINE;
> +		alloc_info->data = alloc_data.packed;
> +		kasan_meta_unlock(alloc_info);
> +		return true;
>  	}
> +	switch (alloc_data.state) {
> +	case KASAN_STATE_QUARANTINE:
> +	case KASAN_STATE_FREE:
> +		kasan_report((unsigned long)object, 0, false,
> +				(unsigned long)__builtin_return_address(1));

__builtin_return_address() is unsafe if argument is non-zero. Use
return_address() instead.

> +		kasan_meta_unlock(alloc_info);
> +		return true;
> +	default:
> +		break;
> +	}
> +	kasan_meta_unlock(alloc_info);
>  	return false;
>  #else
>  	kasan_poison_slab_free(cache, object);
> @@ -568,12 +616,20 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>  		KASAN_KMALLOC_REDZONE);
>  #ifdef CONFIG_SLAB
>  	if (cache->flags & SLAB_KASAN) {
> +		union kasan_alloc_data alloc_data;
>  		struct kasan_alloc_meta *alloc_info =
>  			get_alloc_info(cache, object);
> -
> -		alloc_info->state = KASAN_STATE_ALLOC;
> -		alloc_info->alloc_size = size;
> +		unsigned long flags;
> +
> +		local_irq_save(flags);
> +		kasan_meta_lock(alloc_info);
> +		alloc_data.packed = alloc_info->data;
> +		alloc_data.state = KASAN_STATE_ALLOC;
> +		alloc_data.size_delta = cache->object_size - size;
> +		alloc_info->data = alloc_data.packed;
>  		set_track(&alloc_info->track, flags);

Same as above

> +		kasan_meta_unlock(alloc_info);
> +		local_irq_restore(flags);
>  	}
>  #endif
>  }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..df2724d 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -73,10 +73,19 @@ struct kasan_track {
>  	depot_stack_handle_t stack;
>  };
>  
> +union kasan_alloc_data {
> +	struct {
> +		u32 lock : 1;
> +		u32 state : 2;		/* enum kasan_state */
> +		u32 size_delta : 24;	/* object_size - alloc size */
> +		u32 unused : 5;
> +	};
> +	u32 packed;
> +};
> +
>  struct kasan_alloc_meta {
>  	struct kasan_track track;
> -	u32 state : 2;	/* enum kasan_state */
> -	u32 alloc_size : 30;
> +	u32 data;	/* encoded as union kasan_alloc_data */
>  	u32 reserved;
>  };
>  
> @@ -112,4 +121,6 @@ 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);
>  #endif
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..d59a569 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -144,10 +144,15 @@ static void qlink_free(void **qlink, struct kmem_cache *cache)
>  {
>  	void *object = qlink_to_object(qlink, cache);
>  	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
> +	union kasan_alloc_data alloc_data;
>  	unsigned long flags;
>  
>  	local_irq_save(flags);
> -	alloc_info->state = KASAN_STATE_FREE;
> +	kasan_meta_lock(alloc_info);
> +	alloc_data.packed = alloc_info->data;
> +	alloc_data.state = KASAN_STATE_FREE;
> +	alloc_info->data = alloc_data.packed;
> +	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..cecf2fa 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) {
> +		pr_err("BUG: KASAN: double-free attempt in %pS on object at addr %p\n",
> +				(void *)info->ip, info->access_addr);
> +		pr_err("Double free by task %s/%d\n",
> +				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);
>  
> @@ -131,29 +142,34 @@ 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, char *unused_reason,
> +		struct kasan_access_info *info)
>  {
>  	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
>  	struct kasan_free_meta *free_info;
> +	union kasan_alloc_data alloc_data;
>  
>  	dump_stack();
>  	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);
> +	alloc_data.packed = alloc_info->data;
> +	switch (alloc_data.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);
> +				(cache->object_size - alloc_data.size_delta));
>  		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);
> +				(cache->object_size - alloc_data.size_delta));
>  		free_info = get_free_info(cache, object);
>  		pr_err("Allocation:\n");
>  		print_track(&alloc_info->track);
> @@ -161,6 +177,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 +195,13 @@ 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,
> +					"kasan: bad access detected", 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 3f20800..110d586 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2651,6 +2651,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);
> -- 
> 1.7.1

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Yury Norov]

On Fri, May 06, 2016 at 05:17:27PM +0530, Kuthonuzo Luruo 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().
> 
> 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 'test_kasan' kasan_double_free() test in accompanying
>   patch.
> 
> Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
> ---
> 
> Changes in v2:
> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>   using fewer bits overall.
> - Double-free pr_err promoted to kasan_report().
> - kasan_init_object() introduced to initialize KASAN object metadata
>   during slab creation. KASAN_STATE_INIT initialization removed from
>   kasan_poison_object_data().
>  
> ---
>  include/linux/kasan.h |    8 +++
>  mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++-------------
>  mm/kasan/kasan.h      |   15 +++++-
>  mm/kasan/quarantine.c |    7 +++-
>  mm/kasan/report.c     |   31 +++++++++++--
>  mm/slab.c             |    1 +
>  6 files changed, 142 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 645c280..c7bf625 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -78,6 +78,10 @@ struct kasan_cache {
>  int kasan_module_alloc(void *addr, size_t size);
>  void kasan_free_shadow(const struct vm_struct *vm);
>  
> +#ifdef CONFIG_SLAB
> +void kasan_init_object(struct kmem_cache *cache, void *object);
> +#endif
> +
>  #else /* CONFIG_KASAN */
>  
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -124,6 +128,10 @@ static inline void kasan_poison_slab_free(struct kmem_cache *s, void *object) {}
>  static inline int kasan_module_alloc(void *addr, size_t size) { return 0; }
>  static inline void kasan_free_shadow(const struct vm_struct *vm) {}
>  
> +#ifdef CONFIG_SLAB
> +static inline void kasan_init_object(struct kmem_cache *cache, void *object) {}
> +#endif
> +
>  #endif /* CONFIG_KASAN */
>  
>  #endif /* LINUX_KASAN_H */
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..a840b49 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -34,6 +34,7 @@
>  #include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/vmalloc.h>
> +#include <linux/atomic.h>
>  
>  #include "kasan.h"
>  #include "../slab.h"
> @@ -419,13 +420,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
> @@ -470,6 +464,18 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
>  	return depot_save_stack(&trace, flags);
>  }
>  
> +void kasan_init_object(struct kmem_cache *cache, void *object)
> +{
> +	struct kasan_alloc_meta *alloc_info;
> +
> +	if (cache->flags & SLAB_KASAN) {
> +		kasan_unpoison_object_data(cache, object);
> +		alloc_info = get_alloc_info(cache, object);
> +		__memset(alloc_info, 0, sizeof(*alloc_info));
> +		kasan_poison_object_data(cache, object);
> +	}
> +}
> +
>  static inline void set_track(struct kasan_track *track, gfp_t flags)
>  {
>  	track->pid = current->pid;
> @@ -489,6 +495,39 @@ 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;
>  }
> +
> +/* acquire per-object lock for access to KASAN metadata. */

I believe there's strong reason not to use standard spin_lock() or
similar. I think it's proper place to explain it.

> +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
> +{
> +	union kasan_alloc_data old, new;
> +
> +	preempt_disable();

It's better to disable and enable preemption inside the loop
on each iteration, to decrease contention.

> +	for (;;) {
> +		old.packed = READ_ONCE(alloc_info->data);
> +		if (unlikely(old.lock)) {
> +			cpu_relax();
> +			continue;
> +		}
> +		new.packed = old.packed;
> +		new.lock = 1;
> +		if (likely(cmpxchg(&alloc_info->data, old.packed, new.packed)
> +					== old.packed))
> +			break;
> +	}
> +}
> +
> +/* release lock after a kasan_meta_lock(). */
> +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
> +{
> +	union kasan_alloc_data alloc_data;
> +
> +	alloc_data.packed = READ_ONCE(alloc_info->data);
> +	alloc_data.lock = 0;
> +	if (unlikely(xchg(&alloc_info->data, alloc_data.packed) !=
> +				(alloc_data.packed | 0x1U)))
> +		WARN_ONCE(1, "%s: lock not held!\n", __func__);

Nitpick. It never happens in normal case, correct?. Why don't you place it under
some developer config, or even leave at dev branch? The function will
be twice shorter without it.

> +	preempt_enable();
> +}
>  #endif
>  
>  void kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
> @@ -511,32 +550,41 @@ 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_alloc_meta *alloc_info;
> +	struct kasan_free_meta *free_info;
> +	union kasan_alloc_data alloc_data;
> +
>  	/* 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;
> +
> +	alloc_info = get_alloc_info(cache, object);
> +	kasan_meta_lock(alloc_info);
> +	alloc_data.packed = alloc_info->data;
> +	if (alloc_data.state == KASAN_STATE_ALLOC) {
> +		free_info = get_free_info(cache, object);
> +		quarantine_put(free_info, cache);

I just pulled master and didn't find this function. If your patchset
is based on other branch, please notice it.

> +		set_track(&free_info->track, GFP_NOWAIT);

It may fail for many reasons. Is it OK to ignore it? If OK, I think it
should be explained.

> +		kasan_poison_slab_free(cache, object);
> +		alloc_data.state = KASAN_STATE_QUARANTINE;
> +		alloc_info->data = alloc_data.packed;
> +		kasan_meta_unlock(alloc_info);
> +		return true;
>  	}
> +	switch (alloc_data.state) {
> +	case KASAN_STATE_QUARANTINE:
> +	case KASAN_STATE_FREE:
> +		kasan_report((unsigned long)object, 0, false,
> +				(unsigned long)__builtin_return_address(1));

__builtin_return_address() is unsafe if argument is non-zero. Use
return_address() instead.

> +		kasan_meta_unlock(alloc_info);
> +		return true;
> +	default:
> +		break;
> +	}
> +	kasan_meta_unlock(alloc_info);
>  	return false;
>  #else
>  	kasan_poison_slab_free(cache, object);
> @@ -568,12 +616,20 @@ void kasan_kmalloc(struct kmem_cache *cache, const void *object, size_t size,
>  		KASAN_KMALLOC_REDZONE);
>  #ifdef CONFIG_SLAB
>  	if (cache->flags & SLAB_KASAN) {
> +		union kasan_alloc_data alloc_data;
>  		struct kasan_alloc_meta *alloc_info =
>  			get_alloc_info(cache, object);
> -
> -		alloc_info->state = KASAN_STATE_ALLOC;
> -		alloc_info->alloc_size = size;
> +		unsigned long flags;
> +
> +		local_irq_save(flags);
> +		kasan_meta_lock(alloc_info);
> +		alloc_data.packed = alloc_info->data;
> +		alloc_data.state = KASAN_STATE_ALLOC;
> +		alloc_data.size_delta = cache->object_size - size;
> +		alloc_info->data = alloc_data.packed;
>  		set_track(&alloc_info->track, flags);

Same as above

> +		kasan_meta_unlock(alloc_info);
> +		local_irq_restore(flags);
>  	}
>  #endif
>  }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..df2724d 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -73,10 +73,19 @@ struct kasan_track {
>  	depot_stack_handle_t stack;
>  };
>  
> +union kasan_alloc_data {
> +	struct {
> +		u32 lock : 1;
> +		u32 state : 2;		/* enum kasan_state */
> +		u32 size_delta : 24;	/* object_size - alloc size */
> +		u32 unused : 5;
> +	};
> +	u32 packed;
> +};
> +
>  struct kasan_alloc_meta {
>  	struct kasan_track track;
> -	u32 state : 2;	/* enum kasan_state */
> -	u32 alloc_size : 30;
> +	u32 data;	/* encoded as union kasan_alloc_data */
>  	u32 reserved;
>  };
>  
> @@ -112,4 +121,6 @@ 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);
>  #endif
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..d59a569 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -144,10 +144,15 @@ static void qlink_free(void **qlink, struct kmem_cache *cache)
>  {
>  	void *object = qlink_to_object(qlink, cache);
>  	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
> +	union kasan_alloc_data alloc_data;
>  	unsigned long flags;
>  
>  	local_irq_save(flags);
> -	alloc_info->state = KASAN_STATE_FREE;
> +	kasan_meta_lock(alloc_info);
> +	alloc_data.packed = alloc_info->data;
> +	alloc_data.state = KASAN_STATE_FREE;
> +	alloc_info->data = alloc_data.packed;
> +	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..cecf2fa 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) {
> +		pr_err("BUG: KASAN: double-free attempt in %pS on object at addr %p\n",
> +				(void *)info->ip, info->access_addr);
> +		pr_err("Double free by task %s/%d\n",
> +				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);
>  
> @@ -131,29 +142,34 @@ 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, char *unused_reason,
> +		struct kasan_access_info *info)
>  {
>  	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
>  	struct kasan_free_meta *free_info;
> +	union kasan_alloc_data alloc_data;
>  
>  	dump_stack();
>  	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);
> +	alloc_data.packed = alloc_info->data;
> +	switch (alloc_data.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);
> +				(cache->object_size - alloc_data.size_delta));
>  		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);
> +				(cache->object_size - alloc_data.size_delta));
>  		free_info = get_free_info(cache, object);
>  		pr_err("Allocation:\n");
>  		print_track(&alloc_info->track);
> @@ -161,6 +177,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 +195,13 @@ 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,
> +					"kasan: bad access detected", 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 3f20800..110d586 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2651,6 +2651,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);
> -- 
> 1.7.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

Thank you for the review!

> > +
> > +/* acquire per-object lock for access to KASAN metadata. */
> 
> I believe there's strong reason not to use standard spin_lock() or
> similar. I think it's proper place to explain it.
> 

will do.

> > +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
> > +{
> > +	union kasan_alloc_data old, new;
> > +
> > +	preempt_disable();
> 
> It's better to disable and enable preemption inside the loop
> on each iteration, to decrease contention.
> 

ok, makes sense; will do.

> > +	for (;;) {
> > +		old.packed = READ_ONCE(alloc_info->data);
> > +		if (unlikely(old.lock)) {
> > +			cpu_relax();
> > +			continue;
> > +		}
> > +		new.packed = old.packed;
> > +		new.lock = 1;
> > +		if (likely(cmpxchg(&alloc_info->data, old.packed, new.packed)
> > +					== old.packed))
> > +			break;
> > +	}
> > +}
> > +
> > +/* release lock after a kasan_meta_lock(). */
> > +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
> > +{
> > +	union kasan_alloc_data alloc_data;
> > +
> > +	alloc_data.packed = READ_ONCE(alloc_info->data);
> > +	alloc_data.lock = 0;
> > +	if (unlikely(xchg(&alloc_info->data, alloc_data.packed) !=
> > +				(alloc_data.packed | 0x1U)))
> > +		WARN_ONCE(1, "%s: lock not held!\n", __func__);
> 
> Nitpick. It never happens in normal case, correct?. Why don't you place it under
> some developer config, or even leave at dev branch? The function will
> be twice shorter without it.

ok, will remove/shorten.

> > +	alloc_data.packed = alloc_info->data;
> > +	if (alloc_data.state == KASAN_STATE_ALLOC) {
> > +		free_info = get_free_info(cache, object);
> > +		quarantine_put(free_info, cache);
> 
> I just pulled master and didn't find this function. If your patchset
> is based on other branch, please notice it.

Sorry; patchset is based on linux-next 'next-20160506' which has Alexander
Potapenko's patches for KASAN SLAB support with memory quarantine +
stackdepot features.

> 
> > +		set_track(&free_info->track, GFP_NOWAIT);
> 
> It may fail for many reasons. Is it OK to ignore it? If OK, I think it
> should be explained.

It's ok. A subsequent bug report on object would have a missing alloc/dealloc
stack trace. 

> 
> > +		kasan_poison_slab_free(cache, object);
> > +		alloc_data.state = KASAN_STATE_QUARANTINE;
> > +		alloc_info->data = alloc_data.packed;
> > +		kasan_meta_unlock(alloc_info);
> > +		return true;
> >  	}
> > +	switch (alloc_data.state) {
> > +	case KASAN_STATE_QUARANTINE:
> > +	case KASAN_STATE_FREE:
> > +		kasan_report((unsigned long)object, 0, false,
> > +				(unsigned long)__builtin_return_address(1));
> 
> __builtin_return_address() is unsafe if argument is non-zero. Use
> return_address() instead.

hmm, I/cscope can't seem to find an x86 implementation for return_address().
Will dig further; thanks.

> > +		local_irq_save(flags);
> > +		kasan_meta_lock(alloc_info);
> > +		alloc_data.packed = alloc_info->data;
> > +		alloc_data.state = KASAN_STATE_ALLOC;
> > +		alloc_data.size_delta = cache->object_size - size;
> > +		alloc_info->data = alloc_data.packed;
> >  		set_track(&alloc_info->track, flags);
> 
> Same as above
>
As above. 

Kuthonuzo

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

Thank you for the review!

> > +
> > +/* acquire per-object lock for access to KASAN metadata. */
> 
> I believe there's strong reason not to use standard spin_lock() or
> similar. I think it's proper place to explain it.
> 

will do.

> > +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
> > +{
> > +	union kasan_alloc_data old, new;
> > +
> > +	preempt_disable();
> 
> It's better to disable and enable preemption inside the loop
> on each iteration, to decrease contention.
> 

ok, makes sense; will do.

> > +	for (;;) {
> > +		old.packed = READ_ONCE(alloc_info->data);
> > +		if (unlikely(old.lock)) {
> > +			cpu_relax();
> > +			continue;
> > +		}
> > +		new.packed = old.packed;
> > +		new.lock = 1;
> > +		if (likely(cmpxchg(&alloc_info->data, old.packed, new.packed)
> > +					== old.packed))
> > +			break;
> > +	}
> > +}
> > +
> > +/* release lock after a kasan_meta_lock(). */
> > +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
> > +{
> > +	union kasan_alloc_data alloc_data;
> > +
> > +	alloc_data.packed = READ_ONCE(alloc_info->data);
> > +	alloc_data.lock = 0;
> > +	if (unlikely(xchg(&alloc_info->data, alloc_data.packed) !=
> > +				(alloc_data.packed | 0x1U)))
> > +		WARN_ONCE(1, "%s: lock not held!\n", __func__);
> 
> Nitpick. It never happens in normal case, correct?. Why don't you place it under
> some developer config, or even leave at dev branch? The function will
> be twice shorter without it.

ok, will remove/shorten.

> > +	alloc_data.packed = alloc_info->data;
> > +	if (alloc_data.state == KASAN_STATE_ALLOC) {
> > +		free_info = get_free_info(cache, object);
> > +		quarantine_put(free_info, cache);
> 
> I just pulled master and didn't find this function. If your patchset
> is based on other branch, please notice it.

Sorry; patchset is based on linux-next 'next-20160506' which has Alexander
Potapenko's patches for KASAN SLAB support with memory quarantine +
stackdepot features.

> 
> > +		set_track(&free_info->track, GFP_NOWAIT);
> 
> It may fail for many reasons. Is it OK to ignore it? If OK, I think it
> should be explained.

It's ok. A subsequent bug report on object would have a missing alloc/dealloc
stack trace. 

> 
> > +		kasan_poison_slab_free(cache, object);
> > +		alloc_data.state = KASAN_STATE_QUARANTINE;
> > +		alloc_info->data = alloc_data.packed;
> > +		kasan_meta_unlock(alloc_info);
> > +		return true;
> >  	}
> > +	switch (alloc_data.state) {
> > +	case KASAN_STATE_QUARANTINE:
> > +	case KASAN_STATE_FREE:
> > +		kasan_report((unsigned long)object, 0, false,
> > +				(unsigned long)__builtin_return_address(1));
> 
> __builtin_return_address() is unsafe if argument is non-zero. Use
> return_address() instead.

hmm, I/cscope can't seem to find an x86 implementation for return_address().
Will dig further; thanks.

> > +		local_irq_save(flags);
> > +		kasan_meta_lock(alloc_info);
> > +		alloc_data.packed = alloc_info->data;
> > +		alloc_data.state = KASAN_STATE_ALLOC;
> > +		alloc_data.size_delta = cache->object_size - size;
> > +		alloc_info->data = alloc_data.packed;
> >  		set_track(&alloc_info->track, flags);
> 
> Same as above
>
As above. 

Kuthonuzo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Yury Norov]

On Sat, May 07, 2016 at 03:15:59PM +0000, Luruo, Kuthonuzo wrote:
> Thank you for the review!
> 
> > > +	switch (alloc_data.state) {
> > > +	case KASAN_STATE_QUARANTINE:
> > > +	case KASAN_STATE_FREE:
> > > +		kasan_report((unsigned long)object, 0, false,
> > > +				(unsigned long)__builtin_return_address(1));
> > 
> > __builtin_return_address() is unsafe if argument is non-zero. Use
> > return_address() instead.
> 
> hmm, I/cscope can't seem to find an x86 implementation for return_address().
> Will dig further; thanks.
> 

It seems there's no generic interface to obtain return address. x86
has  working __builtin_return_address() and it's ok with it, others
use their own return_adderss(), and ok as well.

I think unification is needed here.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Yury Norov]

On Sat, May 07, 2016 at 03:15:59PM +0000, Luruo, Kuthonuzo wrote:
> Thank you for the review!
> 
> > > +	switch (alloc_data.state) {
> > > +	case KASAN_STATE_QUARANTINE:
> > > +	case KASAN_STATE_FREE:
> > > +		kasan_report((unsigned long)object, 0, false,
> > > +				(unsigned long)__builtin_return_address(1));
> > 
> > __builtin_return_address() is unsafe if argument is non-zero. Use
> > return_address() instead.
> 
> hmm, I/cscope can't seem to find an x86 implementation for return_address().
> Will dig further; thanks.
> 

It seems there's no generic interface to obtain return address. x86
has  working __builtin_return_address() and it's ok with it, others
use their own return_adderss(), and ok as well.

I think unification is needed here.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Sun, May 8, 2016 at 11:17 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> On Sat, May 07, 2016 at 03:15:59PM +0000, Luruo, Kuthonuzo wrote:
>> Thank you for the review!
>>
>> > > + switch (alloc_data.state) {
>> > > + case KASAN_STATE_QUARANTINE:
>> > > + case KASAN_STATE_FREE:
>> > > +         kasan_report((unsigned long)object, 0, false,
>> > > +                         (unsigned long)__builtin_return_address(1));
>> >
>> > __builtin_return_address() is unsafe if argument is non-zero. Use
>> > return_address() instead.
>>
>> hmm, I/cscope can't seem to find an x86 implementation for return_address().
>> Will dig further; thanks.
>>
>
> It seems there's no generic interface to obtain return address. x86
> has  working __builtin_return_address() and it's ok with it, others
> use their own return_adderss(), and ok as well.
>
> I think unification is needed here.


We use _RET_IP_ in other places in portable part of kasan.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Sun, May 8, 2016 at 11:17 AM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> On Sat, May 07, 2016 at 03:15:59PM +0000, Luruo, Kuthonuzo wrote:
>> Thank you for the review!
>>
>> > > + switch (alloc_data.state) {
>> > > + case KASAN_STATE_QUARANTINE:
>> > > + case KASAN_STATE_FREE:
>> > > +         kasan_report((unsigned long)object, 0, false,
>> > > +                         (unsigned long)__builtin_return_address(1));
>> >
>> > __builtin_return_address() is unsafe if argument is non-zero. Use
>> > return_address() instead.
>>
>> hmm, I/cscope can't seem to find an x86 implementation for return_address().
>> Will dig further; thanks.
>>
>
> It seems there's no generic interface to obtain return address. x86
> has  working __builtin_return_address() and it's ok with it, others
> use their own return_adderss(), and ok as well.
>
> I think unification is needed here.


We use _RET_IP_ in other places in portable part of kasan.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

> >> Thank you for the review!
> >>
> >> > > + switch (alloc_data.state) {
> >> > > + case KASAN_STATE_QUARANTINE:
> >> > > + case KASAN_STATE_FREE:
> >> > > +         kasan_report((unsigned long)object, 0, false,
> >> > > +                         (unsigned long)__builtin_return_address(1));
> >> >
> >> > __builtin_return_address() is unsafe if argument is non-zero. Use
> >> > return_address() instead.
> >>
> >> hmm, I/cscope can't seem to find an x86 implementation for
> return_address().
> >> Will dig further; thanks.
> >>
> >
> > It seems there's no generic interface to obtain return address. x86
> > has  working __builtin_return_address() and it's ok with it, others
> > use their own return_adderss(), and ok as well.
> >
> > I think unification is needed here.
> 
> 
> We use _RET_IP_ in other places in portable part of kasan.

Yeah, _RET_IP_ is the way to go here.

Not directly related but: while looking into kasan_slab_free() callers, it seems
to me that, with SLAB + quarantine, kasan_poison_kfree() should _not_ be
calling into kasan_slab_free(). The intent in the call-chain thru
kasan_poison_kree() seems to be only to poison object shadow, not actually
free it.

Alexander, can you please comment/confirm? Thanks.

Kuthonuzo

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Sat, May 7, 2016 at 5:15 PM, Luruo, Kuthonuzo
<kuthonuzo.luruo@hpe.com> wrote:
> Thank you for the review!
>
>> > +
>> > +/* acquire per-object lock for access to KASAN metadata. */
>>
>> I believe there's strong reason not to use standard spin_lock() or
>> similar. I think it's proper place to explain it.
>>
>
> will do.
>
>> > +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
>> > +{
>> > +   union kasan_alloc_data old, new;
>> > +
>> > +   preempt_disable();
>>
>> It's better to disable and enable preemption inside the loop
>> on each iteration, to decrease contention.
>>
>
> ok, makes sense; will do.
>
>> > +   for (;;) {
>> > +           old.packed = READ_ONCE(alloc_info->data);
>> > +           if (unlikely(old.lock)) {
>> > +                   cpu_relax();
>> > +                   continue;
>> > +           }
>> > +           new.packed = old.packed;
>> > +           new.lock = 1;
>> > +           if (likely(cmpxchg(&alloc_info->data, old.packed, new.packed)
>> > +                                   == old.packed))
>> > +                   break;
>> > +   }
>> > +}
>> > +
>> > +/* release lock after a kasan_meta_lock(). */
>> > +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
>> > +{
>> > +   union kasan_alloc_data alloc_data;
>> > +
>> > +   alloc_data.packed = READ_ONCE(alloc_info->data);
>> > +   alloc_data.lock = 0;
>> > +   if (unlikely(xchg(&alloc_info->data, alloc_data.packed) !=
>> > +                           (alloc_data.packed | 0x1U)))
>> > +           WARN_ONCE(1, "%s: lock not held!\n", __func__);
>>
>> Nitpick. It never happens in normal case, correct?. Why don't you place it under
>> some developer config, or even leave at dev branch? The function will
>> be twice shorter without it.
>
> ok, will remove/shorten

My concern here is performance.
We do lock/unlock 3 times per allocated object. Currently that's 6
atomic RMW. The unlock one is not necessary, so that would reduce
number of atomic RMWs to 3.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Sat, May 7, 2016 at 5:15 PM, Luruo, Kuthonuzo
<kuthonuzo.luruo@hpe.com> wrote:
> Thank you for the review!
>
>> > +
>> > +/* acquire per-object lock for access to KASAN metadata. */
>>
>> I believe there's strong reason not to use standard spin_lock() or
>> similar. I think it's proper place to explain it.
>>
>
> will do.
>
>> > +void kasan_meta_lock(struct kasan_alloc_meta *alloc_info)
>> > +{
>> > +   union kasan_alloc_data old, new;
>> > +
>> > +   preempt_disable();
>>
>> It's better to disable and enable preemption inside the loop
>> on each iteration, to decrease contention.
>>
>
> ok, makes sense; will do.
>
>> > +   for (;;) {
>> > +           old.packed = READ_ONCE(alloc_info->data);
>> > +           if (unlikely(old.lock)) {
>> > +                   cpu_relax();
>> > +                   continue;
>> > +           }
>> > +           new.packed = old.packed;
>> > +           new.lock = 1;
>> > +           if (likely(cmpxchg(&alloc_info->data, old.packed, new.packed)
>> > +                                   == old.packed))
>> > +                   break;
>> > +   }
>> > +}
>> > +
>> > +/* release lock after a kasan_meta_lock(). */
>> > +void kasan_meta_unlock(struct kasan_alloc_meta *alloc_info)
>> > +{
>> > +   union kasan_alloc_data alloc_data;
>> > +
>> > +   alloc_data.packed = READ_ONCE(alloc_info->data);
>> > +   alloc_data.lock = 0;
>> > +   if (unlikely(xchg(&alloc_info->data, alloc_data.packed) !=
>> > +                           (alloc_data.packed | 0x1U)))
>> > +           WARN_ONCE(1, "%s: lock not held!\n", __func__);
>>
>> Nitpick. It never happens in normal case, correct?. Why don't you place it under
>> some developer config, or even leave at dev branch? The function will
>> be twice shorter without it.
>
> ok, will remove/shorten

My concern here is performance.
We do lock/unlock 3 times per allocated object. Currently that's 6
atomic RMW. The unlock one is not necessary, so that would reduce
number of atomic RMWs to 3.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/06/2016 02:47 PM, Kuthonuzo Luruo 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().
> 
> 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 'test_kasan' kasan_double_free() test in accompanying
>   patch.
> 
> Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
> ---
> 
> Changes in v2:
> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>   using fewer bits overall.
> - Double-free pr_err promoted to kasan_report().
> - kasan_init_object() introduced to initialize KASAN object metadata
>   during slab creation. KASAN_STATE_INIT initialization removed from
>   kasan_poison_object_data().
>  
> ---
>  include/linux/kasan.h |    8 +++
>  mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++-------------
>  mm/kasan/kasan.h      |   15 +++++-
>  mm/kasan/quarantine.c |    7 +++-
>  mm/kasan/report.c     |   31 +++++++++++--
>  mm/slab.c             |    1 +
>  6 files changed, 142 insertions(+), 38 deletions(-)
> 

Sorry, but this patch is crap.

Something like this, will fix the race:

---
 mm/kasan/kasan.c      | 20 ++++----------------
 mm/kasan/kasan.h      | 10 +++-------
 mm/kasan/quarantine.c |  1 -
 mm/kasan/report.c     | 11 ++---------
 4 files changed, 9 insertions(+), 33 deletions(-)

diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index ef2e87b..8d078dc 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -419,13 +419,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
@@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache, void *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;
+		if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
+					&alloc_info->state)) {
 			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:
+		} else {
 			pr_err("Double free");
 			dump_stack();
-			break;
-		default:
-			break;
 		}
 	}
 	return false;
@@ -571,7 +559,7 @@ 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;
+		set_bit(KASAN_STATE_ALLOCATED, &alloc_info->state);
 		alloc_info->alloc_size = size;
 		set_track(&alloc_info->track, flags);
 	}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 7da78a6..2dcdc8f 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -60,10 +60,7 @@ struct kasan_global {
  */
 
 enum kasan_state {
-	KASAN_STATE_INIT,
-	KASAN_STATE_ALLOC,
-	KASAN_STATE_QUARANTINE,
-	KASAN_STATE_FREE
+	KASAN_STATE_ALLOCATED,
 };
 
 #define KASAN_STACK_DEPTH 64
@@ -75,9 +72,8 @@ struct kasan_track {
 
 struct kasan_alloc_meta {
 	struct kasan_track track;
-	u32 state : 2;	/* enum kasan_state */
-	u32 alloc_size : 30;
-	u32 reserved;
+	unsigned long state;
+	u32 alloc_size;
 };
 
 struct kasan_free_meta {
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 40159a6..ca33fd3 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache *cache)
 	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..c2b0e51 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -140,18 +140,12 @@ 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:
+	if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
 		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:
+	} else {
 		pr_err("Object freed, allocated with size %u bytes\n",
 		       alloc_info->alloc_size);
 		free_info = get_free_info(cache, object);
@@ -159,7 +153,6 @@ static void object_err(struct kmem_cache *cache, struct page *page,
 		print_track(&alloc_info->track);
 		pr_err("Deallocation:\n");
 		print_track(&free_info->track);
-		break;
 	}
 }
 #endif
-- 
2.7.3

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/06/2016 02:47 PM, Kuthonuzo Luruo 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().
> 
> 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 'test_kasan' kasan_double_free() test in accompanying
>   patch.
> 
> Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
> ---
> 
> Changes in v2:
> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>   using fewer bits overall.
> - Double-free pr_err promoted to kasan_report().
> - kasan_init_object() introduced to initialize KASAN object metadata
>   during slab creation. KASAN_STATE_INIT initialization removed from
>   kasan_poison_object_data().
>  
> ---
>  include/linux/kasan.h |    8 +++
>  mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++-------------
>  mm/kasan/kasan.h      |   15 +++++-
>  mm/kasan/quarantine.c |    7 +++-
>  mm/kasan/report.c     |   31 +++++++++++--
>  mm/slab.c             |    1 +
>  6 files changed, 142 insertions(+), 38 deletions(-)
> 

Sorry, but this patch is crap.

Something like this, will fix the race:

---
 mm/kasan/kasan.c      | 20 ++++----------------
 mm/kasan/kasan.h      | 10 +++-------
 mm/kasan/quarantine.c |  1 -
 mm/kasan/report.c     | 11 ++---------
 4 files changed, 9 insertions(+), 33 deletions(-)

diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index ef2e87b..8d078dc 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -419,13 +419,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
@@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache, void *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;
+		if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
+					&alloc_info->state)) {
 			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:
+		} else {
 			pr_err("Double free");
 			dump_stack();
-			break;
-		default:
-			break;
 		}
 	}
 	return false;
@@ -571,7 +559,7 @@ 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;
+		set_bit(KASAN_STATE_ALLOCATED, &alloc_info->state);
 		alloc_info->alloc_size = size;
 		set_track(&alloc_info->track, flags);
 	}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 7da78a6..2dcdc8f 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -60,10 +60,7 @@ struct kasan_global {
  */
 
 enum kasan_state {
-	KASAN_STATE_INIT,
-	KASAN_STATE_ALLOC,
-	KASAN_STATE_QUARANTINE,
-	KASAN_STATE_FREE
+	KASAN_STATE_ALLOCATED,
 };
 
 #define KASAN_STACK_DEPTH 64
@@ -75,9 +72,8 @@ struct kasan_track {
 
 struct kasan_alloc_meta {
 	struct kasan_track track;
-	u32 state : 2;	/* enum kasan_state */
-	u32 alloc_size : 30;
-	u32 reserved;
+	unsigned long state;
+	u32 alloc_size;
 };
 
 struct kasan_free_meta {
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 40159a6..ca33fd3 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache *cache)
 	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..c2b0e51 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -140,18 +140,12 @@ 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:
+	if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
 		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:
+	} else {
 		pr_err("Object freed, allocated with size %u bytes\n",
 		       alloc_info->alloc_size);
 		free_info = get_free_info(cache, object);
@@ -159,7 +153,6 @@ static void object_err(struct kmem_cache *cache, struct page *page,
 		print_track(&alloc_info->track);
 		pr_err("Deallocation:\n");
 		print_track(&free_info->track);
-		break;
 	}
 }
 #endif
-- 
2.7.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 05/06/2016 02:47 PM, Kuthonuzo Luruo 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().
>>
>> 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 'test_kasan' kasan_double_free() test in accompanying
>>   patch.
>>
>> Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
>> ---
>>
>> Changes in v2:
>> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>>   using fewer bits overall.
>> - Double-free pr_err promoted to kasan_report().
>> - kasan_init_object() introduced to initialize KASAN object metadata
>>   during slab creation. KASAN_STATE_INIT initialization removed from
>>   kasan_poison_object_data().
>>
>> ---
>>  include/linux/kasan.h |    8 +++
>>  mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++-------------
>>  mm/kasan/kasan.h      |   15 +++++-
>>  mm/kasan/quarantine.c |    7 +++-
>>  mm/kasan/report.c     |   31 +++++++++++--
>>  mm/slab.c             |    1 +
>>  6 files changed, 142 insertions(+), 38 deletions(-)
>>
>
> Sorry, but this patch is crap.
>
> Something like this, will fix the race:
>
> ---
>  mm/kasan/kasan.c      | 20 ++++----------------
>  mm/kasan/kasan.h      | 10 +++-------
>  mm/kasan/quarantine.c |  1 -
>  mm/kasan/report.c     | 11 ++---------
>  4 files changed, 9 insertions(+), 33 deletions(-)
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..8d078dc 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -419,13 +419,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
> @@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache, void *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;
> +               if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
> +                                       &alloc_info->state)) {
>                         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:
> +               } else {
>                         pr_err("Double free");
>                         dump_stack();
> -                       break;
> -               default:
> -                       break;
>                 }
>         }
>         return false;
> @@ -571,7 +559,7 @@ 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;
> +               set_bit(KASAN_STATE_ALLOCATED, &alloc_info->state);
>                 alloc_info->alloc_size = size;
>                 set_track(&alloc_info->track, flags);
>         }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..2dcdc8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -60,10 +60,7 @@ struct kasan_global {
>   */
>
>  enum kasan_state {
> -       KASAN_STATE_INIT,
> -       KASAN_STATE_ALLOC,
> -       KASAN_STATE_QUARANTINE,
> -       KASAN_STATE_FREE
> +       KASAN_STATE_ALLOCATED,
>  };
>
>  #define KASAN_STACK_DEPTH 64
> @@ -75,9 +72,8 @@ struct kasan_track {
>
>  struct kasan_alloc_meta {
>         struct kasan_track track;
> -       u32 state : 2;  /* enum kasan_state */
> -       u32 alloc_size : 30;
> -       u32 reserved;
> +       unsigned long state;
> +       u32 alloc_size;
>  };
>
>  struct kasan_free_meta {
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..ca33fd3 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache *cache)
>         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..c2b0e51 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,18 +140,12 @@ 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:
> +       if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
>                 pr_err("Object allocated with size %u bytes.\n",
>                        alloc_info->alloc_size);
>                 pr_err("Allocation:\n");
>                 print_track(&alloc_info->track);

alloc_info->track is not necessary initialized when
KASAN_STATE_ALLOCATED is set. Worse, it can be initialized to a wrong
stack.


> -               break;
> -       case KASAN_STATE_FREE:
> -       case KASAN_STATE_QUARANTINE:
> +       } else {
>                 pr_err("Object freed, allocated with size %u bytes\n",
>                        alloc_info->alloc_size);
>                 free_info = get_free_info(cache, object);
> @@ -159,7 +153,6 @@ static void object_err(struct kmem_cache *cache, struct page *page,
>                 print_track(&alloc_info->track);
>                 pr_err("Deallocation:\n");
>                 print_track(&free_info->track);
> -               break;
>         }
>  }
>  #endif
> --
> 2.7.3
>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 05/06/2016 02:47 PM, Kuthonuzo Luruo 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().
>>
>> 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 'test_kasan' kasan_double_free() test in accompanying
>>   patch.
>>
>> Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
>> ---
>>
>> Changes in v2:
>> - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
>>   lock/unlock functions; kasan_alloc_meta modified to add new state while
>>   using fewer bits overall.
>> - Double-free pr_err promoted to kasan_report().
>> - kasan_init_object() introduced to initialize KASAN object metadata
>>   during slab creation. KASAN_STATE_INIT initialization removed from
>>   kasan_poison_object_data().
>>
>> ---
>>  include/linux/kasan.h |    8 +++
>>  mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++-------------
>>  mm/kasan/kasan.h      |   15 +++++-
>>  mm/kasan/quarantine.c |    7 +++-
>>  mm/kasan/report.c     |   31 +++++++++++--
>>  mm/slab.c             |    1 +
>>  6 files changed, 142 insertions(+), 38 deletions(-)
>>
>
> Sorry, but this patch is crap.
>
> Something like this, will fix the race:
>
> ---
>  mm/kasan/kasan.c      | 20 ++++----------------
>  mm/kasan/kasan.h      | 10 +++-------
>  mm/kasan/quarantine.c |  1 -
>  mm/kasan/report.c     | 11 ++---------
>  4 files changed, 9 insertions(+), 33 deletions(-)
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..8d078dc 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -419,13 +419,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
> @@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache, void *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;
> +               if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
> +                                       &alloc_info->state)) {
>                         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:
> +               } else {
>                         pr_err("Double free");
>                         dump_stack();
> -                       break;
> -               default:
> -                       break;
>                 }
>         }
>         return false;
> @@ -571,7 +559,7 @@ 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;
> +               set_bit(KASAN_STATE_ALLOCATED, &alloc_info->state);
>                 alloc_info->alloc_size = size;
>                 set_track(&alloc_info->track, flags);
>         }
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..2dcdc8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -60,10 +60,7 @@ struct kasan_global {
>   */
>
>  enum kasan_state {
> -       KASAN_STATE_INIT,
> -       KASAN_STATE_ALLOC,
> -       KASAN_STATE_QUARANTINE,
> -       KASAN_STATE_FREE
> +       KASAN_STATE_ALLOCATED,
>  };
>
>  #define KASAN_STACK_DEPTH 64
> @@ -75,9 +72,8 @@ struct kasan_track {
>
>  struct kasan_alloc_meta {
>         struct kasan_track track;
> -       u32 state : 2;  /* enum kasan_state */
> -       u32 alloc_size : 30;
> -       u32 reserved;
> +       unsigned long state;
> +       u32 alloc_size;
>  };
>
>  struct kasan_free_meta {
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..ca33fd3 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache *cache)
>         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..c2b0e51 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,18 +140,12 @@ 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:
> +       if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
>                 pr_err("Object allocated with size %u bytes.\n",
>                        alloc_info->alloc_size);
>                 pr_err("Allocation:\n");
>                 print_track(&alloc_info->track);

alloc_info->track is not necessary initialized when
KASAN_STATE_ALLOCATED is set. Worse, it can be initialized to a wrong
stack.


> -               break;
> -       case KASAN_STATE_FREE:
> -       case KASAN_STATE_QUARANTINE:
> +       } else {
>                 pr_err("Object freed, allocated with size %u bytes\n",
>                        alloc_info->alloc_size);
>                 free_info = get_free_info(cache, object);
> @@ -159,7 +153,6 @@ static void object_err(struct kmem_cache *cache, struct page *page,
>                 print_track(&alloc_info->track);
>                 pr_err("Deallocation:\n");
>                 print_track(&free_info->track);
> -               break;
>         }
>  }
>  #endif
> --
> 2.7.3
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 01:31 PM, Dmitry Vyukov wrote:
> On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>>
>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>> index b3c122d..c2b0e51 100644
>> --- a/mm/kasan/report.c
>> +++ b/mm/kasan/report.c
>> @@ -140,18 +140,12 @@ 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:
>> +       if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
>>                 pr_err("Object allocated with size %u bytes.\n",
>>                        alloc_info->alloc_size);
>>                 pr_err("Allocation:\n");
>>                 print_track(&alloc_info->track);
> 
> alloc_info->track is not necessary initialized when
> KASAN_STATE_ALLOCATED is set.

It should be initialized to something. If it's not initialized, than that object wasn't allocated.
So this would be a *very* random pointer access. Also this would mean that ->state itself might be not initialized too.
Anyway, we can't do much in such scenario since we can't trust any data.
And I don't think that we should because very likely this will cause panic eventually.

> Worse, it can be initialized to a wrong
> stack.
> 

Define "wrong stack" here.

I assume that you are talking about race in the following scenario:

------
Proccess A:                      Proccess B:

p_A = kmalloc();
/* use p_A */
kfree(p_A);
                                 p_A = kmalloc();
                                      ....
                                      set_bit(KASAN_STATE_ALLOCATED); //bit set, but stack is not saved yet.
/* use after free p_A */

if (test_bit(KASAN_STATE_ALLOCATED))
        print_stack() // will print track from Proccess A
-----

So, would be the stack trace from A wrong in such situation? I don't think so.
We could change ->state and save stack trace in the 'right' order with proper barriers,
but would it prevent us from showing wrong stacktrace? - It wouldn't.

Now, the only real problem with current code, is that we don't print free stack if we think that object is in
allocated state. We should do this, because more information is always better, e.g. we might hit long-delayed use-after-free,
in which case free stack would be useful (just like in scenario above).

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 01:31 PM, Dmitry Vyukov wrote:
> On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>>
>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>> index b3c122d..c2b0e51 100644
>> --- a/mm/kasan/report.c
>> +++ b/mm/kasan/report.c
>> @@ -140,18 +140,12 @@ 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:
>> +       if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
>>                 pr_err("Object allocated with size %u bytes.\n",
>>                        alloc_info->alloc_size);
>>                 pr_err("Allocation:\n");
>>                 print_track(&alloc_info->track);
> 
> alloc_info->track is not necessary initialized when
> KASAN_STATE_ALLOCATED is set.

It should be initialized to something. If it's not initialized, than that object wasn't allocated.
So this would be a *very* random pointer access. Also this would mean that ->state itself might be not initialized too.
Anyway, we can't do much in such scenario since we can't trust any data.
And I don't think that we should because very likely this will cause panic eventually.

> Worse, it can be initialized to a wrong
> stack.
> 

Define "wrong stack" here.

I assume that you are talking about race in the following scenario:

------
Proccess A:                      Proccess B:

p_A = kmalloc();
/* use p_A */
kfree(p_A);
                                 p_A = kmalloc();
                                      ....
                                      set_bit(KASAN_STATE_ALLOCATED); //bit set, but stack is not saved yet.
/* use after free p_A */

if (test_bit(KASAN_STATE_ALLOCATED))
        print_stack() // will print track from Proccess A
-----

So, would be the stack trace from A wrong in such situation? I don't think so.
We could change ->state and save stack trace in the 'right' order with proper barriers,
but would it prevent us from showing wrong stacktrace? - It wouldn't.

Now, the only real problem with current code, is that we don't print free stack if we think that object is in
allocated state. We should do this, because more information is always better, e.g. we might hit long-delayed use-after-free,
in which case free stack would be useful (just like in scenario above).




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 2:43 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
> On 05/09/2016 01:31 PM, Dmitry Vyukov wrote:
>> On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
>> <aryabinin@virtuozzo.com> wrote:
>>>
>>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>>> index b3c122d..c2b0e51 100644
>>> --- a/mm/kasan/report.c
>>> +++ b/mm/kasan/report.c
>>> @@ -140,18 +140,12 @@ 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:
>>> +       if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
>>>                 pr_err("Object allocated with size %u bytes.\n",
>>>                        alloc_info->alloc_size);
>>>                 pr_err("Allocation:\n");
>>>                 print_track(&alloc_info->track);
>>
>> alloc_info->track is not necessary initialized when
>> KASAN_STATE_ALLOCATED is set.
>
> It should be initialized to something. If it's not initialized, than that object wasn't allocated.
> So this would be a *very* random pointer access. Also this would mean that ->state itself might be not initialized too.
> Anyway, we can't do much in such scenario since we can't trust any data.
> And I don't think that we should because very likely this will cause panic eventually.
>
>> Worse, it can be initialized to a wrong
>> stack.
>>
>
> Define "wrong stack" here.
>
> I assume that you are talking about race in the following scenario:
>
> ------
> Proccess A:                      Proccess B:
>
> p_A = kmalloc();
> /* use p_A */
> kfree(p_A);
>                                  p_A = kmalloc();
>                                       ....
>                                       set_bit(KASAN_STATE_ALLOCATED); //bit set, but stack is not saved yet.
> /* use after free p_A */
>
> if (test_bit(KASAN_STATE_ALLOCATED))
>         print_stack() // will print track from Proccess A
> -----
>
> So, would be the stack trace from A wrong in such situation? I don't think so.
> We could change ->state and save stack trace in the 'right' order with proper barriers,
> but would it prevent us from showing wrong stacktrace? - It wouldn't.
>
> Now, the only real problem with current code, is that we don't print free stack if we think that object is in
> allocated state. We should do this, because more information is always better, e.g. we might hit long-delayed use-after-free,
> in which case free stack would be useful (just like in scenario above).



If we report a use-after-free on an object, we need to print
allocation stack of that object and free object for that object. If we
report a double-free, we need to print allocation and free stacks for
the object. That's correct stacks. Everything else is wrong.

With your patch we don't print free stack on double-free. We can't
print free_info->track because it is not necessary initialized at that
point. We can't fix it by simply reversing order of stores of state
and track, because then there is a data race on track during
double-free.

I agree that if we have a use-after-free when the block is being
reused for another allocation, we can't always print the right stacks
(they are already overwritten for the new allocation). But we should
avoid printing non-matching malloc/free stacks and treating random
bytes as stack trace handle (both cases are possible with your patch).
If we print bogus info that does not make sense, we compromise trust
in the tool. Some developers postulate that a tool is totally broken
and stop looking at reports as soon as they notice any incorrect info.

Kuthonuzo's patch allows to always print matching malloc/free stacks,
never treat random bytes as stack handle and detect some cases when we
know we have stale information (due to block reuse). There well may be
ways to simplify and improve it, but it introduces and uses a
consistent discipline for header updates/reads that will be useful as
we go forward.

Re block size increase: it's not that is super big deal. But the
increase is completely unnecessary. I've checked /proc/slabinfo on my
non-instrumented machine, and most objects are exactly in 32/64-byte
objects consuming 80MB in total. We've just got rid of fat inlined
stack traces and refactored alloc/free info to occupy 16 bytes each.
Let's not start increasing them again on any occasion.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 2:43 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
> On 05/09/2016 01:31 PM, Dmitry Vyukov wrote:
>> On Mon, May 9, 2016 at 12:26 PM, Andrey Ryabinin
>> <aryabinin@virtuozzo.com> wrote:
>>>
>>> diff --git a/mm/kasan/report.c b/mm/kasan/report.c
>>> index b3c122d..c2b0e51 100644
>>> --- a/mm/kasan/report.c
>>> +++ b/mm/kasan/report.c
>>> @@ -140,18 +140,12 @@ 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:
>>> +       if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
>>>                 pr_err("Object allocated with size %u bytes.\n",
>>>                        alloc_info->alloc_size);
>>>                 pr_err("Allocation:\n");
>>>                 print_track(&alloc_info->track);
>>
>> alloc_info->track is not necessary initialized when
>> KASAN_STATE_ALLOCATED is set.
>
> It should be initialized to something. If it's not initialized, than that object wasn't allocated.
> So this would be a *very* random pointer access. Also this would mean that ->state itself might be not initialized too.
> Anyway, we can't do much in such scenario since we can't trust any data.
> And I don't think that we should because very likely this will cause panic eventually.
>
>> Worse, it can be initialized to a wrong
>> stack.
>>
>
> Define "wrong stack" here.
>
> I assume that you are talking about race in the following scenario:
>
> ------
> Proccess A:                      Proccess B:
>
> p_A = kmalloc();
> /* use p_A */
> kfree(p_A);
>                                  p_A = kmalloc();
>                                       ....
>                                       set_bit(KASAN_STATE_ALLOCATED); //bit set, but stack is not saved yet.
> /* use after free p_A */
>
> if (test_bit(KASAN_STATE_ALLOCATED))
>         print_stack() // will print track from Proccess A
> -----
>
> So, would be the stack trace from A wrong in such situation? I don't think so.
> We could change ->state and save stack trace in the 'right' order with proper barriers,
> but would it prevent us from showing wrong stacktrace? - It wouldn't.
>
> Now, the only real problem with current code, is that we don't print free stack if we think that object is in
> allocated state. We should do this, because more information is always better, e.g. we might hit long-delayed use-after-free,
> in which case free stack would be useful (just like in scenario above).



If we report a use-after-free on an object, we need to print
allocation stack of that object and free object for that object. If we
report a double-free, we need to print allocation and free stacks for
the object. That's correct stacks. Everything else is wrong.

With your patch we don't print free stack on double-free. We can't
print free_info->track because it is not necessary initialized at that
point. We can't fix it by simply reversing order of stores of state
and track, because then there is a data race on track during
double-free.

I agree that if we have a use-after-free when the block is being
reused for another allocation, we can't always print the right stacks
(they are already overwritten for the new allocation). But we should
avoid printing non-matching malloc/free stacks and treating random
bytes as stack trace handle (both cases are possible with your patch).
If we print bogus info that does not make sense, we compromise trust
in the tool. Some developers postulate that a tool is totally broken
and stop looking at reports as soon as they notice any incorrect info.

Kuthonuzo's patch allows to always print matching malloc/free stacks,
never treat random bytes as stack handle and detect some cases when we
know we have stale information (due to block reuse). There well may be
ways to simplify and improve it, but it introduces and uses a
consistent discipline for header updates/reads that will be useful as
we go forward.

Re block size increase: it's not that is super big deal. But the
increase is completely unnecessary. I've checked /proc/slabinfo on my
non-instrumented machine, and most objects are exactly in 32/64-byte
objects consuming 80MB in total. We've just got rid of fat inlined
stack traces and refactored alloc/free info to occupy 16 bytes each.
Let's not start increasing them again on any occasion.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

> > 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().
> >
> > 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 'test_kasan' kasan_double_free() test in accompanying
> >   patch.
> >
> > Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
> > ---
> >
> > Changes in v2:
> > - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
> >   lock/unlock functions; kasan_alloc_meta modified to add new state while
> >   using fewer bits overall.
> > - Double-free pr_err promoted to kasan_report().
> > - kasan_init_object() introduced to initialize KASAN object metadata
> >   during slab creation. KASAN_STATE_INIT initialization removed from
> >   kasan_poison_object_data().
> >
> > ---
> >  include/linux/kasan.h |    8 +++
> >  mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++------
> -------
> >  mm/kasan/kasan.h      |   15 +++++-
> >  mm/kasan/quarantine.c |    7 +++-
> >  mm/kasan/report.c     |   31 +++++++++++--
> >  mm/slab.c             |    1 +
> >  6 files changed, 142 insertions(+), 38 deletions(-)
> >
> 
> Sorry, but this patch is crap.
> 
> Something like this, will fix the race:
> 
> ---
>  mm/kasan/kasan.c      | 20 ++++----------------
>  mm/kasan/kasan.h      | 10 +++-------
>  mm/kasan/quarantine.c |  1 -
>  mm/kasan/report.c     | 11 ++---------
>  4 files changed, 9 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..8d078dc 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -419,13 +419,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
> @@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache,
> void *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;
> +		if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
> +					&alloc_info->state)) {
>  			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:
> +		} else {
>  			pr_err("Double free");
>  			dump_stack();
> -			break;
> -		default:
> -			break;
>  		}
>  	}
>  	return false;
> @@ -571,7 +559,7 @@ 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;
> +		set_bit(KASAN_STATE_ALLOCATED, &alloc_info->state);
>  		alloc_info->alloc_size = size;
>  		set_track(&alloc_info->track, flags);
>  	}
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..2dcdc8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -60,10 +60,7 @@ struct kasan_global {
>   */
> 
>  enum kasan_state {
> -	KASAN_STATE_INIT,
> -	KASAN_STATE_ALLOC,
> -	KASAN_STATE_QUARANTINE,
> -	KASAN_STATE_FREE
> +	KASAN_STATE_ALLOCATED,
>  };
> 
>  #define KASAN_STACK_DEPTH 64
> @@ -75,9 +72,8 @@ struct kasan_track {
> 
>  struct kasan_alloc_meta {
>  	struct kasan_track track;
> -	u32 state : 2;	/* enum kasan_state */
> -	u32 alloc_size : 30;
> -	u32 reserved;
> +	unsigned long state;
> +	u32 alloc_size;
>  };
> 
>  struct kasan_free_meta {
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..ca33fd3 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache
> *cache)
>  	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..c2b0e51 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,18 +140,12 @@ 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:
> +	if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
>  		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:
> +	} else {
>  		pr_err("Object freed, allocated with size %u bytes\n",
>  		       alloc_info->alloc_size);
>  		free_info = get_free_info(cache, object);
> @@ -159,7 +153,6 @@ static void object_err(struct kmem_cache *cache, struct
> page *page,
>  		print_track(&alloc_info->track);
>  		pr_err("Deallocation:\n");
>  		print_track(&free_info->track);
> -		break;
>  	}
>  }
>  #endif
> --
> 2.7.3

This patch with atomic bit op is similar in spirit to v1 except that it increases metadata size.

Kuthonuzo

RE: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Luruo, Kuthonuzo]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6171 bytes --]

> > 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().
> >
> > 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 'test_kasan' kasan_double_free() test in accompanying
> >   patch.
> >
> > Signed-off-by: Kuthonuzo Luruo <kuthonuzo.luruo@hpe.com>
> > ---
> >
> > Changes in v2:
> > - Incorporated suggestions from Dmitry Vyukov. New per-object metadata
> >   lock/unlock functions; kasan_alloc_meta modified to add new state while
> >   using fewer bits overall.
> > - Double-free pr_err promoted to kasan_report().
> > - kasan_init_object() introduced to initialize KASAN object metadata
> >   during slab creation. KASAN_STATE_INIT initialization removed from
> >   kasan_poison_object_data().
> >
> > ---
> >  include/linux/kasan.h |    8 +++
> >  mm/kasan/kasan.c      |  118 ++++++++++++++++++++++++++++++++++++------
> -------
> >  mm/kasan/kasan.h      |   15 +++++-
> >  mm/kasan/quarantine.c |    7 +++-
> >  mm/kasan/report.c     |   31 +++++++++++--
> >  mm/slab.c             |    1 +
> >  6 files changed, 142 insertions(+), 38 deletions(-)
> >
> 
> Sorry, but this patch is crap.
> 
> Something like this, will fix the race:
> 
> ---
>  mm/kasan/kasan.c      | 20 ++++----------------
>  mm/kasan/kasan.h      | 10 +++-------
>  mm/kasan/quarantine.c |  1 -
>  mm/kasan/report.c     | 11 ++---------
>  4 files changed, 9 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index ef2e87b..8d078dc 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -419,13 +419,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
> @@ -521,20 +514,15 @@ bool kasan_slab_free(struct kmem_cache *cache,
> void *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;
> +		if (test_and_clear_bit(KASAN_STATE_ALLOCATED,
> +					&alloc_info->state)) {
>  			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:
> +		} else {
>  			pr_err("Double free");
>  			dump_stack();
> -			break;
> -		default:
> -			break;
>  		}
>  	}
>  	return false;
> @@ -571,7 +559,7 @@ 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;
> +		set_bit(KASAN_STATE_ALLOCATED, &alloc_info->state);
>  		alloc_info->alloc_size = size;
>  		set_track(&alloc_info->track, flags);
>  	}
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 7da78a6..2dcdc8f 100644
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -60,10 +60,7 @@ struct kasan_global {
>   */
> 
>  enum kasan_state {
> -	KASAN_STATE_INIT,
> -	KASAN_STATE_ALLOC,
> -	KASAN_STATE_QUARANTINE,
> -	KASAN_STATE_FREE
> +	KASAN_STATE_ALLOCATED,
>  };
> 
>  #define KASAN_STACK_DEPTH 64
> @@ -75,9 +72,8 @@ struct kasan_track {
> 
>  struct kasan_alloc_meta {
>  	struct kasan_track track;
> -	u32 state : 2;	/* enum kasan_state */
> -	u32 alloc_size : 30;
> -	u32 reserved;
> +	unsigned long state;
> +	u32 alloc_size;
>  };
> 
>  struct kasan_free_meta {
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 40159a6..ca33fd3 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -147,7 +147,6 @@ static void qlink_free(void **qlink, struct kmem_cache
> *cache)
>  	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..c2b0e51 100644
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -140,18 +140,12 @@ 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:
> +	if (test_bit(KASAN_STATE_ALLOCATED, &alloc_info->state)) {
>  		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:
> +	} else {
>  		pr_err("Object freed, allocated with size %u bytes\n",
>  		       alloc_info->alloc_size);
>  		free_info = get_free_info(cache, object);
> @@ -159,7 +153,6 @@ static void object_err(struct kmem_cache *cache, struct
> page *page,
>  		print_track(&alloc_info->track);
>  		pr_err("Deallocation:\n");
>  		print_track(&free_info->track);
> -		break;
>  	}
>  }
>  #endif
> --
> 2.7.3

This patch with atomic bit op is similar in spirit to v1 except that it increases metadata size.

Kuthonuzo
N‹§²æìr¸›zǧu©ž²Æ {\b­†éì¹»\x1c®&Þ–)îÆi¢žØ^n‡r¶‰šŽŠÝ¢j$½§$¢¸\x05¢¹¨­è§~Š'.)îÄÃ,yèm¶ŸÿÃ\f%Š{±šj+ƒðèž×¦j)Z†·Ÿ

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
> 
> This patch with atomic bit op is similar in spirit to v1 except that it increases metadata size.
> 

I don't think that this is a big deal. That will slightly increase size of objects <= (128 - 32) bytes.
And if someone think otherwise, we can completely remove 'alloc_size'
(we use it only to print size in report - not very useful).

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
> 
> This patch with atomic bit op is similar in spirit to v1 except that it increases metadata size.
> 

I don't think that this is a big deal. That will slightly increase size of objects <= (128 - 32) bytes.
And if someone think otherwise, we can completely remove 'alloc_size'
(we use it only to print size in report - not very useful).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 3:01 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
> On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
>>
>> This patch with atomic bit op is similar in spirit to v1 except that it increases metadata size.
>>
>
> I don't think that this is a big deal. That will slightly increase size of objects <= (128 - 32) bytes.
> And if someone think otherwise, we can completely remove 'alloc_size'
> (we use it only to print size in report - not very useful).


Where did 128 come from?
We now should allocate only 32 bytes for 16-byte user object. If not,
there is something to fix.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Dmitry Vyukov]

On Mon, May 9, 2016 at 3:01 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
> On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
>>
>> This patch with atomic bit op is similar in spirit to v1 except that it increases metadata size.
>>
>
> I don't think that this is a big deal. That will slightly increase size of objects <= (128 - 32) bytes.
> And if someone think otherwise, we can completely remove 'alloc_size'
> (we use it only to print size in report - not very useful).


Where did 128 come from?
We now should allocate only 32 bytes for 16-byte user object. If not,
there is something to fix.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 04:20 PM, Dmitry Vyukov wrote:
> On Mon, May 9, 2016 at 3:01 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>
>>
>> On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
>>>
>>> This patch with atomic bit op is similar in spirit to v1 except that it increases metadata size.
>>>
>>
>> I don't think that this is a big deal. That will slightly increase size of objects <= (128 - 32) bytes.
>> And if someone think otherwise, we can completely remove 'alloc_size'
>> (we use it only to print size in report - not very useful).
> 
> 
> Where did 128 come from?
> We now should allocate only 32 bytes for 16-byte user object. If not,
> there is something to fix.
> 

I just said this wrong. I mean that the patch increases size of objects that have object_size <= (128 - 32).
For bigger objects, the new 'struct kasan_[alloc,free]_meta' still fits into optimal redzone.

Re: [PATCH v2 1/2] mm, kasan: improve double-free detection

[Andrey Ryabinin]

On 05/09/2016 04:20 PM, Dmitry Vyukov wrote:
> On Mon, May 9, 2016 at 3:01 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>
>>
>> On 05/09/2016 02:35 PM, Luruo, Kuthonuzo wrote:
>>>
>>> This patch with atomic bit op is similar in spirit to v1 except that it increases metadata size.
>>>
>>
>> I don't think that this is a big deal. That will slightly increase size of objects <= (128 - 32) bytes.
>> And if someone think otherwise, we can completely remove 'alloc_size'
>> (we use it only to print size in report - not very useful).
> 
> 
> Where did 128 come from?
> We now should allocate only 32 bytes for 16-byte user object. If not,
> there is something to fix.
> 

I just said this wrong. I mean that the patch increases size of objects that have object_size <= (128 - 32).
For bigger objects, the new 'struct kasan_[alloc,free]_meta' still fits into optimal redzone.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>