linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] kasan: add memory corruption identification for hw tag-based kasan
@ 2021-06-12  4:51 Kuan-Ying Lee
  2021-06-12  4:51 ` [PATCH v2 1/3] kasan: rename CONFIG_KASAN_SW_TAGS_IDENTIFY to CONFIG_KASAN_TAGS_IDENTIFY Kuan-Ying Lee
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Kuan-Ying Lee @ 2021-06-12  4:51 UTC (permalink / raw)
  To: Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
	Dmitry Vyukov, Andrew Morton
  Cc: kasan-dev, linux-kernel, linux-mm, Kuan-Ying Lee

Add memory corruption identification for hardware tag-based KASAN mode.

Changes since v2:
 - Thanks for Marco's Suggestion
 - Rename the CONFIG_KASAN_SW_TAGS_IDENTIFY
 - Integrate tag-based kasan common part
 - Rebase to latest linux-next

Kuan-Ying Lee (3):
  kasan: rename CONFIG_KASAN_SW_TAGS_IDENTIFY to
    CONFIG_KASAN_TAGS_IDENTIFY
  kasan: integrate the common part of two KASAN tag-based modes
  kasan: add memory corruption identification support for hardware
    tag-based mode

 lib/Kconfig.kasan         |  4 +--
 mm/kasan/Makefile         |  4 +--
 mm/kasan/hw_tags.c        | 22 ---------------
 mm/kasan/kasan.h          |  4 +--
 mm/kasan/report_hw_tags.c |  6 +---
 mm/kasan/report_sw_tags.c | 46 +------------------------------
 mm/kasan/report_tags.h    | 56 +++++++++++++++++++++++++++++++++++++
 mm/kasan/sw_tags.c        | 41 ---------------------------
 mm/kasan/tags.c           | 58 +++++++++++++++++++++++++++++++++++++++
 9 files changed, 122 insertions(+), 119 deletions(-)
 create mode 100644 mm/kasan/report_tags.h
 create mode 100644 mm/kasan/tags.c

-- 
2.25.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] kasan: rename CONFIG_KASAN_SW_TAGS_IDENTIFY to CONFIG_KASAN_TAGS_IDENTIFY
  2021-06-12  4:51 [PATCH v2 0/3] kasan: add memory corruption identification for hw tag-based kasan Kuan-Ying Lee
@ 2021-06-12  4:51 ` Kuan-Ying Lee
  2021-06-12  4:51 ` [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes Kuan-Ying Lee
  2021-06-12  4:51 ` [PATCH v2 3/3] kasan: add memory corruption identification support for hardware tag-based mode Kuan-Ying Lee
  2 siblings, 0 replies; 9+ messages in thread
From: Kuan-Ying Lee @ 2021-06-12  4:51 UTC (permalink / raw)
  To: Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
	Dmitry Vyukov, Andrew Morton
  Cc: kasan-dev, linux-kernel, linux-mm, Kuan-Ying Lee, Marco Elver

This patch renames CONFIG_KASAN_SW_TAGS_IDENTIFY to
CONFIG_KASAN_TAGS_IDENTIFY in order to be compatible
with hardware tag-based mode.

Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com>
Suggested-by: Marco Elver <elver@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 lib/Kconfig.kasan         | 2 +-
 mm/kasan/kasan.h          | 4 ++--
 mm/kasan/report_sw_tags.c | 2 +-
 mm/kasan/sw_tags.c        | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index cffc2ebbf185..6f5d48832139 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -155,7 +155,7 @@ config KASAN_STACK
 	  CONFIG_COMPILE_TEST.	On gcc it is assumed to always be safe
 	  to use and enabled by default.
 
-config KASAN_SW_TAGS_IDENTIFY
+config KASAN_TAGS_IDENTIFY
 	bool "Enable memory corruption identification"
 	depends on KASAN_SW_TAGS
 	help
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 8f450bc28045..b0fc9a1eb7e3 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -153,7 +153,7 @@ struct kasan_track {
 	depot_stack_handle_t stack;
 };
 
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+#ifdef CONFIG_KASAN_TAGS_IDENTIFY
 #define KASAN_NR_FREE_STACKS 5
 #else
 #define KASAN_NR_FREE_STACKS 1
@@ -170,7 +170,7 @@ struct kasan_alloc_meta {
 #else
 	struct kasan_track free_track[KASAN_NR_FREE_STACKS];
 #endif
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+#ifdef CONFIG_KASAN_TAGS_IDENTIFY
 	u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
 	u8 free_track_idx;
 #endif
diff --git a/mm/kasan/report_sw_tags.c b/mm/kasan/report_sw_tags.c
index 3d20d3451d9e..821a14a19a92 100644
--- a/mm/kasan/report_sw_tags.c
+++ b/mm/kasan/report_sw_tags.c
@@ -31,7 +31,7 @@
 
 const char *kasan_get_bug_type(struct kasan_access_info *info)
 {
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+#ifdef CONFIG_KASAN_TAGS_IDENTIFY
 	struct kasan_alloc_meta *alloc_meta;
 	struct kmem_cache *cache;
 	struct page *page;
diff --git a/mm/kasan/sw_tags.c b/mm/kasan/sw_tags.c
index 9362938abbfa..dd05e6c801fa 100644
--- a/mm/kasan/sw_tags.c
+++ b/mm/kasan/sw_tags.c
@@ -177,7 +177,7 @@ void kasan_set_free_info(struct kmem_cache *cache,
 	if (!alloc_meta)
 		return;
 
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+#ifdef CONFIG_KASAN_TAGS_IDENTIFY
 	idx = alloc_meta->free_track_idx;
 	alloc_meta->free_pointer_tag[idx] = tag;
 	alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
@@ -196,7 +196,7 @@ struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
 	if (!alloc_meta)
 		return NULL;
 
-#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
+#ifdef CONFIG_KASAN_TAGS_IDENTIFY
 	for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
 		if (alloc_meta->free_pointer_tag[i] == tag)
 			break;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes
  2021-06-12  4:51 [PATCH v2 0/3] kasan: add memory corruption identification for hw tag-based kasan Kuan-Ying Lee
  2021-06-12  4:51 ` [PATCH v2 1/3] kasan: rename CONFIG_KASAN_SW_TAGS_IDENTIFY to CONFIG_KASAN_TAGS_IDENTIFY Kuan-Ying Lee
@ 2021-06-12  4:51 ` Kuan-Ying Lee
  2021-06-12 14:42   ` Marco Elver
  2021-06-12  4:51 ` [PATCH v2 3/3] kasan: add memory corruption identification support for hardware tag-based mode Kuan-Ying Lee
  2 siblings, 1 reply; 9+ messages in thread
From: Kuan-Ying Lee @ 2021-06-12  4:51 UTC (permalink / raw)
  To: Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
	Dmitry Vyukov, Andrew Morton
  Cc: kasan-dev, linux-kernel, linux-mm, Kuan-Ying Lee, Marco Elver

1. Move kasan_get_free_track() and kasan_set_free_info()
   into tags.c
2. Move kasan_get_bug_type() to header file

Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com>
Suggested-by: Marco Elver <elver@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 mm/kasan/Makefile         |  4 +--
 mm/kasan/hw_tags.c        | 22 ---------------
 mm/kasan/report_hw_tags.c |  6 +---
 mm/kasan/report_sw_tags.c | 46 +------------------------------
 mm/kasan/report_tags.h    | 56 +++++++++++++++++++++++++++++++++++++
 mm/kasan/sw_tags.c        | 41 ---------------------------
 mm/kasan/tags.c           | 58 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 118 insertions(+), 115 deletions(-)
 create mode 100644 mm/kasan/report_tags.h
 create mode 100644 mm/kasan/tags.c

diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
index 9fe39a66388a..634de6c1da9b 100644
--- a/mm/kasan/Makefile
+++ b/mm/kasan/Makefile
@@ -37,5 +37,5 @@ CFLAGS_sw_tags.o := $(CC_FLAGS_KASAN_RUNTIME)
 
 obj-$(CONFIG_KASAN) := common.o report.o
 obj-$(CONFIG_KASAN_GENERIC) += init.o generic.o report_generic.o shadow.o quarantine.o
-obj-$(CONFIG_KASAN_HW_TAGS) += hw_tags.o report_hw_tags.o
-obj-$(CONFIG_KASAN_SW_TAGS) += init.o report_sw_tags.o shadow.o sw_tags.o
+obj-$(CONFIG_KASAN_HW_TAGS) += hw_tags.o report_hw_tags.o tags.o
+obj-$(CONFIG_KASAN_SW_TAGS) += init.o report_sw_tags.o shadow.o sw_tags.o tags.o
diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
index ed5e5b833d61..4ea8c368b5b8 100644
--- a/mm/kasan/hw_tags.c
+++ b/mm/kasan/hw_tags.c
@@ -216,28 +216,6 @@ void __init kasan_init_hw_tags(void)
 	pr_info("KernelAddressSanitizer initialized\n");
 }
 
-void kasan_set_free_info(struct kmem_cache *cache,
-				void *object, u8 tag)
-{
-	struct kasan_alloc_meta *alloc_meta;
-
-	alloc_meta = kasan_get_alloc_meta(cache, object);
-	if (alloc_meta)
-		kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT);
-}
-
-struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
-				void *object, u8 tag)
-{
-	struct kasan_alloc_meta *alloc_meta;
-
-	alloc_meta = kasan_get_alloc_meta(cache, object);
-	if (!alloc_meta)
-		return NULL;
-
-	return &alloc_meta->free_track[0];
-}
-
 void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags)
 {
 	/*
diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c
index 42b2168755d6..ef5e7378f3aa 100644
--- a/mm/kasan/report_hw_tags.c
+++ b/mm/kasan/report_hw_tags.c
@@ -14,11 +14,7 @@
 #include <linux/types.h>
 
 #include "kasan.h"
-
-const char *kasan_get_bug_type(struct kasan_access_info *info)
-{
-	return "invalid-access";
-}
+#include "report_tags.h"
 
 void *kasan_find_first_bad_addr(void *addr, size_t size)
 {
diff --git a/mm/kasan/report_sw_tags.c b/mm/kasan/report_sw_tags.c
index 821a14a19a92..d965a170083e 100644
--- a/mm/kasan/report_sw_tags.c
+++ b/mm/kasan/report_sw_tags.c
@@ -26,51 +26,7 @@
 
 #include <asm/sections.h>
 
-#include "kasan.h"
-#include "../slab.h"
-
-const char *kasan_get_bug_type(struct kasan_access_info *info)
-{
-#ifdef CONFIG_KASAN_TAGS_IDENTIFY
-	struct kasan_alloc_meta *alloc_meta;
-	struct kmem_cache *cache;
-	struct page *page;
-	const void *addr;
-	void *object;
-	u8 tag;
-	int i;
-
-	tag = get_tag(info->access_addr);
-	addr = kasan_reset_tag(info->access_addr);
-	page = kasan_addr_to_page(addr);
-	if (page && PageSlab(page)) {
-		cache = page->slab_cache;
-		object = nearest_obj(cache, page, (void *)addr);
-		alloc_meta = kasan_get_alloc_meta(cache, object);
-
-		if (alloc_meta) {
-			for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
-				if (alloc_meta->free_pointer_tag[i] == tag)
-					return "use-after-free";
-			}
-		}
-		return "out-of-bounds";
-	}
-
-#endif
-	/*
-	 * If access_size is a negative number, then it has reason to be
-	 * defined as out-of-bounds bug type.
-	 *
-	 * Casting negative numbers to size_t would indeed turn up as
-	 * a large size_t and its value will be larger than ULONG_MAX/2,
-	 * so that this can qualify as out-of-bounds.
-	 */
-	if (info->access_addr + info->access_size < info->access_addr)
-		return "out-of-bounds";
-
-	return "invalid-access";
-}
+#include "report_tags.h"
 
 void *kasan_find_first_bad_addr(void *addr, size_t size)
 {
diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h
new file mode 100644
index 000000000000..4f740d4d99ee
--- /dev/null
+++ b/mm/kasan/report_tags.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __MM_KASAN_REPORT_TAGS_H
+#define __MM_KASAN_REPORT_TAGS_H
+
+#include "kasan.h"
+#include "../slab.h"
+
+#ifdef CONFIG_KASAN_TAGS_IDENTIFY
+const char *kasan_get_bug_type(struct kasan_access_info *info)
+{
+	struct kasan_alloc_meta *alloc_meta;
+	struct kmem_cache *cache;
+	struct page *page;
+	const void *addr;
+	void *object;
+	u8 tag;
+	int i;
+
+	tag = get_tag(info->access_addr);
+	addr = kasan_reset_tag(info->access_addr);
+	page = kasan_addr_to_page(addr);
+	if (page && PageSlab(page)) {
+		cache = page->slab_cache;
+		object = nearest_obj(cache, page, (void *)addr);
+		alloc_meta = kasan_get_alloc_meta(cache, object);
+
+		if (alloc_meta) {
+			for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
+				if (alloc_meta->free_pointer_tag[i] == tag)
+					return "use-after-free";
+			}
+		}
+		return "out-of-bounds";
+	}
+
+	/*
+	 * If access_size is a negative number, then it has reason to be
+	 * defined as out-of-bounds bug type.
+	 *
+	 * Casting negative numbers to size_t would indeed turn up as
+	 * a large size_t and its value will be larger than ULONG_MAX/2,
+	 * so that this can qualify as out-of-bounds.
+	 */
+	if (info->access_addr + info->access_size < info->access_addr)
+		return "out-of-bounds";
+
+	return "invalid-access";
+}
+#else
+const char *kasan_get_bug_type(struct kasan_access_info *info)
+{
+	return "invalid-access";
+}
+#endif
+
+#endif
diff --git a/mm/kasan/sw_tags.c b/mm/kasan/sw_tags.c
index dd05e6c801fa..bd3f540feb47 100644
--- a/mm/kasan/sw_tags.c
+++ b/mm/kasan/sw_tags.c
@@ -167,47 +167,6 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)
 }
 EXPORT_SYMBOL(__hwasan_tag_memory);
 
-void kasan_set_free_info(struct kmem_cache *cache,
-				void *object, u8 tag)
-{
-	struct kasan_alloc_meta *alloc_meta;
-	u8 idx = 0;
-
-	alloc_meta = kasan_get_alloc_meta(cache, object);
-	if (!alloc_meta)
-		return;
-
-#ifdef CONFIG_KASAN_TAGS_IDENTIFY
-	idx = alloc_meta->free_track_idx;
-	alloc_meta->free_pointer_tag[idx] = tag;
-	alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
-#endif
-
-	kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
-}
-
-struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
-				void *object, u8 tag)
-{
-	struct kasan_alloc_meta *alloc_meta;
-	int i = 0;
-
-	alloc_meta = kasan_get_alloc_meta(cache, object);
-	if (!alloc_meta)
-		return NULL;
-
-#ifdef CONFIG_KASAN_TAGS_IDENTIFY
-	for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
-		if (alloc_meta->free_pointer_tag[i] == tag)
-			break;
-	}
-	if (i == KASAN_NR_FREE_STACKS)
-		i = alloc_meta->free_track_idx;
-#endif
-
-	return &alloc_meta->free_track[i];
-}
-
 void kasan_tag_mismatch(unsigned long addr, unsigned long access_info,
 			unsigned long ret_ip)
 {
diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
new file mode 100644
index 000000000000..9c33c0ebe1d1
--- /dev/null
+++ b/mm/kasan/tags.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This file contains common tag-based KASAN code.
+ *
+ * Author: Kuan-Ying Lee <kylee0686026@gmail.com>
+ */
+
+#include <linux/init.h>
+#include <linux/kasan.h>
+#include <linux/kernel.h>
+#include <linux/memory.h>
+#include <linux/mm.h>
+#include <linux/static_key.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include "kasan.h"
+
+void kasan_set_free_info(struct kmem_cache *cache,
+				void *object, u8 tag)
+{
+	struct kasan_alloc_meta *alloc_meta;
+	u8 idx = 0;
+
+	alloc_meta = kasan_get_alloc_meta(cache, object);
+	if (!alloc_meta)
+		return;
+
+#ifdef CONFIG_KASAN_TAGS_IDENTIFY
+	idx = alloc_meta->free_track_idx;
+	alloc_meta->free_pointer_tag[idx] = tag;
+	alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
+#endif
+
+	kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
+}
+
+struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
+				void *object, u8 tag)
+{
+	struct kasan_alloc_meta *alloc_meta;
+	int i = 0;
+
+	alloc_meta = kasan_get_alloc_meta(cache, object);
+	if (!alloc_meta)
+		return NULL;
+
+#ifdef CONFIG_KASAN_TAGS_IDENTIFY
+	for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
+		if (alloc_meta->free_pointer_tag[i] == tag)
+			break;
+	}
+	if (i == KASAN_NR_FREE_STACKS)
+		i = alloc_meta->free_track_idx;
+#endif
+
+	return &alloc_meta->free_track[i];
+}
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] kasan: add memory corruption identification support for hardware tag-based mode
  2021-06-12  4:51 [PATCH v2 0/3] kasan: add memory corruption identification for hw tag-based kasan Kuan-Ying Lee
  2021-06-12  4:51 ` [PATCH v2 1/3] kasan: rename CONFIG_KASAN_SW_TAGS_IDENTIFY to CONFIG_KASAN_TAGS_IDENTIFY Kuan-Ying Lee
  2021-06-12  4:51 ` [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes Kuan-Ying Lee
@ 2021-06-12  4:51 ` Kuan-Ying Lee
  2 siblings, 0 replies; 9+ messages in thread
From: Kuan-Ying Lee @ 2021-06-12  4:51 UTC (permalink / raw)
  To: Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
	Dmitry Vyukov, Andrew Morton
  Cc: kasan-dev, linux-kernel, linux-mm, Kuan-Ying Lee, Marco Elver

Add memory corruption identification support for hardware tag-based
mode. We store one old free pointer tag and free backtrace.

Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com>
Suggested-by: Marco Elver <elver@google.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 lib/Kconfig.kasan | 2 +-
 mm/kasan/kasan.h  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 6f5d48832139..2cc25792bc2f 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -157,7 +157,7 @@ config KASAN_STACK
 
 config KASAN_TAGS_IDENTIFY
 	bool "Enable memory corruption identification"
-	depends on KASAN_SW_TAGS
+	depends on KASAN_SW_TAGS || KASAN_HW_TAGS
 	help
 	  This option enables best-effort identification of bug type
 	  (use-after-free or out-of-bounds) at the cost of increased
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index b0fc9a1eb7e3..d6f982b8a84e 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -153,7 +153,7 @@ struct kasan_track {
 	depot_stack_handle_t stack;
 };
 
-#ifdef CONFIG_KASAN_TAGS_IDENTIFY
+#if defined(CONFIG_KASAN_TAGS_IDENTIFY) && defined(CONFIG_KASAN_SW_TAGS)
 #define KASAN_NR_FREE_STACKS 5
 #else
 #define KASAN_NR_FREE_STACKS 1
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes
  2021-06-12  4:51 ` [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes Kuan-Ying Lee
@ 2021-06-12 14:42   ` Marco Elver
  2021-06-12 15:51     ` Kuan-Ying Lee
  2021-06-12 15:52     ` Greg Kroah-Hartman
  0 siblings, 2 replies; 9+ messages in thread
From: Marco Elver @ 2021-06-12 14:42 UTC (permalink / raw)
  To: Kuan-Ying Lee, Greg Kroah-Hartman
  Cc: Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
	Dmitry Vyukov, Andrew Morton, kasan-dev, LKML,
	Linux Memory Management List

On Sat, 12 Jun 2021 at 06:52, Kuan-Ying Lee <kylee0686026@gmail.com> wrote:
> 1. Move kasan_get_free_track() and kasan_set_free_info()
>    into tags.c
> 2. Move kasan_get_bug_type() to header file
>
> Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com>
> Suggested-by: Marco Elver <elver@google.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  mm/kasan/Makefile         |  4 +--
>  mm/kasan/hw_tags.c        | 22 ---------------
>  mm/kasan/report_hw_tags.c |  6 +---
>  mm/kasan/report_sw_tags.c | 46 +------------------------------
>  mm/kasan/report_tags.h    | 56 +++++++++++++++++++++++++++++++++++++
>  mm/kasan/sw_tags.c        | 41 ---------------------------
>  mm/kasan/tags.c           | 58 +++++++++++++++++++++++++++++++++++++++
>  7 files changed, 118 insertions(+), 115 deletions(-)
>  create mode 100644 mm/kasan/report_tags.h
>  create mode 100644 mm/kasan/tags.c
[...]
> diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> index ed5e5b833d61..4ea8c368b5b8 100644
> --- a/mm/kasan/hw_tags.c
> +++ b/mm/kasan/hw_tags.c
> @@ -216,28 +216,6 @@ void __init kasan_init_hw_tags(void)
>         pr_info("KernelAddressSanitizer initialized\n");
>  }
>
> -void kasan_set_free_info(struct kmem_cache *cache,
> -                               void *object, u8 tag)
> -{
> -       struct kasan_alloc_meta *alloc_meta;
> -
> -       alloc_meta = kasan_get_alloc_meta(cache, object);
> -       if (alloc_meta)
> -               kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT);
> -}
> -
> -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> -                               void *object, u8 tag)
> -{
> -       struct kasan_alloc_meta *alloc_meta;
> -
> -       alloc_meta = kasan_get_alloc_meta(cache, object);
> -       if (!alloc_meta)
> -               return NULL;
> -
> -       return &alloc_meta->free_track[0];
> -}
> -
>  void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags)
>  {
>         /*
> diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c
> index 42b2168755d6..ef5e7378f3aa 100644
> --- a/mm/kasan/report_hw_tags.c
> +++ b/mm/kasan/report_hw_tags.c
> @@ -14,11 +14,7 @@
>  #include <linux/types.h>
>
>  #include "kasan.h"
> -
> -const char *kasan_get_bug_type(struct kasan_access_info *info)
> -{
> -       return "invalid-access";
> -}
> +#include "report_tags.h"
>
>  void *kasan_find_first_bad_addr(void *addr, size_t size)
>  {
> diff --git a/mm/kasan/report_sw_tags.c b/mm/kasan/report_sw_tags.c
> index 821a14a19a92..d965a170083e 100644
> --- a/mm/kasan/report_sw_tags.c
> +++ b/mm/kasan/report_sw_tags.c
> @@ -26,51 +26,7 @@
>
>  #include <asm/sections.h>
>
> -#include "kasan.h"
> -#include "../slab.h"
> -
> -const char *kasan_get_bug_type(struct kasan_access_info *info)
> -{
> -#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> -       struct kasan_alloc_meta *alloc_meta;
> -       struct kmem_cache *cache;
> -       struct page *page;
> -       const void *addr;
> -       void *object;
> -       u8 tag;
> -       int i;
> -
> -       tag = get_tag(info->access_addr);
> -       addr = kasan_reset_tag(info->access_addr);
> -       page = kasan_addr_to_page(addr);
> -       if (page && PageSlab(page)) {
> -               cache = page->slab_cache;
> -               object = nearest_obj(cache, page, (void *)addr);
> -               alloc_meta = kasan_get_alloc_meta(cache, object);
> -
> -               if (alloc_meta) {
> -                       for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
> -                               if (alloc_meta->free_pointer_tag[i] == tag)
> -                                       return "use-after-free";
> -                       }
> -               }
> -               return "out-of-bounds";
> -       }
> -
> -#endif
> -       /*
> -        * If access_size is a negative number, then it has reason to be
> -        * defined as out-of-bounds bug type.
> -        *
> -        * Casting negative numbers to size_t would indeed turn up as
> -        * a large size_t and its value will be larger than ULONG_MAX/2,
> -        * so that this can qualify as out-of-bounds.
> -        */
> -       if (info->access_addr + info->access_size < info->access_addr)
> -               return "out-of-bounds";
> -
> -       return "invalid-access";
> -}
> +#include "report_tags.h"
>
>  void *kasan_find_first_bad_addr(void *addr, size_t size)
>  {
> diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h
> new file mode 100644
> index 000000000000..4f740d4d99ee
> --- /dev/null
> +++ b/mm/kasan/report_tags.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __MM_KASAN_REPORT_TAGS_H
> +#define __MM_KASAN_REPORT_TAGS_H
> +
> +#include "kasan.h"
> +#include "../slab.h"
> +
> +#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> +const char *kasan_get_bug_type(struct kasan_access_info *info)
> +{
[...]
> +       /*
> +        * If access_size is a negative number, then it has reason to be
> +        * defined as out-of-bounds bug type.
> +        *
> +        * Casting negative numbers to size_t would indeed turn up as
> +        * a large size_t and its value will be larger than ULONG_MAX/2,
> +        * so that this can qualify as out-of-bounds.
> +        */
> +       if (info->access_addr + info->access_size < info->access_addr)
> +               return "out-of-bounds";

This seems to change behaviour for SW_TAGS because it was there even
if !CONFIG_KASAN_TAGS_IDENTIFY. Does it still work as before?

> +
> +       return "invalid-access";
> +}
> +#else
> +const char *kasan_get_bug_type(struct kasan_access_info *info)
> +{
> +       return "invalid-access";
> +}
> +#endif
> +
> +#endif
> diff --git a/mm/kasan/sw_tags.c b/mm/kasan/sw_tags.c
> index dd05e6c801fa..bd3f540feb47 100644
> --- a/mm/kasan/sw_tags.c
> +++ b/mm/kasan/sw_tags.c
> @@ -167,47 +167,6 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)
>  }
>  EXPORT_SYMBOL(__hwasan_tag_memory);
>
> -void kasan_set_free_info(struct kmem_cache *cache,
> -                               void *object, u8 tag)
> -{
> -       struct kasan_alloc_meta *alloc_meta;
> -       u8 idx = 0;
> -
> -       alloc_meta = kasan_get_alloc_meta(cache, object);
> -       if (!alloc_meta)
> -               return;
> -
> -#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> -       idx = alloc_meta->free_track_idx;
> -       alloc_meta->free_pointer_tag[idx] = tag;
> -       alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
> -#endif
> -
> -       kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
> -}
> -
> -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> -                               void *object, u8 tag)
> -{
> -       struct kasan_alloc_meta *alloc_meta;
> -       int i = 0;
> -
> -       alloc_meta = kasan_get_alloc_meta(cache, object);
> -       if (!alloc_meta)
> -               return NULL;
> -
> -#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> -       for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
> -               if (alloc_meta->free_pointer_tag[i] == tag)
> -                       break;
> -       }
> -       if (i == KASAN_NR_FREE_STACKS)
> -               i = alloc_meta->free_track_idx;
> -#endif
> -
> -       return &alloc_meta->free_track[i];
> -}
> -
>  void kasan_tag_mismatch(unsigned long addr, unsigned long access_info,
>                         unsigned long ret_ip)
>  {
> diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> new file mode 100644
> index 000000000000..9c33c0ebe1d1
> --- /dev/null
> +++ b/mm/kasan/tags.c
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This file contains common tag-based KASAN code.
> + *
> + * Author: Kuan-Ying Lee <kylee0686026@gmail.com>

We appreciate your work on this, but this is misleading. Because you
merely copied/moved the code, have a look what sw_tags.c says -- that
should either be preserved, or we add nothing here.

I prefer to add nothing or the bare minimum (e.g. if the company
requires a Copyright line) for non-substantial additions because this
stuff becomes out-of-date fast and just isn't useful at all. 'git log'
is the source of truth.

Cc'ing Greg for process advice. For moved code, does it have to
preserve the original Copyright line if there was one?

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes
  2021-06-12 14:42   ` Marco Elver
@ 2021-06-12 15:51     ` Kuan-Ying Lee
  2021-06-14  8:48       ` Marco Elver
  2021-06-12 15:52     ` Greg Kroah-Hartman
  1 sibling, 1 reply; 9+ messages in thread
From: Kuan-Ying Lee @ 2021-06-12 15:51 UTC (permalink / raw)
  To: Marco Elver
  Cc: Greg Kroah-Hartman, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, Andrew Morton, kasan-dev, LKML,
	Linux Memory Management List

On Sat, Jun 12, 2021 at 04:42:44PM +0200, Marco Elver wrote:
> On Sat, 12 Jun 2021 at 06:52, Kuan-Ying Lee <kylee0686026@gmail.com> wrote:
> > 1. Move kasan_get_free_track() and kasan_set_free_info()
> >    into tags.c
> > 2. Move kasan_get_bug_type() to header file
> >
> > Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com>
> > Suggested-by: Marco Elver <elver@google.com>
> > Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> > Cc: Alexander Potapenko <glider@google.com>
> > Cc: Andrey Konovalov <andreyknvl@gmail.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  mm/kasan/Makefile         |  4 +--
> >  mm/kasan/hw_tags.c        | 22 ---------------
> >  mm/kasan/report_hw_tags.c |  6 +---
> >  mm/kasan/report_sw_tags.c | 46 +------------------------------
> >  mm/kasan/report_tags.h    | 56 +++++++++++++++++++++++++++++++++++++
> >  mm/kasan/sw_tags.c        | 41 ---------------------------
> >  mm/kasan/tags.c           | 58 +++++++++++++++++++++++++++++++++++++++
> >  7 files changed, 118 insertions(+), 115 deletions(-)
> >  create mode 100644 mm/kasan/report_tags.h
> >  create mode 100644 mm/kasan/tags.c
> [...]
> > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> > index ed5e5b833d61..4ea8c368b5b8 100644
> > --- a/mm/kasan/hw_tags.c
> > +++ b/mm/kasan/hw_tags.c
> > @@ -216,28 +216,6 @@ void __init kasan_init_hw_tags(void)
> >         pr_info("KernelAddressSanitizer initialized\n");
> >  }
> >
> > -void kasan_set_free_info(struct kmem_cache *cache,
> > -                               void *object, u8 tag)
> > -{
> > -       struct kasan_alloc_meta *alloc_meta;
> > -
> > -       alloc_meta = kasan_get_alloc_meta(cache, object);
> > -       if (alloc_meta)
> > -               kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT);
> > -}
> > -
> > -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > -                               void *object, u8 tag)
> > -{
> > -       struct kasan_alloc_meta *alloc_meta;
> > -
> > -       alloc_meta = kasan_get_alloc_meta(cache, object);
> > -       if (!alloc_meta)
> > -               return NULL;
> > -
> > -       return &alloc_meta->free_track[0];
> > -}
> > -
> >  void kasan_alloc_pages(struct page *page, unsigned int order, gfp_t flags)
> >  {
> >         /*
> > diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c
> > index 42b2168755d6..ef5e7378f3aa 100644
> > --- a/mm/kasan/report_hw_tags.c
> > +++ b/mm/kasan/report_hw_tags.c
> > @@ -14,11 +14,7 @@
> >  #include <linux/types.h>
> >
> >  #include "kasan.h"
> > -
> > -const char *kasan_get_bug_type(struct kasan_access_info *info)
> > -{
> > -       return "invalid-access";
> > -}
> > +#include "report_tags.h"
> >
> >  void *kasan_find_first_bad_addr(void *addr, size_t size)
> >  {
> > diff --git a/mm/kasan/report_sw_tags.c b/mm/kasan/report_sw_tags.c
> > index 821a14a19a92..d965a170083e 100644
> > --- a/mm/kasan/report_sw_tags.c
> > +++ b/mm/kasan/report_sw_tags.c
> > @@ -26,51 +26,7 @@
> >
> >  #include <asm/sections.h>
> >
> > -#include "kasan.h"
> > -#include "../slab.h"
> > -
> > -const char *kasan_get_bug_type(struct kasan_access_info *info)
> > -{
> > -#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> > -       struct kasan_alloc_meta *alloc_meta;
> > -       struct kmem_cache *cache;
> > -       struct page *page;
> > -       const void *addr;
> > -       void *object;
> > -       u8 tag;
> > -       int i;
> > -
> > -       tag = get_tag(info->access_addr);
> > -       addr = kasan_reset_tag(info->access_addr);
> > -       page = kasan_addr_to_page(addr);
> > -       if (page && PageSlab(page)) {
> > -               cache = page->slab_cache;
> > -               object = nearest_obj(cache, page, (void *)addr);
> > -               alloc_meta = kasan_get_alloc_meta(cache, object);
> > -
> > -               if (alloc_meta) {
> > -                       for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
> > -                               if (alloc_meta->free_pointer_tag[i] == tag)
> > -                                       return "use-after-free";
> > -                       }
> > -               }
> > -               return "out-of-bounds";
> > -       }
> > -
> > -#endif
> > -       /*
> > -        * If access_size is a negative number, then it has reason to be
> > -        * defined as out-of-bounds bug type.
> > -        *
> > -        * Casting negative numbers to size_t would indeed turn up as
> > -        * a large size_t and its value will be larger than ULONG_MAX/2,
> > -        * so that this can qualify as out-of-bounds.
> > -        */
> > -       if (info->access_addr + info->access_size < info->access_addr)
> > -               return "out-of-bounds";
> > -
> > -       return "invalid-access";
> > -}
> > +#include "report_tags.h"
> >
> >  void *kasan_find_first_bad_addr(void *addr, size_t size)
> >  {
> > diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h
> > new file mode 100644
> > index 000000000000..4f740d4d99ee
> > --- /dev/null
> > +++ b/mm/kasan/report_tags.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __MM_KASAN_REPORT_TAGS_H
> > +#define __MM_KASAN_REPORT_TAGS_H
> > +
> > +#include "kasan.h"
> > +#include "../slab.h"
> > +
> > +#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> > +const char *kasan_get_bug_type(struct kasan_access_info *info)
> > +{
> [...]
> > +       /*
> > +        * If access_size is a negative number, then it has reason to be
> > +        * defined as out-of-bounds bug type.
> > +        *
> > +        * Casting negative numbers to size_t would indeed turn up as
> > +        * a large size_t and its value will be larger than ULONG_MAX/2,
> > +        * so that this can qualify as out-of-bounds.
> > +        */
> > +       if (info->access_addr + info->access_size < info->access_addr)
> > +               return "out-of-bounds";
> 
> This seems to change behaviour for SW_TAGS because it was there even
> if !CONFIG_KASAN_TAGS_IDENTIFY. Does it still work as before?
> 

You are right. It will change the behavior.
However, I think that if !CONFIG_KASAN_TAG_IDENTIFY, it should be reported
"invalid-access".

Or is it better to keep it in both conditions?

> > +
> > +       return "invalid-access";
> > +}
> > +#else
> > +const char *kasan_get_bug_type(struct kasan_access_info *info)
> > +{
> > +       return "invalid-access";
> > +}
> > +#endif
> > +
> > +#endif
> > diff --git a/mm/kasan/sw_tags.c b/mm/kasan/sw_tags.c
> > index dd05e6c801fa..bd3f540feb47 100644
> > --- a/mm/kasan/sw_tags.c
> > +++ b/mm/kasan/sw_tags.c
> > @@ -167,47 +167,6 @@ void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)
> >  }
> >  EXPORT_SYMBOL(__hwasan_tag_memory);
> >
> > -void kasan_set_free_info(struct kmem_cache *cache,
> > -                               void *object, u8 tag)
> > -{
> > -       struct kasan_alloc_meta *alloc_meta;
> > -       u8 idx = 0;
> > -
> > -       alloc_meta = kasan_get_alloc_meta(cache, object);
> > -       if (!alloc_meta)
> > -               return;
> > -
> > -#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> > -       idx = alloc_meta->free_track_idx;
> > -       alloc_meta->free_pointer_tag[idx] = tag;
> > -       alloc_meta->free_track_idx = (idx + 1) % KASAN_NR_FREE_STACKS;
> > -#endif
> > -
> > -       kasan_set_track(&alloc_meta->free_track[idx], GFP_NOWAIT);
> > -}
> > -
> > -struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
> > -                               void *object, u8 tag)
> > -{
> > -       struct kasan_alloc_meta *alloc_meta;
> > -       int i = 0;
> > -
> > -       alloc_meta = kasan_get_alloc_meta(cache, object);
> > -       if (!alloc_meta)
> > -               return NULL;
> > -
> > -#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> > -       for (i = 0; i < KASAN_NR_FREE_STACKS; i++) {
> > -               if (alloc_meta->free_pointer_tag[i] == tag)
> > -                       break;
> > -       }
> > -       if (i == KASAN_NR_FREE_STACKS)
> > -               i = alloc_meta->free_track_idx;
> > -#endif
> > -
> > -       return &alloc_meta->free_track[i];
> > -}
> > -
> >  void kasan_tag_mismatch(unsigned long addr, unsigned long access_info,
> >                         unsigned long ret_ip)
> >  {
> > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> > new file mode 100644
> > index 000000000000..9c33c0ebe1d1
> > --- /dev/null
> > +++ b/mm/kasan/tags.c
> > @@ -0,0 +1,58 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This file contains common tag-based KASAN code.
> > + *
> > + * Author: Kuan-Ying Lee <kylee0686026@gmail.com>
> 
> We appreciate your work on this, but this is misleading. Because you
> merely copied/moved the code, have a look what sw_tags.c says -- that
> should either be preserved, or we add nothing here.
> 
> I prefer to add nothing or the bare minimum (e.g. if the company
> requires a Copyright line) for non-substantial additions because this
> stuff becomes out-of-date fast and just isn't useful at all. 'git log'
> is the source of truth.

This was my first time to upload a new file.
Thanks for the suggestions. :)
I will remove this author tag and wait for Greg's process advice.

> 
> Cc'ing Greg for process advice. For moved code, does it have to
> preserve the original Copyright line if there was one?
> 
> Thanks,
> -- Marco

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes
  2021-06-12 14:42   ` Marco Elver
  2021-06-12 15:51     ` Kuan-Ying Lee
@ 2021-06-12 15:52     ` Greg Kroah-Hartman
  1 sibling, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-12 15:52 UTC (permalink / raw)
  To: Marco Elver
  Cc: Kuan-Ying Lee, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, Andrew Morton, kasan-dev, LKML,
	Linux Memory Management List

On Sat, Jun 12, 2021 at 04:42:44PM +0200, Marco Elver wrote:
> On Sat, 12 Jun 2021 at 06:52, Kuan-Ying Lee <kylee0686026@gmail.com> wrote:
> > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> > new file mode 100644
> > index 000000000000..9c33c0ebe1d1
> > --- /dev/null
> > +++ b/mm/kasan/tags.c
> > @@ -0,0 +1,58 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This file contains common tag-based KASAN code.
> > + *
> > + * Author: Kuan-Ying Lee <kylee0686026@gmail.com>
> 
> We appreciate your work on this, but this is misleading. Because you
> merely copied/moved the code, have a look what sw_tags.c says -- that
> should either be preserved, or we add nothing here.
> 
> I prefer to add nothing or the bare minimum (e.g. if the company
> requires a Copyright line) for non-substantial additions because this
> stuff becomes out-of-date fast and just isn't useful at all. 'git log'
> is the source of truth.
> 
> Cc'ing Greg for process advice. For moved code, does it have to
> preserve the original Copyright line if there was one?

Yes, it does have to.  Unless you want to talk to a lot of lawyers about
the issues involved here and can defend the removal of the copyright
lines to them.

So please keep them.  Unless you can get your corporate lawyer to sign
off on the patch that does the removal.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes
  2021-06-12 15:51     ` Kuan-Ying Lee
@ 2021-06-14  8:48       ` Marco Elver
  2021-06-19  6:39         ` Kuan-Ying Lee
  0 siblings, 1 reply; 9+ messages in thread
From: Marco Elver @ 2021-06-14  8:48 UTC (permalink / raw)
  To: Kuan-Ying Lee
  Cc: Greg Kroah-Hartman, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, Andrew Morton, kasan-dev, LKML,
	Linux Memory Management List

On Sat, 12 Jun 2021 at 17:51, Kuan-Ying Lee <kylee0686026@gmail.com> wrote:
[...]
> > > diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h
> > > new file mode 100644
> > > index 000000000000..4f740d4d99ee
> > > --- /dev/null
> > > +++ b/mm/kasan/report_tags.h
> > > @@ -0,0 +1,56 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __MM_KASAN_REPORT_TAGS_H
> > > +#define __MM_KASAN_REPORT_TAGS_H
> > > +
> > > +#include "kasan.h"
> > > +#include "../slab.h"
> > > +
> > > +#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> > > +const char *kasan_get_bug_type(struct kasan_access_info *info)
> > > +{
> > [...]
> > > +       /*
> > > +        * If access_size is a negative number, then it has reason to be
> > > +        * defined as out-of-bounds bug type.
> > > +        *
> > > +        * Casting negative numbers to size_t would indeed turn up as
> > > +        * a large size_t and its value will be larger than ULONG_MAX/2,
> > > +        * so that this can qualify as out-of-bounds.
> > > +        */
> > > +       if (info->access_addr + info->access_size < info->access_addr)
> > > +               return "out-of-bounds";
> >
> > This seems to change behaviour for SW_TAGS because it was there even
> > if !CONFIG_KASAN_TAGS_IDENTIFY. Does it still work as before?
> >
>
> You are right. It will change the behavior.
> However, I think that if !CONFIG_KASAN_TAG_IDENTIFY, it should be reported
> "invalid-access".

There's no reason that if !CONFIG_KASAN_TAG_IDENTIFY it should be
reported as "invalid-acces" if we can do better without the additional
state that the config option introduces.

It's trivial to give a slightly better report without additional
state, see the comment explaining why it's reasonable to infer
out-of-bounds here.

> Or is it better to keep it in both conditions?

We want to make this patch a non-functional change.

[...]
> > > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> > > new file mode 100644
> > > index 000000000000..9c33c0ebe1d1
> > > --- /dev/null
> > > +++ b/mm/kasan/tags.c
> > > @@ -0,0 +1,58 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * This file contains common tag-based KASAN code.
> > > + *
> > > + * Author: Kuan-Ying Lee <kylee0686026@gmail.com>
> >
> > We appreciate your work on this, but this is misleading. Because you
> > merely copied/moved the code, have a look what sw_tags.c says -- that
> > should either be preserved, or we add nothing here.
> >
> > I prefer to add nothing or the bare minimum (e.g. if the company
> > requires a Copyright line) for non-substantial additions because this
> > stuff becomes out-of-date fast and just isn't useful at all. 'git log'
> > is the source of truth.
>
> This was my first time to upload a new file.
> Thanks for the suggestions. :)
> I will remove this author tag and wait for Greg's process advice.
>
> >
> > Cc'ing Greg for process advice. For moved code, does it have to
> > preserve the original Copyright line if there was one?

Greg responded, see his emails. Please preserve the original header
from the file the code was moved from (hw_tags.c/sw_tags.c).

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes
  2021-06-14  8:48       ` Marco Elver
@ 2021-06-19  6:39         ` Kuan-Ying Lee
  0 siblings, 0 replies; 9+ messages in thread
From: Kuan-Ying Lee @ 2021-06-19  6:39 UTC (permalink / raw)
  To: Marco Elver
  Cc: Greg Kroah-Hartman, Andrey Ryabinin, Alexander Potapenko,
	Andrey Konovalov, Dmitry Vyukov, Andrew Morton, kasan-dev, LKML,
	Linux Memory Management List

On Mon, Jun 14, 2021 at 10:48:27AM +0200, Marco Elver wrote:
> On Sat, 12 Jun 2021 at 17:51, Kuan-Ying Lee <kylee0686026@gmail.com> wrote:
> [...]
> > > > diff --git a/mm/kasan/report_tags.h b/mm/kasan/report_tags.h
> > > > new file mode 100644
> > > > index 000000000000..4f740d4d99ee
> > > > --- /dev/null
> > > > +++ b/mm/kasan/report_tags.h
> > > > @@ -0,0 +1,56 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +#ifndef __MM_KASAN_REPORT_TAGS_H
> > > > +#define __MM_KASAN_REPORT_TAGS_H
> > > > +
> > > > +#include "kasan.h"
> > > > +#include "../slab.h"
> > > > +
> > > > +#ifdef CONFIG_KASAN_TAGS_IDENTIFY
> > > > +const char *kasan_get_bug_type(struct kasan_access_info *info)
> > > > +{
> > > [...]
> > > > +       /*
> > > > +        * If access_size is a negative number, then it has reason to be
> > > > +        * defined as out-of-bounds bug type.
> > > > +        *
> > > > +        * Casting negative numbers to size_t would indeed turn up as
> > > > +        * a large size_t and its value will be larger than ULONG_MAX/2,
> > > > +        * so that this can qualify as out-of-bounds.
> > > > +        */
> > > > +       if (info->access_addr + info->access_size < info->access_addr)
> > > > +               return "out-of-bounds";
> > >
> > > This seems to change behaviour for SW_TAGS because it was there even
> > > if !CONFIG_KASAN_TAGS_IDENTIFY. Does it still work as before?
> > >
> >
> > You are right. It will change the behavior.
> > However, I think that if !CONFIG_KASAN_TAG_IDENTIFY, it should be reported
> > "invalid-access".
> 
> There's no reason that if !CONFIG_KASAN_TAG_IDENTIFY it should be
> reported as "invalid-acces" if we can do better without the additional
> state that the config option introduces.
> 
> It's trivial to give a slightly better report without additional
> state, see the comment explaining why it's reasonable to infer
> out-of-bounds here.
> 
> > Or is it better to keep it in both conditions?
> 
> We want to make this patch a non-functional change.
>

Got it.

> [...]
> > > > diff --git a/mm/kasan/tags.c b/mm/kasan/tags.c
> > > > new file mode 100644
> > > > index 000000000000..9c33c0ebe1d1
> > > > --- /dev/null
> > > > +++ b/mm/kasan/tags.c
> > > > @@ -0,0 +1,58 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * This file contains common tag-based KASAN code.
> > > > + *
> > > > + * Author: Kuan-Ying Lee <kylee0686026@gmail.com>
> > >
> > > We appreciate your work on this, but this is misleading. Because you
> > > merely copied/moved the code, have a look what sw_tags.c says -- that
> > > should either be preserved, or we add nothing here.
> > >
> > > I prefer to add nothing or the bare minimum (e.g. if the company
> > > requires a Copyright line) for non-substantial additions because this
> > > stuff becomes out-of-date fast and just isn't useful at all. 'git log'
> > > is the source of truth.
> >
> > This was my first time to upload a new file.
> > Thanks for the suggestions. :)
> > I will remove this author tag and wait for Greg's process advice.
> >
> > >
> > > Cc'ing Greg for process advice. For moved code, does it have to
> > > preserve the original Copyright line if there was one?
> 
> Greg responded, see his emails. Please preserve the original header
> from the file the code was moved from (hw_tags.c/sw_tags.c).

Ok. I will do it in v3.
Thanks.

> 
> Thanks,
> -- Marco

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-19  6:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-12  4:51 [PATCH v2 0/3] kasan: add memory corruption identification for hw tag-based kasan Kuan-Ying Lee
2021-06-12  4:51 ` [PATCH v2 1/3] kasan: rename CONFIG_KASAN_SW_TAGS_IDENTIFY to CONFIG_KASAN_TAGS_IDENTIFY Kuan-Ying Lee
2021-06-12  4:51 ` [PATCH v2 2/3] kasan: integrate the common part of two KASAN tag-based modes Kuan-Ying Lee
2021-06-12 14:42   ` Marco Elver
2021-06-12 15:51     ` Kuan-Ying Lee
2021-06-14  8:48       ` Marco Elver
2021-06-19  6:39         ` Kuan-Ying Lee
2021-06-12 15:52     ` Greg Kroah-Hartman
2021-06-12  4:51 ` [PATCH v2 3/3] kasan: add memory corruption identification support for hardware tag-based mode Kuan-Ying Lee

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).