kasan: hardware tag-based mode for production use on arm64

Similarly to kasan_init() mark kasan_init_tags() as __init.

Signed-off-by Andrey Konovalov <[email protected]>

Link https://linux-review.googlesource.com/id/I8792e22f1ca5a703c5e979969147968a99312558

---
 include/linux/kasan.h | 4 ++--
 mm/kasan/hw_tags.c    | 2 +-
 mm/kasan/sw_tags.c    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

include/linux/kasan.h +1 -1

index 7be9fb9146ac..af8317b416a8 100644
@@ -185,7 +185,7 @@ static inline void kasan_record_aux_stack(void *ptr) {}
 
 #if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS)
 
-void kasan_init_tags(void);
+void __init kasan_init_tags(void);
 
 void *kasan_reset_tag(const void *addr);
 

include/linux/kasan.h +0 -0

 static inline void *kasan_reset_tag(const void *addr)
 {

mm/kasan/hw_tags.c +1 -1

index 2a38885014e3..0128062320d5 100644
@@ -15,7 +15,7 @@
 
 #include "kasan.h"
 
-void kasan_init_tags(void)
+void __init kasan_init_tags(void)
 {
        init_tags(KASAN_TAG_MAX);
 }

mm/kasan/sw_tags.c +1 -2

index c10863a45775..bf1422282bb5 100644
@@ -35,7 +35,7 @@
 
 static DEFINE_PER_CPU(u32, prng_state);
 
-void kasan_init_tags(void)
+void __init kasan_init_tags(void)
 {
        int cpu;
 
--

2.28.0.1011.ga647a8990f-goog

Quoted 26 lines · Andrey Konovalov

Similarly to kasan_init() mark kasan_init_tags() as __init.

Signed-off-by Andrey Konovalov <[email protected]>

Link https://linux-review.googlesource.com/id/I8792e22f1ca5a703c5e979969147968a99312558

---
 include/linux/kasan.h | 4 ++--
 mm/kasan/hw_tags.c    | 2 +-
 mm/kasan/sw_tags.c    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

include/linux/kasan.h +1 -1

index 7be9fb9146ac..af8317b416a8 100644
@@ -185,7 +185,7 @@ static inline void kasan_record_aux_stack(void *ptr) {}
 
 #if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS)
 
-void kasan_init_tags(void);
+void __init kasan_init_tags(void);
 
 void *kasan_reset_tag(const void *addr);
 

@@ -194,7 +194,7 @@ bool kasan_report(unsigned long addr, size_t size,
 

include/linux/kasan.h bool kasan_report(unsigned long addr, size_t size, · line 197

Quoted 4 lines

include/linux/kasan.h +1 -1

 #else /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS */
 
-static inline void kasan_init_tags(void) { }
+static inline void __init kasan_init_tags(void) { }

Should we mark empty static inline functions __init? __init comes with a bunch of other attributes, but hopefully they don't interfere with inlining?

#ifdef CONFIG_KASAN_HW_TAGS
+#define arch_system_supports_tags() system_supports_mte()

#define arch_init_tags(max_tag) mte_init_tags(max_tag)

On Wed, 14 Oct 2020 22:44:35 +0200

Quoted 3 lines

#ifdef CONFIG_KASAN_HW_TAGS
+#define arch_system_supports_tags() system_supports_mte()

s/system_supports/support/ in order to look more like the brother of

Quoted 1 line

#define arch_init_tags(max_tag) mte_init_tags(max_tag)

TODO: no meaningful description here yet, please see the cover letter
for this RFC series.

Signed-off-by Andrey Konovalov <[email protected]>

Link https://linux-review.googlesource.com/id/If7d37003875b2ed3e0935702c8015c223d6416a4

---
 mm/kasan/common.c  | 69 +++++++++++++++++++++++++---------------------
 mm/kasan/generic.c |  4 +++
 mm/kasan/hw_tags.c | 53 +++++++++++++++++++++++++++++++++++
 mm/kasan/kasan.h   |  8 ++++++
 mm/kasan/report.c  | 10 +++++--
 mm/kasan/sw_tags.c |  4 +++
 6 files changed, 115 insertions(+), 33 deletions(-)

mm/kasan/common.c +30 -28

index a3e67d49b893..d642d5fce1e5 100644
@@ -135,35 +135,37 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
        unsigned int redzone_size;
        int redzone_adjust;
 
-       /* Add alloc meta. */
-       cache->kasan_info.alloc_meta_offset = *size;
-       *size += sizeof(struct kasan_alloc_meta);
-
-       /* Add free meta. */
-       if (IS_ENABLED(CONFIG_KASAN_GENERIC) &&
-           (cache->flags & SLAB_TYPESAFE_BY_RCU || cache->ctor ||
-            cache->object_size < sizeof(struct kasan_free_meta))) {
-               cache->kasan_info.free_meta_offset = *size;
-               *size += sizeof(struct kasan_free_meta);
-       }
-
-       redzone_size = optimal_redzone(cache->object_size);
-       redzone_adjust = redzone_size - (*size - cache->object_size);
-       if (redzone_adjust > 0)
-               *size += redzone_adjust;
-
-       *size = min_t(unsigned int, KMALLOC_MAX_SIZE,
-                       max(*size, cache->object_size + redzone_size));
+       if (static_branch_unlikely(&kasan_debug)) {
+               /* Add alloc meta. */
+               cache->kasan_info.alloc_meta_offset = *size;
+               *size += sizeof(struct kasan_alloc_meta);
+
+               /* Add free meta. */
+               if (IS_ENABLED(CONFIG_KASAN_GENERIC) &&
+                   (cache->flags & SLAB_TYPESAFE_BY_RCU || cache->ctor ||
+                    cache->object_size < sizeof(struct kasan_free_meta))) {
+                       cache->kasan_info.free_meta_offset = *size;
+                       *size += sizeof(struct kasan_free_meta);
+               }
 
-       /*
-        * If the metadata doesn't fit, don't enable KASAN at all.
-        */
-       if (*size <= cache->kasan_info.alloc_meta_offset ||
-                       *size <= cache->kasan_info.free_meta_offset) {
-               cache->kasan_info.alloc_meta_offset = 0;
-               cache->kasan_info.free_meta_offset = 0;
-               *size = orig_size;
-               return;
+               redzone_size = optimal_redzone(cache->object_size);
+               redzone_adjust = redzone_size - (*size - cache->object_size);
+               if (redzone_adjust > 0)
+                       *size += redzone_adjust;
+
+               *size = min_t(unsigned int, KMALLOC_MAX_SIZE,
+                               max(*size, cache->object_size + redzone_size));
+
+               /*
+                * If the metadata doesn't fit, don't enable KASAN at all.
+                */
+               if (*size <= cache->kasan_info.alloc_meta_offset ||
+                               *size <= cache->kasan_info.free_meta_offset) {
+                       cache->kasan_info.alloc_meta_offset = 0;
+                       cache->kasan_info.free_meta_offset = 0;
+                       *size = orig_size;
+                       return;
+               }
        }
 
        *flags |= SLAB_KASAN;

mm/kasan/common.c +6 -3

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

@@ -266,8 +270,10 @@ void * __must_check kasan_init_slab_obj(struct kmem_cache *cache,
        if (!(cache->flags & SLAB_KASAN))
                return (void *)object;
 
-       alloc_meta = kasan_get_alloc_meta(cache, object);
-       __memset(alloc_meta, 0, sizeof(*alloc_meta));
+       if (static_branch_unlikely(&kasan_debug)) {
+               alloc_meta = kasan_get_alloc_meta(cache, object);
+               __memset(alloc_meta, 0, sizeof(*alloc_meta));
+       }
 
        if (IS_ENABLED(CONFIG_KASAN_SW_TAGS) || IS_ENABLED(CONFIG_KASAN_HW_TAGS))
                object = set_tag(object, assign_tag(cache, object, true, false));

@@ -305,6 +311,7 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object,
        kasan_poison_memory(object, rounded_up_size, KASAN_KMALLOC_FREE);
 
        if ((IS_ENABLED(CONFIG_KASAN_GENERIC) && !quarantine) ||
+                       !static_branch_unlikely(&kasan_debug) ||
                        unlikely(!(cache->flags & SLAB_KASAN)))
                return false;
 

@@ -351,7 +358,7 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,
        kasan_poison_memory((void *)redzone_start, redzone_end - redzone_start,
                KASAN_KMALLOC_REDZONE);
 
-       if (cache->flags & SLAB_KASAN)
+       if (static_branch_unlikely(&kasan_debug) && cache->flags & SLAB_KASAN)
                set_alloc_info(cache, (void *)object, flags);
 
        return set_tag(object, tag);

mm/kasan/generic.c +4 -0

index d259e4c3aefd..9d968eaedc98 100644
@@ -33,6 +33,10 @@
 #include "kasan.h"
 #include "../slab.h"
 
+/* See the comments in hw_tags.c */
+DEFINE_STATIC_KEY_TRUE_RO(kasan_enabled);
+DEFINE_STATIC_KEY_TRUE_RO(kasan_debug);
+
 /*
  * All functions below always inlined so compiler could
  * perform better optimizations in each of __asan_loadX/__assn_storeX

mm/kasan/hw_tags.c +9 -0

index b372421258c8..fc6ab1c8b155 100644
@@ -8,6 +8,8 @@
 
 #define pr_fmt(fmt) "kasan: " fmt
 
+#include <linux/init.h>
+#include <linux/jump_label.h>
 #include <linux/kasan.h>
 #include <linux/kernel.h>
 #include <linux/memory.h>

@@ -17,8 +19,57 @@
 
 #include "kasan.h"
 
+enum kasan_mode {
+       KASAN_MODE_OFF,
+       KASAN_MODE_ON,
+       KASAN_MODE_DEBUG,
+};
+
+static enum kasan_mode kasan_mode __ro_after_init;

mm/kasan/hw_tags.c +23 -0

+               kasan_mode = KASAN_MODE_DEBUG;
+       return 0;
+}
+early_param("kasan_mode", early_kasan_mode);
+
 void __init kasan_init_tags(void)
 {
+       /* TODO: system_supports_tags() always returns 0 here, fix. */
+       if (0 /*!system_supports_tags()*/)
+               return;
+
+       switch (kasan_mode) {
+       case KASAN_MODE_OFF:
+               return;
+       case KASAN_MODE_ON:
+               static_branch_enable(&kasan_enabled);
+               break;
+       case KASAN_MODE_DEBUG:
+               static_branch_enable(&kasan_enabled);
+               static_branch_enable(&kasan_debug);
+               kasan_sync = true;
+               break;
+       }
+
+       /* TODO: choose between sync and async based on kasan_sync. */
        init_tags(KASAN_TAG_MAX);
 
        pr_info("KernelAddressSanitizer initialized\n");

mm/kasan/hw_tags.c +1 -0

        alloc_meta = kasan_get_alloc_meta(cache, object);
        kasan_set_track(&alloc_meta->free_track[0], GFP_NOWAIT);
 }

@@ -69,6 +121,7 @@ struct kasan_track *kasan_get_free_track(struct kmem_cache *cache,
 {
        struct kasan_alloc_meta *alloc_meta;
 
+       WARN_ON(!static_branch_unlikely(&kasan_debug));
        alloc_meta = kasan_get_alloc_meta(cache, object);
        return &alloc_meta->free_track[0];
 }

mm/kasan/kasan.h +8 -0

index 47d6074c7958..3712e7a39717 100644
@@ -279,6 +279,14 @@ static inline const void *arch_kasan_set_tag(const void *addr, u8 tag)
 #define get_mem_tag(addr)                      arch_get_mem_tag(addr)
 #define set_mem_tag_range(addr, size, tag)     arch_set_mem_tag_range((addr), (size), (tag))
 
+#ifdef CONFIG_KASAN_HW_TAGS
+DECLARE_STATIC_KEY_FALSE(kasan_enabled);
+DECLARE_STATIC_KEY_FALSE(kasan_debug);
+#else
+DECLARE_STATIC_KEY_TRUE(kasan_enabled);
+DECLARE_STATIC_KEY_TRUE(kasan_debug);
+#endif
+
 /*
  * Exported functions for interfaces called from assembly or from generated
  * code. Declarations here to avoid warning about missing declarations.

mm/kasan/report.c +8 -2

index dee5350b459c..ae956a29ad4e 100644
@@ -159,8 +159,8 @@ static void describe_object_addr(struct kmem_cache *cache, void *object,
                (void *)(object_addr + cache->object_size));
 }
 
-static void describe_object(struct kmem_cache *cache, void *object,
-                               const void *addr, u8 tag)
+static void describe_object_stacks(struct kmem_cache *cache, void *object,
+                                       const void *addr, u8 tag)
 {
        struct kasan_alloc_meta *alloc_meta = kasan_get_alloc_meta(cache, object);
 

@@ -188,7 +188,13 @@ static void describe_object(struct kmem_cache *cache, void *object,
                }
 #endif
        }
+}
 
+static void describe_object(struct kmem_cache *cache, void *object,
+                               const void *addr, u8 tag)
+{
+       if (static_branch_unlikely(&kasan_debug))
+               describe_object_stacks(cache, object, addr, tag);
        describe_object_addr(cache, object, addr);
 }
 

mm/kasan/sw_tags.c +4 -1

index 099af6dc8f7e..50e797a16e17 100644
@@ -33,6 +33,10 @@
 #include "kasan.h"
 #include "../slab.h"
 
+/* See the comments in hw_tags.c */
+DEFINE_STATIC_KEY_TRUE_RO(kasan_enabled);
+DEFINE_STATIC_KEY_TRUE_RO(kasan_debug);
+
 static DEFINE_PER_CPU(u32, prng_state);
 
 void __init kasan_init_tags(void)
--

2.28.0.1011.ga647a8990f-goog

Quoted 1 line · Andrey Konovalov

mm/kasan/common.c size_t kasan_metadata_size(struct kmem_cache *cache) · line 185

Quoted 5 lines

mm/kasan/common.c +1 -0

@@ -180,6 +182,7 @@ size_t kasan_metadata_size(struct kmem_cache *cache)
 struct kasan_alloc_meta *kasan_get_alloc_meta(struct kmem_cache *cache,
                                              const void *object)
 {
+       WARN_ON(!static_branch_unlikely(&kasan_debug));

The WARN_ON condition itself should be unlikely, so that would imply that the static branch here should be likely since you're negating it.

And AFAIK, this function should only be called if kasan_debug is true.

mm/kasan/hw_tags.c line 38

Quoted 2 lines

mm/kasan/hw_tags.c +2 -0

+/* Whether to use syncronous or asynchronous tag checking. */
+static bool kasan_sync __ro_after_init;

s/syncronous/synchronous/

mm/kasan/hw_tags.c line 47

Quoted 9 lines

mm/kasan/hw_tags.c +8 -0

+static int __init early_kasan_mode(char *arg)
+{
+       if (!arg)
+               return -EINVAL;
+
+       if (strcmp(arg, "on") == 0)
+               kasan_mode = KASAN_MODE_ON;
+       else if (strcmp(arg, "debug") == 0)

s/strcmp(..) == 0/!strcmp(..)/ ?

mm/kasan/hw_tags.c void kasan_set_free_info(struct kmem_cache *cache, · line 114

Quoted 5 lines

mm/kasan/hw_tags.c +1 -0

@@ -60,6 +111,7 @@ void kasan_set_free_info(struct kmem_cache *cache,
 {
        struct kasan_alloc_meta *alloc_meta;
 
+       WARN_ON(!static_branch_unlikely(&kasan_debug));

What actually happens if any of these are called with !kasan_debug and the warning triggers? Is it still valid to execute the below, or should it bail out? Or possibly even disable KASAN entirely?

This patchset is not complete (see particular TODOs in the last patch), and I haven't performed any benchmarking yet, but I would like to start the discussion now and hear people's opinions regarding the questions mentioned below.

=== Overview

This patchset adopts the existing hardware tag-based KASAN mode [1] for use in production as a memory corruption mitigation. Hardware tag-based KASAN relies on arm64 Memory Tagging Extension (MTE) [2] to perform memory and pointer tagging. Please see [3] and [4] for detailed analysis of how MTE helps to fight memory safety problems.

The current plan is reuse CONFIG_KASAN_HW_TAGS for production, but add a boot time switch, that allows to choose between a debugging mode, that includes all KASAN features as they are, and a production mode, that only includes the essentials like tag checking.

It is essential that switching between these modes doesn't require rebuilding the kernel with different configs, as this is required by the Android GKI initiative [5].

The last patch of this series adds a new boot time parameter called kasan_mode, which can have the following values:

- "kasan_mode=on" - only production features
- "kasan_mode=debug" - all debug features
- "kasan_mode=off" - no checks at all (not implemented yet)

Currently outlined differences between "on" and "debug":

- "on" doesn't keep track of alloc/free stacks, and therefore doesn't require the additional memory to store those
- "on" uses asyncronous tag checking (not implemented yet)

=== Questions

The intention with this kind of a high level switch is to hide the implementation details. Arguably, we could add multiple switches that allow to separately control each KASAN or MTE feature, but I'm not sure there's much value in that.

Does this make sense? Any preference regarding the name of the parameter and its values?

What should be the default when the parameter is not specified? I would argue that it should be "debug" (for hardware that supports MTE, otherwise "off"), as it's the implied default for all other KASAN modes.

Should we somehow control whether to panic the kernel on a tag fault? Another boot time parameter perhaps?

Any ideas as to how properly estimate the slowdown? As there's no MTE-enabled hardware yet, the only way to test these patches is use an emulator (like QEMU). The delay that is added by the emulator (for setting and checking the tags) is different from the hardware delay, and this skews the results.

A question to KASAN maintainers: what would be the best way to support the "off" mode? I see two potential approaches: add a check into each kasan callback (easier to implement, but we still call kasan callbacks, even though they immediately return), or add inline header wrappers that do the same.

Quoted 42 lines · Andrey Konovalov

This patchset is not complete (see particular TODOs in the last patch), and I haven't performed any benchmarking yet, but I would like to start the discussion now and hear people's opinions regarding the questions mentioned below.

=== Overview

This patchset adopts the existing hardware tag-based KASAN mode [1] for use in production as a memory corruption mitigation. Hardware tag-based KASAN relies on arm64 Memory Tagging Extension (MTE) [2] to perform memory and pointer tagging. Please see [3] and [4] for detailed analysis of how MTE helps to fight memory safety problems.

The current plan is reuse CONFIG_KASAN_HW_TAGS for production, but add a boot time switch, that allows to choose between a debugging mode, that includes all KASAN features as they are, and a production mode, that only includes the essentials like tag checking.

It is essential that switching between these modes doesn't require rebuilding the kernel with different configs, as this is required by the Android GKI initiative [5].

The last patch of this series adds a new boot time parameter called kasan_mode, which can have the following values:

- "kasan_mode=on" - only production features
- "kasan_mode=debug" - all debug features
- "kasan_mode=off" - no checks at all (not implemented yet)

Currently outlined differences between "on" and "debug":

- "on" doesn't keep track of alloc/free stacks, and therefore doesn't require the additional memory to store those
- "on" uses asyncronous tag checking (not implemented yet)

=== Questions

The intention with this kind of a high level switch is to hide the implementation details. Arguably, we could add multiple switches that allow to separately control each KASAN or MTE feature, but I'm not sure there's much value in that.

Quoted 2 lines

Does this make sense? Any preference regarding the name of the parameter and its values?

KASAN itself used to be a debugging tool only. So introducing an "on" mode which no longer follows this convention may be confusing.

Instead, maybe the following might be less confusing:

"full" - current "debug", normal KASAN, all debugging help available. "opt" - current "on", optimized mode for production.

"on" - automatic selection => chooses "full" if CONFIG_DEBUG_KERNEL, "opt" otherwise.
"off" - as before.

Also, if there is no other kernel boot parameter named "kasan" yet, maybe it could just be "kasan=..." ?

Quoted 3 lines

What should be the default when the parameter is not specified? I would argue that it should be "debug" (for hardware that supports MTE, otherwise "off"), as it's the implied default for all other KASAN modes.

Perhaps we could make this dependent on CONFIG_DEBUG_KERNEL as above. I do not think that having the full/debug KASAN enabled on production kernels adds any value because for it to be useful requires somebody to actually look at the stacktraces; I think that choice should be made explicitly if it's a production kernel. My guess is that we'll save explaining performance differences and resulting headaches for ourselves and others that way.

Quoted 2 lines

Should we somehow control whether to panic the kernel on a tag fault? Another boot time parameter perhaps?

It already respects panic_on_warn, correct?

Quoted 10 lines

Quoted 7 lines

The intention with this kind of a high level switch is to hide the implementation details. Arguably, we could add multiple switches that allow to separately control each KASAN or MTE feature, but I'm not sure there's much value in that.

Does this make sense? Any preference regarding the name of the parameter and its values?

KASAN itself used to be a debugging tool only. So introducing an "on" mode which no longer follows this convention may be confusing.

Yeah, perhaps "on" is not the best name here.

Quoted 4 lines

Instead, maybe the following might be less confusing:

"full" - current "debug", normal KASAN, all debugging help available. "opt" - current "on", optimized mode for production.

How about "prod" here?

Quoted 4 lines

Quoted 3 lines

Should we somehow control whether to panic the kernel on a tag fault? Another boot time parameter perhaps?

Quoted 1 line

It already respects panic_on_warn, correct?

Yes, but Android is unlikely to enable panic_on_warn as they have warnings happening all over. AFAIR Pixel 3/4 kernels actually have a custom patch that enables kernel panic for KASAN crashes specifically (even though they don't obviously use KASAN in production), and I think it's better to provide a similar facility upstream. Maybe call it panic_on_kasan or something?

On Fri, 16 Oct 2020 at 15:17, 'Andrey Konovalov' via kasan-dev <[email protected]> wrote:
[...]

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

The intention with this kind of a high level switch is to hide the implementation details. Arguably, we could add multiple switches that allow to separately control each KASAN or MTE feature, but I'm not sure there's much value in that.

Does this make sense? Any preference regarding the name of the parameter and its values?

KASAN itself used to be a debugging tool only. So introducing an "on" mode which no longer follows this convention may be confusing.

Quoted 8 lines

Yeah, perhaps "on" is not the best name here.

Quoted 4 lines

Instead, maybe the following might be less confusing:

"full" - current "debug", normal KASAN, all debugging help available. "opt" - current "on", optimized mode for production.

How about "prod" here?

SGTM.

[...]

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Should we somehow control whether to panic the kernel on a tag fault? Another boot time parameter perhaps?

Quoted 8 lines

Quoted 1 line

It already respects panic_on_warn, correct?

Yes, but Android is unlikely to enable panic_on_warn as they have warnings happening all over. AFAIR Pixel 3/4 kernels actually have a custom patch that enables kernel panic for KASAN crashes specifically (even though they don't obviously use KASAN in production), and I think it's better to provide a similar facility upstream. Maybe call it panic_on_kasan or something?

Best would be if kasan= can take another option, e.g. "kasan=prod,panic". I think you can change the strcmp() to a str_has_prefix() for the checks for full/prod/on/off, and then check if what comes after it is ",panic".

Thanks,
-- Marco

Quoted 40 lines · Marco Elver

On Fri, 16 Oct 2020 at 15:17, 'Andrey Konovalov' via kasan-dev <[email protected]> wrote:
[...]

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

The intention with this kind of a high level switch is to hide the implementation details. Arguably, we could add multiple switches that allow to separately control each KASAN or MTE feature, but I'm not sure there's much value in that.

Does this make sense? Any preference regarding the name of the parameter and its values?

KASAN itself used to be a debugging tool only. So introducing an "on" mode which no longer follows this convention may be confusing.

Quoted 8 lines

Yeah, perhaps "on" is not the best name here.

Quoted 4 lines

Instead, maybe the following might be less confusing:

"full" - current "debug", normal KASAN, all debugging help available. "opt" - current "on", optimized mode for production.

How about "prod" here?

SGTM.

[...]

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Should we somehow control whether to panic the kernel on a tag fault? Another boot time parameter perhaps?

Quoted 8 lines

Quoted 1 line

It already respects panic_on_warn, correct?

Yes, but Android is unlikely to enable panic_on_warn as they have warnings happening all over. AFAIR Pixel 3/4 kernels actually have a custom patch that enables kernel panic for KASAN crashes specifically (even though they don't obviously use KASAN in production), and I think it's better to provide a similar facility upstream. Maybe call it panic_on_kasan or something?

Quoted 7 lines

Best would be if kasan= can take another option, e.g. "kasan=prod,panic". I think you can change the strcmp() to a str_has_prefix() for the checks for full/prod/on/off, and then check if what comes after it is ",panic".

Thanks,
-- Marco

CC Kostya and Serban.

Hi,
I would like to hear opinions from others in CC on these choices:
* Production use of In-kernel MTE should be based on stripped-down KASAN, or implemented independently?

* Should we aim at a single boot-time flag (with several values) or for several independent flags (OFF/SYNC/ASYNC, Stack traces on/off)

Andrey, please give us some idea of the CPU and RAM overheads other than those coming from MTE
* stack trace collection and storage
* adding redzones to every allocation - not strictly needed for MTE, but convenient to store the stack trace IDs.

Andrey: with production MTE we should not be using quarantine, which means storing the stack trace IDs
in the deallocated memory doesn't provide good report quality. We may need to consider another approach, e.g. the one used in HWASAN (separate ring buffer, per thread or per core)

--kcc

Quoted 51 lines · Andrey Konovalov

Quoted 40 lines · Marco Elver

On Fri, 16 Oct 2020 at 15:17, 'Andrey Konovalov' via kasan-dev <[email protected]> wrote:
[...]

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

The intention with this kind of a high level switch is to hide the implementation details. Arguably, we could add multiple switches that allow to separately control each KASAN or MTE feature, but I'm not sure there's much value in that.

Does this make sense? Any preference regarding the name of the parameter and its values?

KASAN itself used to be a debugging tool only. So introducing an "on" mode which no longer follows this convention may be confusing.

Quoted 8 lines

Yeah, perhaps "on" is not the best name here.

Quoted 4 lines

Instead, maybe the following might be less confusing:

"full" - current "debug", normal KASAN, all debugging help available. "opt" - current "on", optimized mode for production.

How about "prod" here?

SGTM.

[...]

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Should we somehow control whether to panic the kernel on a tag fault? Another boot time parameter perhaps?

Quoted 8 lines

Quoted 1 line

It already respects panic_on_warn, correct?

Yes, but Android is unlikely to enable panic_on_warn as they have warnings happening all over. AFAIR Pixel 3/4 kernels actually have a custom patch that enables kernel panic for KASAN crashes specifically (even though they don't obviously use KASAN in production), and I think it's better to provide a similar facility upstream. Maybe call it panic_on_kasan or something?

Quoted 7 lines

Best would be if kasan= can take another option, e.g. "kasan=prod,panic". I think you can change the strcmp() to a str_has_prefix() for the checks for full/prod/on/off, and then check if what comes after it is ",panic".

Thanks,
-- Marco

CC Kostya and Serban.

Quoted 5 lines · Kostya Serebryany

Hi,
I would like to hear opinions from others in CC on these choices:
* Production use of In-kernel MTE should be based on stripped-down KASAN, or implemented independently?

Andrey, what are the fundamental consequences of basing MTE on KASAN? I would assume that there are none as we can change KASAN code and special case some code paths as necessary.

Quoted 2 lines

* Should we aim at a single boot-time flag (with several values) or for several independent flags (OFF/SYNC/ASYNC, Stack traces on/off)

We won't be able to answer this question for several years until we have actual hardware/users...
It's definitely safer to aim at multiple options. I would reuse the fs opt parsing code as we seem to have lots of potential things to configure so that we can do:
kasan_options=quarantine=off,fault=panic,trap=async

I am also always confused by the term "debug" when configuring the kernel. In some cases it's for debugging of the subsystem (for developers of KASAN), in some cases it adds additional checks to catch misuses of the subsystem. in some - it just adds more debugging output on console. And in this case it's actually neither of these. But I am not sure what's a better name ("full"?). Even if we split options into multiple, we still can have some kind of presents that just flip all other options into reasonable values.

Quoted 7 lines

Andrey, please give us some idea of the CPU and RAM overheads other than those coming from MTE
* stack trace collection and storage
* adding redzones to every allocation - not strictly needed for MTE, but convenient to store the stack trace IDs.

Quoted 5 lines

Andrey: with production MTE we should not be using quarantine, which means storing the stack trace IDs
in the deallocated memory doesn't provide good report quality. We may need to consider another approach, e.g. the one used in HWASAN (separate ring buffer, per thread or per core)

Quoted 37 lines · Andrey Konovalov

This patchset is not complete (see particular TODOs in the last patch), and I haven't performed any benchmarking yet, but I would like to start the discussion now and hear people's opinions regarding the questions mentioned below.

=== Overview

This patchset adopts the existing hardware tag-based KASAN mode [1] for use in production as a memory corruption mitigation. Hardware tag-based KASAN relies on arm64 Memory Tagging Extension (MTE) [2] to perform memory and pointer tagging. Please see [3] and [4] for detailed analysis of how MTE helps to fight memory safety problems.

The current plan is reuse CONFIG_KASAN_HW_TAGS for production, but add a boot time switch, that allows to choose between a debugging mode, that includes all KASAN features as they are, and a production mode, that only includes the essentials like tag checking.

It is essential that switching between these modes doesn't require rebuilding the kernel with different configs, as this is required by the Android GKI initiative [5].

The last patch of this series adds a new boot time parameter called kasan_mode, which can have the following values:

- "kasan_mode=on" - only production features
- "kasan_mode=debug" - all debug features
- "kasan_mode=off" - no checks at all (not implemented yet)

Currently outlined differences between "on" and "debug":

- "on" doesn't keep track of alloc/free stacks, and therefore doesn't require the additional memory to store those
- "on" uses asyncronous tag checking (not implemented yet)

=== Questions

Quoted 7 lines

The intention with this kind of a high level switch is to hide the implementation details. Arguably, we could add multiple switches that allow to separately control each KASAN or MTE feature, but I'm not sure there's much value in that.

Does this make sense? Any preference regarding the name of the parameter and its values?

KASAN itself used to be a debugging tool only. So introducing an "on" mode which no longer follows this convention may be confusing. Instead, maybe the following might be less confusing:

"full" - current "debug", normal KASAN, all debugging help available. "opt" - current "on", optimized mode for production.
"on" - automatic selection => chooses "full" if CONFIG_DEBUG_KERNEL, "opt" otherwise.
"off" - as before.

Also, if there is no other kernel boot parameter named "kasan" yet, maybe it could just be "kasan=..." ?

Quoted 3 lines

What should be the default when the parameter is not specified? I would argue that it should be "debug" (for hardware that supports MTE, otherwise "off"), as it's the implied default for all other KASAN modes.

Perhaps we could make this dependent on CONFIG_DEBUG_KERNEL as above. I do not think that having the full/debug KASAN enabled on production kernels adds any value because for it to be useful requires somebody to actually look at the stacktraces; I think that choice should be made explicitly if it's a production kernel. My guess is that we'll save explaining performance differences and resulting headaches for ourselves and others that way.

Quoted 2 lines

Should we somehow control whether to panic the kernel on a tag fault? Another boot time parameter perhaps?

It already respects panic_on_warn, correct?

Quoted 6 lines

Any ideas as to how properly estimate the slowdown? As there's no MTE-enabled hardware yet, the only way to test these patches is use an emulator (like QEMU). The delay that is added by the emulator (for setting and checking the tags) is different from the hardware delay, and this skews the results.

Quoted 5 lines

A question to KASAN maintainers: what would be the best way to support the "off" mode? I see two potential approaches: add a check into each kasan callback (easier to implement, but we still call kasan callbacks, even though they immediately return), or add inline header wrappers that do the same.

[...]

This patchset is not complete (see particular TODOs in the last patch), and I haven't performed any benchmarking yet, but I would like to start the discussion now and hear people's opinions regarding the questions mentioned below.

=== Overview

This patchset adopts the existing hardware tag-based KASAN mode [1] for use in production as a memory corruption mitigation. Hardware tag-based KASAN relies on arm64 Memory Tagging Extension (MTE) [2] to perform memory and pointer tagging. Please see [3] and [4] for detailed analysis of how MTE helps to fight memory safety problems.

The current plan is reuse CONFIG_KASAN_HW_TAGS for production, but add a boot time switch, that allows to choose between a debugging mode, that includes all KASAN features as they are, and a production mode, that only includes the essentials like tag checking.

It is essential that switching between these modes doesn't require rebuilding the kernel with different configs, as this is required by the Android GKI initiative [5].

The last patch of this series adds a new boot time parameter called kasan_mode, which can have the following values:

- "kasan_mode=on" - only production features
- "kasan_mode=debug" - all debug features
- "kasan_mode=off" - no checks at all (not implemented yet)

Currently outlined differences between "on" and "debug":

- "on" doesn't keep track of alloc/free stacks, and therefore doesn't require the additional memory to store those
- "on" uses asyncronous tag checking (not implemented yet)

=== Questions

The intention with this kind of a high level switch is to hide the implementation details. Arguably, we could add multiple switches that allow to separately control each KASAN or MTE feature, but I'm not sure there's much value in that.

Does this make sense? Any preference regarding the name of the parameter and its values?

What should be the default when the parameter is not specified? I would argue that it should be "debug" (for hardware that supports MTE, otherwise "off"), as it's the implied default for all other KASAN modes.

Should we somehow control whether to panic the kernel on a tag fault? Another boot time parameter perhaps?

Any ideas as to how properly estimate the slowdown? As there's no MTE-enabled hardware yet, the only way to test these patches is use an emulator (like QEMU). The delay that is added by the emulator (for setting and checking the tags) is different from the hardware delay, and this skews the results.

A question to KASAN maintainers: what would be the best way to support the "off" mode? I see two potential approaches: add a check into each kasan callback (easier to implement, but we still call kasan callbacks, even though they immediately return), or add inline header wrappers that do the same.

=== Notes

This patchset is available here:

https://github.com/xairy/linux/tree/up-prod-mte-rfc1

and on Gerrit here:

https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/3460

This patchset is based on v5 of "kasan: add hardware tag-based mode for arm64" patchset [1].

For testing in QEMU hardware tag-based KASAN requires:

1. QEMU built from master [6] (use "-machine virt,mte=on -cpu max" arguments to run).
2. GCC version 10.

[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
[2] https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/enhancing-memory-safety
[3] https://arxiv.org/pdf/1802.09517.pdf
[4] https://github.com/microsoft/MSRC-Security-Research/blob/master/papers/2020/Security%20analysis%20of%20memory%20tagging.pdf
[5] https://source.android.com/devices/architecture/kernel/generic-kernel-image
[6] https://github.com/qemu/qemu

Andrey Konovalov (8):
kasan: simplify quarantine_put call
kasan: rename get_alloc/free_info
kasan: introduce set_alloc_info
kasan: unpoison stack only with CONFIG_KASAN_STACK
kasan: mark kasan_init_tags as __init kasan, arm64: move initialization message
arm64: kasan: Add system_supports_tags helper
kasan: add and integrate kasan_mode boot param

 arch/arm64/include/asm/memory.h  |  1 +
 arch/arm64/kernel/sleep.S        |  2 +-
 arch/arm64/mm/kasan_init.c       |  3 ++
 arch/x86/kernel/acpi/wakeup_64.S |  2 +-
 include/linux/kasan.h            | 14 ++---
 mm/kasan/common.c                | 90 ++++++++++++++++++--------------
 mm/kasan/generic.c               | 18 ++++---
 mm/kasan/hw_tags.c               | 63 ++++++++++++++++++++--
 mm/kasan/kasan.h                 | 25 ++++++---
 mm/kasan/quarantine.c            |  5 +-
 mm/kasan/report.c                | 22 +++++---
 mm/kasan/report_sw_tags.c        |  2 +-
 mm/kasan/sw_tags.c               | 14 +++--
 13 files changed, 182 insertions(+), 79 deletions(-)

--
2.28.0.1011.ga647a8990f-goog

A question to KASAN maintainers: what would be the best way to support the "off" mode? I see two potential approaches: add a check into each kasan callback (easier to implement, but we still call kasan callbacks, even though they immediately return), or add inline header wrappers that do the same.

On Wed, 14 Oct 2020 at 22:44, Andrey Konovalov <[email protected]> wrote:
[...]

Quoted 5 lines

A question to KASAN maintainers: what would be the best way to support the "off" mode? I see two potential approaches: add a check into each kasan callback (easier to implement, but we still call kasan callbacks, even though they immediately return), or add inline header wrappers that do the same.

This is tricky, because we don't know how bad the performance will be if we keep them as calls. We'd have to understand the performance impact of keeping them as calls, and if the performance impact is acceptable or not.

Without understanding the performance impact, the only viable option I see is to add __always_inline kasan_foo() wrappers, which use the static branch to guard calls to __kasan_foo().