[PATCH] kasan: add memory corruption identification for software tag-based mode

[Walter Wu]

This patch adds memory corruption identification at bug report for 
software tag-based mode, the report show whether it is "use-after-free" 
or "out-of-bound" error instead of "invalid-access" error.This will make  
it easier for programmers to see the memory corruption problem.

Now we extend the quarantine to support both generic and tag-based kasan. 
For tag-based kasan, the quarantine stores only freed object information 
to check if an object is freed recently. When tag-based kasan reports an 
error, we can check if the tagged addr is in the quarantine and make a 
good guess if the object is more like "use-after-free" or "out-of-bound".

Due to tag-based kasan, the tag values are stored in the shadow memory, 
all tag comparison failures are memory corruption. Even if those freed 
object have been deallocated, we still can get the memory corruption. 
So the freed object doesn't need to be kept in quarantine, it can be 
immediately released after calling kfree(). We only need the freed object 
information in quarantine, the error handler is able to use object 
information to know if it has been allocated or deallocated, therefore 
every slab memory corruption can be identified whether it's 
"use-after-free" or "out-of-bound".

The difference between generic kasan and tag-based kasan quarantine is 
slab memory usage. Tag-based kasan only stores freed object information 
rather than the object itself. So tag-based kasan quarantine memory usage 
is smaller than generic kasan. 


====== Benchmarks

The following numbers were collected in QEMU.
Both generic and tag-based KASAN were used in inline instrumentation mode
and no stack checking.

Boot time :
* ~1.5 sec for clean kernel
* ~3 sec for generic KASAN
* ~3.5  sec for tag-based KASAN
* ~3.5 sec for tag-based KASAN + corruption identification

Slab memory usage after boot :
* ~10500 kb  for clean kernel
* ~30500 kb  for generic KASAN
* ~12300 kb  for tag-based KASAN
* ~17100 kb  for tag-based KASAN + corruption identification


Signed-off-by: Walter Wu <walter...@mediatek.com>
---
 include/linux/kasan.h  |  20 +++++---
 mm/kasan/Makefile      |   4 +-
 mm/kasan/common.c      |  15 +++++-
 mm/kasan/generic.c     |  11 -----
 mm/kasan/kasan.h       |  45 ++++++++++++++++-
 mm/kasan/quarantine.c  | 107 ++++++++++++++++++++++++++++++++++++++---
 mm/kasan/report.c      |  36 +++++++++-----
 mm/kasan/tags.c        |  64 ++++++++++++++++++++++++
 mm/kasan/tags_report.c |   5 +-
 mm/slub.c              |   2 -
 10 files changed, 262 insertions(+), 47 deletions(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b40ea104dd36..bbb52a8bf4a9 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -83,6 +83,9 @@ size_t kasan_metadata_size(struct kmem_cache *cache);
 bool kasan_save_enable_multi_shot(void);
 void kasan_restore_multi_shot(bool enabled);
 
+void kasan_cache_shrink(struct kmem_cache *cache);
+void kasan_cache_shutdown(struct kmem_cache *cache);
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
@@ -153,20 +156,14 @@ static inline void kasan_remove_zero_shadow(void *start,
 static inline void kasan_unpoison_slab(const void *ptr) { }
 static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
 
+static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
+static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
 #endif /* CONFIG_KASAN */
 
 #ifdef CONFIG_KASAN_GENERIC
 
 #define KASAN_SHADOW_INIT 0
 
-void kasan_cache_shrink(struct kmem_cache *cache);
-void kasan_cache_shutdown(struct kmem_cache *cache);
-
-#else /* CONFIG_KASAN_GENERIC */
-
-static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
-static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
-
 #endif /* CONFIG_KASAN_GENERIC */
 
 #ifdef CONFIG_KASAN_SW_TAGS
@@ -180,6 +177,8 @@ void *kasan_reset_tag(const void *addr);
 void kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip);
 
+struct kasan_alloc_meta *get_object_track(void);
+
 #else /* CONFIG_KASAN_SW_TAGS */
 
 static inline void kasan_init_tags(void) { }
@@ -189,6 +188,11 @@ static inline void *kasan_reset_tag(const void *addr)
 	return (void *)addr;
 }
 
+static inline struct kasan_alloc_meta *get_object_track(void)
+{
+	return 0;
+}
+
 #endif /* CONFIG_KASAN_SW_TAGS */
 
 #endif /* LINUX_KASAN_H */
diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
index 5d1065efbd47..03b0fe22ec55 100644
--- a/mm/kasan/Makefile
+++ b/mm/kasan/Makefile
@@ -16,6 +16,6 @@ CFLAGS_common.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
 CFLAGS_generic.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
 CFLAGS_tags.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
 
-obj-$(CONFIG_KASAN) := common.o init.o report.o
-obj-$(CONFIG_KASAN_GENERIC) += generic.o generic_report.o quarantine.o
+obj-$(CONFIG_KASAN) := common.o init.o report.o quarantine.o
+obj-$(CONFIG_KASAN_GENERIC) += generic.o generic_report.o
 obj-$(CONFIG_KASAN_SW_TAGS) += tags.o tags_report.o
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 80bbe62b16cd..919f693a58ab 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -81,7 +81,7 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
 	return depot_save_stack(&trace, flags);
 }
 
-static inline void set_track(struct kasan_track *track, gfp_t flags)
+void set_track(struct kasan_track *track, gfp_t flags)
 {
 	track->pid = current->pid;
 	track->stack = save_stack(flags);
@@ -457,7 +457,7 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object,
 		return false;
 
 	set_track(&get_alloc_info(cache, object)->free_track, GFP_NOWAIT);
-	quarantine_put(get_free_info(cache, object), cache);
+	quarantine_put(get_free_info(cache, tagged_object), cache);
 
 	return IS_ENABLED(CONFIG_KASAN_GENERIC);
 }
@@ -614,6 +614,17 @@ void kasan_free_shadow(const struct vm_struct *vm)
 		vfree(kasan_mem_to_shadow(vm->addr));
 }
 
+void kasan_cache_shrink(struct kmem_cache *cache)
+{
+	quarantine_remove_cache(cache);
+}
+
+void kasan_cache_shutdown(struct kmem_cache *cache)
+{
+	if (!__kmem_cache_empty(cache))
+		quarantine_remove_cache(cache);
+}
+
 #ifdef CONFIG_MEMORY_HOTPLUG
 static bool shadow_mapped(unsigned long addr)
 {
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index 504c79363a34..5f579051dead 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -191,17 +191,6 @@ void check_memory_region(unsigned long addr, size_t size, bool write,
 	check_memory_region_inline(addr, size, write, ret_ip);
 }
 
-void kasan_cache_shrink(struct kmem_cache *cache)
-{
-	quarantine_remove_cache(cache);
-}
-
-void kasan_cache_shutdown(struct kmem_cache *cache)
-{
-	if (!__kmem_cache_empty(cache))
-		quarantine_remove_cache(cache);
-}
-
 static void register_global(struct kasan_global *global)
 {
 	size_t aligned_size = round_up(global->size, KASAN_SHADOW_SCALE_SIZE);
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 3e0c11f7d7a1..6848a93660d9 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -95,9 +95,21 @@ struct kasan_alloc_meta {
 	struct kasan_track free_track;
 };
 
+#ifdef CONFIG_KASAN_GENERIC
 struct qlist_node {
 	struct qlist_node *next;
 };
+#else
+struct qlist_object {
+	unsigned long addr;
+	unsigned int size;
+	struct kasan_alloc_meta free_track;
+};
+struct qlist_node {
+	struct qlist_object *qobject;
+	struct qlist_node *next;
+};
+#endif
 struct kasan_free_meta {
 	/* This field is used while the object is in the quarantine.
 	 * Otherwise it might be used for the allocator freelist.
@@ -133,16 +145,19 @@ void kasan_report(unsigned long addr, size_t size,
 		bool is_write, unsigned long ip);
 void kasan_report_invalid_free(void *object, unsigned long ip);
 
-#if defined(CONFIG_KASAN_GENERIC) && \
+#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS) && \
 	(defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
+
 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 set_track(struct kasan_track *track, gfp_t flags);
 #else
 static inline void quarantine_put(struct kasan_free_meta *info,
 				struct kmem_cache *cache) { }
 static inline void quarantine_reduce(void) { }
 static inline void quarantine_remove_cache(struct kmem_cache *cache) { }
+static inline void set_track(struct kasan_track *track, gfp_t flags) {}
 #endif
 
 #ifdef CONFIG_KASAN_SW_TAGS
@@ -151,6 +166,15 @@ void print_tags(u8 addr_tag, const void *addr);
 
 u8 random_tag(void);
 
+bool quarantine_find_object(void *object);
+
+int qobject_add_size(void);
+
+struct qlist_node *qobject_create(struct kasan_free_meta *info,
+		struct kmem_cache *cache);
+
+void qobject_free(struct qlist_node *qlink, struct kmem_cache *cache);
+
 #else
 
 static inline void print_tags(u8 addr_tag, const void *addr) { }
@@ -160,6 +184,25 @@ static inline u8 random_tag(void)
 	return 0;
 }
 
+static inline bool quarantine_find_object(void *object)
+{
+	return 0;
+}
+
+static inline int qobject_add_size(void)
+{
+	return 0;
+}
+
+static inline struct qlist_node *qobject_create(struct kasan_free_meta *info,
+		struct kmem_cache *cache)
+{
+	return 0;
+}
+
+static inline void qobject_free(struct qlist_node *qlink,
+		struct kmem_cache *cache) {}
+
 #endif
 
 #ifndef arch_kasan_set_tag
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 978bc4a3eb51..f14c8dbec552 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -67,7 +67,10 @@ static void qlist_put(struct qlist_head *q, struct qlist_node *qlink,
 		q->tail->next = qlink;
 	q->tail = qlink;
 	qlink->next = NULL;
-	q->bytes += size;
+	if (IS_ENABLED(CONFIG_KASAN_SW_TAGS))
+		q->bytes += qobject_add_size();
+	else
+		q->bytes += size;
 }
 
 static void qlist_move_all(struct qlist_head *from, struct qlist_head *to)
@@ -139,13 +142,18 @@ static void *qlink_to_object(struct qlist_node *qlink, struct kmem_cache *cache)
 
 static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
 {
-	void *object = qlink_to_object(qlink, cache);
 	unsigned long flags;
+	struct kmem_cache *obj_cache =
+			cache ? cache :	qlink_to_cache(qlink);
+	void *object = qlink_to_object(qlink, obj_cache);
+
+	if (IS_ENABLED(CONFIG_KASAN_SW_TAGS))
+		qobject_free(qlink, cache);
 
 	if (IS_ENABLED(CONFIG_SLAB))
 		local_irq_save(flags);
 
-	___cache_free(cache, object, _THIS_IP_);
+	___cache_free(obj_cache, object, _THIS_IP_);
 
 	if (IS_ENABLED(CONFIG_SLAB))
 		local_irq_restore(flags);
@@ -160,11 +168,9 @@ static void qlist_free_all(struct qlist_head *q, struct kmem_cache *cache)
 
 	qlink = q->head;
 	while (qlink) {
-		struct kmem_cache *obj_cache =
-			cache ? cache :	qlink_to_cache(qlink);
 		struct qlist_node *next = qlink->next;
 
-		qlink_free(qlink, obj_cache);
+		qlink_free(qlink, cache);
 		qlink = next;
 	}
 	qlist_init(q);
@@ -187,7 +193,18 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
 	local_irq_save(flags);
 
 	q = this_cpu_ptr(&cpu_quarantine);
-	qlist_put(q, &info->quarantine_link, cache->size);
+	if (IS_ENABLED(CONFIG_KASAN_SW_TAGS)) {
+		struct qlist_node *free_obj_info = qobject_create(info, cache);
+
+		if (!free_obj_info) {
+			local_irq_restore(flags);
+			return;
+		}
+		qlist_put(q, free_obj_info, cache->size);
+	} else {
+		qlist_put(q, &info->quarantine_link, cache->size);
+	}
+
 	if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
 		qlist_move_all(q, &temp);
 
@@ -327,3 +344,79 @@ void quarantine_remove_cache(struct kmem_cache *cache)
 
 	synchronize_srcu(&remove_cache_srcu);
 }
+
+#ifdef CONFIG_KASAN_SW_TAGS
+static struct kasan_alloc_meta object_free_track;
+
+struct kasan_alloc_meta *get_object_track(void)
+{
+	return &object_free_track;
+}
+
+static bool qlist_find_object(struct qlist_head *from, void *addr)
+{
+	struct qlist_node *curr;
+	struct qlist_object *curr_obj;
+
+	if (unlikely(qlist_empty(from)))
+		return false;
+
+	curr = from->head;
+	while (curr) {
+		struct qlist_node *next = curr->next;
+
+		curr_obj = curr->qobject;
+		if (unlikely(((unsigned long)addr >= curr_obj->addr)
+			&& ((unsigned long)addr <
+					(curr_obj->addr + curr_obj->size)))) {
+			object_free_track = curr_obj->free_track;
+
+			return true;
+		}
+
+		curr = next;
+	}
+	return false;
+}
+
+static int per_cpu_find_object(void *arg)
+{
+	void *addr = arg;
+	struct qlist_head *q;
+
+	q = this_cpu_ptr(&cpu_quarantine);
+	return qlist_find_object(q, addr);
+}
+
+struct cpumask cpu_allowed_mask __read_mostly;
+
+bool quarantine_find_object(void *addr)
+{
+	unsigned long flags, i;
+	bool find = false;
+	int cpu;
+
+	cpumask_copy(&cpu_allowed_mask, cpu_online_mask);
+	for_each_cpu(cpu, &cpu_allowed_mask) {
+		find = smp_call_on_cpu(cpu, per_cpu_find_object, addr, true);
+		if (find)
+			return true;
+	}
+
+	raw_spin_lock_irqsave(&quarantine_lock, flags);
+	for (i = 0; i < QUARANTINE_BATCHES; i++) {
+		if (qlist_empty(&global_quarantine[i]))
+			continue;
+		find = qlist_find_object(&global_quarantine[i], addr);
+		/* Scanning whole quarantine can take a while. */
+		raw_spin_unlock_irqrestore(&quarantine_lock, flags);
+		cond_resched();
+		raw_spin_lock_irqsave(&quarantine_lock, flags);
+	}
+	raw_spin_unlock_irqrestore(&quarantine_lock, flags);
+
+	synchronize_srcu(&remove_cache_srcu);
+
+	return find;
+}
+#endif
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index ca9418fe9232..9cfabf2f0c40 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -150,18 +150,26 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
 }
 
 static void describe_object(struct kmem_cache *cache, void *object,
-				const void *addr)
+				const void *tagged_addr)
 {
+	void *untagged_addr = reset_tag(tagged_addr);
 	struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);
 
 	if (cache->flags & SLAB_KASAN) {
-		print_track(&alloc_info->alloc_track, "Allocated");
-		pr_err("\n");
-		print_track(&alloc_info->free_track, "Freed");
-		pr_err("\n");
+		if (IS_ENABLED(CONFIG_KASAN_SW_TAGS) &&
+			quarantine_find_object((void *)tagged_addr)) {
+			alloc_info = get_object_track();
+			print_track(&alloc_info->free_track, "Freed");
+			pr_err("\n");
+		} else {
+			print_track(&alloc_info->alloc_track, "Allocated");
+			pr_err("\n");
+			print_track(&alloc_info->free_track, "Freed");
+			pr_err("\n");
+		}
 	}
 
-	describe_object_addr(cache, object, addr);
+	describe_object_addr(cache, object, untagged_addr);
 }
 
 static inline bool kernel_or_module_addr(const void *addr)
@@ -180,23 +188,25 @@ static inline bool init_task_stack_addr(const void *addr)
 			sizeof(init_thread_union.stack));
 }
 
-static void print_address_description(void *addr)
+static void print_address_description(void *tagged_addr)
 {
-	struct page *page = addr_to_page(addr);
+	void *untagged_addr = reset_tag(tagged_addr);
+	struct page *page = addr_to_page(untagged_addr);
 
 	dump_stack();
 	pr_err("\n");
 
 	if (page && PageSlab(page)) {
 		struct kmem_cache *cache = page->slab_cache;
-		void *object = nearest_obj(cache, page,	addr);
+		void *object = nearest_obj(cache, page,	untagged_addr);
 
-		describe_object(cache, object, addr);
+		describe_object(cache, object, tagged_addr);
 	}
 
-	if (kernel_or_module_addr(addr) && !init_task_stack_addr(addr)) {
+	if (kernel_or_module_addr(untagged_addr) &&
+			!init_task_stack_addr(untagged_addr)) {
 		pr_err("The buggy address belongs to the variable:\n");
-		pr_err(" %pS\n", addr);
+		pr_err(" %pS\n", untagged_addr);
 	}
 
 	if (page) {
@@ -314,7 +324,7 @@ void kasan_report(unsigned long addr, size_t size,
 	pr_err("\n");
 
 	if (addr_has_shadow(untagged_addr)) {
-		print_address_description(untagged_addr);
+		print_address_description(tagged_addr);
 		pr_err("\n");
 		print_shadow_for_address(info.first_bad_addr);
 	} else {
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
index 63fca3172659..fa5d1e29003d 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -124,6 +124,70 @@ void check_memory_region(unsigned long addr, size_t size, bool write,
 	}
 }
 
+int qobject_add_size(void)
+{
+	return sizeof(struct qlist_object);
+}
+
+static struct kmem_cache *qobject_to_cache(struct qlist_object *qobject)
+{
+	return virt_to_head_page(qobject)->slab_cache;
+}
+
+struct qlist_node *qobject_create(struct kasan_free_meta *info,
+						struct kmem_cache *cache)
+{
+	struct qlist_node *free_obj_info;
+	struct qlist_object *qobject_info;
+	struct kasan_alloc_meta *object_track;
+	void *object;
+
+	object = ((void *)info) - cache->kasan_info.free_meta_offset;
+	qobject_info = kmalloc(sizeof(struct qlist_object), GFP_NOWAIT);
+	if (!qobject_info)
+		return NULL;
+	qobject_info->addr = (unsigned long) object;
+	qobject_info->size = cache->object_size;
+	object_track = &qobject_info->free_track;
+	set_track(&object_track->free_track, GFP_NOWAIT);
+
+	free_obj_info = kmalloc(sizeof(struct qlist_node), GFP_NOWAIT);
+	if (!free_obj_info) {
+		unsigned long flags;
+		struct kmem_cache *qobject_cache =
+			qobject_to_cache(qobject_info);
+
+		if (IS_ENABLED(CONFIG_SLAB))
+			local_irq_save(flags);
+
+		___cache_free(qobject_cache, (void *)qobject_info, _THIS_IP_);
+
+		if (IS_ENABLED(CONFIG_SLAB))
+			local_irq_restore(flags);
+		return NULL;
+	}
+	free_obj_info->qobject = qobject_info;
+
+	return free_obj_info;
+}
+
+void qobject_free(struct qlist_node *qlink, struct kmem_cache *cache)
+{
+	struct qlist_object *qobject = qlink->qobject;
+	unsigned long flags;
+
+	struct kmem_cache *qobject_cache =
+			cache ? cache :	qobject_to_cache(qobject);
+
+	if (IS_ENABLED(CONFIG_SLAB))
+		local_irq_save(flags);
+
+	___cache_free(qobject_cache, (void *)qobject, _THIS_IP_);
+
+	if (IS_ENABLED(CONFIG_SLAB))
+		local_irq_restore(flags);
+}
+
 #define DEFINE_HWASAN_LOAD_STORE(size)					\
 	void __hwasan_load##size##_noabort(unsigned long addr)		\
 	{								\
diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c
index 8eaf5f722271..8c8871b2cb09 100644
--- a/mm/kasan/tags_report.c
+++ b/mm/kasan/tags_report.c
@@ -36,7 +36,10 @@
 
 const char *get_bug_type(struct kasan_access_info *info)
 {
-	return "invalid-access";
+	if (quarantine_find_object((void *)info->access_addr))
+		return "use-after-free";
+	else
+		return "out-of-bounds";
 }
 
 void *find_first_bad_addr(void *addr, size_t size)
diff --git a/mm/slub.c b/mm/slub.c
index 1b08fbcb7e61..11c54f3995c8 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3004,12 +3004,10 @@ static __always_inline void slab_free(struct kmem_cache *s, struct page *page,
 		do_slab_free(s, page, head, tail, cnt, addr);
 }
 
-#ifdef CONFIG_KASAN_GENERIC
 void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
 {
 	do_slab_free(cache, virt_to_head_page(x), x, NULL, 1, addr);
 }
-#endif
 
 void kmem_cache_free(struct kmem_cache *s, void *x)
 {
-- 
2.18.0

************* MEDIATEK Confidentiality Notice
 ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe
 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!

[Dmitry Vyukov]

and On Tue, May 28, 2019 at 9:17 AM Walter Wu

Hi Walter,

Please describe your use case.
For testing context the generic KASAN works better and it does have
quarantine already. For prod/canary environment the quarantine may be
unacceptable in most cases.
I think we also want to use tag-based KASAN as a base for ARM MTE
support in near future and quarantine will be most likely unacceptable
for main MTE use cases. So at the very least I think this should be
configurable. +Catalin for this.

You don't change total quarantine size and charge only sizeof(struct
qlist_object). If I am reading this correctly, this means that
quarantine will have the same large overhead as with generic KASAN. We
will just cache much more objects there. The boot benchmarks may be
unrepresentative for this. Don't we need to reduce quarantine size or
something?

Why do we need to move these functions?
For generic KASAN that's required because we store the objects
themselves in the quarantine, but it's not the case for tag-based mode
with your patch...

Why do we need this change?

Why is this kasan_alloc_meta rather then kasan_track? We don't
memorize alloc stack...

> +};
> +struct qlist_node {
> + struct qlist_object *qobject;
> + struct qlist_node *next;
> +};
> +#endif
> struct kasan_free_meta {
> /* This field is used while the object is in the quarantine.
> * Otherwise it might be used for the allocator freelist.
> @@ -133,16 +145,19 @@ void kasan_report(unsigned long addr, size_t size,
> bool is_write, unsigned long ip);
> void kasan_report_invalid_free(void *object, unsigned long ip);
>
> -#if defined(CONFIG_KASAN_GENERIC) && \
> +#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS) && \

This condition seems to be always true, no?

> (defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
> +
> 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 set_track(struct kasan_track *track, gfp_t flags);
> #else
> static inline void quarantine_put(struct kasan_free_meta *info,
> struct kmem_cache *cache) { }
> static inline void quarantine_reduce(void) { }
> static inline void quarantine_remove_cache(struct kmem_cache *cache) { }
> +static inline void set_track(struct kasan_track *track, gfp_t flags) {}
> #endif
>
> #ifdef CONFIG_KASAN_SW_TAGS
> @@ -151,6 +166,15 @@ void print_tags(u8 addr_tag, const void *addr);
>
> u8 random_tag(void);
>
> +bool quarantine_find_object(void *object);
> +
> +int qobject_add_size(void);

Would be more reasonable to use size_t type for object sizes.

> +
> +struct qlist_node *qobject_create(struct kasan_free_meta *info,
> + struct kmem_cache *cache);
> +
> +void qobject_free(struct qlist_node *qlink, struct kmem_cache *cache);
> +
> #else
>
> static inline void print_tags(u8 addr_tag, const void *addr) { }
> @@ -160,6 +184,25 @@ static inline u8 random_tag(void)
> return 0;
> }
>
> +static inline bool quarantine_find_object(void *object)
> +{
> + return 0;

s/0/false/

> +}
> +
> +static inline int qobject_add_size(void)
> +{
> + return 0;
> +}
> +
> +static inline struct qlist_node *qobject_create(struct kasan_free_meta *info,
> + struct kmem_cache *cache)
> +{
> + return 0;

s/0/NULL/

> +}
> +
> +static inline void qobject_free(struct qlist_node *qlink,
> + struct kmem_cache *cache) {}
> +
> #endif
>
> #ifndef arch_kasan_set_tag
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 978bc4a3eb51..f14c8dbec552 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -67,7 +67,10 @@ static void qlist_put(struct qlist_head *q, struct qlist_node *qlink,
> q->tail->next = qlink;
> q->tail = qlink;
> qlink->next = NULL;
> - q->bytes += size;
> + if (IS_ENABLED(CONFIG_KASAN_SW_TAGS))

It would be more reasonable to pass the right size from the caller. It
already have to have the branch on CONFIG_KASAN_SW_TAGS because it
needs to allocate qobject or not, that would be the right place to
pass the right size.

This global is a dirty solution. It's better passed as argument to the
required functions rather than functions leave part of state in a
global and somebody picks it up later.

There can be multiple qobjects in the quarantine associated with the
address, right? If so, we need to find the last one rather then a
random one.

Can't this be an out-of-bound even if we find the object in quarantine?
For example, if we've freed an object, then reallocated and accessed
out-of-bounds within the object bounds?
Overall suggesting that this is a use-after-free rather than
out-of-bounds without redzones and quarantining the object itself is
quite imprecise. We can confuse a user even more...

Shouldn't this also account for qlist_node?

Why don't we allocate qlist_object and qlist_node in a single
allocation? Doing 2 allocations is both unnecessary slow and leads to
more complex code. We need to allocate them with a single allocations.
Also I think they should be allocated from a dedicated cache that opts
out of quarantine?

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.
> To post to this group, send email to kasa...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/1559027797-30303-1-git-send-email-walter-zh.wu%40mediatek.com.
> For more options, visit https://groups.google.com/d/optout.

[Walter Wu]

> Hi Walter,
>
> Please describe your use case.
> For testing context the generic KASAN works better and it does have
> quarantine already. For prod/canary environment the quarantine may be
> unacceptable in most cases.
> I think we also want to use tag-based KASAN as a base for ARM MTE
> support in near future and quarantine will be most likely unacceptable
> for main MTE use cases. So at the very least I think this should be
> configurable. +Catalin for this.
>

My patch hope the tag-based KASAN bug report make it easier for
programmers to see memory corruption problem.
Because now tag-based KASAN bug report always shows “invalid-access”
error, my patch can identify it whether it is use-after-free or
out-of-bound.

We can try to make our patch is feature option. Thanks your suggestion.
Would you explain why the quarantine is unacceptable for main MTE?
Thanks.

> You don't change total quarantine size and charge only sizeof(struct
> qlist_object). If I am reading this correctly, this means that
> quarantine will have the same large overhead as with generic KASAN. We
> will just cache much more objects there. The boot benchmarks may be
> unrepresentative for this. Don't we need to reduce quarantine size or
> something?
>

Yes, we will try to choose 2. My original idea is belong to it. So we
will reduce quarantine size.

1). If quarantine size is the same with generic KASAN and tag-based
KASAN, then the miss rate of use-after-free case in generic KASAN is
larger than tag-based KASAN.
2). If tag-based KASAN quarantine size is smaller generic KASAN, then
the miss rate of use-after-free case may be the same, but tag-based
KASAN can save slab memory usage.

The quarantine in tag-based KASAN includes new objects which we create.
Those objects are the freed information. They can be shrunk by calling
them. So we move these function into CONFIG_KASAN.

In order to add freed object information into quarantine.
The freed object information is tag address , size, and free backtrace.

Yes, you are right, we only need the free_track of kasan_alloc_meta. We
will change it.

> > +};
> > +struct qlist_node {
> > + struct qlist_object *qobject;
> > + struct qlist_node *next;
> > +};
> > +#endif
> > struct kasan_free_meta {
> > /* This field is used while the object is in the quarantine.
> > * Otherwise it might be used for the allocator freelist.
> > @@ -133,16 +145,19 @@ void kasan_report(unsigned long addr, size_t size,
> > bool is_write, unsigned long ip);
> > void kasan_report_invalid_free(void *object, unsigned long ip);
> >
> > -#if defined(CONFIG_KASAN_GENERIC) && \
> > +#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS) && \
>
> This condition seems to be always true, no?
>

Yes, it is always true, it should be removed.

> > (defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
> > +
> > 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 set_track(struct kasan_track *track, gfp_t flags);
> > #else
> > static inline void quarantine_put(struct kasan_free_meta *info,
> > struct kmem_cache *cache) { }
> > static inline void quarantine_reduce(void) { }
> > static inline void quarantine_remove_cache(struct kmem_cache *cache) { }
> > +static inline void set_track(struct kasan_track *track, gfp_t flags) {}
> > #endif
> >
> > #ifdef CONFIG_KASAN_SW_TAGS
> > @@ -151,6 +166,15 @@ void print_tags(u8 addr_tag, const void *addr);
> >
> > u8 random_tag(void);
> >
> > +bool quarantine_find_object(void *object);
> > +
> > +int qobject_add_size(void);
>
> Would be more reasonable to use size_t type for object sizes.
>

the sum of qobect and qnode size?

> > +
> > +struct qlist_node *qobject_create(struct kasan_free_meta *info,
> > + struct kmem_cache *cache);
> > +
> > +void qobject_free(struct qlist_node *qlink, struct kmem_cache *cache);
> > +
> > #else
> >
> > static inline void print_tags(u8 addr_tag, const void *addr) { }
> > @@ -160,6 +184,25 @@ static inline u8 random_tag(void)
> > return 0;
> > }
> >
> > +static inline bool quarantine_find_object(void *object)
> > +{
> > + return 0;
>
> s/0/false/
>

Thanks for your friendly reminder. we will change it.

> > +}
> > +
> > +static inline int qobject_add_size(void)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline struct qlist_node *qobject_create(struct kasan_free_meta *info,
> > + struct kmem_cache *cache)
> > +{
> > + return 0;
>
> s/0/NULL/
>

Thanks for your friendly reminder. we will change it.

> > +}
> > +
> > +static inline void qobject_free(struct qlist_node *qlink,
> > + struct kmem_cache *cache) {}
> > +
> > #endif
> >
> > #ifndef arch_kasan_set_tag
> > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> > index 978bc4a3eb51..f14c8dbec552 100644
> > --- a/mm/kasan/quarantine.c
> > +++ b/mm/kasan/quarantine.c
> > @@ -67,7 +67,10 @@ static void qlist_put(struct qlist_head *q, struct qlist_node *qlink,
> > q->tail->next = qlink;
> > q->tail = qlink;
> > qlink->next = NULL;
> > - q->bytes += size;
> > + if (IS_ENABLED(CONFIG_KASAN_SW_TAGS))
>
> It would be more reasonable to pass the right size from the caller. It
> already have to have the branch on CONFIG_KASAN_SW_TAGS because it
> needs to allocate qobject or not, that would be the right place to
> pass the right size.
>

In tag-based KASAN, we will pass the sum of qobject and qnode size to it
and review qlist_put() caller whether it pass right size.

Thanks your suggestion, we will change the implementation here.

The qobject includes the address which has tag and range, corruption
address must be satisfied with the same tag and within object address
range, then it is found in the quarantine.
It should not easy to get multiple qobjects have the same tag and within
object address range.

the qobject stores object range and address which has tag, even if
the object reallocate and accessed out-of-bounds, then new object and
old object in quarantine should be different tag value, so it should be
no found in quarantine.

yes, we will count it.

Single allocation is good suggestion, if we only has one allocation.
then we need to move all member of qlist_object to qlist_node?

struct qlist_object {
unsigned long addr;
unsigned int size;
struct kasan_alloc_meta free_track;
};
struct qlist_node {
struct qlist_object *qobject;
struct qlist_node *next;
};


We call call ___cache_free() to free the qobject and qnode, it should be
out of quarantine?


Thanks,
Walter

[Dmitry Vyukov]

On Wed, May 29, 2019 at 11:35 AM Walter Wu <walter...@mediatek.com> wrote:
>
> > Hi Walter,
> >
> > Please describe your use case.
> > For testing context the generic KASAN works better and it does have
> > quarantine already. For prod/canary environment the quarantine may be
> > unacceptable in most cases.
> > I think we also want to use tag-based KASAN as a base for ARM MTE
> > support in near future and quarantine will be most likely unacceptable
> > for main MTE use cases. So at the very least I think this should be
> > configurable. +Catalin for this.
> >
> My patch hope the tag-based KASAN bug report make it easier for
> programmers to see memory corruption problem.
> Because now tag-based KASAN bug report always shows “invalid-access”
> error, my patch can identify it whether it is use-after-free or
> out-of-bound.
>
> We can try to make our patch is feature option. Thanks your suggestion.
> Would you explain why the quarantine is unacceptable for main MTE?
> Thanks.

MTE is supposed to be used on actual production devices.
Consider that by submitting this patch you are actually reducing
amount of available memory on your next phone ;)

Ok, kasan_cache_shrink is to release memory during memory pressure.
But why do we need kasan_cache_shutdown? It seems that we could leave
qobjects in quarantine when the corresponding cache is destroyed. And
in fact it's useful because we still can get use-after-frees on these
objects.

[Dmitry Vyukov]

a On Wed, May 29, 2019 at 11:35 AM Walter Wu

Ah, I see, so we remember the tagged pointer and then search the
object in quarantine using tagged pointer. That's smart.

Yes, using the tag for matching (which I missed) makes the match less likely.

But I think we should at least try to find the newest object in
best-effort manner.
Consider, both slab and slub reallocate objects in LIFO manner and we
don't have a quarantine for objects themselves. So if we have a loop
that allocates and frees an object of same size a dozen of times.
That's enough to get a duplicate pointer+tag qobject.
This includes:
1. walking the global quarantine from quarantine_tail backwards.
2. walking per-cpu lists in the opposite direction: from tail rather
then from head. I guess we don't have links, so we could change the
order and prepend new objects from head.
This way we significantly increase chances of finding the right
object. This also deserves a comment mentioning that we can find a
wrong objects.

I see 2 options:
1. add addr/size/free_track to qlist_node under ifdef CONFIG_KASAN_SW_TAGS
2. or probably better would be to include qlist_node into qlist_object
as first field, then allocate qlist_object and cast it to qlist_node
when adding to quarantine, and then as we iterate quarantine, we cast
qlist_node back to qlist_object and can access size/addr.

> We call call ___cache_free() to free the qobject and qnode, it should be
> out of quarantine?

This should work.

[Walter Wu]

On Wed, 2019-05-29 at 12:00 +0200, Dmitry Vyukov wrote:
> > > There can be multiple qobjects in the quarantine associated with the
> > > address, right? If so, we need to find the last one rather then a
> > > random one.
> > >
> > The qobject includes the address which has tag and range, corruption
> > address must be satisfied with the same tag and within object address
> > range, then it is found in the quarantine.
> > It should not easy to get multiple qobjects have the same tag and within
> > object address range.
>
> Yes, using the tag for matching (which I missed) makes the match less likely.
>
> But I think we should at least try to find the newest object in
> best-effort manner.

We hope it, too.

> Consider, both slab and slub reallocate objects in LIFO manner and we
> don't have a quarantine for objects themselves. So if we have a loop
> that allocates and frees an object of same size a dozen of times.
> That's enough to get a duplicate pointer+tag qobject.
> This includes:
> 1. walking the global quarantine from quarantine_tail backwards.

It is ok.

> 2. walking per-cpu lists in the opposite direction: from tail rather
> then from head. I guess we don't have links, so we could change the
> order and prepend new objects from head.
> This way we significantly increase chances of finding the right
> object. This also deserves a comment mentioning that we can find a
> wrong objects.
>

The current walking per-cpu list direction is from head to trail. we
will modify the direction and find the newest object.

> > > Why don't we allocate qlist_object and qlist_node in a single
> > > allocation? Doing 2 allocations is both unnecessary slow and leads to
> > > more complex code. We need to allocate them with a single allocations.
> > > Also I think they should be allocated from a dedicated cache that opts
> > > out of quarantine?
> > >
> > Single allocation is good suggestion, if we only has one allocation.
> > then we need to move all member of qlist_object to qlist_node?
> >
> > struct qlist_object {
> > unsigned long addr;
> > unsigned int size;
> > struct kasan_alloc_meta free_track;
> > };
> > struct qlist_node {
> > struct qlist_object *qobject;
> > struct qlist_node *next;
> > };
>
> I see 2 options:
> 1. add addr/size/free_track to qlist_node under ifdef CONFIG_KASAN_SW_TAGS
> 2. or probably better would be to include qlist_node into qlist_object
> as first field, then allocate qlist_object and cast it to qlist_node
> when adding to quarantine, and then as we iterate quarantine, we cast
> qlist_node back to qlist_object and can access size/addr.
>

Choice 2 looks better, We first try it.

>
> > We call call ___cache_free() to free the qobject and qnode, it should be
> > out of quarantine?
>
> This should work.

Thanks your good suggestion.
We will implement those solution which you suggested to the second
edition.


Thanks,
Walter

[Walter Wu]

====== Changes

Change since v1:
- add feature option CONFIG_KASAN_SW_TAGS_IDENTIFY.
- change QUARANTINE_FRACTION to reduce quarantine size.
- change the qlist order in order to find the newest object in quarantine
- reduce the number of calling kmalloc() from 2 to 1 time.
- remove global variable to use argument to pass it.
- correct the amount of qobject cache->size into the byes of qlist_head.
- only use kasan_cache_shrink() to shink memory.

Cc: Dmitry Vyukov <dvy...@google.com>

Signed-off-by: Walter Wu <walter...@mediatek.com>
---

include/linux/kasan.h | 4 ++
lib/Kconfig.kasan | 9 +++
mm/kasan/Makefile | 1 +
mm/kasan/common.c | 4 +-
mm/kasan/kasan.h | 50 +++++++++++++-
mm/kasan/quarantine.c | 146 ++++++++++++++++++++++++++++++++++++-----
mm/kasan/report.c | 37 +++++++----
mm/kasan/tags.c | 47 +++++++++++++
mm/kasan/tags_report.c | 8 ++-
mm/slub.c | 2 +-
10 files changed, 273 insertions(+), 35 deletions(-)

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b40ea104dd36..be0667225b58 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -164,7 +164,11 @@ void kasan_cache_shutdown(struct kmem_cache *cache);

#else /* CONFIG_KASAN_GENERIC */

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+void kasan_cache_shrink(struct kmem_cache *cache);
+#else

static inline void kasan_cache_shrink(struct kmem_cache *cache) {}

+#endif

static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}

#endif /* CONFIG_KASAN_GENERIC */
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 9950b660e62d..17a4952c5eee 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -134,6 +134,15 @@ config KASAN_S390_4_LEVEL_PAGING
to 3TB of RAM with KASan enabled). This options allows to force
4-level paging instead.

+config KASAN_SW_TAGS_IDENTIFY
+ bool "Enable memory corruption idenitfication"
+ depends on KASAN_SW_TAGS
+ help
+ Now tag-based KASAN bug report always shows invalid-access error, This
+ options can identify it whether it is use-after-free or out-of-bound.
+ This will make it easier for programmers to see the memory corruption
+ problem.
+
config TEST_KASAN
tristate "Module for testing KASAN for bug detection"
depends on m && KASAN
diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
index 5d1065efbd47..d8540e5070cb 100644
--- a/mm/kasan/Makefile
+++ b/mm/kasan/Makefile
@@ -19,3 +19,4 @@ CFLAGS_tags.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
obj-$(CONFIG_KASAN) := common.o init.o report.o
obj-$(CONFIG_KASAN_GENERIC) += generic.o generic_report.o quarantine.o
obj-$(CONFIG_KASAN_SW_TAGS) += tags.o tags_report.o
+obj-$(CONFIG_KASAN_SW_TAGS_IDENTIFY) += quarantine.o
diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 80bbe62b16cd..e309fbbee831 100644

--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -81,7 +81,7 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
return depot_save_stack(&trace, flags);
}

-static inline void set_track(struct kasan_track *track, gfp_t flags)
+void set_track(struct kasan_track *track, gfp_t flags)
{
track->pid = current->pid;
track->stack = save_stack(flags);
@@ -457,7 +457,7 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object,
return false;

set_track(&get_alloc_info(cache, object)->free_track, GFP_NOWAIT);
- quarantine_put(get_free_info(cache, object), cache);
+ quarantine_put(get_free_info(cache, tagged_object), cache);

return IS_ENABLED(CONFIG_KASAN_GENERIC);
}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 3e0c11f7d7a1..1be04abe2e0d 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -98,6 +98,12 @@ struct kasan_alloc_meta {

struct qlist_node {
struct qlist_node *next;
};

+struct qlist_object {
+ unsigned long addr;
+ unsigned int size;

+ struct kasan_track free_track;
+ struct qlist_node qnode;
+};

struct kasan_free_meta {
/* This field is used while the object is in the quarantine.
* Otherwise it might be used for the allocator freelist.

@@ -133,11 +139,12 @@ void kasan_report(unsigned long addr, size_t size,

bool is_write, unsigned long ip);
void kasan_report_invalid_free(void *object, unsigned long ip);

-#if defined(CONFIG_KASAN_GENERIC) && \

- (defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
+#if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS_IDENTIFY)) \
+ && (defined(CONFIG_SLAB) || defined(CONFIG_SLUB))

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 set_track(struct kasan_track *track, gfp_t flags);
#else
static inline void quarantine_put(struct kasan_free_meta *info,
struct kmem_cache *cache) { }

@@ -151,6 +158,31 @@ void print_tags(u8 addr_tag, const void *addr);

u8 random_tag(void);

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+bool quarantine_find_object(void *object,
+ struct kasan_track *free_track);
+
+struct qlist_object *qobject_create(struct kasan_free_meta *info,

+ struct kmem_cache *cache);
+
+void qobject_free(struct qlist_node *qlink, struct kmem_cache *cache);
+#else

+static inline bool quarantine_find_object(void *object,
+ struct kasan_track *free_track)

+{
+ return false;
+}
+

+static inline struct qlist_object *qobject_create(struct kasan_free_meta *info,

+ struct kmem_cache *cache)
+{

+ return NULL;

+}
+
+static inline void qobject_free(struct qlist_node *qlink,
+ struct kmem_cache *cache) {}
+#endif

+
#else

static inline void print_tags(u8 addr_tag, const void *addr) { }

@@ -160,6 +192,20 @@ static inline u8 random_tag(void)
return 0;
}

+static inline bool quarantine_find_object(void *object,
+ struct kasan_track *free_track)

+{
+ return false;
+}
+

+static inline struct qlist_object *qobject_create(struct kasan_free_meta *info,

+ struct kmem_cache *cache)
+{

+ return NULL;

+}
+
+static inline void qobject_free(struct qlist_node *qlink,
+ struct kmem_cache *cache) {}

#endif

#ifndef arch_kasan_set_tag
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c

index 978bc4a3eb51..43b009659d80 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -61,12 +61,16 @@ static void qlist_init(struct qlist_head *q)

static void qlist_put(struct qlist_head *q, struct qlist_node *qlink,

size_t size)
{
- if (unlikely(qlist_empty(q)))
+ struct qlist_node *prev_qlink = q->head;
+
+ if (unlikely(qlist_empty(q))) {
q->head = qlink;
- else
- q->tail->next = qlink;
- q->tail = qlink;
- qlink->next = NULL;
+ q->tail = qlink;
+ qlink->next = NULL;
+ } else {
+ q->head = qlink;
+ qlink->next = prev_qlink;

+ }
q->bytes += size;
}

@@ -121,7 +125,11 @@ static unsigned long quarantine_batch_size;
* Quarantine doesn't support memory shrinker with SLAB allocator, so we keep
* the ratio low to avoid OOM.
*/
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+#define QUARANTINE_FRACTION 128
+#else
#define QUARANTINE_FRACTION 32
+#endif

static struct kmem_cache *qlink_to_cache(struct qlist_node *qlink)
{
@@ -139,16 +147,24 @@ static void *qlink_to_object(struct qlist_node *qlink, struct kmem_cache *cache)

static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
{
- void *object = qlink_to_object(qlink, cache);
unsigned long flags;

+ struct kmem_cache *obj_cache;
+ void *object;

- if (IS_ENABLED(CONFIG_SLAB))
- local_irq_save(flags);
+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY)) {
+ qobject_free(qlink, cache);
+ } else {
+ obj_cache = cache ? cache : qlink_to_cache(qlink);
+ object = qlink_to_object(qlink, obj_cache);

- ___cache_free(cache, object, _THIS_IP_);
+ if (IS_ENABLED(CONFIG_SLAB))
+ local_irq_save(flags);

- if (IS_ENABLED(CONFIG_SLAB))
- local_irq_restore(flags);
+ ___cache_free(obj_cache, object, _THIS_IP_);

+
+ if (IS_ENABLED(CONFIG_SLAB))
+ local_irq_restore(flags);
+ }
}

static void qlist_free_all(struct qlist_head *q, struct kmem_cache *cache)

@@ -160,11 +176,9 @@ static void qlist_free_all(struct qlist_head *q, struct kmem_cache *cache)

qlink = q->head;
while (qlink) {
- struct kmem_cache *obj_cache =
- cache ? cache : qlink_to_cache(qlink);
struct qlist_node *next = qlink->next;

- qlink_free(qlink, obj_cache);
+ qlink_free(qlink, cache);
qlink = next;
}
qlist_init(q);

@@ -175,6 +189,8 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)
unsigned long flags;
struct qlist_head *q;
struct qlist_head temp = QLIST_INIT;
+ struct kmem_cache *qobject_cache;
+ struct qlist_object *free_obj_info;

/*
* Note: irq must be disabled until after we move the batch to the
@@ -187,7 +203,19 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)

local_irq_save(flags);

q = this_cpu_ptr(&cpu_quarantine);
- qlist_put(q, &info->quarantine_link, cache->size);

+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY)) {
+ free_obj_info = qobject_create(info, cache);

+ if (!free_obj_info) {
+ local_irq_restore(flags);
+ return;
+ }
+

+ qobject_cache = qlink_to_cache(&free_obj_info->qnode);
+ qlist_put(q, &free_obj_info->qnode, qobject_cache->size);

+ } else {
+ qlist_put(q, &info->quarantine_link, cache->size);
+ }
+
if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
qlist_move_all(q, &temp);

@@ -220,7 +248,6 @@ void quarantine_reduce(void)
if (likely(READ_ONCE(quarantine_size) <=
READ_ONCE(quarantine_max_size)))
return;
-
/*
* srcu critical section ensures that quarantine_remove_cache()
* will not miss objects belonging to the cache while they are in our
@@ -327,3 +354,90 @@ void quarantine_remove_cache(struct kmem_cache *cache)

synchronize_srcu(&remove_cache_srcu);
}
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+static noinline bool qlist_find_object(struct qlist_head *from, void *arg)

+{
+ struct qlist_node *curr;
+ struct qlist_object *curr_obj;

+ struct qlist_object *target = (struct qlist_object *)arg;

+
+ if (unlikely(qlist_empty(from)))
+ return false;
+
+ curr = from->head;
+ while (curr) {
+ struct qlist_node *next = curr->next;
+

+ curr_obj = container_of(curr, struct qlist_object, qnode);
+ if (unlikely((target->addr >= curr_obj->addr) &&
+ (target->addr < (curr_obj->addr + curr_obj->size)))) {
+ target->free_track = curr_obj->free_track;

+ return true;
+ }
+
+ curr = next;
+ }
+ return false;
+}
+

+static noinline int per_cpu_find_object(void *arg)
+{

+ struct qlist_head *q;
+
+ q = this_cpu_ptr(&cpu_quarantine);

+ return qlist_find_object(q, arg);

+}
+
+struct cpumask cpu_allowed_mask __read_mostly;
+

+bool quarantine_find_object(void *addr, struct kasan_track *free_track)
+{
+ unsigned long flags;

+ bool find = false;

+ int cpu, i;
+ struct qlist_object target;
+
+ target.addr = (unsigned long)addr;

+
+ cpumask_copy(&cpu_allowed_mask, cpu_online_mask);
+ for_each_cpu(cpu, &cpu_allowed_mask) {
+ find = smp_call_on_cpu(cpu, per_cpu_find_object,

+ (void *)&target, true);
+ if (find) {
+ if (free_track)
+ *free_track = target.free_track;

+ return true;
+ }
+ }
+

+ raw_spin_lock_irqsave(&quarantine_lock, flags);
+ for (i = quarantine_tail; i >= 0; i--) {

+ if (qlist_empty(&global_quarantine[i]))
+ continue;
+ find = qlist_find_object(&global_quarantine[i],

+ (void *)&target);
+ if (find) {
+ if (free_track)
+ *free_track = target.free_track;
+ raw_spin_unlock_irqrestore(&quarantine_lock, flags);

+ return true;
+ }
+ }

+ for (i = QUARANTINE_BATCHES-1; i > quarantine_tail; i--) {

+ if (qlist_empty(&global_quarantine[i]))
+ continue;
+ find = qlist_find_object(&global_quarantine[i],

+ (void *)&target);
+ if (find) {
+ if (free_track)
+ *free_track = target.free_track;
+ raw_spin_unlock_irqrestore(&quarantine_lock, flags);

+ return true;
+ }
+ }

+ raw_spin_unlock_irqrestore(&quarantine_lock, flags);

+
+ return false;
+}

+#endif
diff --git a/mm/kasan/report.c b/mm/kasan/report.c

index ca9418fe9232..3cbc24cd3d43 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -150,18 +150,27 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,

}

static void describe_object(struct kmem_cache *cache, void *object,
- const void *addr)
+ const void *tagged_addr)
{
+ void *untagged_addr = reset_tag(tagged_addr);
struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);

+ struct kasan_track free_track;

if (cache->flags & SLAB_KASAN) {
- print_track(&alloc_info->alloc_track, "Allocated");
- pr_err("\n");
- print_track(&alloc_info->free_track, "Freed");
- pr_err("\n");

+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY) &&
+ quarantine_find_object((void *)tagged_addr,
+ &free_track)) {
+ print_track(&free_track, "Freed");

+ pr_err("\n");
+ } else {
+ print_track(&alloc_info->alloc_track, "Allocated");
+ pr_err("\n");
+ print_track(&alloc_info->free_track, "Freed");
+ pr_err("\n");
+ }
}

- describe_object_addr(cache, object, addr);
+ describe_object_addr(cache, object, untagged_addr);
}

static inline bool kernel_or_module_addr(const void *addr)

@@ -180,23 +189,25 @@ static inline bool init_task_stack_addr(const void *addr)

sizeof(init_thread_union.stack));
}

-static void print_address_description(void *addr)
+static void print_address_description(void *tagged_addr)
{
- struct page *page = addr_to_page(addr);
+ void *untagged_addr = reset_tag(tagged_addr);
+ struct page *page = addr_to_page(untagged_addr);

dump_stack();
pr_err("\n");

if (page && PageSlab(page)) {
struct kmem_cache *cache = page->slab_cache;
- void *object = nearest_obj(cache, page, addr);
+ void *object = nearest_obj(cache, page, untagged_addr);

- describe_object(cache, object, addr);
+ describe_object(cache, object, tagged_addr);
}

- if (kernel_or_module_addr(addr) && !init_task_stack_addr(addr)) {
+ if (kernel_or_module_addr(untagged_addr) &&
+ !init_task_stack_addr(untagged_addr)) {
pr_err("The buggy address belongs to the variable:\n");
- pr_err(" %pS\n", addr);
+ pr_err(" %pS\n", untagged_addr);
}

if (page) {

@@ -314,7 +325,7 @@ void kasan_report(unsigned long addr, size_t size,

pr_err("\n");

if (addr_has_shadow(untagged_addr)) {
- print_address_description(untagged_addr);
+ print_address_description(tagged_addr);
pr_err("\n");
print_shadow_for_address(info.first_bad_addr);
} else {
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c

index 63fca3172659..7804b48f760e 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -124,6 +124,53 @@ void check_memory_region(unsigned long addr, size_t size, bool write,
}
}

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY

+void kasan_cache_shrink(struct kmem_cache *cache)
+{
+ quarantine_remove_cache(cache);
+}
+

+struct qlist_object *qobject_create(struct kasan_free_meta *info,

+ struct kmem_cache *cache)
+{

+ struct qlist_object *qobject_info;

+ void *object;
+
+ object = ((void *)info) - cache->kasan_info.free_meta_offset;
+ qobject_info = kmalloc(sizeof(struct qlist_object), GFP_NOWAIT);
+ if (!qobject_info)
+ return NULL;
+ qobject_info->addr = (unsigned long) object;
+ qobject_info->size = cache->object_size;

+ set_track(&qobject_info->free_track, GFP_NOWAIT);
+
+ return qobject_info;

+}
+
+static struct kmem_cache *qobject_to_cache(struct qlist_object *qobject)
+{
+ return virt_to_head_page(qobject)->slab_cache;
+}
+

+void qobject_free(struct qlist_node *qlink, struct kmem_cache *cache)

+{
+ struct qlist_object *qobject = container_of(qlink,
+ struct qlist_object, qnode);

+ unsigned long flags;
+
+ struct kmem_cache *qobject_cache =
+ cache ? cache : qobject_to_cache(qobject);
+
+ if (IS_ENABLED(CONFIG_SLAB))
+ local_irq_save(flags);
+
+ ___cache_free(qobject_cache, (void *)qobject, _THIS_IP_);
+
+ if (IS_ENABLED(CONFIG_SLAB))
+ local_irq_restore(flags);
+}

+#endif

+
#define DEFINE_HWASAN_LOAD_STORE(size) \
void __hwasan_load##size##_noabort(unsigned long addr) \
{ \
diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c

index 8eaf5f722271..63b0b1f381ff 100644
--- a/mm/kasan/tags_report.c
+++ b/mm/kasan/tags_report.c
@@ -36,7 +36,13 @@

const char *get_bug_type(struct kasan_access_info *info)
{
- return "invalid-access";

+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY)) {
+ if (quarantine_find_object((void *)info->access_addr, NULL))

+ return "use-after-free";
+ else
+ return "out-of-bounds";

+ } else
+ return "invalid-access";

}

void *find_first_bad_addr(void *addr, size_t size)
diff --git a/mm/slub.c b/mm/slub.c

index 1b08fbcb7e61..751429d02846 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3004,7 +3004,7 @@ static __always_inline void slab_free(struct kmem_cache *s, struct page *page,

do_slab_free(s, page, head, tail, cnt, addr);
}

-#ifdef CONFIG_KASAN_GENERIC

+#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS_IDENTIFY)

void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
{
do_slab_free(cache, virt_to_head_page(x), x, NULL, 1, addr);

--
2.18.0

[Dmitry Vyukov]

Please restructure the code so that we don't duplicate this function
name 3 times in this header.

> static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
> +#endif
> static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
>
> #endif /* CONFIG_KASAN_GENERIC */
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index 9950b660e62d..17a4952c5eee 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -134,6 +134,15 @@ config KASAN_S390_4_LEVEL_PAGING
> to 3TB of RAM with KASan enabled). This options allows to force
> 4-level paging instead.
>
> +config KASAN_SW_TAGS_IDENTIFY
> + bool "Enable memory corruption idenitfication"

s/idenitfication/identification/

> + depends on KASAN_SW_TAGS
> + help
> + Now tag-based KASAN bug report always shows invalid-access error, This
> + options can identify it whether it is use-after-free or out-of-bound.
> + This will make it easier for programmers to see the memory corruption
> + problem.

This description looks like a change description, i.e. it describes
the current behavior and how it changes. I think code comments should
not have such, they should describe the current state of the things.
It should also mention the trade-off, otherwise it raises reasonable
questions like "why it's not enabled by default?" and "why do I ever
want to not enable it?".
I would do something like:

This option enables best-effort identification of bug type
(use-after-free or out-of-bounds)
at the cost of increased memory consumption for object quarantine.

> +
> config TEST_KASAN
> tristate "Module for testing KASAN for bug detection"
> depends on m && KASAN
> diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
> index 5d1065efbd47..d8540e5070cb 100644
> --- a/mm/kasan/Makefile
> +++ b/mm/kasan/Makefile
> @@ -19,3 +19,4 @@ CFLAGS_tags.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
> obj-$(CONFIG_KASAN) := common.o init.o report.o
> obj-$(CONFIG_KASAN_GENERIC) += generic.o generic_report.o quarantine.o
> obj-$(CONFIG_KASAN_SW_TAGS) += tags.o tags_report.o
> +obj-$(CONFIG_KASAN_SW_TAGS_IDENTIFY) += quarantine.o
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 80bbe62b16cd..e309fbbee831 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -81,7 +81,7 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
> return depot_save_stack(&trace, flags);
> }
>
> -static inline void set_track(struct kasan_track *track, gfp_t flags)
> +void set_track(struct kasan_track *track, gfp_t flags)

If you make it non-static, it should get kasan_ prefix. The name is too generic.

Please restructure the code so that we don't duplicate this function
name 3 times in this header.

Explain in a comment why we use lower value for sw tags mode.

We could use sizeof(*free_obj_info), which looks simpler. Any reason
to do another hop through the cache?

Find a way to calculate the right index using a single loop, rather
that copy-paste the whole loop body to do a small adjustment to index.

This does not look to be necessary. There are no objects from that
cache in the quarantine in general. Let's not over-complicate this.

> +}
> +
> +struct qlist_object *qobject_create(struct kasan_free_meta *info,
> + struct kmem_cache *cache)
> +{
> + struct qlist_object *qobject_info;
> + void *object;
> +
> + object = ((void *)info) - cache->kasan_info.free_meta_offset;
> + qobject_info = kmalloc(sizeof(struct qlist_object), GFP_NOWAIT);
> + if (!qobject_info)
> + return NULL;
> + qobject_info->addr = (unsigned long) object;
> + qobject_info->size = cache->object_size;
> + set_track(&qobject_info->free_track, GFP_NOWAIT);
> +
> + return qobject_info;
> +}
> +
> +static struct kmem_cache *qobject_to_cache(struct qlist_object *qobject)
> +{
> + return virt_to_head_page(qobject)->slab_cache;

This looks identical to the existing qlink_to_cache, please use the
existing function.

> +}
> +
> +void qobject_free(struct qlist_node *qlink, struct kmem_cache *cache)
> +{
> + struct qlist_object *qobject = container_of(qlink,
> + struct qlist_object, qnode);
> + unsigned long flags;
> +
> + struct kmem_cache *qobject_cache =
> + cache ? cache : qobject_to_cache(qobject);

I don't understand this part.
Will caller ever pass us the right cache? Or cache is always NULL? If
it's always NULL, why do we accept it at all?
We also allocate qobjects with kmalloc always, so we must use kfree,
why do we even mess with caches?

[Walter Wu]

On Fri, 2019-06-07 at 21:18 +0800, Dmitry Vyukov wrote:
> > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > index b40ea104dd36..be0667225b58 100644
> > --- a/include/linux/kasan.h
> > +++ b/include/linux/kasan.h
> > @@ -164,7 +164,11 @@ void kasan_cache_shutdown(struct kmem_cache *cache);
> >
> > #else /* CONFIG_KASAN_GENERIC */
> >
> > +#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> > +void kasan_cache_shrink(struct kmem_cache *cache);
> > +#else
>
> Please restructure the code so that we don't duplicate this function
> name 3 times in this header.
>

We have fixed it, Thank you for your reminder.

> > static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
> > +#endif
> > static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
> >
> > #endif /* CONFIG_KASAN_GENERIC */
> > diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> > index 9950b660e62d..17a4952c5eee 100644
> > --- a/lib/Kconfig.kasan
> > +++ b/lib/Kconfig.kasan
> > @@ -134,6 +134,15 @@ config KASAN_S390_4_LEVEL_PAGING
> > to 3TB of RAM with KASan enabled). This options allows to force
> > 4-level paging instead.
> >
> > +config KASAN_SW_TAGS_IDENTIFY
> > + bool "Enable memory corruption idenitfication"
>
> s/idenitfication/identification/
>

I should replace my glasses.

> > + depends on KASAN_SW_TAGS
> > + help
> > + Now tag-based KASAN bug report always shows invalid-access error, This
> > + options can identify it whether it is use-after-free or out-of-bound.
> > + This will make it easier for programmers to see the memory corruption
> > + problem.
>
> This description looks like a change description, i.e. it describes
> the current behavior and how it changes. I think code comments should
> not have such, they should describe the current state of the things.
> It should also mention the trade-off, otherwise it raises reasonable
> questions like "why it's not enabled by default?" and "why do I ever
> want to not enable it?".
> I would do something like:
>
> This option enables best-effort identification of bug type
> (use-after-free or out-of-bounds)
> at the cost of increased memory consumption for object quarantine.
>

I totally agree with your comments. Would you think we should try to add the cost?
It may be that it consumes about 1/128th of available memory at full quarantine usage rate.

>
>
>
> > +
> > config TEST_KASAN
> > tristate "Module for testing KASAN for bug detection"
> > depends on m && KASAN
> > diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
> > index 5d1065efbd47..d8540e5070cb 100644
> > --- a/mm/kasan/Makefile
> > +++ b/mm/kasan/Makefile
> > @@ -19,3 +19,4 @@ CFLAGS_tags.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
> > obj-$(CONFIG_KASAN) := common.o init.o report.o
> > obj-$(CONFIG_KASAN_GENERIC) += generic.o generic_report.o quarantine.o
> > obj-$(CONFIG_KASAN_SW_TAGS) += tags.o tags_report.o
> > +obj-$(CONFIG_KASAN_SW_TAGS_IDENTIFY) += quarantine.o
> > diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> > index 80bbe62b16cd..e309fbbee831 100644
> > --- a/mm/kasan/common.c
> > +++ b/mm/kasan/common.c
> > @@ -81,7 +81,7 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
> > return depot_save_stack(&trace, flags);
> > }
> >
> > -static inline void set_track(struct kasan_track *track, gfp_t flags)
> > +void set_track(struct kasan_track *track, gfp_t flags)
>
> If you make it non-static, it should get kasan_ prefix. The name is too generic.
>

Ok, We will add it into next version.

We have fixed it.

The comment is below.
"Tag-based KASAN only stores freed object information rather than the
object itself. The quarantine in tag-based KASAN only needs less usage
to achieve the same effect as generic KASAN. So We reduce the
QUARANTINE_FRACTION value to slim the quarantine"

We originally thought we should store the whole slab usage(including metadata)
instead of qobject size.
If we use sizeof(*free_obj_info), then below calculation is incorrect.
total quarantine size = (totalram_pages() << PAGE_SHIFT) / QUARANTINE_FRACTION
- QUARANTINE_PERCPU_SIZE*num_online_cpus()

single loop:

for (i = quarantine_tail, j = 1; i != quarantine_tail || j != 2;
i--) {
if (i < 0) {
i = QUARANTINE_BATCHES;
j = 2;
continue;
}
if (qlist_empty(&global_quarantine[i]))
continue;

find = qlist_find_object(&global_quarantine[i],

(void *)&target);
if (find) {
if (free_track)
*free_track = target.free_track;
raw_spin_unlock_irqrestore(&quarantine_lock, flags);
return true;

Ok, we will remove it.

2 call flow at v2.
a). kmalloc() -> quarantine_reduce() -> qlist_free_all(&to_free, NULL)
-> qlink_free(qlink, NULL) -> qobject_free(qlink, NULL)
b). kmem_cache_shrink() -> kasan_cache_shrink(cache) ->
quarantine_remove_cache() -> qlist_free_all(&to_free, cache); ->
qlink_free(qlink, cache) -> qobject_free(qlink, cache)

It passes the NULL parameter at flow a.
It passes the cache of slab at flow b.

We always need calculate the slab cache to If we remove flow b.

> We also allocate qobjects with kmalloc always, so we must use kfree,
> why do we even mess with caches?
>

We call ___cache_free() to free the qobject instead of kfree(), because
it should be out of quarantine.

[Dmitry Vyukov]

Hi,

I don't understand the question. We should not add costs if not
necessary. Or you mean why we should add _docs_ regarding the cost? Or
what?

So this is total size which is more precise. I see.

I would find the classic loop form easier to follow and then compute
the actual index as necessary.
Something along the following lines:

for (i = 0; i < QUARANTINE_BATCHES; i++) {

idx = quarantine_tail - i;
if (idx < 0)
idx += QUARANTINE_BATCHES;
...

Good. Let's do it. The simpler, the better.

> > We also allocate qobjects with kmalloc always, so we must use kfree,
> > why do we even mess with caches?
> >
> We call ___cache_free() to free the qobject instead of kfree(), because
> it should be out of quarantine.

I see. Probably this mismatch worth a comment.

[Walter Wu]

I mean the description of option. Should it add the description for
memory costs. I see KASAN_SW_TAGS and KASAN_GENERIC options to show the
memory costs. So We originally think it is possible to add the
description, if users want to enable it, maybe they want to know its
memory costs.

If you think it is not necessary, we will not add it.

Thanks your helps. It is smart code.
I am too persistent to treat 'i' as the index rather than calculate new
index.

Yes, we will add the comment at qobject_free() in order to avoid letting
others misunderstand.

[Dmitry Vyukov]

Full description of memory costs for normal KASAN mode and
KASAN_SW_TAGS should probably go into
Documentation/dev-tools/kasan.rst rather then into config description
because it may be too lengthy.

I mentioned memory costs for this config because otherwise it's
unclear why would one ever want to _not_ enable this option. If it
would only have positive effects, then it should be enabled all the
time and should not be a config option at all.

[Walter Wu]

Thanks your reminder.

> I mentioned memory costs for this config because otherwise it's
> unclear why would one ever want to _not_ enable this option. If it
> would only have positive effects, then it should be enabled all the
> time and should not be a config option at all.

Sorry, I don't get your full meaning.
You think not to add the memory costs into the description of config ?
or need to add it? or make it not be a config option(default enabled)?

[Dmitry Vyukov]

Yes, I think we need to include mention of additional cost into _this_
new config.

[Dmitry Vyukov]

I should have been asked this earlier, but: what is your use-case?
Could you use CONFIG_KASAN_GENERIC instead? Why not?
CONFIG_KASAN_GENERIC already has quarantine.

[Walter Wu]

On Tue, 2019-06-11 at 13:39 +0200, Dmitry Vyukov wrote:
> I should have been asked this earlier, but: what is your use-case?

We need KASAN to help us to detect memory corruption at mobile phone. It
is powerful tool.

> Could you use CONFIG_KASAN_GENERIC instead? Why not?
> CONFIG_KASAN_GENERIC already has quarantine.
>

We hope to use tag-based KASAN, because it consumes more less
memory(1/16) than generic KASAN(1/8), but we also hope the tag-based
KASAN report is easy read and able to identify the use-after-free or
out-of-bound.

[Walter Wu]

Thanks your response.
We will fix v2 patch into next version.

Thanks.
Walter

[Walter Wu]

Change since v2:
- remove the shinking memory function kasan_cache_shrink()
- modify the description of the CONFIG_KASAN_SW_TAGS_IDENTIFY
- optimize the quarantine_find_object() and qobject_free()
- fix the duplicating function name 3 times in the header.
- modify the function name set_track() to kasan_set_track()

Cc: Dmitry Vyukov <dvy...@google.com>
Signed-off-by: Walter Wu <walter...@mediatek.com>
---

lib/Kconfig.kasan | 8 +++
mm/kasan/Makefile | 1 +
mm/kasan/common.c | 9 +--
mm/kasan/kasan.h | 36 ++++++++++-
mm/kasan/quarantine.c | 137 +++++++++++++++++++++++++++++++++++++----
mm/kasan/report.c | 37 +++++++----
mm/kasan/tags.c | 40 ++++++++++++

mm/kasan/tags_report.c | 8 ++-
mm/slub.c | 2 +-

9 files changed, 244 insertions(+), 34 deletions(-)

diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 9950b660e62d..f612326f63f0 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -134,6 +134,14 @@ config KASAN_S390_4_LEVEL_PAGING

to 3TB of RAM with KASan enabled). This options allows to force
4-level paging instead.

+config KASAN_SW_TAGS_IDENTIFY

+ bool "Enable memory corruption identification"

+ depends on KASAN_SW_TAGS
+ help

+ This option enables best-effort identification of bug type
+ (use-after-free or out-of-bounds) at the cost of increased
+ memory consumption for object quarantine.

+
config TEST_KASAN
tristate "Module for testing KASAN for bug detection"
depends on m && KASAN
diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
index 5d1065efbd47..d8540e5070cb 100644
--- a/mm/kasan/Makefile
+++ b/mm/kasan/Makefile
@@ -19,3 +19,4 @@ CFLAGS_tags.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
obj-$(CONFIG_KASAN) := common.o init.o report.o
obj-$(CONFIG_KASAN_GENERIC) += generic.o generic_report.o quarantine.o
obj-$(CONFIG_KASAN_SW_TAGS) += tags.o tags_report.o
+obj-$(CONFIG_KASAN_SW_TAGS_IDENTIFY) += quarantine.o
diff --git a/mm/kasan/common.c b/mm/kasan/common.c

index 80bbe62b16cd..0375a37d36cb 100644

--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -81,7 +81,7 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
return depot_save_stack(&trace, flags);
}

-static inline void set_track(struct kasan_track *track, gfp_t flags)

+void kasan_set_track(struct kasan_track *track, gfp_t flags)

{
track->pid = current->pid;
track->stack = save_stack(flags);

@@ -456,8 +456,8 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object,
unlikely(!(cache->flags & SLAB_KASAN)))
return false;

- set_track(&get_alloc_info(cache, object)->free_track, GFP_NOWAIT);
- quarantine_put(get_free_info(cache, object), cache);
+ kasan_set_track(&get_alloc_info(cache, object)->free_track, GFP_NOWAIT);

+ quarantine_put(get_free_info(cache, tagged_object), cache);

return IS_ENABLED(CONFIG_KASAN_GENERIC);
}

@@ -494,7 +494,8 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,
KASAN_KMALLOC_REDZONE);

if (cache->flags & SLAB_KASAN)

- set_track(&get_alloc_info(cache, object)->alloc_track, flags);
+ kasan_set_track(&get_alloc_info(cache, object)->alloc_track,
+ flags);

return set_tag(object, tag);
}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 3e0c11f7d7a1..0ff9fb5afb91 100644

--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -98,6 +98,12 @@ struct kasan_alloc_meta {
struct qlist_node {
struct qlist_node *next;
};
+struct qlist_object {
+ unsigned long addr;
+ unsigned int size;
+ struct kasan_track free_track;
+ struct qlist_node qnode;
+};
struct kasan_free_meta {
/* This field is used while the object is in the quarantine.
* Otherwise it might be used for the allocator freelist.

@@ -133,11 +139,13 @@ void kasan_report(unsigned long addr, size_t size,

bool is_write, unsigned long ip);
void kasan_report_invalid_free(void *object, unsigned long ip);

-#if defined(CONFIG_KASAN_GENERIC) && \
- (defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
+#if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS_IDENTIFY)) \
+ && (defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
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_set_track(struct kasan_track *track, gfp_t flags);
+struct kmem_cache *qlink_to_cache(struct qlist_node *qlink);

#else
static inline void quarantine_put(struct kasan_free_meta *info,
struct kmem_cache *cache) { }

@@ -162,6 +170,30 @@ static inline u8 random_tag(void)

#endif

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+bool quarantine_find_object(void *object,
+ struct kasan_track *free_track);
+
+struct qlist_object *qobject_create(struct kasan_free_meta *info,
+ struct kmem_cache *cache);
+

+void qobject_free(struct qlist_node *qlink);

+#else
+static inline bool quarantine_find_object(void *object,
+ struct kasan_track *free_track)
+{
+ return false;
+}
+
+static inline struct qlist_object *qobject_create(struct kasan_free_meta *info,
+ struct kmem_cache *cache)
+{
+ return NULL;
+}
+

+static inline void qobject_free(struct qlist_node *qlink) {}
+#endif
+
#ifndef arch_kasan_set_tag
#define arch_kasan_set_tag(addr, tag) ((void *)(addr))
#endif
diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 978bc4a3eb51..495507a71a77 100644

--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -61,12 +61,16 @@ static void qlist_init(struct qlist_head *q)
static void qlist_put(struct qlist_head *q, struct qlist_node *qlink,
size_t size)
{
- if (unlikely(qlist_empty(q)))
+ struct qlist_node *prev_qlink = q->head;
+
+ if (unlikely(qlist_empty(q))) {

+ q->head = qlink;
+ q->tail = qlink;
+ qlink->next = NULL;
+ } else {

q->head = qlink;
- else
- q->tail->next = qlink;
- q->tail = qlink;
- qlink->next = NULL;

+ qlink->next = prev_qlink;
+ }
q->bytes += size;
}

@@ -121,9 +125,19 @@ static unsigned long quarantine_batch_size;

* Quarantine doesn't support memory shrinker with SLAB allocator, so we keep
* the ratio low to avoid OOM.
*/
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY

+/*
+ * Tag-based KASAN only stores freed object information rather than the
+ * object itself. The quarantine in tag-based KASAN only needs less usage
+ * to achieve the same effect as generic KASAN. So We modify the
+ * QUARANTINE_FRACTION to slim the quarantine.
+ */
+#define QUARANTINE_FRACTION 128

+#else
#define QUARANTINE_FRACTION 32
+#endif

-static struct kmem_cache *qlink_to_cache(struct qlist_node *qlink)
+struct kmem_cache *qlink_to_cache(struct qlist_node *qlink)
{
return virt_to_head_page(qlink)->slab_cache;
}
@@ -139,16 +153,22 @@ static void *qlink_to_object(struct qlist_node *qlink, struct kmem_cache *cache)

static void qlink_free(struct qlist_node *qlink, struct kmem_cache *cache)
{
- void *object = qlink_to_object(qlink, cache);

+ void *object;
unsigned long flags;

- if (IS_ENABLED(CONFIG_SLAB))
- local_irq_save(flags);
+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY)) {

+ qobject_free(qlink);
+ } else {
+ object = qlink_to_object(qlink, cache);

- ___cache_free(cache, object, _THIS_IP_);
+ if (IS_ENABLED(CONFIG_SLAB))
+ local_irq_save(flags);

- if (IS_ENABLED(CONFIG_SLAB))
- local_irq_restore(flags);

+ ___cache_free(cache, object, _THIS_IP_);

+
+ if (IS_ENABLED(CONFIG_SLAB))
+ local_irq_restore(flags);
+ }
}

static void qlist_free_all(struct qlist_head *q, struct kmem_cache *cache)

@@ -175,6 +195,8 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)

unsigned long flags;
struct qlist_head *q;
struct qlist_head temp = QLIST_INIT;
+ struct kmem_cache *qobject_cache;
+ struct qlist_object *free_obj_info;

/*
* Note: irq must be disabled until after we move the batch to the

@@ -187,7 +209,18 @@ void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache)

local_irq_save(flags);

q = this_cpu_ptr(&cpu_quarantine);
- qlist_put(q, &info->quarantine_link, cache->size);
+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY)) {
+ free_obj_info = qobject_create(info, cache);
+ if (!free_obj_info) {
+ local_irq_restore(flags);
+ return;
+ }
+ qobject_cache = qlink_to_cache(&free_obj_info->qnode);
+ qlist_put(q, &free_obj_info->qnode, qobject_cache->size);

+ } else {
+ qlist_put(q, &info->quarantine_link, cache->size);
+ }
+
if (unlikely(q->bytes > QUARANTINE_PERCPU_SIZE)) {
qlist_move_all(q, &temp);

@@ -327,3 +360,81 @@ void quarantine_remove_cache(struct kmem_cache *cache)

synchronize_srcu(&remove_cache_srcu);
}
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+static bool qlist_find_object(struct qlist_head *from, void *arg)

+{
+ struct qlist_node *curr;
+ struct qlist_object *curr_obj;
+ struct qlist_object *target = (struct qlist_object *)arg;
+
+ if (unlikely(qlist_empty(from)))
+ return false;
+
+ curr = from->head;
+ while (curr) {
+ struct qlist_node *next = curr->next;
+
+ curr_obj = container_of(curr, struct qlist_object, qnode);
+ if (unlikely((target->addr >= curr_obj->addr) &&
+ (target->addr < (curr_obj->addr + curr_obj->size)))) {
+ target->free_track = curr_obj->free_track;
+ return true;
+ }
+
+ curr = next;
+ }
+ return false;
+}
+

+static int per_cpu_find_object(void *arg)

+{
+ struct qlist_head *q;
+
+ q = this_cpu_ptr(&cpu_quarantine);
+ return qlist_find_object(q, arg);
+}
+
+struct cpumask cpu_allowed_mask __read_mostly;
+
+bool quarantine_find_object(void *addr, struct kasan_track *free_track)
+{
+ unsigned long flags;
+ bool find = false;

+ int cpu, i, idx;

+ struct qlist_object target;
+
+ target.addr = (unsigned long)addr;
+
+ cpumask_copy(&cpu_allowed_mask, cpu_online_mask);
+ for_each_cpu(cpu, &cpu_allowed_mask) {
+ find = smp_call_on_cpu(cpu, per_cpu_find_object,
+ (void *)&target, true);
+ if (find) {
+ if (free_track)
+ *free_track = target.free_track;
+ return true;
+ }
+ }
+
+ raw_spin_lock_irqsave(&quarantine_lock, flags);

+ for (i = 0; i < QUARANTINE_BATCHES; i++) {

+ idx = quarantine_tail - i;
+ if (idx < 0)
+ idx += QUARANTINE_BATCHES;
+ if (qlist_empty(&global_quarantine[idx]))
+ continue;
+ find = qlist_find_object(&global_quarantine[idx],

index 63fca3172659..68de6e44f74e 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -124,6 +124,46 @@ void check_memory_region(unsigned long addr, size_t size, bool write,
}
}

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY

+struct qlist_object *qobject_create(struct kasan_free_meta *info,
+ struct kmem_cache *cache)
+{
+ struct qlist_object *qobject_info;
+ void *object;
+
+ object = ((void *)info) - cache->kasan_info.free_meta_offset;
+ qobject_info = kmalloc(sizeof(struct qlist_object), GFP_NOWAIT);
+ if (!qobject_info)
+ return NULL;
+ qobject_info->addr = (unsigned long) object;
+ qobject_info->size = cache->object_size;

+ kasan_set_track(&qobject_info->free_track, GFP_NOWAIT);

+
+ return qobject_info;
+}
+

+void qobject_free(struct qlist_node *qlink)

+{
+ struct qlist_object *qobject = container_of(qlink,
+ struct qlist_object, qnode);
+ unsigned long flags;
+

+ struct kmem_cache *qobject_cache = qlink_to_cache(qlink);

+
+ if (IS_ENABLED(CONFIG_SLAB))
+ local_irq_save(flags);
+

+ /*
+ * ___cache_free() free the qobject instead of kfree()
+ * in order to be out of quarantine.
+ */

--
2.18.0

[Andrey Ryabinin]

On 6/13/19 11:13 AM, Walter Wu wrote:
> This patch adds memory corruption identification at bug report for
> software tag-based mode, the report show whether it is "use-after-free"
> or "out-of-bound" error instead of "invalid-access" error.This will make
> it easier for programmers to see the memory corruption problem.
>
> Now we extend the quarantine to support both generic and tag-based kasan.
> For tag-based kasan, the quarantine stores only freed object information
> to check if an object is freed recently. When tag-based kasan reports an
> error, we can check if the tagged addr is in the quarantine and make a
> good guess if the object is more like "use-after-free" or "out-of-bound".
>

We already have all the information and don't need the quarantine to make such guess.
Basically if shadow of the first byte of object has the same tag as tag in pointer than it's out-of-bounds,
otherwise it's use-after-free.

In pseudo-code it's something like this:

u8 object_tag = *(u8 *)kasan_mem_to_shadow(nearest_object(cacche, page, access_addr));

if (access_addr_tag == object_tag && object_tag != KASAN_TAG_INVALID)
// out-of-bounds
else
// use-after-free

[Dmitry Vyukov]

But we don't have redzones in tag mode (intentionally), so unless I am
missing something we don't have the necessary info. Both cases look
the same -- we hit a different tag.
There may only be a small trailer for kmalloc-allocated objects that
is painted with a different tag. I don't remember if we actually use a
different tag for the trailer. Since tag mode granularity is 16 bytes,
for smaller objects the trailer is impossible at all.

[Andrey Ryabinin]

On 6/13/19 4:05 PM, Dmitry Vyukov wrote:
> On Thu, Jun 13, 2019 at 2:27 PM Andrey Ryabinin <arya...@virtuozzo.com> wrote:
>> On 6/13/19 11:13 AM, Walter Wu wrote:
>>> This patch adds memory corruption identification at bug report for
>>> software tag-based mode, the report show whether it is "use-after-free"
>>> or "out-of-bound" error instead of "invalid-access" error.This will make
>>> it easier for programmers to see the memory corruption problem.
>>>
>>> Now we extend the quarantine to support both generic and tag-based kasan.
>>> For tag-based kasan, the quarantine stores only freed object information
>>> to check if an object is freed recently. When tag-based kasan reports an
>>> error, we can check if the tagged addr is in the quarantine and make a
>>> good guess if the object is more like "use-after-free" or "out-of-bound".
>>>
>>
>>
>> We already have all the information and don't need the quarantine to make such guess.
>> Basically if shadow of the first byte of object has the same tag as tag in pointer than it's out-of-bounds,
>> otherwise it's use-after-free.
>>
>> In pseudo-code it's something like this:
>>
>> u8 object_tag = *(u8 *)kasan_mem_to_shadow(nearest_object(cacche, page, access_addr));
>>
>> if (access_addr_tag == object_tag && object_tag != KASAN_TAG_INVALID)
>> // out-of-bounds
>> else
>> // use-after-free
>
> But we don't have redzones in tag mode (intentionally), so unless I am
> missing something we don't have the necessary info. Both cases look
> the same -- we hit a different tag.

We always have some redzone. We need a place to store 'struct kasan_alloc_meta',
and sometimes also kasan_free_meta plus alignment to the next object.

> There may only be a small trailer for kmalloc-allocated objects that
> is painted with a different tag. I don't remember if we actually use a
> different tag for the trailer. Since tag mode granularity is 16 bytes,
> for smaller objects the trailer is impossible at all.
>

Smaller that 16-bytes objects have 16 bytes of kasan_alloc_meta.
Redzones and freed objects always painted with KASAN_TAG_INVALID.

[Walter Wu]

Thanks your explanation.
I see, we can use it to decide corruption type.
But some use-after-free issues, it may not have accurate free-backtrace.
Unfortunately in that situation, free-backtrace is the most important.
please see below example

In generic KASAN, it gets accurate free-backrace(ptr1).
In tag-based KASAN, it gets wrong free-backtrace(ptr2). It will make
programmer misjudge, so they may not believe tag-based KASAN.
So We provide this patch, we hope tag-based KASAN bug report is the same
accurate with generic KASAN.

---
ptr1 = kmalloc(size, GFP_KERNEL);
ptr1_free(ptr1);

ptr2 = kmalloc(size, GFP_KERNEL);
ptr2_free(ptr2);

ptr1[size] = 'x'; //corruption here


static noinline void ptr1_free(char* ptr)
{
kfree(ptr);
}
static noinline void ptr2_free(char* ptr)
{
kfree(ptr);
}
---

[Walter Wu]

We think of another question about deciding by that shadow of the first
byte.
In tag-based KASAN, it is immediately released after calling kfree(), so
the slub is easy to be used by another pointer, then it will change
shadow memory to the tag of new pointer, it will not be the
KASAN_TAG_INVALID, so there are many false negative cases, especially in
small size allocation.

Our patch is to solve those problems. so please consider it, thanks.

[Walter Wu]

Hi, Andrey and Dmitry,

I am sorry to bother you.
Would you tell me what you think about this patch?
We want to use tag-based KASAN, so we hope its bug report is clear and
correct as generic KASAN.

Thanks your review.
Walter

[Dmitry Vyukov]

Hi Walter,

I will probably be busy till the next week. Sorry for delays.

[Walter Wu]

It's ok. Thanks your kindly help.
I hope I can contribute to tag-based KASAN. It is a very important tool
for us.

[Walter Wu]

Hi, Dmitry,

Would you have free time to discuss this patch together?
Thanks.

Walter

[Dmitry Vyukov]

Sorry for delays. I am overwhelm by some urgent work. I afraid to
promise any dates because the next week I am on a conference, then
again a backlog and an intern starting...

Andrey, do you still have concerns re this patch? This change allows
to print the free stack.
We also have a quarantine for hwasan in user-space. Though it works a
bit differently then the normal asan quarantine. We keep a per-thread
fixed-size ring-buffer of recent allocations:
https://github.com/llvm-mirror/compiler-rt/blob/master/lib/hwasan/hwasan_report.cpp#L274-L284
and scan these ring buffers during reports.

[Andrey Ryabinin]

I 'm not sure that quarantine is a best way to do that. Quarantine is made to delay freeing, but we don't that here.
If we want to remember more free stacks wouldn't be easier simply to remember more stacks in object itself?
Same for previously used tags for better use-after-free identification.

[Walter Wu]

Hi Andrey,

We ever tried to use object itself to determine use-after-free
identification, but tag-based KASAN immediately released the pointer
after call kfree(), the original object will be used by another
pointer, if we use object itself to determine use-after-free issue, then
it has many false negative cases. so we create a lite quarantine(ring
buffers) to record recent free stacks in order to avoid those false
negative situations.

We hope to have one solution to cover all cases and be accurate. Our
patch is configurable feature option, it can provide some programmers to
easy see the tag-based KASAN report.

> > We also have a quarantine for hwasan in user-space. Though it works a
> > bit differently then the normal asan quarantine. We keep a per-thread
> > fixed-size ring-buffer of recent allocations:
> > https://github.com/llvm-mirror/compiler-rt/blob/master/lib/hwasan/hwasan_report.cpp#L274-L284
> > and scan these ring buffers during reports.
> >

Thanks your information, it looks like the same idea with our patch.

[Andrey Ryabinin]

On 7/9/19 5:53 AM, Walter Wu wrote:
> On Mon, 2019-07-08 at 19:33 +0300, Andrey Ryabinin wrote:
>>
>> On 7/5/19 4:34 PM, Dmitry Vyukov wrote:
>>> On Mon, Jul 1, 2019 at 11:56 AM Walter Wu <walter...@mediatek.com> wrote:

>>>
>>> Sorry for delays. I am overwhelm by some urgent work. I afraid to
>>> promise any dates because the next week I am on a conference, then
>>> again a backlog and an intern starting...
>>>
>>> Andrey, do you still have concerns re this patch? This change allows
>>> to print the free stack.
>>
>> I 'm not sure that quarantine is a best way to do that. Quarantine is made to delay freeing, but we don't that here.
>> If we want to remember more free stacks wouldn't be easier simply to remember more stacks in object itself?
>> Same for previously used tags for better use-after-free identification.
>>
>
> Hi Andrey,
>
> We ever tried to use object itself to determine use-after-free
> identification, but tag-based KASAN immediately released the pointer
> after call kfree(), the original object will be used by another
> pointer, if we use object itself to determine use-after-free issue, then
> it has many false negative cases. so we create a lite quarantine(ring
> buffers) to record recent free stacks in order to avoid those false
> negative situations.

I'm telling that *more* than one free stack and also tags per object can be stored.
If object reused we would still have information about n-last usages of the object.
It seems like much easier and more efficient solution than patch you proposing.

As for other concern about this particular patch
- It wasn't tested. There is deadlock (sleep in atomic) on the report path which would have been noticed it tested.
Also GFP_NOWAIT allocation which fails very noisy and very often, especially in memory constraint enviromnent where tag-based KASAN supposed to be used.

- Inefficient usage of memory:
48 bytes (sizeof (qlist_object) + sizeof(kasan_alloc_meta)) per kfree() call seems like a lot. It could be less.

The same 'struct kasan_track' stored twice in two different places (in object and in quarantine).
Basically, at least some part of the quarantine always duplicates information that we already know about
recently freed object.

Since now we call kmalloc() from kfree() path, every unique kfree() stacktrace now generates additional unique stacktrace that
takes space in stackdepot.

[Walter Wu]

On Wed, 2019-07-10 at 21:24 +0300, Andrey Ryabinin wrote:
>
> On 7/9/19 5:53 AM, Walter Wu wrote:
> > On Mon, 2019-07-08 at 19:33 +0300, Andrey Ryabinin wrote:
> >>
> >> On 7/5/19 4:34 PM, Dmitry Vyukov wrote:
> >>> On Mon, Jul 1, 2019 at 11:56 AM Walter Wu <walter...@mediatek.com> wrote:
>
> >>>
> >>> Sorry for delays. I am overwhelm by some urgent work. I afraid to
> >>> promise any dates because the next week I am on a conference, then
> >>> again a backlog and an intern starting...
> >>>
> >>> Andrey, do you still have concerns re this patch? This change allows
> >>> to print the free stack.
> >>
> >> I 'm not sure that quarantine is a best way to do that. Quarantine is made to delay freeing, but we don't that here.
> >> If we want to remember more free stacks wouldn't be easier simply to remember more stacks in object itself?
> >> Same for previously used tags for better use-after-free identification.
> >>
> >
> > Hi Andrey,
> >
> > We ever tried to use object itself to determine use-after-free
> > identification, but tag-based KASAN immediately released the pointer
> > after call kfree(), the original object will be used by another
> > pointer, if we use object itself to determine use-after-free issue, then
> > it has many false negative cases. so we create a lite quarantine(ring
> > buffers) to record recent free stacks in order to avoid those false
> > negative situations.
>
> I'm telling that *more* than one free stack and also tags per object can be stored.
> If object reused we would still have information about n-last usages of the object.
> It seems like much easier and more efficient solution than patch you proposing.
>

To make the object reused, we must ensure that no other pointers uses it
after kfree() release the pointer.
Scenario:
1). The object reused information is valid when no another pointer uses
it.
2). The object reused information is invalid when another pointer uses
it.
Do you mean that the object reused is scenario 1) ?
If yes, maybe we can change the calling quarantine_put() location. It
will be fully use that quarantine, but at scenario 2) it looks like to
need this patch.
If no, maybe i miss your meaning, would you tell me how to use invalid
object information? or?

> As for other concern about this particular patch
> - It wasn't tested. There is deadlock (sleep in atomic) on the report path which would have been noticed it tested.

we already used it on qemu and ran kasan UT. It look like ok.

> Also GFP_NOWAIT allocation which fails very noisy and very often, especially in memory constraint enviromnent where tag-based KASAN supposed to be used.
>

Maybe, we can change it into GFP_KERNEL.

> - Inefficient usage of memory:
> 48 bytes (sizeof (qlist_object) + sizeof(kasan_alloc_meta)) per kfree() call seems like a lot. It could be less.
>

We will think it.

> The same 'struct kasan_track' stored twice in two different places (in object and in quarantine).
> Basically, at least some part of the quarantine always duplicates information that we already know about
> recently freed object.
>
> Since now we call kmalloc() from kfree() path, every unique kfree() stacktrace now generates additional unique stacktrace that
> takes space in stackdepot.
>

Duplicate information is solved after change the calling
quarantine_put() location.

[Andrey Ryabinin]

KASAN keeps information about object with the object, right after payload in the kasan_alloc_meta struct.
This information is always valid as long as slab page allocated. Currently it keeps only one last free stacktrace.
It could be extended to record more free stacktraces and also record previously used tags which will allow you
to identify use-after-free and extract right free stacktrace.

[Walter Wu]

Thanks for your explanation.

For extend slub object, if one record is 9B (sizeof(u8)+ sizeof(struct
kasan_track)) and add five records into slub object, every slub object
may add 45B usage after the system runs longer.
Slub object number is easy more than 1,000,000(maybe it may be more
bigger), then the extending object memory usage should be 45MB, and
unfortunately it is no limit. The memory usage is more bigger than our
patch.

We hope tag-based KASAN advantage is smaller memory usage. If it’s
possible, we should spend less memory in order to identify
use-after-free. Would you accept our patch after fine tune it?

[Andrey Ryabinin]

No, it's not necessarily more.
And there are other aspects to consider such as performance, how simple reliable the code is.

>
> We hope tag-based KASAN advantage is smaller memory usage. If it’s
> possible, we should spend less memory in order to identify
> use-after-free. Would you accept our patch after fine tune it?

Sure, if you manage to fix issues and demonstrate that performance penalty of your
patch is close to zero.

[Walter Wu]

I remember that there are already the lists which you concern. Maybe we
can try to solve those problems one by one.

1. deadlock issue? cause by kmalloc() after kfree()?
2. decrease allocation fail, to modify GFP_NOWAIT flag to GFP_KERNEL?
3. check whether slim 48 bytes (sizeof (qlist_object) +
sizeof(kasan_alloc_meta)) and additional unique stacktrace in
stackdepot?
4. duplicate struct 'kasan_track' information in two different places

Would you have any other concern? or?

[Andrey Ryabinin]

smp_call_on_cpu()

> 2. decrease allocation fail, to modify GFP_NOWAIT flag to GFP_KERNEL?

No, this is not gonna work. Ideally we shouldn't have any allocations there.
It's not reliable and it hurts performance.

> 3. check whether slim 48 bytes (sizeof (qlist_object) +
> sizeof(kasan_alloc_meta)) and additional unique stacktrace in
> stackdepot?
> 4. duplicate struct 'kasan_track' information in two different places
>

Yup.

> Would you have any other concern? or?
>

It would be nice to see some performance numbers. Something that uses slab allocations a lot, e.g. netperf STREAM_STREAM test.

[Walter Wu]

I dont know this meaning, we need create a qobject and put into
quarantine, so may need to call kmem_cache_alloc(), would you agree this
action?

>
> > 3. check whether slim 48 bytes (sizeof (qlist_object) +
> > sizeof(kasan_alloc_meta)) and additional unique stacktrace in
> > stackdepot?
> > 4. duplicate struct 'kasan_track' information in two different places
> >
>
> Yup.
>
> > Would you have any other concern? or?
> >
>
> It would be nice to see some performance numbers. Something that uses slab allocations a lot, e.g. netperf STREAM_STREAM test.
>

ok, we will do it.

[Andrey Ryabinin]

How is this any different from what you have now?

[Walter Wu]

I originally thought you already agreed the free-list(tag-based
quarantine) after fix those issue. If no allocation there, i think maybe
only move generic quarantine into tag-based kasan, but its memory
consumption is more bigger our patch. what do you think?

[Andrey Ryabinin]

On 7/26/19 4:19 PM, Walter Wu wrote:
> On Fri, 2019-07-26 at 15:52 +0300, Andrey Ryabinin wrote:
>>
>> On 7/26/19 3:28 PM, Walter Wu wrote:
>>> On Fri, 2019-07-26 at 15:00 +0300, Andrey Ryabinin wrote:
>>>>
>>>
>>>>>
>>>>>
>>>>> I remember that there are already the lists which you concern. Maybe we
>>>>> can try to solve those problems one by one.
>>>>>
>>>>> 1. deadlock issue? cause by kmalloc() after kfree()?
>>>>
>>>> smp_call_on_cpu()
>>>
>>>>> 2. decrease allocation fail, to modify GFP_NOWAIT flag to GFP_KERNEL?
>>>>
>>>> No, this is not gonna work. Ideally we shouldn't have any allocations there.
>>>> It's not reliable and it hurts performance.
>>>>
>>> I dont know this meaning, we need create a qobject and put into
>>> quarantine, so may need to call kmem_cache_alloc(), would you agree this
>>> action?
>>>
>>
>> How is this any different from what you have now?
>
> I originally thought you already agreed the free-list(tag-based
> quarantine) after fix those issue. If no allocation there,

If no allocation there, than it must be somewhere else.
We known exactly the amount of memory we need, so it's possible to preallocate it in advance.

[Walter Wu]

On Wed, 2019-07-31 at 20:04 +0300, Andrey Ryabinin wrote:
>
> On 7/26/19 4:19 PM, Walter Wu wrote:
> > On Fri, 2019-07-26 at 15:52 +0300, Andrey Ryabinin wrote:
> >>
> >> On 7/26/19 3:28 PM, Walter Wu wrote:
> >>> On Fri, 2019-07-26 at 15:00 +0300, Andrey Ryabinin wrote:
> >>>>
> >>>
> >>>>>
> >>>>>
> >>>>> I remember that there are already the lists which you concern. Maybe we
> >>>>> can try to solve those problems one by one.
> >>>>>
> >>>>> 1. deadlock issue? cause by kmalloc() after kfree()?
> >>>>
> >>>> smp_call_on_cpu()
> >>>
> >>>>> 2. decrease allocation fail, to modify GFP_NOWAIT flag to GFP_KERNEL?
> >>>>
> >>>> No, this is not gonna work. Ideally we shouldn't have any allocations there.
> >>>> It's not reliable and it hurts performance.
> >>>>
> >>> I dont know this meaning, we need create a qobject and put into
> >>> quarantine, so may need to call kmem_cache_alloc(), would you agree this
> >>> action?
> >>>
> >>
> >> How is this any different from what you have now?
> >
> > I originally thought you already agreed the free-list(tag-based
> > quarantine) after fix those issue. If no allocation there,
>
> If no allocation there, than it must be somewhere else.
> We known exactly the amount of memory we need, so it's possible to preallocate it in advance.
>

I see. We will implement an extend slub to record five free backtrack
and free pointer tag, and determine whether it is oob or uaf by the free
pointer tag. If you have other ideas, please tell me. Thanks.

[Walter Wu]

This patch adds memory corruption identification at bug report for
software tag-based mode, the report show whether it is "use-after-free"

or "out-of-bound" error instead of "invalid-access" error. This will make

it easier for programmers to see the memory corruption problem.

We extend the slab to store five old free pointer tag and free backtrace,
we can check if the tagged address is in the slab record and make a

good guess if the object is more like "use-after-free" or "out-of-bound".

therefore every slab memory corruption can be identified whether it's
"use-after-free" or "out-of-bound".

====== Changes
Change since v1:
- add feature option CONFIG_KASAN_SW_TAGS_IDENTIFY.
- change QUARANTINE_FRACTION to reduce quarantine size.
- change the qlist order in order to find the newest object in quarantine
- reduce the number of calling kmalloc() from 2 to 1 time.
- remove global variable to use argument to pass it.
- correct the amount of qobject cache->size into the byes of qlist_head.
- only use kasan_cache_shrink() to shink memory.

Change since v2:
- remove the shinking memory function kasan_cache_shrink()
- modify the description of the CONFIG_KASAN_SW_TAGS_IDENTIFY
- optimize the quarantine_find_object() and qobject_free()
- fix the duplicating function name 3 times in the header.
- modify the function name set_track() to kasan_set_track()

Change since v3:
- change tag-based quarantine to extend slab to identify memory corruption

Cc: Andrey Ryabinin <arya...@virtuozzo.com>

Cc: Dmitry Vyukov <dvy...@google.com>
Signed-off-by: Walter Wu <walter...@mediatek.com>
---

lib/Kconfig.kasan | 8 ++++
mm/kasan/common.c | 14 +++++--
mm/kasan/kasan.h | 37 ++++++++++++++++++
mm/kasan/report.c | 53 +++++++++++++++-----------
mm/kasan/tags.c | 86 ++++++++++++++++++++++++++++++++++++++++++
mm/kasan/tags_report.c | 5 ++-
6 files changed, 177 insertions(+), 26 deletions(-)

diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 4fafba1a923b..70b55e1c4834 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -135,6 +135,14 @@ config KASAN_S390_4_LEVEL_PAGING

to 3TB of RAM with KASan enabled). This options allows to force
4-level paging instead.

+config KASAN_SW_TAGS_IDENTIFY
+ bool "Enable memory corruption identification"
+ depends on KASAN_SW_TAGS
+ help
+ This option enables best-effort identification of bug type
+ (use-after-free or out-of-bounds) at the cost of increased

+ memory consumption for slab extending.

+
config TEST_KASAN
tristate "Module for testing KASAN for bug detection"
depends on m && KASAN

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 2277b82902d8..6bbb044708e6 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -71,7 +71,7 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
return stack_depot_save(entries, nr_entries, flags);

}

-static inline void set_track(struct kasan_track *track, gfp_t flags)
+void kasan_set_track(struct kasan_track *track, gfp_t flags)
{
track->pid = current->pid;
track->stack = save_stack(flags);

@@ -304,7 +304,8 @@ size_t kasan_metadata_size(struct kmem_cache *cache)
struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
const void *object)
{
- BUILD_BUG_ON(sizeof(struct kasan_alloc_meta) > 32);
+ if (!IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY))
+ BUILD_BUG_ON(sizeof(struct kasan_alloc_meta) > 32);
return (void *)object + cache->kasan_info.alloc_meta_offset;
}

@@ -446,7 +447,11 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object,

unlikely(!(cache->flags & SLAB_KASAN)))
return false;

- set_track(&get_alloc_info(cache, object)->free_track, GFP_NOWAIT);

+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY))
+ kasan_set_free_info(cache, object, tag);
+ else
+ kasan_set_track(&get_alloc_info(cache, object)->free_track,
+ GFP_NOWAIT);
quarantine_put(get_free_info(cache, object), cache);

return IS_ENABLED(CONFIG_KASAN_GENERIC);
@@ -484,7 +489,8 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,

KASAN_KMALLOC_REDZONE);

if (cache->flags & SLAB_KASAN)
- set_track(&get_alloc_info(cache, object)->alloc_track, flags);
+ kasan_set_track(&get_alloc_info(cache, object)->alloc_track,
+ flags);

return set_tag(object, tag);
}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h

index 014f19e76247..531a5823e8c6 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -95,9 +95,23 @@ struct kasan_track {
depot_stack_handle_t stack;
};

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+#define KASAN_EXTRA_FREE_INFO_COUNT 4
+#define KASAN_TOTAL_FREE_INFO_COUNT (KASAN_EXTRA_FREE_INFO_COUNT + 1)
+struct extra_free_info {
+ /* Round-robin FIFO array. */
+ struct kasan_track free_track[KASAN_EXTRA_FREE_INFO_COUNT];
+ u8 free_pointer_tag[KASAN_TOTAL_FREE_INFO_COUNT];
+ u8 free_track_tail;
+};
+#endif
+
struct kasan_alloc_meta {
struct kasan_track alloc_track;
struct kasan_track free_track;
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ struct extra_free_info free_info;
+#endif
};

struct qlist_node {
@@ -146,6 +160,29 @@ void kasan_report(unsigned long addr, size_t size,

bool is_write, unsigned long ip);
void kasan_report_invalid_free(void *object, unsigned long ip);

+struct page *addr_to_page(const void *addr);
+
+void kasan_set_track(struct kasan_track *track, gfp_t flags);
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+void kasan_set_free_info(struct kmem_cache *cache, void *object, u8 tag);
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag);
+char *kasan_get_corruption_type(void *addr);
+#else
+static inline void kasan_set_free_info(struct kmem_cache *cache,
+ void *object, u8 tag) { }
+static inline struct kasan_track *kasan_get_free_track(
+ struct kmem_cache *cache, void *object, u8 tag)

+{
+ return NULL;
+}

+static inline char *kasan_get_corruption_type(void *addr)

+{
+ return NULL;
+}

+#endif
+
#if defined(CONFIG_KASAN_GENERIC) && \

(defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 0e5f965f1882..9ea7a4265b42 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -111,14 +111,6 @@ static void print_track(struct kasan_track *track, const char *prefix)
}
}

-static struct page *addr_to_page(const void *addr)
-{
- if ((addr >= (void *)PAGE_OFFSET) &&
- (addr < high_memory))
- return virt_to_head_page(addr);
- return NULL;
-}
-

static void describe_object_addr(struct kmem_cache *cache, void *object,

const void *addr)
{
@@ -152,18 +144,27 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,

}

static void describe_object(struct kmem_cache *cache, void *object,
- const void *addr)
+ const void *tagged_addr)
{
+ void *untagged_addr = reset_tag(tagged_addr);
struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);

if (cache->flags & SLAB_KASAN) {
print_track(&alloc_info->alloc_track, "Allocated");

pr_err("\n");
- print_track(&alloc_info->free_track, "Freed");
- pr_err("\n");

+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY)) {
+ struct kasan_track *free_track;
+ u8 tag = get_tag(tagged_addr);
+
+ free_track = kasan_get_free_track(cache, object, tag);
+ print_track(free_track, "Freed");
+ } else {

+ print_track(&alloc_info->free_track, "Freed");
+ pr_err("\n");
+ }
}

- describe_object_addr(cache, object, addr);
+ describe_object_addr(cache, object, untagged_addr);
}

static inline bool kernel_or_module_addr(const void *addr)

@@ -344,23 +345,25 @@ static void print_address_stack_frame(const void *addr)
print_decoded_frame_descr(frame_descr);

}

-static void print_address_description(void *addr)
+static void print_address_description(void *tagged_addr)
{
- struct page *page = addr_to_page(addr);
+ void *untagged_addr = reset_tag(tagged_addr);
+ struct page *page = addr_to_page(untagged_addr);

dump_stack();
pr_err("\n");

if (page && PageSlab(page)) {
struct kmem_cache *cache = page->slab_cache;
- void *object = nearest_obj(cache, page, addr);
+ void *object = nearest_obj(cache, page, untagged_addr);

- describe_object(cache, object, addr);
+ describe_object(cache, object, tagged_addr);
}

- if (kernel_or_module_addr(addr) && !init_task_stack_addr(addr)) {
+ if (kernel_or_module_addr(untagged_addr) &&
+ !init_task_stack_addr(untagged_addr)) {
pr_err("The buggy address belongs to the variable:\n");
- pr_err(" %pS\n", addr);

+ pr_err(" %pS\n", tagged_addr);
}

if (page) {
@@ -368,7 +371,7 @@ static void print_address_description(void *addr)
dump_page(page, "kasan: bad access detected");
}

- print_address_stack_frame(addr);
+ print_address_stack_frame(untagged_addr);
}

static bool row_is_guilty(const void *row, const void *guilty)
@@ -432,6 +435,14 @@ static bool report_enabled(void)
return !test_and_set_bit(KASAN_BIT_REPORTED, &kasan_flags);
}

+struct page *addr_to_page(const void *addr)
+{
+ if ((addr >= (void *)PAGE_OFFSET) &&
+ (addr < high_memory))
+ return virt_to_head_page(addr);

+ return NULL;
+}
+

void kasan_report_invalid_free(void *object, unsigned long ip)

{
unsigned long flags;
@@ -439,10 +450,10 @@ void kasan_report_invalid_free(void *object, unsigned long ip)
start_report(&flags);
pr_err("BUG: KASAN: double-free or invalid-free in %pS\n", (void *)ip);
print_tags(get_tag(object), reset_tag(object));
- object = reset_tag(object);
pr_err("\n");
print_address_description(object);
pr_err("\n");
+ object = reset_tag(object);
print_shadow_for_address(object);
end_report(&flags);
}
@@ -479,7 +490,7 @@ void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned lon

pr_err("\n");

if (addr_has_shadow(untagged_addr)) {
- print_address_description(untagged_addr);
+ print_address_description(tagged_addr);
pr_err("\n");
print_shadow_for_address(info.first_bad_addr);
} else {
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c

index 0e987c9ca052..05a11f1cfff7 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -161,3 +161,89 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)
kasan_poison_shadow((void *)addr, size, tag);
}
EXPORT_SYMBOL(__hwasan_tag_memory);
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+void kasan_set_free_info(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ struct kasan_alloc_meta *alloc_meta;
+ struct extra_free_info *free_info;
+ u8 idx;
+
+ alloc_meta = get_alloc_info(cache, object);
+ free_info = &alloc_meta->free_info;
+
+ if (free_info->free_track_tail == 0)
+ free_info->free_track_tail = KASAN_EXTRA_FREE_INFO_COUNT;
+ else
+ free_info->free_track_tail -= 1;
+
+ idx = free_info->free_track_tail;
+ free_info->free_pointer_tag[idx] = tag;
+
+ if (idx == KASAN_EXTRA_FREE_INFO_COUNT)
+ kasan_set_track(&alloc_meta->free_track, GFP_NOWAIT);
+ else
+ kasan_set_track(&free_info->free_track[idx], GFP_NOWAIT);
+}
+
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ struct kasan_alloc_meta *alloc_meta;
+ struct extra_free_info *free_info;
+ int idx, i;
+
+ alloc_meta = get_alloc_info(cache, object);
+ free_info = &alloc_meta->free_info;
+
+ for (i = 0; i < KASAN_TOTAL_FREE_INFO_COUNT; i++) {
+ idx = free_info->free_track_tail + i;
+ if (idx >= KASAN_TOTAL_FREE_INFO_COUNT)
+ idx -= KASAN_TOTAL_FREE_INFO_COUNT;
+
+ if (free_info->free_pointer_tag[idx] == tag) {
+ if (idx == KASAN_EXTRA_FREE_INFO_COUNT)
+ return &alloc_meta->free_track;
+ else
+ return &free_info->free_track[idx];
+ }
+ }
+ if (free_info->free_track_tail == KASAN_EXTRA_FREE_INFO_COUNT)
+ return &alloc_meta->free_track;
+ else
+ return &free_info->free_track[free_info->free_track_tail];
+}
+
+char *kasan_get_corruption_type(void *addr)
+{
+ struct kasan_alloc_meta *alloc_meta;
+ struct extra_free_info *free_info;
+ struct page *page;
+ struct kmem_cache *cache;
+ void *object;
+ u8 tag;
+ int idx, i;
+
+ tag = get_tag(addr);
+ addr = reset_tag(addr);
+ page = addr_to_page(addr);
+ if (page && PageSlab(page)) {
+ cache = page->slab_cache;
+ object = nearest_obj(cache, page, addr);
+ alloc_meta = get_alloc_info(cache, object);
+ free_info = &alloc_meta->free_info;
+
+ for (i = 0; i < KASAN_TOTAL_FREE_INFO_COUNT; i++) {
+ idx = free_info->free_track_tail + i;
+ if (idx >= KASAN_TOTAL_FREE_INFO_COUNT)
+ idx -= KASAN_TOTAL_FREE_INFO_COUNT;
+
+ if (free_info->free_pointer_tag[idx] == tag)
+ return "use-after-free";
+ }
+ return "out-of-bounds";
+ }
+ return "invalid-access";
+}
+#endif
diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c
index 8eaf5f722271..6d8cdb91c4b6 100644
--- a/mm/kasan/tags_report.c
+++ b/mm/kasan/tags_report.c
@@ -36,7 +36,10 @@

const char *get_bug_type(struct kasan_access_info *info)
{
- return "invalid-access";
+ if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY))

+ return(kasan_get_corruption_type((void *)info->access_addr));

+ else
+ return "invalid-access";
}

void *find_first_bad_addr(void *addr, size_t size)

--
2.18.0

[Walter Wu]

Hi,Andrey,

Would you review the patch,please?
This patch is to pre-allocate slub record(tag and free backtrace) during
create slub object. When kernel has memory corruption, it will print
correct corruption type and free backtrace.

Thanks.

Walter

[Andrey Ryabinin]

I didn't notice anything fundamentally wrong, but I find there are some
questionable implementation choices that makes code look weirder than necessary
and harder to understand. So I ended up with cleaning it up, see the diff bellow.
I'll send v5 with that diff folded.




diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 26cb3bcc9258..6c9682ce0254 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -140,7 +140,7 @@ config KASAN_SW_TAGS_IDENTIFY
help

This option enables best-effort identification of bug type

(use-after-free or out-of-bounds) at the cost of increased

- memory consumption for slab extending.
+ memory consumption.

config TEST_KASAN
tristate "Module for testing KASAN for bug detection"

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 2cdcb16b9c2d..6814d6d6a023 100644

--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -71,7 +71,7 @@ static inline depot_stack_handle_t save_stack(gfp_t flags)
return stack_depot_save(entries, nr_entries, flags);
}

-void kasan_set_track(struct kasan_track *track, gfp_t flags)
+static inline void set_track(struct kasan_track *track, gfp_t flags)

{
track->pid = current->pid;
track->stack = save_stack(flags);

@@ -304,8 +304,6 @@ size_t kasan_metadata_size(struct kmem_cache *cache)

struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
const void *object)
{

- if (!IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY))
- BUILD_BUG_ON(sizeof(struct kasan_alloc_meta) > 32);

return (void *)object + cache->kasan_info.alloc_meta_offset;
}

@@ -316,6 +314,24 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
return (void *)object + cache->kasan_info.free_meta_offset;
}

+
+static void kasan_set_free_info(struct kmem_cache *cache,

+ void *object, u8 tag)

+{
+ struct kasan_alloc_meta *alloc_meta;

+ u8 idx = 0;

+
+ alloc_meta = get_alloc_info(cache, object);
+

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ idx = alloc_meta->free_track_idx;
+ alloc_meta->free_pointer_tag[idx] = tag;
+ alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
+#endif
+
+ set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
+}
+
void kasan_poison_slab(struct page *page)
{
unsigned long i;
@@ -452,11 +468,8 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object,

unlikely(!(cache->flags & SLAB_KASAN)))
return false;

- if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY))
- kasan_set_free_info(cache, object, tag);
- else
- kasan_set_track(&get_alloc_info(cache, object)->free_track,
- GFP_NOWAIT);

+ kasan_set_free_info(cache, object, tag);
+

quarantine_put(get_free_info(cache, object), cache);

return IS_ENABLED(CONFIG_KASAN_GENERIC);

@@ -494,8 +507,7 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,

KASAN_KMALLOC_REDZONE);

if (cache->flags & SLAB_KASAN)

- kasan_set_track(&get_alloc_info(cache, object)->alloc_track,
- flags);
+ set_track(&get_alloc_info(cache, object)->alloc_track, flags);

return set_tag(object, tag);
}
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h

index 531a5823e8c6..35cff6bbb716 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -96,21 +96,17 @@ struct kasan_track {
};

#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
-#define KASAN_EXTRA_FREE_INFO_COUNT 4
-#define KASAN_TOTAL_FREE_INFO_COUNT (KASAN_EXTRA_FREE_INFO_COUNT + 1)
-struct extra_free_info {
- /* Round-robin FIFO array. */
- struct kasan_track free_track[KASAN_EXTRA_FREE_INFO_COUNT];
- u8 free_pointer_tag[KASAN_TOTAL_FREE_INFO_COUNT];
- u8 free_track_tail;
-};
+#define KASAN_NR_FREE_STACKS 5
+#else
+#define KASAN_NR_FREE_STACKS 1
#endif

struct kasan_alloc_meta {
struct kasan_track alloc_track;

- struct kasan_track free_track;
+ struct kasan_track free_track[KASAN_NR_FREE_STACKS];
#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
- struct extra_free_info free_info;
+ u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
+ u8 free_track_idx;
#endif
};

@@ -160,28 +156,7 @@ void kasan_report(unsigned long addr, size_t size,

bool is_write, unsigned long ip);
void kasan_report_invalid_free(void *object, unsigned long ip);

-struct page *addr_to_page(const void *addr);
-
-void kasan_set_track(struct kasan_track *track, gfp_t flags);
-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
-void kasan_set_free_info(struct kmem_cache *cache, void *object, u8 tag);
-struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
- void *object, u8 tag);
-char *kasan_get_corruption_type(void *addr);
-#else
-static inline void kasan_set_free_info(struct kmem_cache *cache,
- void *object, u8 tag) { }
-static inline struct kasan_track *kasan_get_free_track(
- struct kmem_cache *cache, void *object, u8 tag)
-{
- return NULL;
-}
-static inline char *kasan_get_corruption_type(void *addr)
-{
- return NULL;
-}
-#endif
+struct page *kasan_addr_to_page(const void *addr);

#if defined(CONFIG_KASAN_GENERIC) && \
(defined(CONFIG_SLAB) || defined(CONFIG_SLUB))

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 9ea7a4265b42..621782100eaa 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -111,6 +111,14 @@ static void print_track(struct kasan_track *track, const char *prefix)
}
}

+struct page *kasan_addr_to_page(const void *addr)

+{
+ if ((addr >= (void *)PAGE_OFFSET) &&
+ (addr < high_memory))
+ return virt_to_head_page(addr);
+ return NULL;
+}
+

static void describe_object_addr(struct kmem_cache *cache, void *object,
const void *addr)
{

@@ -143,28 +151,42 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
(void *)(object_addr + cache->object_size));
}

+static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,

+ void *object, u8 tag)
+{
+ struct kasan_alloc_meta *alloc_meta;

+ int i = 0;

+
+ alloc_meta = get_alloc_info(cache, object);
+

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
+ if (alloc_meta->free_pointer_tag[i] == tag)
+ break;
+ }
+ if (i == KASAN_NR_FREE_STACKS)
+ i = alloc_meta->free_track_idx;
+#endif
+
+ return &alloc_meta->free_track[i];
+}
+

static void describe_object(struct kmem_cache *cache, void *object,

- const void *tagged_addr)
+ const void *addr, u8 tag)
{
- void *untagged_addr = reset_tag(tagged_addr);

struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);

if (cache->flags & SLAB_KASAN) {

+ struct kasan_track *free_track;
+

print_track(&alloc_info->alloc_track, "Allocated");
pr_err("\n");

- if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY)) {
- struct kasan_track *free_track;
- u8 tag = get_tag(tagged_addr);
-
- free_track = kasan_get_free_track(cache, object, tag);
- print_track(free_track, "Freed");
- } else {

- print_track(&alloc_info->free_track, "Freed");
- pr_err("\n");

- }

+ free_track = kasan_get_free_track(cache, object, tag);
+ print_track(free_track, "Freed");

+ pr_err("\n");
}

- describe_object_addr(cache, object, untagged_addr);
+ describe_object_addr(cache, object, addr);

}

static inline bool kernel_or_module_addr(const void *addr)

@@ -345,25 +367,23 @@ static void print_address_stack_frame(const void *addr)
print_decoded_frame_descr(frame_descr);
}

-static void print_address_description(void *tagged_addr)
+static void print_address_description(void *addr, u8 tag)
{
- void *untagged_addr = reset_tag(tagged_addr);
- struct page *page = addr_to_page(untagged_addr);
+ struct page *page = kasan_addr_to_page(addr);

dump_stack();
pr_err("\n");

if (page && PageSlab(page)) {
struct kmem_cache *cache = page->slab_cache;

- void *object = nearest_obj(cache, page, untagged_addr);
+ void *object = nearest_obj(cache, page, addr);

- describe_object(cache, object, tagged_addr);
+ describe_object(cache, object, addr, tag);
}

- if (kernel_or_module_addr(untagged_addr) &&
- !init_task_stack_addr(untagged_addr)) {
+ if (kernel_or_module_addr(addr) && !init_task_stack_addr(addr)) {

pr_err("The buggy address belongs to the variable:\n");

- pr_err(" %pS\n", tagged_addr);
+ pr_err(" %pS\n", addr);
}

if (page) {
@@ -371,7 +391,7 @@ static void print_address_description(void *tagged_addr)

dump_page(page, "kasan: bad access detected");
}

- print_address_stack_frame(untagged_addr);
+ print_address_stack_frame(addr);

}

static bool row_is_guilty(const void *row, const void *guilty)

@@ -435,25 +455,18 @@ static bool report_enabled(void)
return !test_and_set_bit(KASAN_BIT_REPORTED, &kasan_flags);
}

-struct page *addr_to_page(const void *addr)

-{
- if ((addr >= (void *)PAGE_OFFSET) &&
- (addr < high_memory))
- return virt_to_head_page(addr);
- return NULL;
-}
-

void kasan_report_invalid_free(void *object, unsigned long ip)
{
unsigned long flags;

+ u8 tag = get_tag(object);

+ object = reset_tag(object);

start_report(&flags);
pr_err("BUG: KASAN: double-free or invalid-free in %pS\n", (void *)ip);

- print_tags(get_tag(object), reset_tag(object));
+ print_tags(tag, object);
pr_err("\n");
- print_address_description(object);
+ print_address_description(object, tag);
pr_err("\n");
- object = reset_tag(object);
print_shadow_for_address(object);
end_report(&flags);
}
@@ -490,7 +503,7 @@ void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned lon

pr_err("\n");

if (addr_has_shadow(untagged_addr)) {

- print_address_description(tagged_addr);
+ print_address_description(untagged_addr, get_tag(tagged_addr));

pr_err("\n");
print_shadow_for_address(info.first_bad_addr);
} else {
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c

index 05a11f1cfff7..0e987c9ca052 100644
--- a/mm/kasan/tags.c
+++ b/mm/kasan/tags.c
@@ -161,89 +161,3 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)

kasan_poison_shadow((void *)addr, size, tag);
}
EXPORT_SYMBOL(__hwasan_tag_memory);

-
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
-void kasan_set_free_info(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- struct extra_free_info *free_info;
- u8 idx;
-
- alloc_meta = get_alloc_info(cache, object);
- free_info = &alloc_meta->free_info;
-
- if (free_info->free_track_tail == 0)
- free_info->free_track_tail = KASAN_EXTRA_FREE_INFO_COUNT;
- else
- free_info->free_track_tail -= 1;
-
- idx = free_info->free_track_tail;
- free_info->free_pointer_tag[idx] = tag;
-
- if (idx == KASAN_EXTRA_FREE_INFO_COUNT)
- kasan_set_track(&alloc_meta->free_track, GFP_NOWAIT);
- else
- kasan_set_track(&free_info->free_track[idx], GFP_NOWAIT);
-}
-
-struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
- void *object, u8 tag)
-{
- struct kasan_alloc_meta *alloc_meta;
- struct extra_free_info *free_info;
- int idx, i;
-
- alloc_meta = get_alloc_info(cache, object);
- free_info = &alloc_meta->free_info;
-
- for (i = 0; i < KASAN_TOTAL_FREE_INFO_COUNT; i++) {
- idx = free_info->free_track_tail + i;
- if (idx >= KASAN_TOTAL_FREE_INFO_COUNT)
- idx -= KASAN_TOTAL_FREE_INFO_COUNT;
-
- if (free_info->free_pointer_tag[idx] == tag) {
- if (idx == KASAN_EXTRA_FREE_INFO_COUNT)
- return &alloc_meta->free_track;
- else
- return &free_info->free_track[idx];
- }
- }
- if (free_info->free_track_tail == KASAN_EXTRA_FREE_INFO_COUNT)
- return &alloc_meta->free_track;
- else
- return &free_info->free_track[free_info->free_track_tail];
-}
-
-char *kasan_get_corruption_type(void *addr)
-{
- struct kasan_alloc_meta *alloc_meta;
- struct extra_free_info *free_info;
- struct page *page;
- struct kmem_cache *cache;
- void *object;
- u8 tag;
- int idx, i;
-
- tag = get_tag(addr);
- addr = reset_tag(addr);
- page = addr_to_page(addr);
- if (page && PageSlab(page)) {
- cache = page->slab_cache;
- object = nearest_obj(cache, page, addr);
- alloc_meta = get_alloc_info(cache, object);
- free_info = &alloc_meta->free_info;
-
- for (i = 0; i < KASAN_TOTAL_FREE_INFO_COUNT; i++) {
- idx = free_info->free_track_tail + i;
- if (idx >= KASAN_TOTAL_FREE_INFO_COUNT)
- idx -= KASAN_TOTAL_FREE_INFO_COUNT;
-
- if (free_info->free_pointer_tag[idx] == tag)
- return "use-after-free";
- }
- return "out-of-bounds";
- }
- return "invalid-access";
-}
-#endif
diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c
index 6d8cdb91c4b6..969ae08f59d7 100644
--- a/mm/kasan/tags_report.c
+++ b/mm/kasan/tags_report.c
@@ -36,10 +36,31 @@

const char *get_bug_type(struct kasan_access_info *info)
{

- if (IS_ENABLED(CONFIG_KASAN_SW_TAGS_IDENTIFY))
- return(kasan_get_corruption_type((void *)info->access_addr));
- else
- return "invalid-access";
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ struct kasan_alloc_meta *alloc_meta;
+ struct kmem_cache *cache;
+ struct page *page;
+ const void *addr;

+ void *object;
+ u8 tag;

+ int i;
+
+ tag = get_tag(info->access_addr);
+ addr = reset_tag(info->access_addr);
+ page = kasan_addr_to_page(addr);

+ if (page && PageSlab(page)) {
+ cache = page->slab_cache;

+ object = nearest_obj(cache, page, (void *)addr);

+ alloc_meta = get_alloc_info(cache, object);
+

+ for (i = 0; i < KASAN_NR_FREE_STACKS; i++)
+ if (alloc_meta->free_pointer_tag[i] == tag)
+ return "use-after-free";
+ return "out-of-bounds";
+ }
+
+#endif

[Andrey Ryabinin]

From: Walter Wu <walter...@mediatek.com>

This patch adds memory corruption identification at bug report for
software tag-based mode, the report show whether it is "use-after-free"
or "out-of-bound" error instead of "invalid-access" error. This will make
it easier for programmers to see the memory corruption problem.

We extend the slab to store five old free pointer tag and free backtrace,
we can check if the tagged address is in the slab record and make a
good guess if the object is more like "use-after-free" or "out-of-bound".
therefore every slab memory corruption can be identified whether it's
"use-after-free" or "out-of-bound".

[arya...@virtuozzo.com: simplify & clenup code:
https://lkml.kernel.org/r/3318f9d7-a760-3cc8...@virtuozzo.com]
Signed-off-by: Walter Wu <walter...@mediatek.com>
Signed-off-by: Andrey Ryabinin <arya...@virtuozzo.com>
---

====== Changes
Change since v1:
- add feature option CONFIG_KASAN_SW_TAGS_IDENTIFY.
- change QUARANTINE_FRACTION to reduce quarantine size.
- change the qlist order in order to find the newest object in quarantine
- reduce the number of calling kmalloc() from 2 to 1 time.
- remove global variable to use argument to pass it.
- correct the amount of qobject cache->size into the byes of qlist_head.
- only use kasan_cache_shrink() to shink memory.

Change since v2:
- remove the shinking memory function kasan_cache_shrink()
- modify the description of the CONFIG_KASAN_SW_TAGS_IDENTIFY
- optimize the quarantine_find_object() and qobject_free()
- fix the duplicating function name 3 times in the header.
- modify the function name set_track() to kasan_set_track()

Change since v3:
- change tag-based quarantine to extend slab to identify memory corruption

Changes since v4:
- Simplify and cleanup code.

lib/Kconfig.kasan | 8 ++++++++
mm/kasan/common.c | 22 +++++++++++++++++++--
mm/kasan/kasan.h | 14 +++++++++++++-
mm/kasan/report.c | 44 ++++++++++++++++++++++++++++++++----------
mm/kasan/tags_report.c | 24 +++++++++++++++++++++++
5 files changed, 99 insertions(+), 13 deletions(-)

diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 7fa97a8b5717..6c9682ce0254 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -134,6 +134,14 @@ config KASAN_S390_4_LEVEL_PAGING

to 3TB of RAM with KASan enabled). This options allows to force
4-level paging instead.

+config KASAN_SW_TAGS_IDENTIFY
+ bool "Enable memory corruption identification"
+ depends on KASAN_SW_TAGS
+ help

+ This option enables best-effort identification of bug type
+ (use-after-free or out-of-bounds) at the cost of increased
+ memory consumption.
+

config TEST_KASAN
tristate "Module for testing KASAN for bug detection"

depends on m && KASAN

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index 3b8cde0cb5b2..6814d6d6a023 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -304,7 +304,6 @@ size_t kasan_metadata_size(struct kmem_cache *cache)

struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
const void *object)
{

- BUILD_BUG_ON(sizeof(struct kasan_alloc_meta) > 32);
return (void *)object + cache->kasan_info.alloc_meta_offset;
}

@@ -315,6 +314,24 @@ struct kasan_free_meta *get_free_info(struct kmem_cache *cache,

return (void *)object + cache->kasan_info.free_meta_offset;
}

+
+static void kasan_set_free_info(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ struct kasan_alloc_meta *alloc_meta;
+ u8 idx = 0;
+
+ alloc_meta = get_alloc_info(cache, object);
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ idx = alloc_meta->free_track_idx;
+ alloc_meta->free_pointer_tag[idx] = tag;
+ alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
+#endif
+
+ set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
+}
+
void kasan_poison_slab(struct page *page)
{
unsigned long i;

@@ -451,7 +468,8 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object,

unlikely(!(cache->flags & SLAB_KASAN)))
return false;

- set_track(&get_alloc_info(cache, object)->free_track, GFP_NOWAIT);

+ kasan_set_free_info(cache, object, tag);
+
quarantine_put(get_free_info(cache, object), cache);

return IS_ENABLED(CONFIG_KASAN_GENERIC);

diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 014f19e76247..35cff6bbb716 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -95,9 +95,19 @@ struct kasan_track {
depot_stack_handle_t stack;
};

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY

+#define KASAN_NR_FREE_STACKS 5
+#else
+#define KASAN_NR_FREE_STACKS 1

+#endif
+

struct kasan_alloc_meta {
struct kasan_track alloc_track;
- struct kasan_track free_track;
+ struct kasan_track free_track[KASAN_NR_FREE_STACKS];

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY

+ u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
+ u8 free_track_idx;

+#endif
};

struct qlist_node {
@@ -146,6 +156,8 @@ void kasan_report(unsigned long addr, size_t size,

bool is_write, unsigned long ip);
void kasan_report_invalid_free(void *object, unsigned long ip);

+struct page *kasan_addr_to_page(const void *addr);
+

#if defined(CONFIG_KASAN_GENERIC) && \
(defined(CONFIG_SLAB) || defined(CONFIG_SLUB))

void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);

diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 0e5f965f1882..621782100eaa 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -111,7 +111,7 @@ static void print_track(struct kasan_track *track, const char *prefix)

}
}

-static struct page *addr_to_page(const void *addr)

+struct page *kasan_addr_to_page(const void *addr)

{

if ((addr >= (void *)PAGE_OFFSET) &&

(addr < high_memory))
@@ -151,15 +151,38 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,

(void *)(object_addr + cache->object_size));
}

+static struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+ void *object, u8 tag)
+{
+ struct kasan_alloc_meta *alloc_meta;
+ int i = 0;
+
+ alloc_meta = get_alloc_info(cache, object);
+
+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
+ if (alloc_meta->free_pointer_tag[i] == tag)
+ break;
+ }
+ if (i == KASAN_NR_FREE_STACKS)
+ i = alloc_meta->free_track_idx;
+#endif
+
+ return &alloc_meta->free_track[i];
+}
+
static void describe_object(struct kmem_cache *cache, void *object,

- const void *addr)

+ const void *addr, u8 tag)
{

struct kasan_alloc_meta *alloc_info = get_alloc_info(cache, object);

if (cache->flags & SLAB_KASAN) {
+ struct kasan_track *free_track;
+
print_track(&alloc_info->alloc_track, "Allocated");
pr_err("\n");

- print_track(&alloc_info->free_track, "Freed");

+ free_track = kasan_get_free_track(cache, object, tag);
+ print_track(free_track, "Freed");

pr_err("\n");
}

@@ -344,9 +367,9 @@ static void print_address_stack_frame(const void *addr)
print_decoded_frame_descr(frame_descr);
}

-static void print_address_description(void *addr)

+static void print_address_description(void *addr, u8 tag)
{

- struct page *page = addr_to_page(addr);

+ struct page *page = kasan_addr_to_page(addr);

dump_stack();
pr_err("\n");

@@ -355,7 +378,7 @@ static void print_address_description(void *addr)

struct kmem_cache *cache = page->slab_cache;

void *object = nearest_obj(cache, page, addr);

- describe_object(cache, object, addr);

+ describe_object(cache, object, addr, tag);
}

if (kernel_or_module_addr(addr) && !init_task_stack_addr(addr)) {
@@ -435,13 +458,14 @@ static bool report_enabled(void)

void kasan_report_invalid_free(void *object, unsigned long ip)
{
unsigned long flags;
+ u8 tag = get_tag(object);

+ object = reset_tag(object);
start_report(&flags);
pr_err("BUG: KASAN: double-free or invalid-free in %pS\n", (void *)ip);
- print_tags(get_tag(object), reset_tag(object));

- object = reset_tag(object);

+ print_tags(tag, object);
pr_err("\n");
- print_address_description(object);
+ print_address_description(object, tag);
pr_err("\n");

print_shadow_for_address(object);
end_report(&flags);
@@ -479,7 +503,7 @@ void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned lon

pr_err("\n");

if (addr_has_shadow(untagged_addr)) {

- print_address_description(untagged_addr);

+ print_address_description(untagged_addr, get_tag(tagged_addr));
pr_err("\n");
print_shadow_for_address(info.first_bad_addr);
} else {

diff --git a/mm/kasan/tags_report.c b/mm/kasan/tags_report.c
index 8eaf5f722271..969ae08f59d7 100644
--- a/mm/kasan/tags_report.c
+++ b/mm/kasan/tags_report.c
@@ -36,6 +36,30 @@

const char *get_bug_type(struct kasan_access_info *info)
{

+#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+ struct kasan_alloc_meta *alloc_meta;
+ struct kmem_cache *cache;
+ struct page *page;
+ const void *addr;
+ void *object;
+ u8 tag;
+ int i;
+
+ tag = get_tag(info->access_addr);
+ addr = reset_tag(info->access_addr);
+ page = kasan_addr_to_page(addr);
+ if (page && PageSlab(page)) {
+ cache = page->slab_cache;
+ object = nearest_obj(cache, page, (void *)addr);
+ alloc_meta = get_alloc_info(cache, object);
+
+ for (i = 0; i < KASAN_NR_FREE_STACKS; i++)
+ if (alloc_meta->free_pointer_tag[i] == tag)
+ return "use-after-free";
+ return "out-of-bounds";
+ }
+
+#endif

return "invalid-access";
}

--
2.21.0

[Walter Wu]

Thanks your review and suggestion.

Walter

[Andrey Konovalov]

I think we should keep the "invalid-access" bug type here if we failed
to identify the bug as a "use-after-free" (and change the patch
description accordingly).

Other than that:

Acked-by: Andrey Konovalov <andre...@google.com>

[Walter Wu]

Thanks your suggestion.
If slab records is not found, it may be use-after-free or out-of-bounds.
Maybe We can think how to avoid the situation(check object range or
other?), if possible, I will send patch or adopt your suggestion
modification.

regards,
Walter