[PATCH 0/1] kasan: add memory corruption identification for hardware tag-based

[PATCH 0/1] kasan: add memory corruption identification for hardware tag-based

[Kuan-Ying Lee]

This patch is for hardware tag-based kasan to make a good guess
on the kasan bug. Report kasan bug is slab out-of-bound or
use-after-free. Refer to software tag-based kasan detection
mechanism.

Kuan-Ying Lee (1):
  kasan: add memory corruption identification for hardware tag-based
    mode

 lib/Kconfig.kasan         |  8 ++++++++
 mm/kasan/hw_tags.c        | 25 ++++++++++++++++++++++---
 mm/kasan/kasan.h          |  4 ++--
 mm/kasan/report_hw_tags.c | 28 ++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH 1/1] kasan: add memory corruption identification for hardware tag-based mode

[Kuan-Ying Lee]

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

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

Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com>
---
 lib/Kconfig.kasan         |  8 ++++++++
 mm/kasan/hw_tags.c        | 25 ++++++++++++++++++++++---
 mm/kasan/kasan.h          |  4 ++--
 mm/kasan/report_hw_tags.c | 28 ++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index cffc2ebbf185..f7e666b23058 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -163,6 +163,14 @@ config KASAN_SW_TAGS_IDENTIFY
 	  (use-after-free or out-of-bounds) at the cost of increased
 	  memory consumption.
 
+config KASAN_HW_TAGS_IDENTIFY
+	bool "Enable memory corruption identification"
+	depends on 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
+	  memory consumption.
+
 config KASAN_VMALLOC
 	bool "Back mappings in vmalloc space with real shadow memory"
 	depends on KASAN_GENERIC && HAVE_ARCH_KASAN_VMALLOC
diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
index 4004388b4e4b..b1c6bb116600 100644
--- a/mm/kasan/hw_tags.c
+++ b/mm/kasan/hw_tags.c
@@ -220,22 +220,41 @@ 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)
-		kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT);
+	if (!alloc_meta)
+		return;
+
+#ifdef CONFIG_KASAN_HW_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;
 
-	return &alloc_meta->free_track[0];
+#ifdef CONFIG_KASAN_HW_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];
 }
 
 #if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST)
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index 8f450bc28045..41b47f456130 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
+#if defined(CONFIG_KASAN_SW_TAGS_IDENTIFY) || defined(CONFIG_KASAN_HW_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
+#if defined(CONFIG_KASAN_SW_TAGS_IDENTIFY) || defined(CONFIG_KASAN_HW_TAGS_IDENTIFY)
 	u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
 	u8 free_track_idx;
 #endif
diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c
index 42b2168755d6..d77109b85a09 100644
--- a/mm/kasan/report_hw_tags.c
+++ b/mm/kasan/report_hw_tags.c
@@ -14,9 +14,37 @@
 #include <linux/types.h>
 
 #include "kasan.h"
+#include "../slab.h"
 
 const char *kasan_get_bug_type(struct kasan_access_info *info)
 {
+#ifdef CONFIG_KASAN_HW_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
 	return "invalid-access";
 }
 
-- 
2.17.1

Re: [PATCH 1/1] kasan: add memory corruption identification for hardware tag-based mode

[Marco Elver]

On Sun, May 30, 2021 at 12:47PM +0800, Kuan-Ying Lee wrote:
> Add memory corruption identification at bug report for hardware tag-based
> mode. The report shows whether it is "use-after-free" or "out-of-bound"
> error instead of "invalid-access" error. This will make it easier for
> programmers to see the memory corruption problem.
> 
> We extend the slab to store five old free pointer tag and free backtrace,
> we can check if the tagged address is in the slab record and make a good
> guess if the object is more like "use-after-free" or "out-of-bound".
> therefore every slab memory corruption can be identified whether it's
> "use-after-free" or "out-of-bound".
> 
> Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com>

On a whole this makes sense because SW_TAGS mode supports this, too.

My main complaints are the copy-paste of the SW_TAGS code.

Does it make sense to refactor per my suggestions below?

This is also a question to KASAN maintainers (Andrey, any preference?).

> ---
>  lib/Kconfig.kasan         |  8 ++++++++
>  mm/kasan/hw_tags.c        | 25 ++++++++++++++++++++++---
>  mm/kasan/kasan.h          |  4 ++--
>  mm/kasan/report_hw_tags.c | 28 ++++++++++++++++++++++++++++
>  4 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index cffc2ebbf185..f7e666b23058 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -163,6 +163,14 @@ config KASAN_SW_TAGS_IDENTIFY
>  	  (use-after-free or out-of-bounds) at the cost of increased
>  	  memory consumption.
>  
> +config KASAN_HW_TAGS_IDENTIFY
> +	bool "Enable memory corruption identification"
> +	depends on 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
> +	  memory consumption.

Can we rename KASAN_SW_TAGS_IDENTIFY -> KASAN_TAGS_IDENTIFY in a
separate patch and then use that?

Or do we have a problem renaming this options if there are existing
users of it?

>  config KASAN_VMALLOC
>  	bool "Back mappings in vmalloc space with real shadow memory"
>  	depends on KASAN_GENERIC && HAVE_ARCH_KASAN_VMALLOC
> diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> index 4004388b4e4b..b1c6bb116600 100644
> --- a/mm/kasan/hw_tags.c
> +++ b/mm/kasan/hw_tags.c
> @@ -220,22 +220,41 @@ 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)
> -		kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT);
> +	if (!alloc_meta)
> +		return;
> +
> +#ifdef CONFIG_KASAN_HW_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;
>  
> -	return &alloc_meta->free_track[0];
> +#ifdef CONFIG_KASAN_HW_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];
>  }

Again, we now have code duplication. These functions are now identical
to the sw_tags.c ones?

Does it make sense to also move them in a preparatory patch to a new
'tags.c'?

>  #if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST)
> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> index 8f450bc28045..41b47f456130 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
> +#if defined(CONFIG_KASAN_SW_TAGS_IDENTIFY) || defined(CONFIG_KASAN_HW_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
> +#if defined(CONFIG_KASAN_SW_TAGS_IDENTIFY) || defined(CONFIG_KASAN_HW_TAGS_IDENTIFY)
>  	u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
>  	u8 free_track_idx;
>  #endif
> diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c
> index 42b2168755d6..d77109b85a09 100644
> --- a/mm/kasan/report_hw_tags.c
> +++ b/mm/kasan/report_hw_tags.c
> @@ -14,9 +14,37 @@
>  #include <linux/types.h>
>  
>  #include "kasan.h"
> +#include "../slab.h"
>  
>  const char *kasan_get_bug_type(struct kasan_access_info *info)
>  {
> +#ifdef CONFIG_KASAN_HW_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
>  	return "invalid-access";
>  }

This function is an almost copy-paste of what we have in
report_sw_tags.c. Does it make sense to try and share this code or would
it complicate things?

I imagine we could have a header report_tags.h, which defines a static
const char *kasan_try_get_bug_type(..), and simply returns NULL if it
couldn't identify it:

	#if defined(CONFIG_KASAN_SW_TAGS_IDENTIFY) || defined(CONFIG_KASAN_HW_TAGS_IDENTIFY)
	static const char *kasan_try_get_bug_type(struct kasan_access_info *info)
	{
		... the code above ...

		return NULL;
	}
	#else
	static const char *kasan_try_get_bug_type(struct kasan_access_info *info) { return NULL; }
	#endif


Thanks,
-- Marco

Re: [PATCH 1/1] kasan: add memory corruption identification for hardware tag-based mode

[Kuan-Ying Lee]

On Mon, May 31, 2021 at 10:50:24AM +0200, Marco Elver wrote:
> On Sun, May 30, 2021 at 12:47PM +0800, Kuan-Ying Lee wrote:
> > Add memory corruption identification at bug report for hardware tag-based
> > mode. The report shows whether it is "use-after-free" or "out-of-bound"
> > error instead of "invalid-access" error. This will make it easier for
> > programmers to see the memory corruption problem.
> > 
> > We extend the slab to store five old free pointer tag and free backtrace,
> > we can check if the tagged address is in the slab record and make a good
> > guess if the object is more like "use-after-free" or "out-of-bound".
> > therefore every slab memory corruption can be identified whether it's
> > "use-after-free" or "out-of-bound".
> > 
> > Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com>
> 

> On a whole this makes sense because SW_TAGS mode supports this, too.
> 
> My main complaints are the copy-paste of the SW_TAGS code.
> 
> Does it make sense to refactor per my suggestions below?

Thanks for your suggestions.
I will refactor them in v2.
> 
> This is also a question to KASAN maintainers (Andrey, any preference?).
> 
> > ---
> >  lib/Kconfig.kasan         |  8 ++++++++
> >  mm/kasan/hw_tags.c        | 25 ++++++++++++++++++++++---
> >  mm/kasan/kasan.h          |  4 ++--
> >  mm/kasan/report_hw_tags.c | 28 ++++++++++++++++++++++++++++
> >  4 files changed, 60 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> > index cffc2ebbf185..f7e666b23058 100644
> > --- a/lib/Kconfig.kasan
> > +++ b/lib/Kconfig.kasan
> > @@ -163,6 +163,14 @@ config KASAN_SW_TAGS_IDENTIFY
> >  	  (use-after-free or out-of-bounds) at the cost of increased
> >  	  memory consumption.
> >  
> > +config KASAN_HW_TAGS_IDENTIFY
> > +	bool "Enable memory corruption identification"
> > +	depends on 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
> > +	  memory consumption.
> 
> Can we rename KASAN_SW_TAGS_IDENTIFY -> KASAN_TAGS_IDENTIFY in a
> separate patch and then use that?
> 
> Or do we have a problem renaming this options if there are existing
> users of it?

I tend to keep KASAN_SW_TAGS_IDENTIFY and KASAN_HW_TAGS_IDENTIFY
separately.

We need these two configs to decide how many stacks we will store.

If we store as many stacks as SW tag-based kasan does(5 stacks), we might
mistake out-of-bound issues for use-after-free sometime. Becuase HW
tag-based kasan only has 16 kinds of tags. When Out-of-bound issues happened, it might
find the same tag in the stack we just stored and mistake happened.
There is high probability that this mistake will happen.
> 
> >  config KASAN_VMALLOC
> >  	bool "Back mappings in vmalloc space with real shadow memory"
> >  	depends on KASAN_GENERIC && HAVE_ARCH_KASAN_VMALLOC
> > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> > index 4004388b4e4b..b1c6bb116600 100644
> > --- a/mm/kasan/hw_tags.c
> > +++ b/mm/kasan/hw_tags.c
> > @@ -220,22 +220,41 @@ 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)
> > -		kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT);
> > +	if (!alloc_meta)
> > +		return;
> > +
> > +#ifdef CONFIG_KASAN_HW_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;
> >  
> > -	return &alloc_meta->free_track[0];
> > +#ifdef CONFIG_KASAN_HW_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];
> >  }
> 
> Again, we now have code duplication. These functions are now identical
> to the sw_tags.c ones?
> 
> Does it make sense to also move them in a preparatory patch to a new
> 'tags.c'?
> 
Yes, moving them into tags.c will be better.
I will refactor in v2.
> >  #if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST)
> > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > index 8f450bc28045..41b47f456130 100644
> > --- a/mm/kasan/kasan.h
> > +++ b/mm/kasan/kasan.h
> > @@ -153,7 +153,7 @@ struct kasan_track {
> >  	depot_stack_handle_t stack;
> >  };
> >  

I think my v1 patch sets KASAN_NR_FREE_STACKS to 5 is not suitable.
The same reason as above.

I am thinking to store 2 or 1 stacks is acceptable in HW tag-based kasan mode.
Does it make sense?
Any suggetions are appreciated.

> > -#ifdef CONFIG_KASAN_SW_TAGS_IDENTIFY
> > +#if defined(CONFIG_KASAN_SW_TAGS_IDENTIFY) || defined(CONFIG_KASAN_HW_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
> > +#if defined(CONFIG_KASAN_SW_TAGS_IDENTIFY) || defined(CONFIG_KASAN_HW_TAGS_IDENTIFY)
> >  	u8 free_pointer_tag[KASAN_NR_FREE_STACKS];
> >  	u8 free_track_idx;
> >  #endif
> > diff --git a/mm/kasan/report_hw_tags.c b/mm/kasan/report_hw_tags.c
> > index 42b2168755d6..d77109b85a09 100644
> > --- a/mm/kasan/report_hw_tags.c
> > +++ b/mm/kasan/report_hw_tags.c
> > @@ -14,9 +14,37 @@
> >  #include <linux/types.h>
> >  
> >  #include "kasan.h"
> > +#include "../slab.h"
> >  
> >  const char *kasan_get_bug_type(struct kasan_access_info *info)
> >  {
> > +#ifdef CONFIG_KASAN_HW_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
> >  	return "invalid-access";
> >  }
> 
> This function is an almost copy-paste of what we have in
> report_sw_tags.c. Does it make sense to try and share this code or would
> it complicate things?
> 

I got your point.
I will refactor them in v2.

Thanks,
Kuan-Ying Lee

> I imagine we could have a header report_tags.h, which defines a static
> const char *kasan_try_get_bug_type(..), and simply returns NULL if it
> couldn't identify it:
> 
> 	#if defined(CONFIG_KASAN_SW_TAGS_IDENTIFY) || defined(CONFIG_KASAN_HW_TAGS_IDENTIFY)
> 	static const char *kasan_try_get_bug_type(struct kasan_access_info *info)
> 	{
> 		... the code above ...
> 
> 		return NULL;
> 	}
> 	#else
> 	static const char *kasan_try_get_bug_type(struct kasan_access_info *info) { return NULL; }
> 	#endif
> 
> 
> Thanks,
> -- Marco

Re: [PATCH 1/1] kasan: add memory corruption identification for hardware tag-based mode

[Andrey Konovalov]

On Mon, May 31, 2021 at 6:59 PM Kuan-Ying Lee <kylee0686026@gmail.com> wrote:
>
> > >
> > > +config KASAN_HW_TAGS_IDENTIFY
> > > +   bool "Enable memory corruption identification"
> > > +   depends on 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
> > > +     memory consumption.
> >
> > Can we rename KASAN_SW_TAGS_IDENTIFY -> KASAN_TAGS_IDENTIFY in a
> > separate patch and then use that?
> >
> > Or do we have a problem renaming this options if there are existing
> > users of it?

Using the single KASAN_TAGS_IDENTIFY config option is what I would like to see.

Since this is a purely debugging feature for a less popular KASAN
mode, I think renaming the config name is OK.

> I tend to keep KASAN_SW_TAGS_IDENTIFY and KASAN_HW_TAGS_IDENTIFY
> separately.
>
> We need these two configs to decide how many stacks we will store.

You can define KASAN_NR_FREE_STACKS to different values depending on
whether HW_TAGS or SW_TAGS is in use. I don't see a problem here.

> If we store as many stacks as SW tag-based kasan does(5 stacks), we might
> mistake out-of-bound issues for use-after-free sometime. Becuase HW
> tag-based kasan only has 16 kinds of tags. When Out-of-bound issues happened, it might
> find the same tag in the stack we just stored and mistake happened.
> There is high probability that this mistake will happen.

Re: [PATCH 1/1] kasan: add memory corruption identification for hardware tag-based mode

[Andrey Konovalov]

On Mon, May 31, 2021 at 11:50 AM Marco Elver <elver@google.com> wrote:
>
> On Sun, May 30, 2021 at 12:47PM +0800, Kuan-Ying Lee wrote:
> > Add memory corruption identification at bug report for hardware tag-based
> > mode. The report shows whether it is "use-after-free" or "out-of-bound"
> > error instead of "invalid-access" error. This will make it easier for
> > programmers to see the memory corruption problem.
> >
> > We extend the slab to store five old free pointer tag and free backtrace,
> > we can check if the tagged address is in the slab record and make a good
> > guess if the object is more like "use-after-free" or "out-of-bound".
> > therefore every slab memory corruption can be identified whether it's
> > "use-after-free" or "out-of-bound".
> >
> > Signed-off-by: Kuan-Ying Lee <kylee0686026@gmail.com>
>
> On a whole this makes sense because SW_TAGS mode supports this, too.
>
> My main complaints are the copy-paste of the SW_TAGS code.
>
> Does it make sense to refactor per my suggestions below?
>
> This is also a question to KASAN maintainers (Andrey, any preference?).

All of your comments are valid. Thank you, Marco.