[PATCH 0/5] mm/kasan: advanced check

[PATCH 0/5] mm/kasan: advanced check

[Wengang Wang]

Kasan advanced check, I'm going to add this feature.
Currently Kasan provide the detection of use-after-free and out-of-bounds
problems. It is not able to find the overwrite-on-allocated-memory issue.
We sometimes hit this kind of issue: We have a messed up structure
(usually dynamially allocated), some of the fields in the structure were
overwritten with unreasaonable values. And kernel may panic due to those
overeritten values. We know those fields were overwritten somehow, but we
have no easy way to find out which path did the overwritten. The advanced
check wants to help in this scenario.

The idea is to define the memory owner. When write accesses come from
non-owner, error should be reported. Normally the write accesses on a given
structure happen in only several or a dozen of functions if the structure
is not that complicated. We call those functions "allowed functions".
The work of defining the owner and binding memory to owner is expected to
be done by the memory consumer. In the above case, memory consume register
the owner as the functions which have write accesses to the structure then
bind all the structures to the owner. Then kasan will do the "owner check"
after the basic checks.

As implementation, kasan provides a API to it's user to register their
allowed functions. The API returns a token to users.  At run time, users
bind the memory ranges they are interested in to the check they registered.
Kasan then checks the bound memory ranges with the allowed functions.


Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

0001-mm-kasan-make-space-in-shadow-bytes-for-advanced-che.patch
0002-mm-kasan-pass-access-mode-to-poison-check-functions.patch
0003-mm-kasan-do-advanced-check.patch
0004-mm-kasan-register-check-and-bind-it-to-memory.patch
0005-mm-kasan-add-advanced-check-test-case.patch

 include/linux/kasan.h |   16 ++
 lib/test_kasan.c      |   73 ++++++++++++
 mm/kasan/kasan.c      |  292 +++++++++++++++++++++++++++++++++++++++++++-------
 mm/kasan/kasan.h      |   42 +++++++
 mm/kasan/report.c     |   44 ++++++-
 5 files changed, 424 insertions(+), 43 deletions(-)

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

[PATCH 1/5] mm/kasan: make space in shadow bytes for advanced check

[Wengang Wang]

Kasan advanced check, I'm going to add this feature.
Currently Kasan provide the detection of use-after-free and out-of-bounds
problems. It is not able to find the overwrite-on-allocated-memory issue.
We sometimes hit this kind of issue: We have a messed up structure
(dynamially allocated), some of the fields in the structure were
overwritten with unreasaonable values. We know those fields were
overwritten somehow, but we have no easy way to find out which path did the
overwritten. The advanced check wants to help in this scenario.

Normally the write accesses on a given structure happen in only several or
a dozen of functions if the structure is not that complicated. We call
those functions "allowed functions". The idea is that we check if the write
accesses are from the allowed functions and report error accordingly.

As implementation, kasan provides a API to it's user to register their
allowed functions. The API returns a token to users.  At run time, users
bind the memory ranges they are interested in to the check they registered.
Kasan then checks the bound memory ranges with the allowed functions.

This is the first patch in the series it makes room for check in shadow
bytes.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 6f319fb..060ed72 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -128,7 +128,8 @@ static __always_inline bool memory_is_poisoned_1(unsigned long addr)
 
 	if (unlikely(shadow_value)) {
 		s8 last_accessible_byte = addr & KASAN_SHADOW_MASK;
-		return unlikely(last_accessible_byte >= shadow_value);
+		return unlikely(last_accessible_byte >=
+				KASAN_GET_POISON(shadow_value));
 	}
 
 	return false;
@@ -144,7 +145,8 @@ static __always_inline bool memory_is_poisoned_2_4_8(unsigned long addr,
 	 * into 2 shadow bytes, so we need to check them both.
 	 */
 	if (unlikely(((addr + size - 1) & KASAN_SHADOW_MASK) < size - 1))
-		return *shadow_addr || memory_is_poisoned_1(addr + size - 1);
+		return KASAN_GET_POISON(*shadow_addr) ||
+		       memory_is_poisoned_1(addr + size - 1);
 
 	return memory_is_poisoned_1(addr + size - 1);
 }
@@ -155,7 +157,8 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
 
 	/* Unaligned 16-bytes access maps into 3 shadow bytes. */
 	if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
-		return *shadow_addr || memory_is_poisoned_1(addr + 15);
+		return KASAN_GET_POISON_16(*shadow_addr) ||
+		       memory_is_poisoned_1(addr + 15);
 
 	return *shadow_addr;
 }
@@ -164,7 +167,7 @@ static __always_inline unsigned long bytes_is_nonzero(const u8 *start,
 					size_t size)
 {
 	while (size) {
-		if (unlikely(*start))
+		if (unlikely(KASAN_GET_POISON(*start)))
 			return (unsigned long)start;
 		start++;
 		size--;
@@ -193,7 +196,7 @@ static __always_inline unsigned long memory_is_nonzero(const void *start,
 
 	words = (end - start) / 8;
 	while (words) {
-		if (unlikely(*(u64 *)start))
+		if (unlikely(KASAN_GET_POISON_64(*(u64 *)start)))
 			return bytes_is_nonzero(start, 8);
 		start += 8;
 		words--;
@@ -215,7 +218,8 @@ static __always_inline bool memory_is_poisoned_n(unsigned long addr,
 		s8 *last_shadow = (s8 *)kasan_mem_to_shadow((void *)last_byte);
 
 		if (unlikely(ret != (unsigned long)last_shadow ||
-			((long)(last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
+			((long)(last_byte & KASAN_SHADOW_MASK) >=
+			KASAN_GET_POISON(*last_shadow))))
 			return true;
 	}
 	return false;
@@ -504,13 +508,15 @@ static void kasan_poison_slab_free(struct kmem_cache *cache, void *object)
 bool kasan_slab_free(struct kmem_cache *cache, void *object)
 {
 	s8 shadow_byte;
+	s8 poison;
 
 	/* RCU slabs could be legally used after free within the RCU period */
 	if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
 		return false;
 
 	shadow_byte = READ_ONCE(*(s8 *)kasan_mem_to_shadow(object));
-	if (shadow_byte < 0 || shadow_byte >= KASAN_SHADOW_SCALE_SIZE) {
+	poison = KASAN_GET_POISON(shadow_byte);
+	if (poison < 0 || poison >= KASAN_SHADOW_SCALE_SIZE) {
 		kasan_report_double_free(cache, object,
 				__builtin_return_address(1));
 		return true;
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index c70851a..df7fbfe 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -8,6 +8,18 @@
 #define KASAN_SHADOW_SCALE_SIZE (1UL << KASAN_SHADOW_SCALE_SHIFT)
 #define KASAN_SHADOW_MASK       (KASAN_SHADOW_SCALE_SIZE - 1)
 
+/* We devide one shadow byte into two parts: "check" and "poison".
+ * "check" is a used for advanced check.
+ * "poison" is used to store the foot print of the tracked memory,
+ * For a paticular address, one extra check is enough. So we can have up to
+ * (1 << (KASAN_CHECK_BITS) - 1) - 1 checks. That's 0b001 to 0b110 (0b111 is
+ * reserved for poison values)
+ *
+ * The bits occupition in shadow bytes (P for poison, C for check):
+ *
+ * |P|C|C|C|P|P|P|P|
+ *
+ */
 #define KASAN_FREE_PAGE         0xFF  /* page was freed */
 #define KASAN_PAGE_REDZONE      0xFE  /* redzone for kmalloc_large allocations */
 #define KASAN_KMALLOC_REDZONE   0xFC  /* redzone inside slub object */
@@ -29,6 +41,19 @@
 #define KASAN_ABI_VERSION 1
 #endif
 
+#define KASAN_POISON_MASK	0x8F
+#define KASAN_POISON_MASK_16	0x8F8F
+#define KASAN_POISON_MASK_64	0x8F8F8F8F8F8F8F8F
+#define KASAN_CHECK_MASK	0x70
+#define KASAN_CHECK_SHIFT	4
+#define KASAN_CHECK_BITS	3
+
+#define KASAN_GET_POISON(val) ((s8)((val) & KASAN_POISON_MASK))
+#define KASAN_CHECK_LOWMASK (KASAN_CHECK_MASK >> KASAN_CHECK_SHIFT)
+/* 16 bits and 64 bits version */
+#define KASAN_GET_POISON_16(val) ((val) & KASAN_POISON_MASK_16)
+#define KASAN_GET_POISON_64(val) ((val) & KASAN_POISON_MASK_64)
+
 struct kasan_access_info {
 	const void *access_addr;
 	const void *first_bad_addr;
@@ -113,4 +138,11 @@ static inline void quarantine_reduce(void) { }
 static inline void quarantine_remove_cache(struct kmem_cache *cache) { }
 #endif
 
+static inline u8 kasan_get_check(u8 val)
+{
+	val &= KASAN_CHECK_MASK;
+	val >>= KASAN_CHECK_SHIFT;
+
+	return (val ^ KASAN_CHECK_LOWMASK) ?  val : 0;
+}
 #endif
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 6bcfb01..caf3a13 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -61,20 +61,24 @@ static const char *get_shadow_bug_type(struct kasan_access_info *info)
 {
 	const char *bug_type = "unknown-crash";
 	u8 *shadow_addr;
+	s8 poison;
 
 	info->first_bad_addr = find_first_bad_addr(info->access_addr,
 						info->access_size);
 
 	shadow_addr = (u8 *)kasan_mem_to_shadow(info->first_bad_addr);
-
+	poison = KASAN_GET_POISON(*shadow_addr);
 	/*
 	 * If shadow byte value is in [0, KASAN_SHADOW_SCALE_SIZE) we can look
 	 * at the next shadow byte to determine the type of the bad access.
 	 */
-	if (*shadow_addr > 0 && *shadow_addr <= KASAN_SHADOW_SCALE_SIZE - 1)
-		shadow_addr++;
+	if (poison > 0 && poison <= KASAN_SHADOW_SCALE_SIZE - 1)
+		poison = KASAN_GET_POISON(*(shadow_addr + 1));
+
+	if (poison < 0)
+		poison |= KASAN_CHECK_MASK;
 
-	switch (*shadow_addr) {
+	switch ((u8)poison) {
 	case 0 ... KASAN_SHADOW_SCALE_SIZE - 1:
 		/*
 		 * In theory it's still possible to see these shadow values
-- 
2.9.4

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

[PATCH 2/5] mm/kasan: pass access mode to poison check functions

[Wengang Wang]

This is the second patch for the Kasan advanced check feature.
The advanced check would need access mode to make decision.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 060ed72..4501422 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -122,7 +122,7 @@ void kasan_unpoison_stack_above_sp_to(const void *watermark)
  * depending on memory access size X.
  */
 
-static __always_inline bool memory_is_poisoned_1(unsigned long addr)
+static __always_inline bool memory_is_poisoned_1(unsigned long addr, bool write)
 {
 	s8 shadow_value = *(s8 *)kasan_mem_to_shadow((void *)addr);
 
@@ -136,7 +136,8 @@ static __always_inline bool memory_is_poisoned_1(unsigned long addr)
 }
 
 static __always_inline bool memory_is_poisoned_2_4_8(unsigned long addr,
-						unsigned long size)
+						     unsigned long size,
+						     bool write)
 {
 	u8 *shadow_addr = (u8 *)kasan_mem_to_shadow((void *)addr);
 
@@ -146,25 +147,27 @@ static __always_inline bool memory_is_poisoned_2_4_8(unsigned long addr,
 	 */
 	if (unlikely(((addr + size - 1) & KASAN_SHADOW_MASK) < size - 1))
 		return KASAN_GET_POISON(*shadow_addr) ||
-		       memory_is_poisoned_1(addr + size - 1);
+		       memory_is_poisoned_1(addr + size - 1, write);
 
-	return memory_is_poisoned_1(addr + size - 1);
+	return memory_is_poisoned_1(addr + size - 1, write);
 }
 
-static __always_inline bool memory_is_poisoned_16(unsigned long addr)
+static __always_inline bool memory_is_poisoned_16(unsigned long addr,
+						  bool write)
 {
 	u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
 
 	/* Unaligned 16-bytes access maps into 3 shadow bytes. */
 	if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
 		return KASAN_GET_POISON_16(*shadow_addr) ||
-		       memory_is_poisoned_1(addr + 15);
+		       memory_is_poisoned_1(addr + 15, write);
 
 	return *shadow_addr;
 }
 
 static __always_inline unsigned long bytes_is_nonzero(const u8 *start,
-					size_t size)
+						      size_t size,
+						      bool write)
 {
 	while (size) {
 		if (unlikely(KASAN_GET_POISON(*start)))
@@ -177,18 +180,19 @@ static __always_inline unsigned long bytes_is_nonzero(const u8 *start,
 }
 
 static __always_inline unsigned long memory_is_nonzero(const void *start,
-						const void *end)
+						       const void *end,
+						       bool write)
 {
 	unsigned int words;
 	unsigned long ret;
 	unsigned int prefix = (unsigned long)start % 8;
 
 	if (end - start <= 16)
-		return bytes_is_nonzero(start, end - start);
+		return bytes_is_nonzero(start, end - start, write);
 
 	if (prefix) {
 		prefix = 8 - prefix;
-		ret = bytes_is_nonzero(start, prefix);
+		ret = bytes_is_nonzero(start, prefix, write);
 		if (unlikely(ret))
 			return ret;
 		start += prefix;
@@ -197,21 +201,23 @@ static __always_inline unsigned long memory_is_nonzero(const void *start,
 	words = (end - start) / 8;
 	while (words) {
 		if (unlikely(KASAN_GET_POISON_64(*(u64 *)start)))
-			return bytes_is_nonzero(start, 8);
+			return bytes_is_nonzero(start, 8, write);
 		start += 8;
 		words--;
 	}
 
-	return bytes_is_nonzero(start, (end - start) % 8);
+	return bytes_is_nonzero(start, (end - start) % 8, write);
 }
 
 static __always_inline bool memory_is_poisoned_n(unsigned long addr,
-						size_t size)
+						 size_t size,
+						 bool write)
 {
 	unsigned long ret;
 
 	ret = memory_is_nonzero(kasan_mem_to_shadow((void *)addr),
-			kasan_mem_to_shadow((void *)addr + size - 1) + 1);
+			kasan_mem_to_shadow((void *)addr + size - 1) + 1,
+			write);
 
 	if (unlikely(ret)) {
 		unsigned long last_byte = addr + size - 1;
@@ -225,24 +231,25 @@ static __always_inline bool memory_is_poisoned_n(unsigned long addr,
 	return false;
 }
 
-static __always_inline bool memory_is_poisoned(unsigned long addr, size_t size)
+static __always_inline bool memory_is_poisoned(unsigned long addr, size_t size,
+					       bool write)
 {
 	if (__builtin_constant_p(size)) {
 		switch (size) {
 		case 1:
-			return memory_is_poisoned_1(addr);
+			return memory_is_poisoned_1(addr, write);
 		case 2:
 		case 4:
 		case 8:
-			return memory_is_poisoned_2_4_8(addr, size);
+			return memory_is_poisoned_2_4_8(addr, size, write);
 		case 16:
-			return memory_is_poisoned_16(addr);
+			return memory_is_poisoned_16(addr, write);
 		default:
 			BUILD_BUG();
 		}
 	}
 
-	return memory_is_poisoned_n(addr, size);
+	return memory_is_poisoned_n(addr, size, write);
 }
 
 static __always_inline void check_memory_region_inline(unsigned long addr,
@@ -258,7 +265,7 @@ static __always_inline void check_memory_region_inline(unsigned long addr,
 		return;
 	}
 
-	if (likely(!memory_is_poisoned(addr, size)))
+	if (likely(!memory_is_poisoned(addr, size, write)))
 		return;
 
 	kasan_report(addr, size, write, ret_ip);
-- 
2.9.4

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

[PATCH 3/5] mm/kasan: do advanced check

[Wengang Wang]

This is the 3rd patch in the Kasan advanced check feature.
It does advanced check in the poison check functions and report for
advanced check.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index 5017269..ba00594 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -16,6 +16,13 @@ struct task_struct;
 #include <asm/kasan.h>
 #include <asm/pgtable.h>
 
+/* advanced check type */
+enum kasan_adv_chk_type {
+	/* write access is allowed only for the owner */
+	KASAN_ADVCHK_OWNER,
+	__KASAN_ADVCHK_TYPE_COUNT,
+};
+
 extern unsigned char kasan_zero_page[PAGE_SIZE];
 extern pte_t kasan_zero_pte[PTRS_PER_PTE];
 extern pmd_t kasan_zero_pmd[PTRS_PER_PMD];
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 4501422..e945df7 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -40,6 +40,51 @@
 #include "kasan.h"
 #include "../slab.h"
 
+struct kasan_adv_check kasan_adv_checks[(1 << KASAN_CHECK_BITS)-2];
+static int kasan_adv_nr_checks;
+static DEFINE_SPINLOCK(kasan_adv_lock);
+
+/* we don't take lock kasan_adv_lock. Locking can either cause deadload
+ * or kill the performance further.
+ * We are still safe without lock since kasan_adv_nr_checks increases only.
+ * The worst and rare case is kasan_adv_nr_checks is stale (smaller than it
+ * really is) and we miss a check.
+ */
+struct kasan_adv_check *get_check_by_nr(int nr)
+{
+	if (nr > kasan_adv_nr_checks || nr <= 0)
+		return NULL;
+	return &kasan_adv_checks[nr-1];
+}
+
+static __always_inline bool adv_check(bool write, s8 check)
+{
+	struct kasan_adv_check *chk = get_check_by_nr(check);
+
+	if (likely(chk)) {
+		bool violation = chk->ac_check_func(write, chk->ac_data);
+
+		if (unlikely(violation))
+			chk->ac_violation = violation;
+		return violation;
+	}
+	return false;
+}
+
+static __always_inline unsigned long adv_check_shadow(const s8 *shadow_addr,
+					     size_t shadow_size, bool write)
+{
+	s8 check;
+	int i;
+
+	for (i = 0; i < shadow_size; i++) {
+		check = kasan_get_check(*(shadow_addr + i));
+		if (unlikely(check && adv_check(write, check)))
+			return (unsigned long)(shadow_addr + i);
+	}
+	return 0;
+}
+
 void kasan_enable_current(void)
 {
 	current->kasan_depth++;
@@ -128,8 +173,11 @@ static __always_inline bool memory_is_poisoned_1(unsigned long addr, bool write)
 
 	if (unlikely(shadow_value)) {
 		s8 last_accessible_byte = addr & KASAN_SHADOW_MASK;
-		return unlikely(last_accessible_byte >=
-				KASAN_GET_POISON(shadow_value));
+		if (unlikely(KASAN_GET_POISON(shadow_value) &&
+			last_accessible_byte >= KASAN_GET_POISON(shadow_value)))
+			return true;
+		if (unlikely(kasan_get_check(shadow_value)))
+			return adv_check(write, kasan_get_check(shadow_value));
 	}
 
 	return false;
@@ -145,9 +193,14 @@ static __always_inline bool memory_is_poisoned_2_4_8(unsigned long addr,
 	 * Access crosses 8(shadow size)-byte boundary. Such access maps
 	 * into 2 shadow bytes, so we need to check them both.
 	 */
-	if (unlikely(((addr + size - 1) & KASAN_SHADOW_MASK) < size - 1))
-		return KASAN_GET_POISON(*shadow_addr) ||
-		       memory_is_poisoned_1(addr + size - 1, write);
+	if (unlikely(((addr + size - 1) & KASAN_SHADOW_MASK) < size - 1)) {
+		u8 check = kasan_get_check(*shadow_addr);
+
+		if (unlikely(KASAN_GET_POISON(*shadow_addr)))
+			return true;
+		if (unlikely(check && adv_check(write, check)))
+			return true;
+	}
 
 	return memory_is_poisoned_1(addr + size - 1, write);
 }
@@ -157,21 +210,31 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr,
 {
 	u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
 
-	/* Unaligned 16-bytes access maps into 3 shadow bytes. */
-	if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
-		return KASAN_GET_POISON_16(*shadow_addr) ||
-		       memory_is_poisoned_1(addr + 15, write);
+	if (unlikely(KASAN_GET_POISON_16(*shadow_addr)))
+		return true;
+
+	if (unlikely(adv_check_shadow((s8 *)shadow_addr, 2, write)))
+		return true;
 
-	return *shadow_addr;
+	if (likely(IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
+		return false;
+
+	/* Unaligned 16-bytes access maps into 3 shadow bytes. */
+	return memory_is_poisoned_1(addr + 15, write);
 }
 
 static __always_inline unsigned long bytes_is_nonzero(const u8 *start,
 						      size_t size,
 						      bool write)
 {
+	int check;
+
 	while (size) {
 		if (unlikely(KASAN_GET_POISON(*start)))
 			return (unsigned long)start;
+		check = kasan_get_check(*start);
+		if (unlikely(check && adv_check(write, check)))
+			return (unsigned long)start;
 		start++;
 		size--;
 	}
@@ -202,6 +265,9 @@ static __always_inline unsigned long memory_is_nonzero(const void *start,
 	while (words) {
 		if (unlikely(KASAN_GET_POISON_64(*(u64 *)start)))
 			return bytes_is_nonzero(start, 8, write);
+		ret = adv_check_shadow(start, sizeof(u64), write);
+		if (unlikely(ret))
+			return (unsigned long)ret;
 		start += 8;
 		words--;
 	}
@@ -227,6 +293,11 @@ static __always_inline bool memory_is_poisoned_n(unsigned long addr,
 			((long)(last_byte & KASAN_SHADOW_MASK) >=
 			KASAN_GET_POISON(*last_shadow))))
 			return true;
+		else {
+			s8 check = kasan_get_check(*last_shadow);
+
+			return unlikely(check && adv_check(write, check));
+		}
 	}
 	return false;
 }
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index df7fbfe..2e2af6d 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -111,6 +111,16 @@ struct kasan_free_meta {
 	struct qlist_node quarantine_link;
 };
 
+struct kasan_adv_check {
+	enum kasan_adv_chk_type	ac_type;
+	bool			(*ac_check_func)(bool, void *);
+	void			*ac_data;
+	char			*ac_msg;
+	bool			ac_violation;
+};
+
+extern struct kasan_adv_check *get_check_by_nr(int nr);
+
 struct kasan_alloc_meta *get_alloc_info(struct kmem_cache *cache,
 					const void *object);
 struct kasan_free_meta *get_free_info(struct kmem_cache *cache,
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index caf3a13..403bae1 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -57,10 +57,26 @@ static bool addr_has_shadow(struct kasan_access_info *info)
 		kasan_shadow_to_mem((void *)KASAN_SHADOW_START));
 }
 
+static bool is_clean_byte(u8 shadow_val)
+{
+	u8 poison = KASAN_GET_POISON(shadow_val);
+	u8 check = kasan_get_check(shadow_val);
+
+	if (poison > 0 && poison <= KASAN_SHADOW_SCALE_SIZE - 1) {
+		struct kasan_adv_check *chk = get_check_by_nr(check);
+
+		if (chk && chk->ac_violation)
+			return false;
+		return true;
+	}
+
+	return false;
+}
+
 static const char *get_shadow_bug_type(struct kasan_access_info *info)
 {
 	const char *bug_type = "unknown-crash";
-	u8 *shadow_addr;
+	u8 *shadow_addr, check;
 	s8 poison;
 
 	info->first_bad_addr = find_first_bad_addr(info->access_addr,
@@ -68,12 +84,15 @@ static const char *get_shadow_bug_type(struct kasan_access_info *info)
 
 	shadow_addr = (u8 *)kasan_mem_to_shadow(info->first_bad_addr);
 	poison = KASAN_GET_POISON(*shadow_addr);
+	check = kasan_get_check(*shadow_addr);
 	/*
 	 * If shadow byte value is in [0, KASAN_SHADOW_SCALE_SIZE) we can look
 	 * at the next shadow byte to determine the type of the bad access.
 	 */
-	if (poison > 0 && poison <= KASAN_SHADOW_SCALE_SIZE - 1)
+	if (is_clean_byte(*shadow_addr)) {
 		poison = KASAN_GET_POISON(*(shadow_addr + 1));
+		check = check = kasan_get_check(*(shadow_addr + 1));
+	}
 
 	if (poison < 0)
 		poison |= KASAN_CHECK_MASK;
@@ -108,6 +127,15 @@ static const char *get_shadow_bug_type(struct kasan_access_info *info)
 		break;
 	}
 
+	if (check) {
+		struct kasan_adv_check *chk = get_check_by_nr(check);
+
+		if (chk && chk->ac_violation) {
+			bug_type = chk->ac_msg;
+			chk->ac_violation = false;
+		}
+	}
+
 	return bug_type;
 }
 
-- 
2.9.4

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

[PATCH 4/5] mm/kasan: register check and bind it to memory

[Wengang Wang]

This is a part of the Kasan advanced check patch series. This patch
introduces "owner check". It defines the owner of the memory range as a
dozen of functions. Only those functions are allowed of write access to the
bound memory ranges. Write access from other functions are treated as
violation and corresponding error will be reported.

Two APIs are provided. One is used to register a check with owning
functions , the other binds memory ranges to the check. The check token is
stored in the "check" bits in shadow bytes. For each memory access, besides
does poison detects, kasan scans those "check" bits and does violation
check accordingly.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index ba00594..721da3e 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -16,6 +16,7 @@ struct task_struct;
 #include <asm/kasan.h>
 #include <asm/pgtable.h>
 
+#define KASAN_OWNER_MAX	32
 /* advanced check type */
 enum kasan_adv_chk_type {
 	/* write access is allowed only for the owner */
@@ -32,6 +33,11 @@ extern p4d_t kasan_zero_p4d[PTRS_PER_P4D];
 void kasan_populate_zero_shadow(const void *shadow_start,
 				const void *shadow_end);
 
+struct kasan_owner_set {
+	unsigned int	s_nr;	/* # of function pointers in the following */
+	void		*s_ptrs[KASAN_OWNER_MAX];
+};
+
 static inline void *kasan_mem_to_shadow(const void *addr)
 {
 	return (void *)((unsigned long)addr >> KASAN_SHADOW_SCALE_SHIFT)
@@ -87,6 +93,9 @@ size_t kasan_metadata_size(struct kmem_cache *cache);
 bool kasan_save_enable_multi_shot(void);
 void kasan_restore_multi_shot(bool enabled);
 
+extern int kasan_register_adv_check(unsigned int ac_type, void *p);
+extern int kasan_bind_adv_addr(void *addr, size_t size, u8 check);
+
 #else /* CONFIG_KASAN */
 
 static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index e945df7..753a285 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -36,6 +36,7 @@
 #include <linux/types.h>
 #include <linux/vmalloc.h>
 #include <linux/bug.h>
+#include <linux/kallsyms.h>
 
 #include "kasan.h"
 #include "../slab.h"
@@ -44,6 +45,139 @@ struct kasan_adv_check kasan_adv_checks[(1 << KASAN_CHECK_BITS)-2];
 static int kasan_adv_nr_checks;
 static DEFINE_SPINLOCK(kasan_adv_lock);
 
+/* owner of the memory, the allowed write-access functions. */
+struct kasan_adv_owner {
+	unsigned long	ao_start;	/* the start of the owning function */
+	unsigned long	ao_end;		/* the end of the owning function */
+};
+
+struct kasan_adv_owners {
+	int	ao_nr;	/* # of kasan_adv_owner in the following */
+	struct kasan_adv_owner ao_owners[KASAN_OWNER_MAX];
+};
+
+static __always_inline bool owner_check(bool write, void *p)
+{
+	if (write) {
+		struct kasan_adv_owners *owners;
+		struct kasan_adv_owner *o;
+		unsigned long caller;
+		int i;
+
+		owners = p;
+		caller = (unsigned long)__builtin_return_address(1);
+
+		for (i = 0; i < owners->ao_nr; i++) {
+			o = &owners->ao_owners[i];
+			if (caller >= o->ao_start && caller <= o->ao_end)
+				return false;
+		}
+		return true;
+	}
+	return false;
+}
+
+static struct kasan_adv_owners *create_new_owners(struct kasan_owner_set *s)
+{
+	struct kasan_adv_owners *owners;
+	struct kasan_adv_owner *owner;
+	unsigned int nr = s->s_nr, i;
+
+	if (nr > KASAN_OWNER_MAX)
+		return ERR_PTR(-EINVAL);
+
+	owners = kmalloc(sizeof(struct kasan_adv_owners), GFP_KERNEL);
+	if (!owners)
+		return ERR_PTR(-ENOMEM);
+
+	owners->ao_nr = nr;
+	for (i = 0; i < nr; i++) {
+		owner = &owners->ao_owners[i];
+		if (!kallsyms_lookup_size_offset((unsigned long)s->s_ptrs[i],
+						 &owner->ao_end, NULL)) {
+			kfree(owners);
+			return ERR_PTR(-EINVAL);
+		}
+		owner->ao_start = (unsigned long)s->s_ptrs[i];
+		owner->ao_end += owner->ao_start;
+	}
+
+	return owners;
+}
+/* don't call this in irq/soft-irq context */
+int kasan_register_adv_check(unsigned int ac_type, void *p)
+{
+	struct kasan_adv_owners *owners;
+	struct kasan_adv_check *pck;
+	int ret;
+
+	if (ac_type >= __KASAN_ADVCHK_TYPE_COUNT)
+		return -EINVAL;
+
+	spin_lock(&kasan_adv_lock);
+	if (kasan_adv_nr_checks >= KASAN_CHECK_LOWMASK - 1) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	pck = &kasan_adv_checks[kasan_adv_nr_checks];
+	pck->ac_violation = false;
+	switch (ac_type) {
+	case	KASAN_ADVCHK_OWNER:
+		pck->ac_check_func = owner_check;
+		owners = create_new_owners((struct kasan_owner_set *)p);
+		if (IS_ERR(owners)) {
+			ret = PTR_ERR(owners);
+			goto out;
+		}
+		pck->ac_data = owners;
+		pck->ac_msg = "Non-owner write access violation";
+		break;
+	}
+
+	ret = ++kasan_adv_nr_checks;
+out:
+	spin_unlock(&kasan_adv_lock);
+	return ret;
+}
+EXPORT_SYMBOL(kasan_register_adv_check);
+
+/* Bind memory to check. The 'check' parameter should be the one returned
+ * by kasan_register_adv_check. The really bound start is aligned with
+ * KASAN_SHADOW_SCALE_SIZE. The real start is aligned higher if it's not
+ * exact on the boundary; the end is aligned lower if it's not exactly on the
+ * boundary - 1.
+ *
+ * return negative if error happened; 0 if fully marked and 1 if partially.
+ */
+int kasan_bind_adv_addr(void *addr, size_t size, u8 check)
+{
+	unsigned long r_start = round_up((unsigned long)addr,
+					 KASAN_SHADOW_SCALE_SIZE);
+	unsigned long r_end = round_down(((unsigned long)addr + size),
+					 KASAN_SHADOW_SCALE_SIZE) - 1;
+	u8 *shadow_start, *shadow_end;
+
+	if (unlikely(check >= KASAN_CHECK_LOWMASK - 1 || check == 0))
+		return -EINVAL;
+
+	if (r_end < r_start)
+		return -EINVAL;
+
+	shadow_start = (u8 *) kasan_mem_to_shadow((void *)r_start);
+	shadow_end = (u8 *) kasan_mem_to_shadow((void *)r_end);
+	check <<= KASAN_CHECK_SHIFT;
+	while (shadow_start <= shadow_end) {
+		*shadow_start |= check;
+		shadow_start++;
+	}
+
+	if ((void *)r_start == addr && (void *)r_end == (addr + size - 1))
+		return 0;
+	return 1;
+}
+EXPORT_SYMBOL(kasan_bind_adv_addr);
+
 /* we don't take lock kasan_adv_lock. Locking can either cause deadload
  * or kill the performance further.
  * We are still safe without lock since kasan_adv_nr_checks increases only.
-- 
2.9.4

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

[PATCH 5/5] mm/kasan: add advanced check test case

[Wengang Wang]

This patch is for Kasan advanced check feature.
It adds the advanced check test case in lib/test_kasan.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index a25c976..0ff0101 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -473,6 +473,78 @@ static noinline void __init use_after_scope_test(void)
 	p[1023] = 1;
 }
 
+static noinline void funcA(char *addr)
+{
+	*addr = 'A';
+}
+
+static noinline void funcB(char *addr)
+{
+	*addr = 'B';
+}
+
+static noinline void funcC(char *addr)
+{
+	*addr = 'C';
+}
+
+static noinline void __init kasan_adv(void)
+{
+	struct kasan_owner_set set;
+	int check, ret;
+	char *p;
+
+	pr_info("kasan: advanced check\n");
+
+	set.s_nr = 2;
+	set.s_ptrs[0] = (void *)funcA;
+	set.s_ptrs[1] = (void *)funcC;
+	check = kasan_register_adv_check(KASAN_ADVCHK_OWNER, &set);
+	if (check <= 0) {
+		pr_err("kasan_register_adv_check failedd with %d\n", check);
+		return;
+	}
+
+	p = kmalloc(62, GFP_KERNEL);
+	if (!p) {
+		pr_err("kmalloc failed with 62 bytes request\n");
+		return;
+	}
+
+	ret = kasan_bind_adv_addr(p, 32, check);
+	if (ret < 0) {
+		pr_err("kasan_bind_adv_addr failed with %d\n", ret);
+		kfree(p);
+		return;
+	}
+
+	funcA(&p[12]);
+	funcB(&p[12]);
+	funcC(&p[12]);
+
+	set.s_nr = 1;
+	set.s_ptrs[0] = (void *)funcA;
+
+	check = kasan_register_adv_check(KASAN_ADVCHK_OWNER, &set);
+	if (check <= 0) {
+		pr_err("kasan_register_adv_check failed with %d\n", check);
+		kfree(p);
+		return;
+	}
+
+	ret = kasan_bind_adv_addr(p+32, 30, check);
+	if (ret < 0) {
+		pr_err("kasan_bind_adv_addr failed with %d\n", ret);
+		kfree(p);
+		return;
+	}
+
+	funcA(&p[42]);
+	funcB(&p[42]);
+	funcC(&p[42]);
+
+	kfree(p);
+}
 static int __init kmalloc_tests_init(void)
 {
 	/*
@@ -506,6 +578,7 @@ static int __init kmalloc_tests_init(void)
 	ksize_unpoisons_memory();
 	copy_user_test();
 	use_after_scope_test();
+	kasan_adv();
 
 	kasan_restore_multi_shot(multishot);
 
-- 
2.9.4

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Wengang Wang]

This patch seems only work for OUTLINE compile type. Anyway to make it 
work for INLINE type?

thanks,
Wengang


On 2017/11/17 14:30, Wengang Wang wrote:
> Kasan advanced check, I'm going to add this feature.
> Currently Kasan provide the detection of use-after-free and out-of-bounds
> problems. It is not able to find the overwrite-on-allocated-memory issue.
> We sometimes hit this kind of issue: We have a messed up structure
> (usually dynamially allocated), some of the fields in the structure were
> overwritten with unreasaonable values. And kernel may panic due to those
> overeritten values. We know those fields were overwritten somehow, but we
> have no easy way to find out which path did the overwritten. The advanced
> check wants to help in this scenario.
>
> The idea is to define the memory owner. When write accesses come from
> non-owner, error should be reported. Normally the write accesses on a given
> structure happen in only several or a dozen of functions if the structure
> is not that complicated. We call those functions "allowed functions".
> The work of defining the owner and binding memory to owner is expected to
> be done by the memory consumer. In the above case, memory consume register
> the owner as the functions which have write accesses to the structure then
> bind all the structures to the owner. Then kasan will do the "owner check"
> after the basic checks.
>
> As implementation, kasan provides a API to it's user to register their
> allowed functions. The API returns a token to users.  At run time, users
> bind the memory ranges they are interested in to the check they registered.
> Kasan then checks the bound memory ranges with the allowed functions.
>
>
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>
> 0001-mm-kasan-make-space-in-shadow-bytes-for-advanced-che.patch
> 0002-mm-kasan-pass-access-mode-to-poison-check-functions.patch
> 0003-mm-kasan-do-advanced-check.patch
> 0004-mm-kasan-register-check-and-bind-it-to-memory.patch
> 0005-mm-kasan-add-advanced-check-test-case.patch
>
>   include/linux/kasan.h |   16 ++
>   lib/test_kasan.c      |   73 ++++++++++++
>   mm/kasan/kasan.c      |  292 +++++++++++++++++++++++++++++++++++++++++++-------
>   mm/kasan/kasan.h      |   42 +++++++
>   mm/kasan/report.c     |   44 ++++++-
>   5 files changed, 424 insertions(+), 43 deletions(-)

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Dmitry Vyukov]

On Fri, Nov 17, 2017 at 11:30 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
> Kasan advanced check, I'm going to add this feature.
> Currently Kasan provide the detection of use-after-free and out-of-bounds
> problems. It is not able to find the overwrite-on-allocated-memory issue.
> We sometimes hit this kind of issue: We have a messed up structure
> (usually dynamially allocated), some of the fields in the structure were
> overwritten with unreasaonable values. And kernel may panic due to those
> overeritten values. We know those fields were overwritten somehow, but we
> have no easy way to find out which path did the overwritten. The advanced
> check wants to help in this scenario.
>
> The idea is to define the memory owner. When write accesses come from
> non-owner, error should be reported. Normally the write accesses on a given
> structure happen in only several or a dozen of functions if the structure
> is not that complicated. We call those functions "allowed functions".
> The work of defining the owner and binding memory to owner is expected to
> be done by the memory consumer. In the above case, memory consume register
> the owner as the functions which have write accesses to the structure then
> bind all the structures to the owner. Then kasan will do the "owner check"
> after the basic checks.
>
> As implementation, kasan provides a API to it's user to register their
> allowed functions. The API returns a token to users.  At run time, users
> bind the memory ranges they are interested in to the check they registered.
> Kasan then checks the bound memory ranges with the allowed functions.
>
>
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>
> 0001-mm-kasan-make-space-in-shadow-bytes-for-advanced-che.patch
> 0002-mm-kasan-pass-access-mode-to-poison-check-functions.patch
> 0003-mm-kasan-do-advanced-check.patch
> 0004-mm-kasan-register-check-and-bind-it-to-memory.patch
> 0005-mm-kasan-add-advanced-check-test-case.patch
>
>  include/linux/kasan.h |   16 ++
>  lib/test_kasan.c      |   73 ++++++++++++
>  mm/kasan/kasan.c      |  292 +++++++++++++++++++++++++++++++++++++++++++-------
>  mm/kasan/kasan.h      |   42 +++++++
>  mm/kasan/report.c     |   44 ++++++-
>  5 files changed, 424 insertions(+), 43 deletions(-)


+kasan-dev mailing list

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Joonsoo Kim]

On Fri, Nov 17, 2017 at 11:56:21PM +0100, Dmitry Vyukov wrote:
> On Fri, Nov 17, 2017 at 11:30 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
> > Kasan advanced check, I'm going to add this feature.
> > Currently Kasan provide the detection of use-after-free and out-of-bounds
> > problems. It is not able to find the overwrite-on-allocated-memory issue.
> > We sometimes hit this kind of issue: We have a messed up structure
> > (usually dynamially allocated), some of the fields in the structure were
> > overwritten with unreasaonable values. And kernel may panic due to those
> > overeritten values. We know those fields were overwritten somehow, but we
> > have no easy way to find out which path did the overwritten. The advanced
> > check wants to help in this scenario.
> >
> > The idea is to define the memory owner. When write accesses come from
> > non-owner, error should be reported. Normally the write accesses on a given
> > structure happen in only several or a dozen of functions if the structure
> > is not that complicated. We call those functions "allowed functions".
> > The work of defining the owner and binding memory to owner is expected to
> > be done by the memory consumer. In the above case, memory consume register
> > the owner as the functions which have write accesses to the structure then
> > bind all the structures to the owner. Then kasan will do the "owner check"
> > after the basic checks.
> >
> > As implementation, kasan provides a API to it's user to register their
> > allowed functions. The API returns a token to users.  At run time, users
> > bind the memory ranges they are interested in to the check they registered.
> > Kasan then checks the bound memory ranges with the allowed functions.
> >
> >
> > Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

Hello, Wengang.

Nice idea. I also think that we need this kind of debugging tool. It's very
hard to detect overwritten bugs.

In fact, I made a quite similar tool, valid access checker (A.K.A.
vchecker). See the following link.

https://github.com/JoonsooKim/linux/tree/vchecker-master-v0.3-next-20170106

Vchecker has some advanced features compared to yours.

1. Target object can be choosen at runtime by debugfs. It doesn't
require re-compile to register the target object.

2. It has another feature that checks the value stored in the object.
Usually, invalid writer stores odd value into the object and vchecker
can detect this case.

3. It has a callstack checker (memory owner checker in yours). It
checks all the callstack rather than just the caller. It's important
since invalid writer could call the parent function of owner function
and it would not be catched by checking just the caller.

4. The callstack checker is more automated. vchecker collects the valid
callstack by running the system.


FYI, I attach some commit descriptions of the vchecker.

    vchecker: store/report callstack of value writer
    
    The purpose of the value checker is finding invalid user writing
    invalid value at the moment that the value is written. However, there is
    a missing infrastructure that passes writing value to the checker
    since we temporarilly piggyback on the KASAN. So, we cannot easily
    detect this case in time.
    
    However, by following way, we can emulate similar effect.
    
    1. Store callstack when memory is written.
    2. If check is failed in next access, report previous write-access
    callstack
    
    It will caught offending user properly.
    
    
    Following output "Call trace: Invalid writer" part is the result
    of this patch. We find the invalid value at workfn+0x71 but report
    writer at workfn+0x61.
    
    [  133.024076] ==================================================================
    [  133.025576] BUG: VCHECKER: invalid access in workfn+0x71/0xc0 at addr ffff8800683dd6c8
    [  133.027196] Read of size 8 by task kworker/1:1/48
    [  133.028020] 0x8 0x10 value
    [  133.028020] 0xffff 4
    [  133.028020] Call trace: Invalid writer
    [  133.028020]
    [  133.028020] [<ffffffff81043b1b>] save_stack_trace+0x1b/0x20
    [  133.028020]
    [  133.028020] [<ffffffff812c0db9>] save_stack+0x39/0x70
    [  133.028020]
    [  133.028020] [<ffffffff812c0fe3>] check_value+0x43/0x80
    [  133.028020]
    [  133.028020] [<ffffffff812c1762>] vchecker_check+0x1c2/0x380
    [  133.028020]
    [  133.028020] [<ffffffff812be49d>] __asan_store8+0x8d/0xc0
    [  133.028020]
    [  133.028020] [<ffffffff815eadd1>] workfn+0x61/0xc0
    [  133.028020]
    [  133.028020] [<ffffffff810be3df>] process_one_work+0x28f/0x680
    [  133.028020]
    [  133.028020] [<ffffffff810bf272>] worker_thread+0xa2/0x870
    [  133.028020]
    [  133.028020] [<ffffffff810c86a5>] kthread+0x195/0x1e0
    [  133.028020]
    [  133.028020] [<ffffffff81b9d3d2>] ret_from_fork+0x22/0x30
    [  133.028020] CPU: 1 PID: 48 Comm: kworker/1:1 Not tainted 4.10.0-rc2-next-20170106+ #1179
    [  133.028020] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
    [  133.028020] Workqueue: events workfn
    [  133.028020] Call Trace:
    [  133.028020]  dump_stack+0x4d/0x63
    [  133.028020]  kasan_object_err+0x21/0x80
    [  133.028020]  vchecker_check+0x2af/0x380
    [  133.028020]  ? workfn+0x71/0xc0
    [  133.028020]  ? workfn+0x71/0xc0
    [  133.028020]  __asan_load8+0x87/0xb0
    [  133.028020]  workfn+0x71/0xc0
    [  133.028020]  process_one_work+0x28f/0x680
    [  133.028020]  worker_thread+0xa2/0x870
    [  133.028020]  kthread+0x195/0x1e0
    [  133.028020]  ? put_pwq_unlocked+0xc0/0xc0
    [  133.028020]  ? kthread_park+0xd0/0xd0
    [  133.028020]  ret_from_fork+0x22/0x30
    [  133.028020] Object at ffff8800683dd6c0, in cache vchecker_test size: 24
    [  133.028020] Allocated:
    [  133.028020] PID = 48


    vchecker: Add 'callstack' checker
    
    The callstack checker is to find invalid code paths accessing to a
    certain field in an object.  Currently it only saves all stack traces at
    the given offset.  Reporting will be added in the next patch.
    
    The below example checks callstack of anon_vma:
    
      # cd /sys/kernel/debug/vchecker
      # echo 0 8 > anon_vma/callstack  # offset 0, size 8
      # echo 1 > anon_vma/enable
    
      # cat anon_vma/callstack        # show saved callstacks
      0x0 0x8 callstack
      total: 42
      callstack #0
        anon_vma_fork+0x101/0x280
        copy_process.part.10+0x15ff/0x2a40
        _do_fork+0x155/0x7d0
        SyS_clone+0x19/0x20
        do_syscall_64+0xdf/0x460
        return_from_SYSCALL_64+0x0/0x7a
      ...


    vchecker: Support toggle on/off of callstack check
    
    By default, callstack checker only collects callchains.  When a user
    writes 'on' to the callstack file in debugfs, it checks and reports new
    callstacks.  Writing 'off' to disable it again.
    
      # cd /sys/kernel/debug/vchecker
      # echo 0 8 > anon_vma/callstack
      # echo 1 > anon_vma/enable
    
      ... (do some work to collect enough callstacks) ...
    
      # echo on > anon_vma/callstack
    

The reason I didn't submit the vchecker to mainline is that I didn't find
the case that this tool is useful in real life. Most of the system broken case
can be debugged by other ways. Do you see the real case that this tool is
helpful? If so, I think that vchecker is more appropriate to be upstreamed.
Could you share your opinion?

Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Dmitry Vyukov]

On Mon, Nov 20, 2017 at 2:50 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
> On Fri, Nov 17, 2017 at 11:56:21PM +0100, Dmitry Vyukov wrote:
>> On Fri, Nov 17, 2017 at 11:30 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
>> > Kasan advanced check, I'm going to add this feature.
>> > Currently Kasan provide the detection of use-after-free and out-of-bounds
>> > problems. It is not able to find the overwrite-on-allocated-memory issue.
>> > We sometimes hit this kind of issue: We have a messed up structure
>> > (usually dynamially allocated), some of the fields in the structure were
>> > overwritten with unreasaonable values. And kernel may panic due to those
>> > overeritten values. We know those fields were overwritten somehow, but we
>> > have no easy way to find out which path did the overwritten. The advanced
>> > check wants to help in this scenario.
>> >
>> > The idea is to define the memory owner. When write accesses come from
>> > non-owner, error should be reported. Normally the write accesses on a given
>> > structure happen in only several or a dozen of functions if the structure
>> > is not that complicated. We call those functions "allowed functions".
>> > The work of defining the owner and binding memory to owner is expected to
>> > be done by the memory consumer. In the above case, memory consume register
>> > the owner as the functions which have write accesses to the structure then
>> > bind all the structures to the owner. Then kasan will do the "owner check"
>> > after the basic checks.
>> >
>> > As implementation, kasan provides a API to it's user to register their
>> > allowed functions. The API returns a token to users.  At run time, users
>> > bind the memory ranges they are interested in to the check they registered.
>> > Kasan then checks the bound memory ranges with the allowed functions.
>> >
>> >
>> > Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>
> Hello, Wengang.
>
> Nice idea. I also think that we need this kind of debugging tool. It's very
> hard to detect overwritten bugs.
>
> In fact, I made a quite similar tool, valid access checker (A.K.A.
> vchecker). See the following link.
>
> https://github.com/JoonsooKim/linux/tree/vchecker-master-v0.3-next-20170106
>
> Vchecker has some advanced features compared to yours.
>
> 1. Target object can be choosen at runtime by debugfs. It doesn't
> require re-compile to register the target object.
>
> 2. It has another feature that checks the value stored in the object.
> Usually, invalid writer stores odd value into the object and vchecker
> can detect this case.
>
> 3. It has a callstack checker (memory owner checker in yours). It
> checks all the callstack rather than just the caller. It's important
> since invalid writer could call the parent function of owner function
> and it would not be catched by checking just the caller.
>
> 4. The callstack checker is more automated. vchecker collects the valid
> callstack by running the system.
>
>
> FYI, I attach some commit descriptions of the vchecker.
>
>     vchecker: store/report callstack of value writer
>
>     The purpose of the value checker is finding invalid user writing
>     invalid value at the moment that the value is written. However, there is
>     a missing infrastructure that passes writing value to the checker
>     since we temporarilly piggyback on the KASAN. So, we cannot easily
>     detect this case in time.
>
>     However, by following way, we can emulate similar effect.
>
>     1. Store callstack when memory is written.
>     2. If check is failed in next access, report previous write-access
>     callstack
>
>     It will caught offending user properly.
>
>
>     Following output "Call trace: Invalid writer" part is the result
>     of this patch. We find the invalid value at workfn+0x71 but report
>     writer at workfn+0x61.
>
>     [  133.024076] ==================================================================
>     [  133.025576] BUG: VCHECKER: invalid access in workfn+0x71/0xc0 at addr ffff8800683dd6c8
>     [  133.027196] Read of size 8 by task kworker/1:1/48
>     [  133.028020] 0x8 0x10 value
>     [  133.028020] 0xffff 4
>     [  133.028020] Call trace: Invalid writer
>     [  133.028020]
>     [  133.028020] [<ffffffff81043b1b>] save_stack_trace+0x1b/0x20
>     [  133.028020]
>     [  133.028020] [<ffffffff812c0db9>] save_stack+0x39/0x70
>     [  133.028020]
>     [  133.028020] [<ffffffff812c0fe3>] check_value+0x43/0x80
>     [  133.028020]
>     [  133.028020] [<ffffffff812c1762>] vchecker_check+0x1c2/0x380
>     [  133.028020]
>     [  133.028020] [<ffffffff812be49d>] __asan_store8+0x8d/0xc0
>     [  133.028020]
>     [  133.028020] [<ffffffff815eadd1>] workfn+0x61/0xc0
>     [  133.028020]
>     [  133.028020] [<ffffffff810be3df>] process_one_work+0x28f/0x680
>     [  133.028020]
>     [  133.028020] [<ffffffff810bf272>] worker_thread+0xa2/0x870
>     [  133.028020]
>     [  133.028020] [<ffffffff810c86a5>] kthread+0x195/0x1e0
>     [  133.028020]
>     [  133.028020] [<ffffffff81b9d3d2>] ret_from_fork+0x22/0x30
>     [  133.028020] CPU: 1 PID: 48 Comm: kworker/1:1 Not tainted 4.10.0-rc2-next-20170106+ #1179
>     [  133.028020] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>     [  133.028020] Workqueue: events workfn
>     [  133.028020] Call Trace:
>     [  133.028020]  dump_stack+0x4d/0x63
>     [  133.028020]  kasan_object_err+0x21/0x80
>     [  133.028020]  vchecker_check+0x2af/0x380
>     [  133.028020]  ? workfn+0x71/0xc0
>     [  133.028020]  ? workfn+0x71/0xc0
>     [  133.028020]  __asan_load8+0x87/0xb0
>     [  133.028020]  workfn+0x71/0xc0
>     [  133.028020]  process_one_work+0x28f/0x680
>     [  133.028020]  worker_thread+0xa2/0x870
>     [  133.028020]  kthread+0x195/0x1e0
>     [  133.028020]  ? put_pwq_unlocked+0xc0/0xc0
>     [  133.028020]  ? kthread_park+0xd0/0xd0
>     [  133.028020]  ret_from_fork+0x22/0x30
>     [  133.028020] Object at ffff8800683dd6c0, in cache vchecker_test size: 24
>     [  133.028020] Allocated:
>     [  133.028020] PID = 48
>
>
>     vchecker: Add 'callstack' checker
>
>     The callstack checker is to find invalid code paths accessing to a
>     certain field in an object.  Currently it only saves all stack traces at
>     the given offset.  Reporting will be added in the next patch.
>
>     The below example checks callstack of anon_vma:
>
>       # cd /sys/kernel/debug/vchecker
>       # echo 0 8 > anon_vma/callstack  # offset 0, size 8
>       # echo 1 > anon_vma/enable
>
>       # cat anon_vma/callstack        # show saved callstacks
>       0x0 0x8 callstack
>       total: 42
>       callstack #0
>         anon_vma_fork+0x101/0x280
>         copy_process.part.10+0x15ff/0x2a40
>         _do_fork+0x155/0x7d0
>         SyS_clone+0x19/0x20
>         do_syscall_64+0xdf/0x460
>         return_from_SYSCALL_64+0x0/0x7a
>       ...
>
>
>     vchecker: Support toggle on/off of callstack check
>
>     By default, callstack checker only collects callchains.  When a user
>     writes 'on' to the callstack file in debugfs, it checks and reports new
>     callstacks.  Writing 'off' to disable it again.
>
>       # cd /sys/kernel/debug/vchecker
>       # echo 0 8 > anon_vma/callstack
>       # echo 1 > anon_vma/enable
>
>       ... (do some work to collect enough callstacks) ...
>
>       # echo on > anon_vma/callstack
>
>
> The reason I didn't submit the vchecker to mainline is that I didn't find
> the case that this tool is useful in real life. Most of the system broken case
> can be debugged by other ways. Do you see the real case that this tool is
> helpful?

Hi,

Yes, this is the main question here.
How is it going to be used in real life? How widely?



> If so, I think that vchecker is more appropriate to be upstreamed.
> Could you share your opinion?
>
> Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Wengang]

On 11/19/2017 05:50 PM, Joonsoo Kim wrote:
> On Fri, Nov 17, 2017 at 11:56:21PM +0100, Dmitry Vyukov wrote:
>> On Fri, Nov 17, 2017 at 11:30 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
>>> Kasan advanced check, I'm going to add this feature.
>>> Currently Kasan provide the detection of use-after-free and out-of-bounds
>>> problems. It is not able to find the overwrite-on-allocated-memory issue.
>>> We sometimes hit this kind of issue: We have a messed up structure
>>> (usually dynamially allocated), some of the fields in the structure were
>>> overwritten with unreasaonable values. And kernel may panic due to those
>>> overeritten values. We know those fields were overwritten somehow, but we
>>> have no easy way to find out which path did the overwritten. The advanced
>>> check wants to help in this scenario.
>>>
>>> The idea is to define the memory owner. When write accesses come from
>>> non-owner, error should be reported. Normally the write accesses on a given
>>> structure happen in only several or a dozen of functions if the structure
>>> is not that complicated. We call those functions "allowed functions".
>>> The work of defining the owner and binding memory to owner is expected to
>>> be done by the memory consumer. In the above case, memory consume register
>>> the owner as the functions which have write accesses to the structure then
>>> bind all the structures to the owner. Then kasan will do the "owner check"
>>> after the basic checks.
>>>
>>> As implementation, kasan provides a API to it's user to register their
>>> allowed functions. The API returns a token to users.  At run time, users
>>> bind the memory ranges they are interested in to the check they registered.
>>> Kasan then checks the bound memory ranges with the allowed functions.
>>>
>>>
>>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> Hello, Wengang.
>
> Nice idea. I also think that we need this kind of debugging tool. It's very
> hard to detect overwritten bugs.
>
> In fact, I made a quite similar tool, valid access checker (A.K.A.
> vchecker). See the following link.
>
> https://github.com/JoonsooKim/linux/tree/vchecker-master-v0.3-next-20170106
>
> Vchecker has some advanced features compared to yours.
>
> 1. Target object can be choosen at runtime by debugfs. It doesn't
> require re-compile to register the target object.
Hi Joonsoo, good to know you are also interested in this!

Yes, if can be choosen via debugfs, it doesn't need re-compile.
Well, I wonder what do you expect to be chosen from use space?

>
> 2. It has another feature that checks the value stored in the object.
> Usually, invalid writer stores odd value into the object and vchecker
> can detect this case.
It's good to do the check. Well, as I understand, it tells something bad 
(overwitten) happened.
But it can't tell who did the overwritten, right?  (I didn't look at 
your patch yet,) do you recall the last write somewhere?

>
> 3. It has a callstack checker (memory owner checker in yours). It
> checks all the callstack rather than just the caller. It's important
> since invalid writer could call the parent function of owner function
> and it would not be catched by checking just the caller.
>
> 4. The callstack checker is more automated. vchecker collects the valid
> callstack by running the system.
I think we can merge the above two into one.
So you are doing full stack check.  Well, finding out the all the paths 
which have the write access may be not a very easy thing.
Missing some paths may cause dmesg flooding, and those log won't help at 
all. Finding out all the (owning) caller only is relatively much easier.
There do is the case you pointed out here. In this case, the debugger 
can make slight change to the calling path. And as I understand,
most of the overwritten are happening in quite different call paths, 
they are not calling the (owning) caller.

>
> FYI, I attach some commit descriptions of the vchecker.
>
>      vchecker: store/report callstack of value writer
>      
>      The purpose of the value checker is finding invalid user writing
>      invalid value at the moment that the value is written. However, there is
>      a missing infrastructure that passes writing value to the checker
>      since we temporarilly piggyback on the KASAN. So, we cannot easily
>      detect this case in time.
>      
>      However, by following way, we can emulate similar effect.
>      
>      1. Store callstack when memory is written.

Oh, seems you are storing the callstack for each write. -- I am not sure 
if that would too heavy.
Actually I was thinking to have a check on the new value. But seems 
compiler doesn't provide that.
>      2. If check is failed in next access, report previous write-access
>      callstack
>      
>      It will caught offending user properly.
>      
>      
>      Following output "Call trace: Invalid writer" part is the result
>      of this patch. We find the invalid value at workfn+0x71 but report
>      writer at workfn+0x61.
>      
>      [  133.024076] ==================================================================
>      [  133.025576] BUG: VCHECKER: invalid access in workfn+0x71/0xc0 at addr ffff8800683dd6c8
>      [  133.027196] Read of size 8 by task kworker/1:1/48
>      [  133.028020] 0x8 0x10 value
>      [  133.028020] 0xffff 4
>      [  133.028020] Call trace: Invalid writer
>      [  133.028020]
>      [  133.028020] [<ffffffff81043b1b>] save_stack_trace+0x1b/0x20
>      [  133.028020]
>      [  133.028020] [<ffffffff812c0db9>] save_stack+0x39/0x70
>      [  133.028020]
>      [  133.028020] [<ffffffff812c0fe3>] check_value+0x43/0x80
>      [  133.028020]
>      [  133.028020] [<ffffffff812c1762>] vchecker_check+0x1c2/0x380
>      [  133.028020]
>      [  133.028020] [<ffffffff812be49d>] __asan_store8+0x8d/0xc0
>      [  133.028020]
>      [  133.028020] [<ffffffff815eadd1>] workfn+0x61/0xc0
>      [  133.028020]
>      [  133.028020] [<ffffffff810be3df>] process_one_work+0x28f/0x680
>      [  133.028020]
>      [  133.028020] [<ffffffff810bf272>] worker_thread+0xa2/0x870
>      [  133.028020]
>      [  133.028020] [<ffffffff810c86a5>] kthread+0x195/0x1e0
>      [  133.028020]
>      [  133.028020] [<ffffffff81b9d3d2>] ret_from_fork+0x22/0x30
>      [  133.028020] CPU: 1 PID: 48 Comm: kworker/1:1 Not tainted 4.10.0-rc2-next-20170106+ #1179
>      [  133.028020] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>      [  133.028020] Workqueue: events workfn
>      [  133.028020] Call Trace:
>      [  133.028020]  dump_stack+0x4d/0x63
>      [  133.028020]  kasan_object_err+0x21/0x80
>      [  133.028020]  vchecker_check+0x2af/0x380
>      [  133.028020]  ? workfn+0x71/0xc0
>      [  133.028020]  ? workfn+0x71/0xc0
>      [  133.028020]  __asan_load8+0x87/0xb0
>      [  133.028020]  workfn+0x71/0xc0
>      [  133.028020]  process_one_work+0x28f/0x680
>      [  133.028020]  worker_thread+0xa2/0x870
>      [  133.028020]  kthread+0x195/0x1e0
>      [  133.028020]  ? put_pwq_unlocked+0xc0/0xc0
>      [  133.028020]  ? kthread_park+0xd0/0xd0
>      [  133.028020]  ret_from_fork+0x22/0x30
>      [  133.028020] Object at ffff8800683dd6c0, in cache vchecker_test size: 24
>      [  133.028020] Allocated:
>      [  133.028020] PID = 48
>
>
>      vchecker: Add 'callstack' checker
>      
>      The callstack checker is to find invalid code paths accessing to a
>      certain field in an object.  Currently it only saves all stack traces at
>      the given offset.  Reporting will be added in the next patch.
>      
>      The below example checks callstack of anon_vma:
>      
>        # cd /sys/kernel/debug/vchecker
>        # echo 0 8 > anon_vma/callstack  # offset 0, size 8
>        # echo 1 > anon_vma/enable
an echo "anon_vma" > <something> first?
How do you define and path the valid (owning) full stack to kasan?

>      
>        # cat anon_vma/callstack        # show saved callstacks
>        0x0 0x8 callstack
>        total: 42
>        callstack #0
>          anon_vma_fork+0x101/0x280
>          copy_process.part.10+0x15ff/0x2a40
>          _do_fork+0x155/0x7d0
>          SyS_clone+0x19/0x20
>          do_syscall_64+0xdf/0x460
>          return_from_SYSCALL_64+0x0/0x7a
>        ...
>
>
>      vchecker: Support toggle on/off of callstack check
>      
>      By default, callstack checker only collects callchains.  When a user
>      writes 'on' to the callstack file in debugfs, it checks and reports new
>      callstacks.  Writing 'off' to disable it again.
>      
>        # cd /sys/kernel/debug/vchecker
>        # echo 0 8 > anon_vma/callstack
>        # echo 1 > anon_vma/enable
>      
>        ... (do some work to collect enough callstacks) ...
How to define "enough" here?
>      
>        # echo on > anon_vma/callstack
>      
>
> The reason I didn't submit the vchecker to mainline is that I didn't find
> the case that this tool is useful in real life. Most of the system broken case
> can be debugged by other ways. Do you see the real case that this tool is
> helpful? If so, I think that vchecker is more appropriate to be upstreamed.
> Could you share your opinion?
Yes, people find other ways to solve overwritten issue (so did I) in the 
past. If kasan doesn't provide this functionality, developers have no 
way to choose it.
Though people have other ways to find the root cause, the other ways 
maybe take (maybe much) longer. I didn't solve problems with the owner 
check yet since I just make available recently.  But considering the 
overwritten issues I have ever hit, the owner check definitely helps and 
I definitely will try the owner check when I have a new overwritten issue!

Why not send your patch for review?

thanks,
wengang

>
> Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Wengang]

On 11/20/2017 12:41 AM, Dmitry Vyukov wrote:
>
>>
>> The reason I didn't submit the vchecker to mainline is that I didn't find
>> the case that this tool is useful in real life. Most of the system broken case
>> can be debugged by other ways. Do you see the real case that this tool is
>> helpful?
> Hi,
>
> Yes, this is the main question here.
> How is it going to be used in real life? How widely?
>

I think the owner check can be enabled in the cases where KASAN is used. 
-- That is that we found there is memory issue, but don't know how it 
happened.
It's not the first hit of problem -- no production system will run with 
KASAN enabled, KASAN is performance killer. And the use case is that we 
found the overwritten happened on some particular victims, we then want 
to add owner check on those victims.

thanks,
wengang

>
>> If so, I think that vchecker is more appropriate to be upstreamed.
>> Could you share your opinion?
>>
>> Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Dmitry Vyukov]

On Mon, Nov 20, 2017 at 9:05 PM, Wengang <wen.gang.wang@oracle.com> wrote:
>
>
> On 11/20/2017 12:41 AM, Dmitry Vyukov wrote:
>>
>>
>>>
>>> The reason I didn't submit the vchecker to mainline is that I didn't find
>>> the case that this tool is useful in real life. Most of the system broken
>>> case
>>> can be debugged by other ways. Do you see the real case that this tool is
>>> helpful?
>>
>> Hi,
>>
>> Yes, this is the main question here.
>> How is it going to be used in real life? How widely?
>>
>
> I think the owner check can be enabled in the cases where KASAN is used. --
> That is that we found there is memory issue, but don't know how it happened.


But KASAN generally pinpoints the corruption as it happens. Why do we
need something else?

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Wengang]

On 11/20/2017 12:20 PM, Dmitry Vyukov wrote:
> On Mon, Nov 20, 2017 at 9:05 PM, Wengang <wen.gang.wang@oracle.com> wrote:
>>
>> On 11/20/2017 12:41 AM, Dmitry Vyukov wrote:
>>>
>>>> The reason I didn't submit the vchecker to mainline is that I didn't find
>>>> the case that this tool is useful in real life. Most of the system broken
>>>> case
>>>> can be debugged by other ways. Do you see the real case that this tool is
>>>> helpful?
>>> Hi,
>>>
>>> Yes, this is the main question here.
>>> How is it going to be used in real life? How widely?
>>>
>> I think the owner check can be enabled in the cases where KASAN is used. --
>> That is that we found there is memory issue, but don't know how it happened.
>
> But KASAN generally pinpoints the corruption as it happens. Why do we
> need something else?

Currently (without this patch set) kasan can't detect the overwritten 
issues that happen on allocated memory.

Say, A allocated a 128 bytes memory and B write to that memory at offset 
0 with length 100 unexpectedly.  Currently kasan won't report error for 
any writing to the offset 0 with len <= 128 including the B writting.  
This patch lets kasan report the B writing to offset 0 with length 100.

thanks,
wengang

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Dmitry Vyukov]

On Mon, Nov 20, 2017 at 9:29 PM, Wengang <wen.gang.wang@oracle.com> wrote:
>
>
> On 11/20/2017 12:20 PM, Dmitry Vyukov wrote:
>>
>> On Mon, Nov 20, 2017 at 9:05 PM, Wengang <wen.gang.wang@oracle.com> wrote:
>>>
>>>
>>> On 11/20/2017 12:41 AM, Dmitry Vyukov wrote:
>>>>
>>>>
>>>>> The reason I didn't submit the vchecker to mainline is that I didn't
>>>>> find
>>>>> the case that this tool is useful in real life. Most of the system
>>>>> broken
>>>>> case
>>>>> can be debugged by other ways. Do you see the real case that this tool
>>>>> is
>>>>> helpful?
>>>>
>>>> Hi,
>>>>
>>>> Yes, this is the main question here.
>>>> How is it going to be used in real life? How widely?
>>>>
>>> I think the owner check can be enabled in the cases where KASAN is used.
>>> --
>>> That is that we found there is memory issue, but don't know how it
>>> happened.
>>
>>
>> But KASAN generally pinpoints the corruption as it happens. Why do we
>> need something else?
>
>
> Currently (without this patch set) kasan can't detect the overwritten issues
> that happen on allocated memory.
>
> Say, A allocated a 128 bytes memory and B write to that memory at offset 0
> with length 100 unexpectedly.  Currently kasan won't report error for any
> writing to the offset 0 with len <= 128 including the B writting.  This
> patch lets kasan report the B writing to offset 0 with length 100.


So this will be used for manual debugging and you don't have plans to
annotate kernel code with additional tags, right?

If this meant to be used by kernel developers during debugging, this
feature needs to be documented in Documentation/dev-tools/kasan.rst
including an example. It's hard to spread knowledge about such
features especially if there are no mentions in docs. Documentation
can then be quickly referenced e.g. as a suggestion of how to tackle a
particular bug.

General comments:

1. The check must not affect fast-path. I think we need to move it
into kasan_report (and then rename kasan_report to something else).
Closer to what Joonsoo did in his version, but move then check even
further. This will also make inline instrumentation work because it
calls kasan_report, then kasan_report will do the additional check and
potentially return without actually reporting a bug.
The idea is that the check reserves some range of bad values in shadow
and poison the object with that special value. Then all accesses to
the protected memory will be detected as bad and go into kasan_report.
Then kasan_report will do the additional check and potentially return
without reporting.
This has 0 overhead when the feature is not used, enables inline
instrumentation and is less intrusive.

2. Moving this to a separate .c/.h files sounds like a good idea.
kasan.c is a bit of a mess already. If we do (1), changes to kasan.c
will be minimal. Again closer to what Joonsoo did.

3. We need to rename it from "advanced" to something else (owner
check?). Features must be named based on what they do, rather then how
advanced they are. If we add other complex checks, how should we name
them? even_more_advanced?

I am fine with adding such feature provided that it does not affect
performance/memory consumption if not used, works with inline
instrumentation and is separated into separate files. But it also
needs to be advertised somehow among kernel developers, otherwise only
you guys will use it.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Wengang Wang]

On 2017/11/21 1:54, Dmitry Vyukov wrote:
> On Mon, Nov 20, 2017 at 9:29 PM, Wengang <wen.gang.wang@oracle.com> wrote:
>>
>> On 11/20/2017 12:20 PM, Dmitry Vyukov wrote:
>>> On Mon, Nov 20, 2017 at 9:05 PM, Wengang <wen.gang.wang@oracle.com> wrote:
>>>>
>>>> On 11/20/2017 12:41 AM, Dmitry Vyukov wrote:
>>>>>
>>>>>> The reason I didn't submit the vchecker to mainline is that I didn't
>>>>>> find
>>>>>> the case that this tool is useful in real life. Most of the system
>>>>>> broken
>>>>>> case
>>>>>> can be debugged by other ways. Do you see the real case that this tool
>>>>>> is
>>>>>> helpful?
>>>>> Hi,
>>>>>
>>>>> Yes, this is the main question here.
>>>>> How is it going to be used in real life? How widely?
>>>>>
>>>> I think the owner check can be enabled in the cases where KASAN is used.
>>>> --
>>>> That is that we found there is memory issue, but don't know how it
>>>> happened.
>>>
>>> But KASAN generally pinpoints the corruption as it happens. Why do we
>>> need something else?
>>
>> Currently (without this patch set) kasan can't detect the overwritten issues
>> that happen on allocated memory.
>>
>> Say, A allocated a 128 bytes memory and B write to that memory at offset 0
>> with length 100 unexpectedly.  Currently kasan won't report error for any
>> writing to the offset 0 with len <= 128 including the B writting.  This
>> patch lets kasan report the B writing to offset 0 with length 100.
>
> So this will be used for manual debugging and you don't have plans to
> annotate kernel code with additional tags, right?
I am not sure what do you mean by "manual debugging". What is needed to 
use the owner check is:
The memory user needs to do:
1)A  code change: register the checker with the allowed functions
2)A  code change: bind the memory to the checker
3)A  recompile the kernel
4)A  run with the recompiled kernel and reproduce the issue

By "additional tags", if you meant "add some explanation comment", I 
think one can refer to the commit message about the code change;
if you meant "additional kernel config item to enable/disable code", I 
have no such plan.A  If no "owner checker" is registered, it just acts 
like the basic kasan (without this patch) with almost same performance. 
Even with "owner checker" registered,A  and memories are bound to the 
checker,A  it's still the rare case to do the owner check. So the 
overheard caused by owner check is slight. I don't find the reason we 
need an additional kernel config.

>
> If this meant to be used by kernel developers during debugging, this
> feature needs to be documented in Documentation/dev-tools/kasan.rst
> including an example. It's hard to spread knowledge about such
> features especially if there are no mentions in docs. Documentation
> can then be quickly referenced e.g. as a suggestion of how to tackle a
> particular bug.
Yes, this is a good idea. I was/am thinking so.

> General comments:
>
> 1. The check must not affect fast-path. I think we need to move it
> into kasan_report (and then rename kasan_report to something else).
> Closer to what Joonsoo did in his version, but move then check even
> further. This will also make inline instrumentation work because it
> calls kasan_report, then kasan_report will do the additional check and
> potentially return without actually reporting a bug.
> The idea is that the check reserves some range of bad values in shadow
> and poison the object with that special value. Then all accesses to
> the protected memory will be detected as bad and go into kasan_report.
> Then kasan_report will do the additional check and potentially return
> without reporting.
> This has 0 overhead when the feature is not used, enables inline
> instrumentation and is less intrusive.
The owner check can be moved to kasan_report() by letting the poison check
routine return "possible violation" when the memory is bound to a owner
and then kasan_report() will get the chance to do further (owner) check.

Well I wonder how that moving would benefit.
If the purpose is to remove overhead,A A  the moving didn't remove of any 
run of
owner check. It would just move it to a different place and it will run 
just a bit later.
I think even current implementation, it has almost 0 overhead when no 
memory is
bound to owners.A  The owner check is performed only when the memory is 
bound (the
bound check is light), if memory is not bound, no owner check is performed.
I am predicting the code that has owner check routine moved to 
kasan_report(), it
should be like this:
(fake code)
in poison check routines:
 A A A A A A  ...
 A A A A A A  after all case that returns "Yes",
 A A A A A A  if bound check returns true (memory is bound):A A A  --> bound 
check is here
 A A A  A A A  A A A A  return "possible"
 A A A A A A  ...
in the caller of poison check routines:
 A A A A A A  ...
 A A A A A A  if poison check routine returns "yes" or "possible":
 A A A A A A A A A A A A  calls kasan_report()

in kasan_report():
 A A A A A A  ....
 A A A A A A  if no basic violation found:
 A A A  A A A A A A  run owner 
checkA A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A  --> owner check 
is here
 A A A A A A  ...

Current code is like this:
in poison check routines:
 A A A A A A  ...
 A A A A A A  after all case that returns "yes",
 A A A A A A  if bound check returns true (memory is bound):A  --> bound check 
is here
 A A A  A A  A A A  A A A  run owner 
checkA A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A  --> owner check is here

Comparing to current implementation,
anyway the "bound check" is done either in the poison check routines or 
in kasan_report().
anyway the "owner check" is done either in the poison check routines or 
in kasan_report().
I don't see we have reduced number of calls of "bound check" and/or 
"owner check".
Can you pinpoint which part will be reduced?

If the purpose is to make inline instrumentation work for owner check, 
it interests
me!A  This implementation only works fine in outline instrumentation and 
seems the
poison checks are not called at all with inline compile type. Could you 
share more on this?

The badness of moving owner check to kasan_report() is that it breaks 
the function
clearness in the code.A  From this point of view, check is just check, it 
should say "yes" or
"no", not "possible";A  report is just report, no checks should be 
performed in report.

> 2. Moving this to a separate .c/.h files sounds like a good idea.
> kasan.c is a bit of a mess already. If we do (1), changes to kasan.c
> will be minimal. Again closer to what Joonsoo did.
If the owner checks would remain in the poison check routines, it would 
be in kasan.c.
If we have enough points to support the moving, say that makes inline 
instrumentation
work, it can be in a separated .c/.h and yes that would be better then.

> 3. We need to rename it from "advanced" to something else (owner
> check?). Features must be named based on what they do, rather then how
> advanced they are. If we add other complex checks, how should we name
> them? even_more_advanced?

LoL,A  No and Yes.
The feature I am adding is "owner check" and I define it as one of the 
"advanced check",
By looking at the patch its self (especially enum kasan_adv_chk_type in 
patch 4/5)A  , you
can see, I was leaving spaces for other kind of "advanced checks". And 
(future) different
"advanced checks" can be added -- say "old value validation", "new value 
validation"
 A -- though the new value is notA  supported by compiler yet.A  But yes 
the name "advanced"
is really not what I want, but I failed to find an accurate one. How do 
you think?


>
> I am fine with adding such feature provided that it does not affect
> performance/memory consumption if not used, works with inline
> instrumentation and is separated into separate files. But it also
> needs to be advertised somehow among kernel developers, otherwise only
> you guys will use it.
So far it should has almost same performance if feature is not used; 
definitely
no more memory consumption.A  Now it doesn't work with inline 
instrumentation,
could you share more information on how to make it also work with inline 
mode?
It technically can be moved to separated files. I will add the doc.

thanks,
wengang

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Joonsoo Kim]

On Mon, Nov 20, 2017 at 11:56:05AM -0800, Wengang wrote:
> 
> 
> On 11/19/2017 05:50 PM, Joonsoo Kim wrote:
> >On Fri, Nov 17, 2017 at 11:56:21PM +0100, Dmitry Vyukov wrote:
> >>On Fri, Nov 17, 2017 at 11:30 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
> >>>Kasan advanced check, I'm going to add this feature.
> >>>Currently Kasan provide the detection of use-after-free and out-of-bounds
> >>>problems. It is not able to find the overwrite-on-allocated-memory issue.
> >>>We sometimes hit this kind of issue: We have a messed up structure
> >>>(usually dynamially allocated), some of the fields in the structure were
> >>>overwritten with unreasaonable values. And kernel may panic due to those
> >>>overeritten values. We know those fields were overwritten somehow, but we
> >>>have no easy way to find out which path did the overwritten. The advanced
> >>>check wants to help in this scenario.
> >>>
> >>>The idea is to define the memory owner. When write accesses come from
> >>>non-owner, error should be reported. Normally the write accesses on a given
> >>>structure happen in only several or a dozen of functions if the structure
> >>>is not that complicated. We call those functions "allowed functions".
> >>>The work of defining the owner and binding memory to owner is expected to
> >>>be done by the memory consumer. In the above case, memory consume register
> >>>the owner as the functions which have write accesses to the structure then
> >>>bind all the structures to the owner. Then kasan will do the "owner check"
> >>>after the basic checks.
> >>>
> >>>As implementation, kasan provides a API to it's user to register their
> >>>allowed functions. The API returns a token to users.  At run time, users
> >>>bind the memory ranges they are interested in to the check they registered.
> >>>Kasan then checks the bound memory ranges with the allowed functions.
> >>>
> >>>
> >>>Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> >Hello, Wengang.
> >
> >Nice idea. I also think that we need this kind of debugging tool. It's very
> >hard to detect overwritten bugs.
> >
> >In fact, I made a quite similar tool, valid access checker (A.K.A.
> >vchecker). See the following link.
> >
> >https://github.com/JoonsooKim/linux/tree/vchecker-master-v0.3-next-20170106
> >
> >Vchecker has some advanced features compared to yours.
> >
> >1. Target object can be choosen at runtime by debugfs. It doesn't
> >require re-compile to register the target object.
> Hi Joonsoo, good to know you are also interested in this!
> 
> Yes, if can be choosen via debugfs, it doesn't need re-compile.
> Well, I wonder what do you expect to be chosen from use space?

As you mentioned somewhere, this tool can be used when we find the
overwritten happend on some particular victims. I assumes that most of
the problem would happen on slab objects and userspace can choose the
target slab cache via debugfs interface of the vchecker.

> 
> >
> >2. It has another feature that checks the value stored in the object.
> >Usually, invalid writer stores odd value into the object and vchecker
> >can detect this case.
> It's good to do the check. Well, as I understand, it tells something
> bad (overwitten) happened.
> But it can't tell who did the overwritten, right?  (I didn't look at
> your patch yet,) do you recall the last write somewhere?

Yes, it stores the callstack of the last write and report it when
the error is found.

> 
> >
> >3. It has a callstack checker (memory owner checker in yours). It
> >checks all the callstack rather than just the caller. It's important
> >since invalid writer could call the parent function of owner function
> >and it would not be catched by checking just the caller.
> >
> >4. The callstack checker is more automated. vchecker collects the valid
> >callstack by running the system.
> I think we can merge the above two into one.
> So you are doing full stack check.  Well, finding out the all the
> paths which have the write access may be not a very easy thing.
> Missing some paths may cause dmesg flooding, and those log won't
> help at all. Finding out all the (owning) caller only is relatively
> much easier.

Vchecker can be easily modified to store only the caller. It just
requires modifying callstack depth parameter so it's so easy.
Moreover, it can be accomplished by adding debugfs interface.

Anyway, I don't think that finding out all the (owning) caller only
is much easier. Think about dentry or inode object. It is accessed by
various code path and it's not easy to cover all the owning caller by
manual approach.


> There do is the case you pointed out here. In this case, the
> debugger can make slight change to the calling path. And as I
> understand,
> most of the overwritten are happening in quite different call paths,
> they are not calling the (owning) caller.

Agreed.

> 
> >
> >FYI, I attach some commit descriptions of the vchecker.
> >
> >     vchecker: store/report callstack of value writer
> >     The purpose of the value checker is finding invalid user writing
> >     invalid value at the moment that the value is written. However, there is
> >     a missing infrastructure that passes writing value to the checker
> >     since we temporarilly piggyback on the KASAN. So, we cannot easily
> >     detect this case in time.
> >     However, by following way, we can emulate similar effect.
> >     1. Store callstack when memory is written.
> 
> Oh, seems you are storing the callstack for each write. -- I am not
> sure if that would too heavy.

Unlike KASAN that checks all type of the objects, this debugging
feature is only enabled on the specific type of the objects so
overhead would not be too heavy in terms of system overall
performance.

> Actually I was thinking to have a check on the new value. But seems
> compiler doesn't provide that.

Yes, look like we have a similar idea. I have some another ideas if
ASAN hook provides the value to be written. However, it's not
supported by compiler yet.

> >     2. If check is failed in next access, report previous write-access
> >     callstack
> >     It will caught offending user properly.
> >     Following output "Call trace: Invalid writer" part is the result
> >     of this patch. We find the invalid value at workfn+0x71 but report
> >     writer at workfn+0x61.
> >     [  133.024076] ==================================================================
> >     [  133.025576] BUG: VCHECKER: invalid access in workfn+0x71/0xc0 at addr ffff8800683dd6c8
> >     [  133.027196] Read of size 8 by task kworker/1:1/48
> >     [  133.028020] 0x8 0x10 value
> >     [  133.028020] 0xffff 4
> >     [  133.028020] Call trace: Invalid writer
> >     [  133.028020]
> >     [  133.028020] [<ffffffff81043b1b>] save_stack_trace+0x1b/0x20
> >     [  133.028020]
> >     [  133.028020] [<ffffffff812c0db9>] save_stack+0x39/0x70
> >     [  133.028020]
> >     [  133.028020] [<ffffffff812c0fe3>] check_value+0x43/0x80
> >     [  133.028020]
> >     [  133.028020] [<ffffffff812c1762>] vchecker_check+0x1c2/0x380
> >     [  133.028020]
> >     [  133.028020] [<ffffffff812be49d>] __asan_store8+0x8d/0xc0
> >     [  133.028020]
> >     [  133.028020] [<ffffffff815eadd1>] workfn+0x61/0xc0
> >     [  133.028020]
> >     [  133.028020] [<ffffffff810be3df>] process_one_work+0x28f/0x680
> >     [  133.028020]
> >     [  133.028020] [<ffffffff810bf272>] worker_thread+0xa2/0x870
> >     [  133.028020]
> >     [  133.028020] [<ffffffff810c86a5>] kthread+0x195/0x1e0
> >     [  133.028020]
> >     [  133.028020] [<ffffffff81b9d3d2>] ret_from_fork+0x22/0x30
> >     [  133.028020] CPU: 1 PID: 48 Comm: kworker/1:1 Not tainted 4.10.0-rc2-next-20170106+ #1179
> >     [  133.028020] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >     [  133.028020] Workqueue: events workfn
> >     [  133.028020] Call Trace:
> >     [  133.028020]  dump_stack+0x4d/0x63
> >     [  133.028020]  kasan_object_err+0x21/0x80
> >     [  133.028020]  vchecker_check+0x2af/0x380
> >     [  133.028020]  ? workfn+0x71/0xc0
> >     [  133.028020]  ? workfn+0x71/0xc0
> >     [  133.028020]  __asan_load8+0x87/0xb0
> >     [  133.028020]  workfn+0x71/0xc0
> >     [  133.028020]  process_one_work+0x28f/0x680
> >     [  133.028020]  worker_thread+0xa2/0x870
> >     [  133.028020]  kthread+0x195/0x1e0
> >     [  133.028020]  ? put_pwq_unlocked+0xc0/0xc0
> >     [  133.028020]  ? kthread_park+0xd0/0xd0
> >     [  133.028020]  ret_from_fork+0x22/0x30
> >     [  133.028020] Object at ffff8800683dd6c0, in cache vchecker_test size: 24
> >     [  133.028020] Allocated:
> >     [  133.028020] PID = 48
> >
> >
> >     vchecker: Add 'callstack' checker
> >     The callstack checker is to find invalid code paths accessing to a
> >     certain field in an object.  Currently it only saves all stack traces at
> >     the given offset.  Reporting will be added in the next patch.
> >     The below example checks callstack of anon_vma:
> >       # cd /sys/kernel/debug/vchecker
> >       # echo 0 8 > anon_vma/callstack  # offset 0, size 8
> >       # echo 1 > anon_vma/enable
> an echo "anon_vma" > <something> first?
> How do you define and path the valid (owning) full stack to kasan?

This interface only enables to store all the callstacks. No validation
check here. I think that this feature would also be helpful to debug.
If error happens, we can check all the previous callstacks and track
the buggy caller.

> >       # cat anon_vma/callstack        # show saved callstacks
> >       0x0 0x8 callstack
> >       total: 42
> >       callstack #0
> >         anon_vma_fork+0x101/0x280
> >         copy_process.part.10+0x15ff/0x2a40
> >         _do_fork+0x155/0x7d0
> >         SyS_clone+0x19/0x20
> >         do_syscall_64+0xdf/0x460
> >         return_from_SYSCALL_64+0x0/0x7a
> >       ...
> >
> >
> >     vchecker: Support toggle on/off of callstack check
> >     By default, callstack checker only collects callchains.  When a user
> >     writes 'on' to the callstack file in debugfs, it checks and reports new
> >     callstacks.  Writing 'off' to disable it again.
> >       # cd /sys/kernel/debug/vchecker
> >       # echo 0 8 > anon_vma/callstack
> >       # echo 1 > anon_vma/enable
> >       ... (do some work to collect enough callstacks) ...
> How to define "enough" here?

The bug usually doesn't happen immediately since it usually happens on
the corner case. When debugging, we run the workload that causes the
bug and then wait for some time until the bug happens. "Enough" can
be defined as the middle of this waiting time. After some warm-up
time, all the common callstack would be collected. Then,
switching on this feature that reports a new callstack. If the corner
case that is on a new callstack happens, this new callstack will be
reported and we can check whether it is a true bug or not.

> >       # echo on > anon_vma/callstack
> >
> >The reason I didn't submit the vchecker to mainline is that I didn't find
> >the case that this tool is useful in real life. Most of the system broken case
> >can be debugged by other ways. Do you see the real case that this tool is
> >helpful? If so, I think that vchecker is more appropriate to be upstreamed.
> >Could you share your opinion?
> Yes, people find other ways to solve overwritten issue (so did I) in
> the past. If kasan doesn't provide this functionality, developers
> have no way to choose it.
> Though people have other ways to find the root cause, the other ways
> maybe take (maybe much) longer. I didn't solve problems with the
> owner check yet since I just make available recently.  But
> considering the overwritten issues I have ever hit, the owner check
> definitely helps and I definitely will try the owner check when I
> have a new overwritten issue!
> 
> Why not send your patch for review?

Okay! I hope to find more people that have interest on this feature
and it seems that you are the one of them. :)

I will send my patches soon. I think that we can be cooperative to
improve this feature.

Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Dmitry Vyukov]

On Tue, Nov 21, 2017 at 8:17 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
>>>> On Mon, Nov 20, 2017 at 9:05 PM, Wengang <wen.gang.wang@oracle.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 11/20/2017 12:41 AM, Dmitry Vyukov wrote:
>>>>>>
>>>>>>
>>>>>>> The reason I didn't submit the vchecker to mainline is that I didn't
>>>>>>> find
>>>>>>> the case that this tool is useful in real life. Most of the system
>>>>>>> broken
>>>>>>> case
>>>>>>> can be debugged by other ways. Do you see the real case that this
>>>>>>> tool
>>>>>>> is
>>>>>>> helpful?
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Yes, this is the main question here.
>>>>>> How is it going to be used in real life? How widely?
>>>>>>
>>>>> I think the owner check can be enabled in the cases where KASAN is
>>>>> used.
>>>>> --
>>>>> That is that we found there is memory issue, but don't know how it
>>>>> happened.
>>>>
>>>>
>>>> But KASAN generally pinpoints the corruption as it happens. Why do we
>>>> need something else?
>>>
>>>
>>> Currently (without this patch set) kasan can't detect the overwritten
>>> issues
>>> that happen on allocated memory.
>>>
>>> Say, A allocated a 128 bytes memory and B write to that memory at offset
>>> 0
>>> with length 100 unexpectedly.  Currently kasan won't report error for any
>>> writing to the offset 0 with len <= 128 including the B writting.  This
>>> patch lets kasan report the B writing to offset 0 with length 100.
>>
>>
>> So this will be used for manual debugging and you don't have plans to
>> annotate kernel code with additional tags, right?
>
> I am not sure what do you mean by "manual debugging". What is needed to use
> the owner check is:
> The memory user needs to do:
> 1)  code change: register the checker with the allowed functions
> 2)  code change: bind the memory to the checker
> 3)  recompile the kernel
> 4)  run with the recompiled kernel and reproduce the issue

Yes, I meant exactly this -- developer does manual work, including
code changes, on a per-bug basis.
This is not the current usage model for KASAN, hence I am asking.


> By "additional tags", if you meant "add some explanation comment", I think
> one can refer to the commit message about the code change;
> if you meant "additional kernel config item to enable/disable code", I have
> no such plan.  If no "owner checker" is registered, it just acts like the
> basic kasan (without this patch) with almost same performance. Even with
> "owner checker" registered,  and memories are bound to the checker,  it's
> still the rare case to do the owner check. So the overheard caused by owner
> check is slight. I don't find the reason we need an additional kernel
> config.

I meant committing some registration of allowed functions and binding
of objects to tags into mainline kernel.




>> If this meant to be used by kernel developers during debugging, this
>> feature needs to be documented in Documentation/dev-tools/kasan.rst
>> including an example. It's hard to spread knowledge about such
>> features especially if there are no mentions in docs. Documentation
>> can then be quickly referenced e.g. as a suggestion of how to tackle a
>> particular bug.
>
> Yes, this is a good idea. I was/am thinking so.
>
>> General comments:
>>
>> 1. The check must not affect fast-path. I think we need to move it
>> into kasan_report (and then rename kasan_report to something else).
>> Closer to what Joonsoo did in his version, but move then check even
>> further. This will also make inline instrumentation work because it
>> calls kasan_report, then kasan_report will do the additional check and
>> potentially return without actually reporting a bug.
>> The idea is that the check reserves some range of bad values in shadow
>> and poison the object with that special value. Then all accesses to
>> the protected memory will be detected as bad and go into kasan_report.
>> Then kasan_report will do the additional check and potentially return
>> without reporting.
>> This has 0 overhead when the feature is not used, enables inline
>> instrumentation and is less intrusive.
>
> The owner check can be moved to kasan_report() by letting the poison check
> routine return "possible violation" when the memory is bound to a owner
> and then kasan_report() will get the chance to do further (owner) check.
>
> Well I wonder how that moving would benefit.
> If the purpose is to remove overhead,   the moving didn't remove of any run
> of
> owner check. It would just move it to a different place and it will run just
> a bit later.
> I think even current implementation, it has almost 0 overhead when no memory
> is
> bound to owners.  The owner check is performed only when the memory is bound
> (the
> bound check is light), if memory is not bound, no owner check is performed.

3 main goals as I outlined:
 - removing _all_ overhead when the feature is not used
 - making inline instrumentation work
 - code separation


> I am predicting the code that has owner check routine moved to
> kasan_report(), it
> should be like this:
> (fake code)
> in poison check routines:
>        ...
>        after all case that returns "Yes",
>        if bound check returns true (memory is bound):    --> bound check is
> here
>              return "possible"

/\/\/\/\

No, just return "possible". Main routine won't know anything about
bounds and owners.


>        ...
> in the caller of poison check routines:
>        ...
>        if poison check routine returns "yes" or "possible":
>              calls kasan_report()
>
> in kasan_report():
>        ....
>        if no basic violation found:
>            run owner check
> --> owner check is here
>        ...
>
> Current code is like this:
> in poison check routines:
>        ...
>        after all case that returns "yes",
>        if bound check returns true (memory is bound):  --> bound check is
> here
>                run owner check
> --> owner check is here
>
> Comparing to current implementation,
> anyway the "bound check" is done either in the poison check routines or in
> kasan_report().
> anyway the "owner check" is done either in the poison check routines or in
> kasan_report().
> I don't see we have reduced number of calls of "bound check" and/or "owner
> check".
> Can you pinpoint which part will be reduced?
>
> If the purpose is to make inline instrumentation work for owner check, it
> interests
> me!  This implementation only works fine in outline instrumentation and
> seems the
> poison checks are not called at all with inline compile type. Could you
> share more on this?
>
> The badness of moving owner check to kasan_report() is that it breaks the
> function
> clearness in the code.  From this point of view, check is just check, it
> should say "yes" or
> "no", not "possible";  report is just report, no checks should be performed
> in report.
>
>> 2. Moving this to a separate .c/.h files sounds like a good idea.
>> kasan.c is a bit of a mess already. If we do (1), changes to kasan.c
>> will be minimal. Again closer to what Joonsoo did.
>
> If the owner checks would remain in the poison check routines, it would be
> in kasan.c.
> If we have enough points to support the moving, say that makes inline
> instrumentation
> work, it can be in a separated .c/.h and yes that would be better then.
>
>> 3. We need to rename it from "advanced" to something else (owner
>> check?). Features must be named based on what they do, rather then how
>> advanced they are. If we add other complex checks, how should we name
>> them? even_more_advanced?
>
>
> LoL,  No and Yes.
> The feature I am adding is "owner check" and I define it as one of the
> "advanced check",
> By looking at the patch its self (especially enum kasan_adv_chk_type in
> patch 4/5)  , you
> can see, I was leaving spaces for other kind of "advanced checks". And
> (future) different
> "advanced checks" can be added -- say "old value validation", "new value
> validation"
>  -- though the new value is not  supported by compiler yet.  But yes the
> name "advanced"
> is really not what I want, but I failed to find an accurate one. How do you
> think?
>
>
>>
>> I am fine with adding such feature provided that it does not affect
>> performance/memory consumption if not used, works with inline
>> instrumentation and is separated into separate files. But it also
>> needs to be advertised somehow among kernel developers, otherwise only
>> you guys will use it.
>
> So far it should has almost same performance if feature is not used;
> definitely
> no more memory consumption.  Now it doesn't work with inline
> instrumentation,
> could you share more information on how to make it also work with inline
> mode?

Move the check into kasan_report and leave the rest of the code as is.

> It technically can be moved to separated files. I will add the doc.
>
> thanks,
> wengang

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Dmitry Vyukov]

On Wed, Nov 22, 2017 at 5:30 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
> On Mon, Nov 20, 2017 at 11:56:05AM -0800, Wengang wrote:
>>
>>
>> On 11/19/2017 05:50 PM, Joonsoo Kim wrote:
>> >On Fri, Nov 17, 2017 at 11:56:21PM +0100, Dmitry Vyukov wrote:
>> >>On Fri, Nov 17, 2017 at 11:30 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
>> >>>Kasan advanced check, I'm going to add this feature.
>> >>>Currently Kasan provide the detection of use-after-free and out-of-bounds
>> >>>problems. It is not able to find the overwrite-on-allocated-memory issue.
>> >>>We sometimes hit this kind of issue: We have a messed up structure
>> >>>(usually dynamially allocated), some of the fields in the structure were
>> >>>overwritten with unreasaonable values. And kernel may panic due to those
>> >>>overeritten values. We know those fields were overwritten somehow, but we
>> >>>have no easy way to find out which path did the overwritten. The advanced
>> >>>check wants to help in this scenario.
>> >>>
>> >>>The idea is to define the memory owner. When write accesses come from
>> >>>non-owner, error should be reported. Normally the write accesses on a given
>> >>>structure happen in only several or a dozen of functions if the structure
>> >>>is not that complicated. We call those functions "allowed functions".
>> >>>The work of defining the owner and binding memory to owner is expected to
>> >>>be done by the memory consumer. In the above case, memory consume register
>> >>>the owner as the functions which have write accesses to the structure then
>> >>>bind all the structures to the owner. Then kasan will do the "owner check"
>> >>>after the basic checks.
>> >>>
>> >>>As implementation, kasan provides a API to it's user to register their
>> >>>allowed functions. The API returns a token to users.  At run time, users
>> >>>bind the memory ranges they are interested in to the check they registered.
>> >>>Kasan then checks the bound memory ranges with the allowed functions.
>> >>>
>> >>>
>> >>>Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>> >Hello, Wengang.
>> >
>> >Nice idea. I also think that we need this kind of debugging tool. It's very
>> >hard to detect overwritten bugs.
>> >
>> >In fact, I made a quite similar tool, valid access checker (A.K.A.
>> >vchecker). See the following link.
>> >
>> >https://github.com/JoonsooKim/linux/tree/vchecker-master-v0.3-next-20170106
>> >
>> >Vchecker has some advanced features compared to yours.
>> >
>> >1. Target object can be choosen at runtime by debugfs. It doesn't
>> >require re-compile to register the target object.
>> Hi Joonsoo, good to know you are also interested in this!
>>
>> Yes, if can be choosen via debugfs, it doesn't need re-compile.
>> Well, I wonder what do you expect to be chosen from use space?
>
> As you mentioned somewhere, this tool can be used when we find the
> overwritten happend on some particular victims. I assumes that most of
> the problem would happen on slab objects and userspace can choose the
> target slab cache via debugfs interface of the vchecker.


Most objects are allocated from kmalloc slabs. And this feature can't
work for all objects allocated from a kmalloc slab. I think checks
needs to be tied to allocation sites.


>
>>
>> >
>> >2. It has another feature that checks the value stored in the object.
>> >Usually, invalid writer stores odd value into the object and vchecker
>> >can detect this case.
>> It's good to do the check. Well, as I understand, it tells something
>> bad (overwitten) happened.
>> But it can't tell who did the overwritten, right?  (I didn't look at
>> your patch yet,) do you recall the last write somewhere?
>
> Yes, it stores the callstack of the last write and report it when
> the error is found.
>
>>
>> >
>> >3. It has a callstack checker (memory owner checker in yours). It
>> >checks all the callstack rather than just the caller. It's important
>> >since invalid writer could call the parent function of owner function
>> >and it would not be catched by checking just the caller.
>> >
>> >4. The callstack checker is more automated. vchecker collects the valid
>> >callstack by running the system.
>> I think we can merge the above two into one.
>> So you are doing full stack check.  Well, finding out the all the
>> paths which have the write access may be not a very easy thing.
>> Missing some paths may cause dmesg flooding, and those log won't
>> help at all. Finding out all the (owning) caller only is relatively
>> much easier.
>
> Vchecker can be easily modified to store only the caller. It just
> requires modifying callstack depth parameter so it's so easy.
> Moreover, it can be accomplished by adding debugfs interface.
>
> Anyway, I don't think that finding out all the (owning) caller only
> is much easier. Think about dentry or inode object. It is accessed by
> various code path and it's not easy to cover all the owning caller by
> manual approach.
>
>
>> There do is the case you pointed out here. In this case, the
>> debugger can make slight change to the calling path. And as I
>> understand,
>> most of the overwritten are happening in quite different call paths,
>> they are not calling the (owning) caller.
>
> Agreed.
>
>>
>> >
>> >FYI, I attach some commit descriptions of the vchecker.
>> >
>> >     vchecker: store/report callstack of value writer
>> >     The purpose of the value checker is finding invalid user writing
>> >     invalid value at the moment that the value is written. However, there is
>> >     a missing infrastructure that passes writing value to the checker
>> >     since we temporarilly piggyback on the KASAN. So, we cannot easily
>> >     detect this case in time.
>> >     However, by following way, we can emulate similar effect.
>> >     1. Store callstack when memory is written.
>>
>> Oh, seems you are storing the callstack for each write. -- I am not
>> sure if that would too heavy.
>
> Unlike KASAN that checks all type of the objects, this debugging
> feature is only enabled on the specific type of the objects so
> overhead would not be too heavy in terms of system overall
> performance.
>
>> Actually I was thinking to have a check on the new value. But seems
>> compiler doesn't provide that.
>
> Yes, look like we have a similar idea. I have some another ideas if
> ASAN hook provides the value to be written. However, it's not
> supported by compiler yet.
>
>> >     2. If check is failed in next access, report previous write-access
>> >     callstack
>> >     It will caught offending user properly.
>> >     Following output "Call trace: Invalid writer" part is the result
>> >     of this patch. We find the invalid value at workfn+0x71 but report
>> >     writer at workfn+0x61.
>> >     [  133.024076] ==================================================================
>> >     [  133.025576] BUG: VCHECKER: invalid access in workfn+0x71/0xc0 at addr ffff8800683dd6c8
>> >     [  133.027196] Read of size 8 by task kworker/1:1/48
>> >     [  133.028020] 0x8 0x10 value
>> >     [  133.028020] 0xffff 4
>> >     [  133.028020] Call trace: Invalid writer
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff81043b1b>] save_stack_trace+0x1b/0x20
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff812c0db9>] save_stack+0x39/0x70
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff812c0fe3>] check_value+0x43/0x80
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff812c1762>] vchecker_check+0x1c2/0x380
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff812be49d>] __asan_store8+0x8d/0xc0
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff815eadd1>] workfn+0x61/0xc0
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff810be3df>] process_one_work+0x28f/0x680
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff810bf272>] worker_thread+0xa2/0x870
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff810c86a5>] kthread+0x195/0x1e0
>> >     [  133.028020]
>> >     [  133.028020] [<ffffffff81b9d3d2>] ret_from_fork+0x22/0x30
>> >     [  133.028020] CPU: 1 PID: 48 Comm: kworker/1:1 Not tainted 4.10.0-rc2-next-20170106+ #1179
>> >     [  133.028020] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> >     [  133.028020] Workqueue: events workfn
>> >     [  133.028020] Call Trace:
>> >     [  133.028020]  dump_stack+0x4d/0x63
>> >     [  133.028020]  kasan_object_err+0x21/0x80
>> >     [  133.028020]  vchecker_check+0x2af/0x380
>> >     [  133.028020]  ? workfn+0x71/0xc0
>> >     [  133.028020]  ? workfn+0x71/0xc0
>> >     [  133.028020]  __asan_load8+0x87/0xb0
>> >     [  133.028020]  workfn+0x71/0xc0
>> >     [  133.028020]  process_one_work+0x28f/0x680
>> >     [  133.028020]  worker_thread+0xa2/0x870
>> >     [  133.028020]  kthread+0x195/0x1e0
>> >     [  133.028020]  ? put_pwq_unlocked+0xc0/0xc0
>> >     [  133.028020]  ? kthread_park+0xd0/0xd0
>> >     [  133.028020]  ret_from_fork+0x22/0x30
>> >     [  133.028020] Object at ffff8800683dd6c0, in cache vchecker_test size: 24
>> >     [  133.028020] Allocated:
>> >     [  133.028020] PID = 48
>> >
>> >
>> >     vchecker: Add 'callstack' checker
>> >     The callstack checker is to find invalid code paths accessing to a
>> >     certain field in an object.  Currently it only saves all stack traces at
>> >     the given offset.  Reporting will be added in the next patch.
>> >     The below example checks callstack of anon_vma:
>> >       # cd /sys/kernel/debug/vchecker
>> >       # echo 0 8 > anon_vma/callstack  # offset 0, size 8
>> >       # echo 1 > anon_vma/enable
>> an echo "anon_vma" > <something> first?
>> How do you define and path the valid (owning) full stack to kasan?
>
> This interface only enables to store all the callstacks. No validation
> check here. I think that this feature would also be helpful to debug.
> If error happens, we can check all the previous callstacks and track
> the buggy caller.
>
>> >       # cat anon_vma/callstack        # show saved callstacks
>> >       0x0 0x8 callstack
>> >       total: 42
>> >       callstack #0
>> >         anon_vma_fork+0x101/0x280
>> >         copy_process.part.10+0x15ff/0x2a40
>> >         _do_fork+0x155/0x7d0
>> >         SyS_clone+0x19/0x20
>> >         do_syscall_64+0xdf/0x460
>> >         return_from_SYSCALL_64+0x0/0x7a
>> >       ...
>> >
>> >
>> >     vchecker: Support toggle on/off of callstack check
>> >     By default, callstack checker only collects callchains.  When a user
>> >     writes 'on' to the callstack file in debugfs, it checks and reports new
>> >     callstacks.  Writing 'off' to disable it again.
>> >       # cd /sys/kernel/debug/vchecker
>> >       # echo 0 8 > anon_vma/callstack
>> >       # echo 1 > anon_vma/enable
>> >       ... (do some work to collect enough callstacks) ...
>> How to define "enough" here?
>
> The bug usually doesn't happen immediately since it usually happens on
> the corner case. When debugging, we run the workload that causes the
> bug and then wait for some time until the bug happens. "Enough" can
> be defined as the middle of this waiting time. After some warm-up
> time, all the common callstack would be collected. Then,
> switching on this feature that reports a new callstack. If the corner
> case that is on a new callstack happens, this new callstack will be
> reported and we can check whether it is a true bug or not.
>
>> >       # echo on > anon_vma/callstack
>> >
>> >The reason I didn't submit the vchecker to mainline is that I didn't find
>> >the case that this tool is useful in real life. Most of the system broken case
>> >can be debugged by other ways. Do you see the real case that this tool is
>> >helpful? If so, I think that vchecker is more appropriate to be upstreamed.
>> >Could you share your opinion?
>> Yes, people find other ways to solve overwritten issue (so did I) in
>> the past. If kasan doesn't provide this functionality, developers
>> have no way to choose it.
>> Though people have other ways to find the root cause, the other ways
>> maybe take (maybe much) longer. I didn't solve problems with the
>> owner check yet since I just make available recently.  But
>> considering the overwritten issues I have ever hit, the owner check
>> definitely helps and I definitely will try the owner check when I
>> have a new overwritten issue!
>>
>> Why not send your patch for review?
>
> Okay! I hope to find more people that have interest on this feature
> and it seems that you are the one of them. :)
>
> I will send my patches soon. I think that we can be cooperative to
> improve this feature.
>
> Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Andrey Ryabinin]

On 11/18/2017 01:30 AM, Wengang Wang wrote:
> Kasan advanced check, I'm going to add this feature.
> Currently Kasan provide the detection of use-after-free and out-of-bounds
> problems. It is not able to find the overwrite-on-allocated-memory issue.
> We sometimes hit this kind of issue: We have a messed up structure
> (usually dynamially allocated), some of the fields in the structure were
> overwritten with unreasaonable values. And kernel may panic due to those
> overeritten values. We know those fields were overwritten somehow, but we
> have no easy way to find out which path did the overwritten. The advanced
> check wants to help in this scenario.
> 
> The idea is to define the memory owner. When write accesses come from
> non-owner, error should be reported. Normally the write accesses on a given
> structure happen in only several or a dozen of functions if the structure
> is not that complicated. We call those functions "allowed functions".
> The work of defining the owner and binding memory to owner is expected to
> be done by the memory consumer. In the above case, memory consume register
> the owner as the functions which have write accesses to the structure then
> bind all the structures to the owner. Then kasan will do the "owner check"
> after the basic checks.
> 
> As implementation, kasan provides a API to it's user to register their
> allowed functions. The API returns a token to users.  At run time, users
> bind the memory ranges they are interested in to the check they registered.
> Kasan then checks the bound memory ranges with the allowed functions.
> 

NAK. We don't add APIs with no users in the kernel.
If nothing in the kernel uses this API than there is no way to tell if this works or not.

Besides, I'm bit skeptical about usefulness of this feature. Those kinds of issues that
advanced check is supposed to catch, is almost always is just some sort of longstanding
use after free, which eventually should be caught by kasan.



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

Re: [PATCH 0/5] mm/kasan: advanced check

[Andrey Ryabinin]

On 11/20/2017 04:50 AM, Joonsoo Kim wrote:
> 
> The reason I didn't submit the vchecker to mainline is that I didn't find
> the case that this tool is useful in real life. Most of the system broken case
> can be debugged by other ways. Do you see the real case that this tool is
> helpful? If so, I think that vchecker is more appropriate to be upstreamed.
> Could you share your opinion?
> 

Isn't everything that vchecker can do and far beyond that can be done via systemtap
script using watchpoints?


> Thanks.
> 

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Wengang Wang]

On 2017/11/22 4:04, Andrey Ryabinin wrote:
> On 11/18/2017 01:30 AM, Wengang Wang wrote:
>> Kasan advanced check, I'm going to add this feature.
>> Currently Kasan provide the detection of use-after-free and out-of-bounds
>> problems. It is not able to find the overwrite-on-allocated-memory issue.
>> We sometimes hit this kind of issue: We have a messed up structure
>> (usually dynamially allocated), some of the fields in the structure were
>> overwritten with unreasaonable values. And kernel may panic due to those
>> overeritten values. We know those fields were overwritten somehow, but we
>> have no easy way to find out which path did the overwritten. The advanced
>> check wants to help in this scenario.
>>
>> The idea is to define the memory owner. When write accesses come from
>> non-owner, error should be reported. Normally the write accesses on a given
>> structure happen in only several or a dozen of functions if the structure
>> is not that complicated. We call those functions "allowed functions".
>> The work of defining the owner and binding memory to owner is expected to
>> be done by the memory consumer. In the above case, memory consume register
>> the owner as the functions which have write accesses to the structure then
>> bind all the structures to the owner. Then kasan will do the "owner check"
>> after the basic checks.
>>
>> As implementation, kasan provides a API to it's user to register their
>> allowed functions. The API returns a token to users.  At run time, users
>> bind the memory ranges they are interested in to the check they registered.
>> Kasan then checks the bound memory ranges with the allowed functions.
>>
> NAK. We don't add APIs with no users in the kernel.
> If nothing in the kernel uses this API than there is no way to tell if this works or not.
In production kernel, we don't want unnecessary APIs without users in 
the kernel because that
would consume binary size (a pure space waste) and leave "dead" code.
KASAN code is a bit different from other kernel components, its self is 
debugging purpose only.
When KASAN is enabled, the APIs would have potential users and the code 
is not "dead" code.
The size increasing in binary would be acceptable since the kernel with 
KASAN enabled only has
a short time life -- only used to find the root cause, when root caused 
is found, it will be no
longer used;A  Also the KASAN enabled kernel is used by limited user 
where they have a particular
issue. I say "potential users" because this functionality its self is 
dynamically used or to say a
one-shot use. The functionality is helpful.

I think even KASAN its self we don't know if it works or not when it is 
not enabled.
-- Before I tried it, I am curious if this can work well; After testing 
it, I know it works.
If we don't give users the chance, they will never know there is such a 
functionality and will never
get benefit from it.


> Besides, I'm bit skeptical about usefulness of this feature. Those kinds of issues that
> advanced check is supposed to catch, is almost always is just some sort of longstanding
> use after free, which eventually should be caught by kasan.
Yes, if luckily, the issue is possible to be catched by UAF check.
Well considering busy production systems, the memory is very likely to 
be reallocated rather than
staying in free state for very long time.A  That is the 
overwritten-to-allocated-memory is more
likely to happen than UAF does I think.A  When 
overwritten-to-allocated-memory happened,
UAF check has no chance to detect the problem.

KASAN is helpful to detect problematic memory usage, so does this patch set!
I really hope this can be included and developers can get benefit from it.

Thanks,
Wengang

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Wengang Wang]

On 2017/11/21 20:30, Joonsoo Kim wrote:
> On Mon, Nov 20, 2017 at 11:56:05AM -0800, Wengang wrote:
>>
>> On 11/19/2017 05:50 PM, Joonsoo Kim wrote:
>>> On Fri, Nov 17, 2017 at 11:56:21PM +0100, Dmitry Vyukov wrote:
>>>> On Fri, Nov 17, 2017 at 11:30 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
>>>>> Kasan advanced check, I'm going to add this feature.
>>>>> Currently Kasan provide the detection of use-after-free and out-of-bounds
>>>>> problems. It is not able to find the overwrite-on-allocated-memory issue.
>>>>> We sometimes hit this kind of issue: We have a messed up structure
>>>>> (usually dynamially allocated), some of the fields in the structure were
>>>>> overwritten with unreasaonable values. And kernel may panic due to those
>>>>> overeritten values. We know those fields were overwritten somehow, but we
>>>>> have no easy way to find out which path did the overwritten. The advanced
>>>>> check wants to help in this scenario.
>>>>>
>>>>> The idea is to define the memory owner. When write accesses come from
>>>>> non-owner, error should be reported. Normally the write accesses on a given
>>>>> structure happen in only several or a dozen of functions if the structure
>>>>> is not that complicated. We call those functions "allowed functions".
>>>>> The work of defining the owner and binding memory to owner is expected to
>>>>> be done by the memory consumer. In the above case, memory consume register
>>>>> the owner as the functions which have write accesses to the structure then
>>>>> bind all the structures to the owner. Then kasan will do the "owner check"
>>>>> after the basic checks.
>>>>>
>>>>> As implementation, kasan provides a API to it's user to register their
>>>>> allowed functions. The API returns a token to users.  At run time, users
>>>>> bind the memory ranges they are interested in to the check they registered.
>>>>> Kasan then checks the bound memory ranges with the allowed functions.
>>>>>
>>>>>
>>>>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>>> Hello, Wengang.
>>>
>>> Nice idea. I also think that we need this kind of debugging tool. It's very
>>> hard to detect overwritten bugs.
>>>
>>> In fact, I made a quite similar tool, valid access checker (A.K.A.
>>> vchecker). See the following link.
>>>
>>> https://github.com/JoonsooKim/linux/tree/vchecker-master-v0.3-next-20170106
>>>
>>> Vchecker has some advanced features compared to yours.
>>>
>>> 1. Target object can be choosen at runtime by debugfs. It doesn't
>>> require re-compile to register the target object.
>> Hi Joonsoo, good to know you are also interested in this!
>>
>> Yes, if can be choosen via debugfs, it doesn't need re-compile.
>> Well, I wonder what do you expect to be chosen from use space?
> As you mentioned somewhere, this tool can be used when we find the
> overwritten happend on some particular victims. I assumes that most of
> the problem would happen on slab objects and userspace can choose the
> target slab cache via debugfs interface of the vchecker.
Yes, I agree it would be slab objects.
If there is a way to set the slab objects to be subject of check via 
name, it is good.
One question is how about common kmalloc slabs?A  They are widely used 
and many
problems happens with them.

>
>>> 2. It has another feature that checks the value stored in the object.
>>> Usually, invalid writer stores odd value into the object and vchecker
>>> can detect this case.
>> It's good to do the check. Well, as I understand, it tells something
>> bad (overwitten) happened.
>> But it can't tell who did the overwritten, right?  (I didn't look at
>> your patch yet,) do you recall the last write somewhere?
> Yes, it stores the callstack of the last write and report it when
> the error is found.
>
>>> 3. It has a callstack checker (memory owner checker in yours). It
>>> checks all the callstack rather than just the caller. It's important
>>> since invalid writer could call the parent function of owner function
>>> and it would not be catched by checking just the caller.
>>>
>>> 4. The callstack checker is more automated. vchecker collects the valid
>>> callstack by running the system.
>> I think we can merge the above two into one.
>> So you are doing full stack check.  Well, finding out the all the
>> paths which have the write access may be not a very easy thing.
>> Missing some paths may cause dmesg flooding, and those log won't
>> help at all. Finding out all the (owning) caller only is relatively
>> much easier.
> Vchecker can be easily modified to store only the caller. It just
> requires modifying callstack depth parameter so it's so easy.
> Moreover, it can be accomplished by adding debugfs interface.

That's good.
> Anyway, I don't think that finding out all the (owning) caller only
> is much easier. Think about dentry or inode object. It is accessed by
> various code path and it's not easy to cover all the owning caller by
> manual approach.
Comparing to finding out full stack, it's much easier.A  If we take 
dentry as example,
I agree dentries are widely accessed and maybe finding out all the 
owning caller is not that
easy, but comparing to finding out the full stack, it's easier.

>
>
>> There do is the case you pointed out here. In this case, the
>> debugger can make slight change to the calling path. And as I
>> understand,
>> most of the overwritten are happening in quite different call paths,
>> they are not calling the (owning) caller.
> Agreed.
>
>>> FYI, I attach some commit descriptions of the vchecker.
>>>
>>>      vchecker: store/report callstack of value writer
>>>      The purpose of the value checker is finding invalid user writing
>>>      invalid value at the moment that the value is written. However, there is
>>>      a missing infrastructure that passes writing value to the checker
>>>      since we temporarilly piggyback on the KASAN. So, we cannot easily
>>>      detect this case in time.
>>>      However, by following way, we can emulate similar effect.
>>>      1. Store callstack when memory is written.
>> Oh, seems you are storing the callstack for each write. -- I am not
>> sure if that would too heavy.
> Unlike KASAN that checks all type of the objects, this debugging
> feature is only enabled on the specific type of the objects so
> overhead would not be too heavy in terms of system overall
> performance.
Yes, only specific type of objects do the extra stuff, but I am not sure 
if the overall
performance to be affected. Actually I was thinking of tracking last 
write stack.
At that time, I had two concerns: one is the performance affect; the 
other is if it's safe
since memory access can happen in any context -- process context, soft 
irq and irq..

BTW, how much extra memory is needed for each objects?

>
>> Actually I was thinking to have a check on the new value. But seems
>> compiler doesn't provide that.
> Yes, look like we have a similar idea. I have some another ideas if
> ASAN hook provides the value to be written. However, it's not
> supported by compiler yet.

Right!

>
>>>      2. If check is failed in next access, report previous write-access
>>>      callstack
>>>      It will caught offending user properly.
>>>      Following output "Call trace: Invalid writer" part is the result
>>>      of this patch. We find the invalid value at workfn+0x71 but report
>>>      writer at workfn+0x61.
>>>      [  133.024076] ==================================================================
>>>      [  133.025576] BUG: VCHECKER: invalid access in workfn+0x71/0xc0 at addr ffff8800683dd6c8
>>>      [  133.027196] Read of size 8 by task kworker/1:1/48
>>>      [  133.028020] 0x8 0x10 value
>>>      [  133.028020] 0xffff 4
>>>      [  133.028020] Call trace: Invalid writer
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff81043b1b>] save_stack_trace+0x1b/0x20
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff812c0db9>] save_stack+0x39/0x70
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff812c0fe3>] check_value+0x43/0x80
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff812c1762>] vchecker_check+0x1c2/0x380
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff812be49d>] __asan_store8+0x8d/0xc0
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff815eadd1>] workfn+0x61/0xc0
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff810be3df>] process_one_work+0x28f/0x680
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff810bf272>] worker_thread+0xa2/0x870
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff810c86a5>] kthread+0x195/0x1e0
>>>      [  133.028020]
>>>      [  133.028020] [<ffffffff81b9d3d2>] ret_from_fork+0x22/0x30
>>>      [  133.028020] CPU: 1 PID: 48 Comm: kworker/1:1 Not tainted 4.10.0-rc2-next-20170106+ #1179
>>>      [  133.028020] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>>      [  133.028020] Workqueue: events workfn
>>>      [  133.028020] Call Trace:
>>>      [  133.028020]  dump_stack+0x4d/0x63
>>>      [  133.028020]  kasan_object_err+0x21/0x80
>>>      [  133.028020]  vchecker_check+0x2af/0x380
>>>      [  133.028020]  ? workfn+0x71/0xc0
>>>      [  133.028020]  ? workfn+0x71/0xc0
>>>      [  133.028020]  __asan_load8+0x87/0xb0
>>>      [  133.028020]  workfn+0x71/0xc0
>>>      [  133.028020]  process_one_work+0x28f/0x680
>>>      [  133.028020]  worker_thread+0xa2/0x870
>>>      [  133.028020]  kthread+0x195/0x1e0
>>>      [  133.028020]  ? put_pwq_unlocked+0xc0/0xc0
>>>      [  133.028020]  ? kthread_park+0xd0/0xd0
>>>      [  133.028020]  ret_from_fork+0x22/0x30
>>>      [  133.028020] Object at ffff8800683dd6c0, in cache vchecker_test size: 24
>>>      [  133.028020] Allocated:
>>>      [  133.028020] PID = 48
>>>
>>>
>>>      vchecker: Add 'callstack' checker
>>>      The callstack checker is to find invalid code paths accessing to a
>>>      certain field in an object.  Currently it only saves all stack traces at
>>>      the given offset.  Reporting will be added in the next patch.
>>>      The below example checks callstack of anon_vma:
>>>        # cd /sys/kernel/debug/vchecker
>>>        # echo 0 8 > anon_vma/callstack  # offset 0, size 8
>>>        # echo 1 > anon_vma/enable
>> an echo "anon_vma" > <something> first?
>> How do you define and path the valid (owning) full stack to kasan?
> This interface only enables to store all the callstacks. No validation
> check here. I think that this feature would also be helpful to debug.
> If error happens, we can check all the previous callstacks and track
> the buggy caller.
Too much extra memory needed for each object? or you stores in just one 
global copy.

>
>>>        # cat anon_vma/callstack        # show saved callstacks
>>>        0x0 0x8 callstack
>>>        total: 42
>>>        callstack #0
>>>          anon_vma_fork+0x101/0x280
>>>          copy_process.part.10+0x15ff/0x2a40
>>>          _do_fork+0x155/0x7d0
>>>          SyS_clone+0x19/0x20
>>>          do_syscall_64+0xdf/0x460
>>>          return_from_SYSCALL_64+0x0/0x7a
>>>        ...
>>>
>>>
>>>      vchecker: Support toggle on/off of callstack check
>>>      By default, callstack checker only collects callchains.  When a user
>>>      writes 'on' to the callstack file in debugfs, it checks and reports new
>>>      callstacks.  Writing 'off' to disable it again.
>>>        # cd /sys/kernel/debug/vchecker
>>>        # echo 0 8 > anon_vma/callstack
>>>        # echo 1 > anon_vma/enable
>>>        ... (do some work to collect enough callstacks) ...
>> How to define "enough" here?
> The bug usually doesn't happen immediately since it usually happens on
> the corner case. When debugging, we run the workload that causes the
> bug and then wait for some time until the bug happens. "Enough" can
> be defined as the middle of this waiting time. After some warm-up
> time, all the common callstack would be collected. Then,
> switching on this feature that reports a new callstack. If the corner
> case that is on a new callstack happens, this new callstack will be
> reported and we can check whether it is a true bug or not.
What if it's not?
I am still not convinced on if we can get "enough". We may never have a 
workload that
make sure it covers all call stacks.

>
>>>        # echo on > anon_vma/callstack
>>>
>>> The reason I didn't submit the vchecker to mainline is that I didn't find
>>> the case that this tool is useful in real life. Most of the system broken case
>>> can be debugged by other ways. Do you see the real case that this tool is
>>> helpful? If so, I think that vchecker is more appropriate to be upstreamed.
>>> Could you share your opinion?
>> Yes, people find other ways to solve overwritten issue (so did I) in
>> the past. If kasan doesn't provide this functionality, developers
>> have no way to choose it.
>> Though people have other ways to find the root cause, the other ways
>> maybe take (maybe much) longer. I didn't solve problems with the
>> owner check yet since I just make available recently.  But
>> considering the overwritten issues I have ever hit, the owner check
>> definitely helps and I definitely will try the owner check when I
>> have a new overwritten issue!
>>
>> Why not send your patch for review?
> Okay! I hope to find more people that have interest on this feature
> and it seems that you are the one of them. :)
Right!

> I will send my patches soon. I think that we can be cooperative to
> improve this feature.
>
> Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Wengang Wang]

On 2017/11/22 0:48, Dmitry Vyukov wrote:
> On Tue, Nov 21, 2017 at 8:17 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
>>>>> On Mon, Nov 20, 2017 at 9:05 PM, Wengang <wen.gang.wang@oracle.com>
>>>>> wrote:
>>>>>>
>>>>>> On 11/20/2017 12:41 AM, Dmitry Vyukov wrote:
>>>>>>>
>>>>>>>> The reason I didn't submit the vchecker to mainline is that I didn't
>>>>>>>> find
>>>>>>>> the case that this tool is useful in real life. Most of the system
>>>>>>>> broken
>>>>>>>> case
>>>>>>>> can be debugged by other ways. Do you see the real case that this
>>>>>>>> tool
>>>>>>>> is
>>>>>>>> helpful?
>>>>>>> Hi,
>>>>>>>
>>>>>>> Yes, this is the main question here.
>>>>>>> How is it going to be used in real life? How widely?
>>>>>>>
>>>>>> I think the owner check can be enabled in the cases where KASAN is
>>>>>> used.
>>>>>> --
>>>>>> That is that we found there is memory issue, but don't know how it
>>>>>> happened.
>>>>>
>>>>> But KASAN generally pinpoints the corruption as it happens. Why do we
>>>>> need something else?
>>>>
>>>> Currently (without this patch set) kasan can't detect the overwritten
>>>> issues
>>>> that happen on allocated memory.
>>>>
>>>> Say, A allocated a 128 bytes memory and B write to that memory at offset
>>>> 0
>>>> with length 100 unexpectedly.  Currently kasan won't report error for any
>>>> writing to the offset 0 with len <= 128 including the B writting.  This
>>>> patch lets kasan report the B writing to offset 0 with length 100.
>>>
>>> So this will be used for manual debugging and you don't have plans to
>>> annotate kernel code with additional tags, right?
>> I am not sure what do you mean by "manual debugging". What is needed to use
>> the owner check is:
>> The memory user needs to do:
>> 1)  code change: register the checker with the allowed functions
>> 2)  code change: bind the memory to the checker
>> 3)  recompile the kernel
>> 4)  run with the recompiled kernel and reproduce the issue
> Yes, I meant exactly this -- developer does manual work, including
> code changes, on a per-bug basis.
> This is not the current usage model for KASAN, hence I am asking.

OK, I see. Your understanding is correct.
>
>> By "additional tags", if you meant "add some explanation comment", I think
>> one can refer to the commit message about the code change;
>> if you meant "additional kernel config item to enable/disable code", I have
>> no such plan.  If no "owner checker" is registered, it just acts like the
>> basic kasan (without this patch) with almost same performance. Even with
>> "owner checker" registered,  and memories are bound to the checker,  it's
>> still the rare case to do the owner check. So the overheard caused by owner
>> check is slight. I don't find the reason we need an additional kernel
>> config.
> I meant committing some registration of allowed functions and binding
> of objects to tags into mainline kernel.
>
>
>
>
>>> If this meant to be used by kernel developers during debugging, this
>>> feature needs to be documented in Documentation/dev-tools/kasan.rst
>>> including an example. It's hard to spread knowledge about such
>>> features especially if there are no mentions in docs. Documentation
>>> can then be quickly referenced e.g. as a suggestion of how to tackle a
>>> particular bug.
>> Yes, this is a good idea. I was/am thinking so.
>>
>>> General comments:
>>>
>>> 1. The check must not affect fast-path. I think we need to move it
>>> into kasan_report (and then rename kasan_report to something else).
>>> Closer to what Joonsoo did in his version, but move then check even
>>> further. This will also make inline instrumentation work because it
>>> calls kasan_report, then kasan_report will do the additional check and
>>> potentially return without actually reporting a bug.
>>> The idea is that the check reserves some range of bad values in shadow
>>> and poison the object with that special value. Then all accesses to
>>> the protected memory will be detected as bad and go into kasan_report.
>>> Then kasan_report will do the additional check and potentially return
>>> without reporting.
>>> This has 0 overhead when the feature is not used, enables inline
>>> instrumentation and is less intrusive.
>> The owner check can be moved to kasan_report() by letting the poison check
>> routine return "possible violation" when the memory is bound to a owner
>> and then kasan_report() will get the chance to do further (owner) check.
>>
>> Well I wonder how that moving would benefit.
>> If the purpose is to remove overhead,   the moving didn't remove of any run
>> of
>> owner check. It would just move it to a different place and it will run just
>> a bit later.
>> I think even current implementation, it has almost 0 overhead when no memory
>> is
>> bound to owners.  The owner check is performed only when the memory is bound
>> (the
>> bound check is light), if memory is not bound, no owner check is performed.
> 3 main goals as I outlined:
>   - removing _all_ overhead when the feature is not used
Anyhow, one can't make it really 0 overhead.
For example, when the feature is in code, we have to check if the 
feature is enabled.
We may do that like this code:
 A A A  if (xx feature is enabled) {
 A A A A A A A A A A A  do feature related stuff.
 A A A  }

So anyway the "if (xx feature is enabled)" is needed and is not 0 overhead.
In my cause, the overhead would be:
 A A A A  if (is memory is bound).

>   - making inline instrumentation work
I want this to be done too.


>   - code separation
Yes, when possible and not introduce badness.
>
>> I am predicting the code that has owner check routine moved to
>> kasan_report(), it
>> should be like this:
>> (fake code)
>> in poison check routines:
>>         ...
>>         after all case that returns "Yes",
>>         if bound check returns true (memory is bound):    --> bound check is
>> here
>>               return "possible"
> /\/\/\/\
>
> No, just return "possible". Main routine won't know anything about
> bounds and owners.

Right, caller has same action on "Yes" or "Possible", I just wanted make 
the logic more clear
to understand.

>
>
>>         ...
>> in the caller of poison check routines:
>>         ...
>>         if poison check routine returns "yes" or "possible":
>>               calls kasan_report()
>>
>> in kasan_report():
>>         ....
>>         if no basic violation found:
>>             run owner check
>> --> owner check is here
>>         ...
>>
>> Current code is like this:
>> in poison check routines:
>>         ...
>>         after all case that returns "yes",
>>         if bound check returns true (memory is bound):  --> bound check is
>> here
>>                 run owner check
>> --> owner check is here
>>
>> Comparing to current implementation,
>> anyway the "bound check" is done either in the poison check routines or in
>> kasan_report().
>> anyway the "owner check" is done either in the poison check routines or in
>> kasan_report().
>> I don't see we have reduced number of calls of "bound check" and/or "owner
>> check".
>> Can you pinpoint which part will be reduced?
>>
>> If the purpose is to make inline instrumentation work for owner check, it
>> interests
>> me!  This implementation only works fine in outline instrumentation and
>> seems the
>> poison checks are not called at all with inline compile type. Could you
>> share more on this?
>>
>> The badness of moving owner check to kasan_report() is that it breaks the
>> function
>> clearness in the code.  From this point of view, check is just check, it
>> should say "yes" or
>> "no", not "possible";  report is just report, no checks should be performed
>> in report.
>>
>>> 2. Moving this to a separate .c/.h files sounds like a good idea.
>>> kasan.c is a bit of a mess already. If we do (1), changes to kasan.c
>>> will be minimal. Again closer to what Joonsoo did.
>> If the owner checks would remain in the poison check routines, it would be
>> in kasan.c.
>> If we have enough points to support the moving, say that makes inline
>> instrumentation
>> work, it can be in a separated .c/.h and yes that would be better then.
>>
>>> 3. We need to rename it from "advanced" to something else (owner
>>> check?). Features must be named based on what they do, rather then how
>>> advanced they are. If we add other complex checks, how should we name
>>> them? even_more_advanced?
>>
>> LoL,  No and Yes.
>> The feature I am adding is "owner check" and I define it as one of the
>> "advanced check",
>> By looking at the patch its self (especially enum kasan_adv_chk_type in
>> patch 4/5)  , you
>> can see, I was leaving spaces for other kind of "advanced checks". And
>> (future) different
>> "advanced checks" can be added -- say "old value validation", "new value
>> validation"
>>   -- though the new value is not  supported by compiler yet.  But yes the
>> name "advanced"
>> is really not what I want, but I failed to find an accurate one. How do you
>> think?
>>
>>
>>> I am fine with adding such feature provided that it does not affect
>>> performance/memory consumption if not used, works with inline
>>> instrumentation and is separated into separate files. But it also
>>> needs to be advertised somehow among kernel developers, otherwise only
>>> you guys will use it.
>> So far it should has almost same performance if feature is not used;
>> definitely
>> no more memory consumption.  Now it doesn't work with inline
>> instrumentation,
>> could you share more information on how to make it also work with inline
>> mode?
> Move the check into kasan_report and leave the rest of the code as is.

So the kasan_report() is anyway called (I don't think so)? or it needs 
some special
poison value set in shadows bytes then inline instrumentation calls 
kasan_report()
according to the special poison value? Could you please point me the 
compiler
spec if you know it?

thanks,
wengang
>
>> It technically can be moved to separated files. I will add the doc.
>>
>> thanks,
>> wengang

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Joonsoo Kim]

On Wed, Nov 22, 2017 at 03:04:51PM +0300, Andrey Ryabinin wrote:
> On 11/20/2017 04:50 AM, Joonsoo Kim wrote:
> > 
> > The reason I didn't submit the vchecker to mainline is that I didn't find
> > the case that this tool is useful in real life. Most of the system broken case
> > can be debugged by other ways. Do you see the real case that this tool is
> > helpful? If so, I think that vchecker is more appropriate to be upstreamed.
> > Could you share your opinion?
> > 
> 
> Isn't everything that vchecker can do and far beyond that can be done via systemtap
> script using watchpoints?

I don't have enough knowledge about systemtap however I guess that
it's not possible. vchecker uses the ASAN hooks which are placed on
all memory read/write around the kernel. We cannot easily add
watchpoints for them.

Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Joonsoo Kim]

On Wed, Nov 22, 2017 at 09:51:11AM +0100, Dmitry Vyukov wrote:
> On Wed, Nov 22, 2017 at 5:30 AM, Joonsoo Kim <iamjoonsoo.kim@lge.com> wrote:
> > On Mon, Nov 20, 2017 at 11:56:05AM -0800, Wengang wrote:
> >>
> >>
> >> On 11/19/2017 05:50 PM, Joonsoo Kim wrote:
> >> >On Fri, Nov 17, 2017 at 11:56:21PM +0100, Dmitry Vyukov wrote:
> >> >>On Fri, Nov 17, 2017 at 11:30 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
> >> >>>Kasan advanced check, I'm going to add this feature.
> >> >>>Currently Kasan provide the detection of use-after-free and out-of-bounds
> >> >>>problems. It is not able to find the overwrite-on-allocated-memory issue.
> >> >>>We sometimes hit this kind of issue: We have a messed up structure
> >> >>>(usually dynamially allocated), some of the fields in the structure were
> >> >>>overwritten with unreasaonable values. And kernel may panic due to those
> >> >>>overeritten values. We know those fields were overwritten somehow, but we
> >> >>>have no easy way to find out which path did the overwritten. The advanced
> >> >>>check wants to help in this scenario.
> >> >>>
> >> >>>The idea is to define the memory owner. When write accesses come from
> >> >>>non-owner, error should be reported. Normally the write accesses on a given
> >> >>>structure happen in only several or a dozen of functions if the structure
> >> >>>is not that complicated. We call those functions "allowed functions".
> >> >>>The work of defining the owner and binding memory to owner is expected to
> >> >>>be done by the memory consumer. In the above case, memory consume register
> >> >>>the owner as the functions which have write accesses to the structure then
> >> >>>bind all the structures to the owner. Then kasan will do the "owner check"
> >> >>>after the basic checks.
> >> >>>
> >> >>>As implementation, kasan provides a API to it's user to register their
> >> >>>allowed functions. The API returns a token to users.  At run time, users
> >> >>>bind the memory ranges they are interested in to the check they registered.
> >> >>>Kasan then checks the bound memory ranges with the allowed functions.
> >> >>>
> >> >>>
> >> >>>Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> >> >Hello, Wengang.
> >> >
> >> >Nice idea. I also think that we need this kind of debugging tool. It's very
> >> >hard to detect overwritten bugs.
> >> >
> >> >In fact, I made a quite similar tool, valid access checker (A.K.A.
> >> >vchecker). See the following link.
> >> >
> >> >https://github.com/JoonsooKim/linux/tree/vchecker-master-v0.3-next-20170106
> >> >
> >> >Vchecker has some advanced features compared to yours.
> >> >
> >> >1. Target object can be choosen at runtime by debugfs. It doesn't
> >> >require re-compile to register the target object.
> >> Hi Joonsoo, good to know you are also interested in this!
> >>
> >> Yes, if can be choosen via debugfs, it doesn't need re-compile.
> >> Well, I wonder what do you expect to be chosen from use space?
> >
> > As you mentioned somewhere, this tool can be used when we find the
> > overwritten happend on some particular victims. I assumes that most of
> > the problem would happen on slab objects and userspace can choose the
> > target slab cache via debugfs interface of the vchecker.
> 
> 
> Most objects are allocated from kmalloc slabs. And this feature can't
> work for all objects allocated from a kmalloc slab. I think checks
> needs to be tied to allocation sites.

I think that this problem can be easily solved by introducing filter.
If the user specify interesting kmalloc() caller via debugfs, vchecker
can distinguish the interesting objects. Manual code addition would
not be needed. I will implement this feature and submit soon.

Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Joonsoo Kim]

On Wed, Nov 22, 2017 at 11:43:00AM -0800, Wengang Wang wrote:
> 
> 
> On 2017/11/21 20:30, Joonsoo Kim wrote:
> >On Mon, Nov 20, 2017 at 11:56:05AM -0800, Wengang wrote:
> >>
> >>On 11/19/2017 05:50 PM, Joonsoo Kim wrote:
> >>>On Fri, Nov 17, 2017 at 11:56:21PM +0100, Dmitry Vyukov wrote:
> >>>>On Fri, Nov 17, 2017 at 11:30 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
> >>>>>Kasan advanced check, I'm going to add this feature.
> >>>>>Currently Kasan provide the detection of use-after-free and out-of-bounds
> >>>>>problems. It is not able to find the overwrite-on-allocated-memory issue.
> >>>>>We sometimes hit this kind of issue: We have a messed up structure
> >>>>>(usually dynamially allocated), some of the fields in the structure were
> >>>>>overwritten with unreasaonable values. And kernel may panic due to those
> >>>>>overeritten values. We know those fields were overwritten somehow, but we
> >>>>>have no easy way to find out which path did the overwritten. The advanced
> >>>>>check wants to help in this scenario.
> >>>>>
> >>>>>The idea is to define the memory owner. When write accesses come from
> >>>>>non-owner, error should be reported. Normally the write accesses on a given
> >>>>>structure happen in only several or a dozen of functions if the structure
> >>>>>is not that complicated. We call those functions "allowed functions".
> >>>>>The work of defining the owner and binding memory to owner is expected to
> >>>>>be done by the memory consumer. In the above case, memory consume register
> >>>>>the owner as the functions which have write accesses to the structure then
> >>>>>bind all the structures to the owner. Then kasan will do the "owner check"
> >>>>>after the basic checks.
> >>>>>
> >>>>>As implementation, kasan provides a API to it's user to register their
> >>>>>allowed functions. The API returns a token to users.  At run time, users
> >>>>>bind the memory ranges they are interested in to the check they registered.
> >>>>>Kasan then checks the bound memory ranges with the allowed functions.
> >>>>>
> >>>>>
> >>>>>Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> >>>Hello, Wengang.
> >>>
> >>>Nice idea. I also think that we need this kind of debugging tool. It's very
> >>>hard to detect overwritten bugs.
> >>>
> >>>In fact, I made a quite similar tool, valid access checker (A.K.A.
> >>>vchecker). See the following link.
> >>>
> >>>https://github.com/JoonsooKim/linux/tree/vchecker-master-v0.3-next-20170106
> >>>
> >>>Vchecker has some advanced features compared to yours.
> >>>
> >>>1. Target object can be choosen at runtime by debugfs. It doesn't
> >>>require re-compile to register the target object.
> >>Hi Joonsoo, good to know you are also interested in this!
> >>
> >>Yes, if can be choosen via debugfs, it doesn't need re-compile.
> >>Well, I wonder what do you expect to be chosen from use space?
> >As you mentioned somewhere, this tool can be used when we find the
> >overwritten happend on some particular victims. I assumes that most of
> >the problem would happen on slab objects and userspace can choose the
> >target slab cache via debugfs interface of the vchecker.
> Yes, I agree it would be slab objects.
> If there is a way to set the slab objects to be subject of check via
> name, it is good.
> One question is how about common kmalloc slabs?  They are widely
> used and many
> problems happens with them.

Yes, kmalloc slab should be supported. I have a simple solution to
this problem and mentioned on another reply. Please reference it.

> 
> >
> >>>2. It has another feature that checks the value stored in the object.
> >>>Usually, invalid writer stores odd value into the object and vchecker
> >>>can detect this case.
> >>It's good to do the check. Well, as I understand, it tells something
> >>bad (overwitten) happened.
> >>But it can't tell who did the overwritten, right?  (I didn't look at
> >>your patch yet,) do you recall the last write somewhere?
> >Yes, it stores the callstack of the last write and report it when
> >the error is found.
> >
> >>>3. It has a callstack checker (memory owner checker in yours). It
> >>>checks all the callstack rather than just the caller. It's important
> >>>since invalid writer could call the parent function of owner function
> >>>and it would not be catched by checking just the caller.
> >>>
> >>>4. The callstack checker is more automated. vchecker collects the valid
> >>>callstack by running the system.
> >>I think we can merge the above two into one.
> >>So you are doing full stack check.  Well, finding out the all the
> >>paths which have the write access may be not a very easy thing.
> >>Missing some paths may cause dmesg flooding, and those log won't
> >>help at all. Finding out all the (owning) caller only is relatively
> >>much easier.
> >Vchecker can be easily modified to store only the caller. It just
> >requires modifying callstack depth parameter so it's so easy.
> >Moreover, it can be accomplished by adding debugfs interface.
> 
> That's good.
> >Anyway, I don't think that finding out all the (owning) caller only
> >is much easier. Think about dentry or inode object. It is accessed by
> >various code path and it's not easy to cover all the owning caller by
> >manual approach.
> Comparing to finding out full stack, it's much easier.  If we take
> dentry as example,
> I agree dentries are widely accessed and maybe finding out all the
> owning caller is not that
> easy, but comparing to finding out the full stack, it's easier.

Okay. I mean that it's not that difficult with automatic search. Just
running the workload for a while will find almost full stacks.

> >
> >
> >>There do is the case you pointed out here. In this case, the
> >>debugger can make slight change to the calling path. And as I
> >>understand,
> >>most of the overwritten are happening in quite different call paths,
> >>they are not calling the (owning) caller.
> >Agreed.
> >
> >>>FYI, I attach some commit descriptions of the vchecker.
> >>>
> >>>     vchecker: store/report callstack of value writer
> >>>     The purpose of the value checker is finding invalid user writing
> >>>     invalid value at the moment that the value is written. However, there is
> >>>     a missing infrastructure that passes writing value to the checker
> >>>     since we temporarilly piggyback on the KASAN. So, we cannot easily
> >>>     detect this case in time.
> >>>     However, by following way, we can emulate similar effect.
> >>>     1. Store callstack when memory is written.
> >>Oh, seems you are storing the callstack for each write. -- I am not
> >>sure if that would too heavy.
> >Unlike KASAN that checks all type of the objects, this debugging
> >feature is only enabled on the specific type of the objects so
> >overhead would not be too heavy in terms of system overall
> >performance.
> Yes, only specific type of objects do the extra stuff, but I am not
> sure if the overall
> performance to be affected. Actually I was thinking of tracking last
> write stack.
> At that time, I had two concerns: one is the performance affect; the
> other is if it's safe
> since memory access can happen in any context -- process context,
> soft irq and irq..

In my test, there is no performance problem. However, it's easy to
store only the caller. It would be cheaper. I will make it configurable.

> 
> BTW, how much extra memory is needed for each objects?

4 bytes per object.

> >
> >>Actually I was thinking to have a check on the new value. But seems
> >>compiler doesn't provide that.
> >Yes, look like we have a similar idea. I have some another ideas if
> >ASAN hook provides the value to be written. However, it's not
> >supported by compiler yet.
> 
> Right!
> 
> >
> >>>     2. If check is failed in next access, report previous write-access
> >>>     callstack
> >>>     It will caught offending user properly.
> >>>     Following output "Call trace: Invalid writer" part is the result
> >>>     of this patch. We find the invalid value at workfn+0x71 but report
> >>>     writer at workfn+0x61.
> >>>     [  133.024076] ==================================================================
> >>>     [  133.025576] BUG: VCHECKER: invalid access in workfn+0x71/0xc0 at addr ffff8800683dd6c8
> >>>     [  133.027196] Read of size 8 by task kworker/1:1/48
> >>>     [  133.028020] 0x8 0x10 value
> >>>     [  133.028020] 0xffff 4
> >>>     [  133.028020] Call trace: Invalid writer
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff81043b1b>] save_stack_trace+0x1b/0x20
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff812c0db9>] save_stack+0x39/0x70
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff812c0fe3>] check_value+0x43/0x80
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff812c1762>] vchecker_check+0x1c2/0x380
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff812be49d>] __asan_store8+0x8d/0xc0
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff815eadd1>] workfn+0x61/0xc0
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff810be3df>] process_one_work+0x28f/0x680
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff810bf272>] worker_thread+0xa2/0x870
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff810c86a5>] kthread+0x195/0x1e0
> >>>     [  133.028020]
> >>>     [  133.028020] [<ffffffff81b9d3d2>] ret_from_fork+0x22/0x30
> >>>     [  133.028020] CPU: 1 PID: 48 Comm: kworker/1:1 Not tainted 4.10.0-rc2-next-20170106+ #1179
> >>>     [  133.028020] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >>>     [  133.028020] Workqueue: events workfn
> >>>     [  133.028020] Call Trace:
> >>>     [  133.028020]  dump_stack+0x4d/0x63
> >>>     [  133.028020]  kasan_object_err+0x21/0x80
> >>>     [  133.028020]  vchecker_check+0x2af/0x380
> >>>     [  133.028020]  ? workfn+0x71/0xc0
> >>>     [  133.028020]  ? workfn+0x71/0xc0
> >>>     [  133.028020]  __asan_load8+0x87/0xb0
> >>>     [  133.028020]  workfn+0x71/0xc0
> >>>     [  133.028020]  process_one_work+0x28f/0x680
> >>>     [  133.028020]  worker_thread+0xa2/0x870
> >>>     [  133.028020]  kthread+0x195/0x1e0
> >>>     [  133.028020]  ? put_pwq_unlocked+0xc0/0xc0
> >>>     [  133.028020]  ? kthread_park+0xd0/0xd0
> >>>     [  133.028020]  ret_from_fork+0x22/0x30
> >>>     [  133.028020] Object at ffff8800683dd6c0, in cache vchecker_test size: 24
> >>>     [  133.028020] Allocated:
> >>>     [  133.028020] PID = 48
> >>>
> >>>
> >>>     vchecker: Add 'callstack' checker
> >>>     The callstack checker is to find invalid code paths accessing to a
> >>>     certain field in an object.  Currently it only saves all stack traces at
> >>>     the given offset.  Reporting will be added in the next patch.
> >>>     The below example checks callstack of anon_vma:
> >>>       # cd /sys/kernel/debug/vchecker
> >>>       # echo 0 8 > anon_vma/callstack  # offset 0, size 8
> >>>       # echo 1 > anon_vma/enable
> >>an echo "anon_vma" > <something> first?
> >>How do you define and path the valid (owning) full stack to kasan?
> >This interface only enables to store all the callstacks. No validation
> >check here. I think that this feature would also be helpful to debug.
> >If error happens, we can check all the previous callstacks and track
> >the buggy caller.
> Too much extra memory needed for each object? or you stores in just
> one global copy.

Just one global copy. vchecker uses stackdepot introduced for this
purpose.

> 
> >
> >>>       # cat anon_vma/callstack        # show saved callstacks
> >>>       0x0 0x8 callstack
> >>>       total: 42
> >>>       callstack #0
> >>>         anon_vma_fork+0x101/0x280
> >>>         copy_process.part.10+0x15ff/0x2a40
> >>>         _do_fork+0x155/0x7d0
> >>>         SyS_clone+0x19/0x20
> >>>         do_syscall_64+0xdf/0x460
> >>>         return_from_SYSCALL_64+0x0/0x7a
> >>>       ...
> >>>
> >>>
> >>>     vchecker: Support toggle on/off of callstack check
> >>>     By default, callstack checker only collects callchains.  When a user
> >>>     writes 'on' to the callstack file in debugfs, it checks and reports new
> >>>     callstacks.  Writing 'off' to disable it again.
> >>>       # cd /sys/kernel/debug/vchecker
> >>>       # echo 0 8 > anon_vma/callstack
> >>>       # echo 1 > anon_vma/enable
> >>>       ... (do some work to collect enough callstacks) ...
> >>How to define "enough" here?
> >The bug usually doesn't happen immediately since it usually happens on
> >the corner case. When debugging, we run the workload that causes the
> >bug and then wait for some time until the bug happens. "Enough" can
> >be defined as the middle of this waiting time. After some warm-up
> >time, all the common callstack would be collected. Then,
> >switching on this feature that reports a new callstack. If the corner
> >case that is on a new callstack happens, this new callstack will be
> >reported and we can check whether it is a true bug or not.
> What if it's not?
> I am still not convinced on if we can get "enough". We may never
> have a workload that
> make sure it covers all call stacks.

If it's not a true bug, we just need to continue the workload until a
true bug happens.

Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Joonsoo Kim]

On Thu, Nov 23, 2017 at 03:23:17PM +0900, Joonsoo Kim wrote:
> On Wed, Nov 22, 2017 at 11:43:00AM -0800, Wengang Wang wrote:
> > >
> > >
> > >>There do is the case you pointed out here. In this case, the
> > >>debugger can make slight change to the calling path. And as I
> > >>understand,
> > >>most of the overwritten are happening in quite different call paths,
> > >>they are not calling the (owning) caller.
> > >Agreed.
> > >
> > >>>FYI, I attach some commit descriptions of the vchecker.
> > >>>
> > >>>     vchecker: store/report callstack of value writer
> > >>>     The purpose of the value checker is finding invalid user writing
> > >>>     invalid value at the moment that the value is written. However, there is
> > >>>     a missing infrastructure that passes writing value to the checker
> > >>>     since we temporarilly piggyback on the KASAN. So, we cannot easily
> > >>>     detect this case in time.
> > >>>     However, by following way, we can emulate similar effect.
> > >>>     1. Store callstack when memory is written.
> > >>Oh, seems you are storing the callstack for each write. -- I am not
> > >>sure if that would too heavy.
> > >Unlike KASAN that checks all type of the objects, this debugging
> > >feature is only enabled on the specific type of the objects so
> > >overhead would not be too heavy in terms of system overall
> > >performance.
> > Yes, only specific type of objects do the extra stuff, but I am not
> > sure if the overall
> > performance to be affected. Actually I was thinking of tracking last
> > write stack.
> > At that time, I had two concerns: one is the performance affect; the
> > other is if it's safe
> > since memory access can happen in any context -- process context,
> > soft irq and irq..

Oops. I missed this question. vchecker works well on all contexts
however there is a possibilty to miss the callstack due to memory
allocation failure in stackdepot. It would be rare case since
stackdepot has some protection to this problem. Note that KASAN has
the same problem and it works well until now.

Thanks.

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

Re: [PATCH 0/5] mm/kasan: advanced check

[Wengang Wang]

On 2017/11/22 11:29, Wengang Wang wrote:
>
>
> On 2017/11/22 4:04, Andrey Ryabinin wrote:
>> On 11/18/2017 01:30 AM, Wengang Wang wrote:
>>> Kasan advanced check, I'm going to add this feature.
>>> Currently Kasan provide the detection of use-after-free and 
>>> out-of-bounds
>>> problems. It is not able to find the overwrite-on-allocated-memory 
>>> issue.
>>> We sometimes hit this kind of issue: We have a messed up structure
>>> (usually dynamially allocated), some of the fields in the structure 
>>> were
>>> overwritten with unreasaonable values. And kernel may panic due to 
>>> those
>>> overeritten values. We know those fields were overwritten somehow, 
>>> but we
>>> have no easy way to find out which path did the overwritten. The 
>>> advanced
>>> check wants to help in this scenario.
>>>
>>> The idea is to define the memory owner. When write accesses come from
>>> non-owner, error should be reported. Normally the write accesses on 
>>> a given
>>> structure happen in only several or a dozen of functions if the 
>>> structure
>>> is not that complicated. We call those functions "allowed functions".
>>> The work of defining the owner and binding memory to owner is 
>>> expected to
>>> be done by the memory consumer. In the above case, memory consume 
>>> register
>>> the owner as the functions which have write accesses to the 
>>> structure then
>>> bind all the structures to the owner. Then kasan will do the "owner 
>>> check"
>>> after the basic checks.
>>>
>>> As implementation, kasan provides a API to it's user to register their
>>> allowed functions. The API returns a token to users.A  At run time, 
>>> users
>>> bind the memory ranges they are interested in to the check they 
>>> registered.
>>> Kasan then checks the bound memory ranges with the allowed functions.
>>>
>> NAK. We don't add APIs with no users in the kernel.
>> If nothing in the kernel uses this API than there is no way to tell 
>> if this works or not.
If the concern is just if this works or not. I think the last patch in 
the set is a user of owner check. It shows the owner check works well.

A copy of one report is like this:
2134 [A  448.477923] 
==================================================================
2135 [A  448.565140] BUG: KASAN: Non-owner write access violation in 
funcB+0xd/0x3d [test_kasan]
2136 [A  448.661699] Write of size 1 at addr ffff881fa516e90c by task 
insmod/5606
2137 [A  448.742314]
2138 [A  448.760514] CPU: 3 PID: 5606 Comm: insmod Tainted: G BA A A A A  OEA A  
4.14.0-rc8 #9
2139 [A  448.760517] Hardware name: Oracle Corporation ORACLE SERVER 
X6-2/ASM,MOTHERBOARD,1U, BIOS 38050100 08/30/2016
2140 [A  448.760519] Call Trace:
2141 [A  448.760529]A  dump_stack+0x63/0x8d
2142 [A  448.760538]A  print_address_description+0x7c/0x290
2143 [A  448.760547]A  kasan_report+0x274/0x3d0
2144 [A  448.760554]A  ? kasan_kmalloc+0xad/0xe0
2145 [A  448.760566]A  ? funcB+0xd/0x3d [test_kasan]
2146 [A  448.760578]A  ? kasan_adv+0x1f3/0x1f3 [test_kasan]
2147 [A  448.760585]A  __asan_store1+0xa4/0xb0
2148 [A  448.760597]A  ? funcB+0xd/0x3d [test_kasan]
2149 [A  448.760608]A  funcB+0xd/0x3d [test_kasan]
2150 [A  448.760620]A  kasan_adv+0x12a/0x1f3 [test_kasan]
2151 [A  448.760633]A  ? copy_user_test+0x1ba/0x1ba [test_kasan]
2152 [A  448.760641]A  ? percpu_counter_add_batch+0x22/0xa0
2153 [A  448.760646]A  ? 0xffffffffa0dd0000
2154 [A  448.760657]A  ? funcA+0x20/0x20 [test_kasan]
2155 [A  448.760667]A  ? do_munmap+0x52e/0x6a0
2156 [A  448.760675]A  ? vm_munmap+0xd8/0x110
2157 [A  448.760684]A  ? kasan_slab_free+0x89/0xc0
2158 [A  448.760690]A  ? kfree+0x95/0x190
2159 [A  448.760702]A  ? kasan_adv+0x1f3/0x1f3 [test_kasan]
2160 [A  448.760714]A  ? copy_user_test+0x1b3/0x1ba [test_kasan]
2161 [A  448.760726]A  kmalloc_tests_init+0x84/0xf89 [test_kasan]
2162 [A  448.760733]A  do_one_initcall+0xa6/0x210
2163 [A  448.760740]A  ? initcall_blacklisted+0x150/0x150
2164 [A  448.760748]A  ? kasan_unpoison_shadow+0x36/0x50
2165 [A  448.760755]A  ? kasan_kmalloc+0xad/0xe0
2166 [A  448.760762]A  ? kasan_unpoison_shadow+0x36/0x50
2167 [A  448.760770]A  ? __asan_register_globals+0x87/0xa0
2168 [A  448.760779]A  do_init_module+0xf4/0x312
2169 [A  448.760786]A  load_module+0x283a/0x3120
2170 [A  448.760802]A  ? layout_and_allocate+0x18b0/0x18b0
2171 [A  448.760809]A  ? vmap_page_range_noflush+0x2e3/0x400
2172 [A  448.760821]A  SYSC_init_module+0x1c3/0x1e0
2173 [A  448.760826]A  ? SYSC_init_module+0x1c3/0x1e0
2174 [A  448.760831]A  ? load_module+0x3120/0x3120
2175 [A  448.760839]A  ? SYSC_finit_module+0x1a0/0x1a0
2176 [A  448.760845]A  SyS_init_module+0xe/0x10
2177 [A  448.760851]A  do_syscall_64+0xe3/0x270
2178 [A  448.760860]A  entry_SYSCALL64_slow_path+0x25/0x25
2179 [A  448.760865] RIP: 0033:0x35f80e923a
2180 [A  448.760868] RSP: 002b:00007ffc8835e9a8 EFLAGS: 00000202 
ORIG_RAX: 00000000000000af
2181 [A  448.760875] RAX: ffffffffffffffda RBX: 00007ffc8835f4ff RCX: 
00000035f80e923a
2182 [A  448.760879] RDX: 00000000016d3010 RSI: 0000000000044f78 RDI: 
00007fedf04b9010
2183 [A  448.760883] RBP: 00000000016d3010 R08: 0000000000081000 R09: 
0000000000041000
2184 [A  448.760886] R10: 00000035f80db710 R11: 0000000000000202 R12: 
0000000000044f78
2185 [A  448.760890] R13: 0000000000080000 R14: 00007fedf04b9010 R15: 
0000000000000003
2186 [A  448.760894]
2187 [A  448.779081] Allocated by task 5606:
2188 [A  448.821206]A  save_stack_trace+0x1b/0x20
2189 [A  448.821212]A  save_stack+0x46/0xd0
2190 [A  448.821218]A  kasan_kmalloc+0xad/0xe0
2191 [A  448.821224]A  kmem_cache_alloc_trace+0xf0/0x1e0
2192 [A  448.821235]A  kasan_adv+0xe1/0x1f3 [test_kasan]
2193 [A  448.821246]A  kmalloc_tests_init+0x84/0xf89 [test_kasan]
2194 [A  448.821252]A  do_one_initcall+0xa6/0x210
2195 [A  448.821256]A  do_init_module+0xf4/0x312
2196 [A  448.821261]A  load_module+0x283a/0x3120
2197 [A  448.821265]A  SYSC_init_module+0x1c3/0x1e0
2198 [A  448.821269]A  SyS_init_module+0xe/0x10
2199 [A  448.821275]A  do_syscall_64+0xe3/0x270
2200 [A  448.821282]A  return_from_SYSCALL_64+0x0/0x6a
2201 [A  448.821284]
2202 [A  448.839471] Freed by task 0:
2203 [A  448.874305] (stack is not available)
2204 [A  448.917458]
2205 [A  448.935655] The buggy address belongs to the object at 
ffff881fa516e900
2206 [A  448.935655]A  which belongs to the cache kmalloc-64 of size 64
2207 [A  449.084234] The buggy address is located 12 bytes inside of
2208 [A  449.084234]A  64-byte region [ffff881fa516e900, ffff881fa516e940)
2209 [A  449.223442] The buggy address belongs to the page:
2210 [A  449.281170] page:ffffea007e945b80 count:1 mapcount:0 
mapping:A A A A A A A A A  (null) index:0x0
2211 [A  449.377720] flags: 0x2fffff80000100(slab)
2212 [A  449.426094] raw: 002fffff80000100 0000000000000000 
0000000000000000 00000001802a002a
2213 [A  449.519525] raw: dead000000000100 dead000000000200 
ffff881fff40f640 0000000000000000
2214 [A  449.612961] page dumped because: kasan: bad access detected
2215 [A  449.680054]
2216 [A  449.698244] Memory state around the buggy address:
2217 [A  449.755972]A  ffff881fa516e800: fb fb fb fb fc fc fc fc fb fb fb 
fb fb fb fb fb
2218 [A  449.843154]A  ffff881fa516e880: fc fc fc fc fb fb fb fb fb fb fb 
fb fc fc fc fc
2219 [A  449.930345] >ffff881fa516e900: 30 30 30 30 00 00 00 06 fc fc fc 
fc fc fc fc fc
2220 [A  450.017535]A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A A  ^
2221 [A  450.059660]A  ffff881fa516e980: fc fc fc fc fc fc fc fc fc fc fc 
fc fc fc fc fc
2222 [A  450.146846]A  ffff881fa516ea00: fc fc fc fc fc fc fc fc fc fc fc 
fc fc fc fc fc
2223 [A  450.234027] 
==================================================================

The "^" is pointing to the second "30" in above line (not sure it shows 
correctly after copy/paste)

thanks,
wengang

> In production kernel, we don't want unnecessary APIs without users in 
> the kernel because that
> would consume binary size (a pure space waste) and leave "dead" code.
> KASAN code is a bit different from other kernel components, its self 
> is debugging purpose only.
> When KASAN is enabled, the APIs would have potential users and the 
> code is not "dead" code.
> The size increasing in binary would be acceptable since the kernel 
> with KASAN enabled only has
> a short time life -- only used to find the root cause, when root 
> caused is found, it will be no
> longer used;A  Also the KASAN enabled kernel is used by limited user 
> where they have a particular
> issue. I say "potential users" because this functionality its self is 
> dynamically used or to say a
> one-shot use. The functionality is helpful.
>
> I think even KASAN its self we don't know if it works or not when it 
> is not enabled.
> -- Before I tried it, I am curious if this can work well; After 
> testing it, I know it works.
> If we don't give users the chance, they will never know there is such 
> a functionality and will never
> get benefit from it.
>
>
>> Besides, I'm bit skeptical about usefulness of this feature. Those 
>> kinds of issues that
>> advanced check is supposed to catch, is almost always is just some 
>> sort of longstanding
>> use after free, which eventually should be caught by kasan.
> Yes, if luckily, the issue is possible to be catched by UAF check.
> Well considering busy production systems, the memory is very likely to 
> be reallocated rather than
> staying in free state for very long time.A  That is the 
> overwritten-to-allocated-memory is more
> likely to happen than UAF does I think.A  When 
> overwritten-to-allocated-memory happened,
> UAF check has no chance to detect the problem.
>
> KASAN is helpful to detect problematic memory usage, so does this 
> patch set!
> I really hope this can be included and developers can get benefit from 
> it.
>
> Thanks,
> Wengang

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