khwasan: kernel hardware assisted address sanitizer

A KHWASAN shadow memory cell contains a memory tag, that corresponds to the tag in the top byte of the pointer, that points to that memory. The native top byte value of kernel pointers is 0xff, so with KHWASAN we need to initialize shadow memory to 0xff. This commit does that.

---
 arch/arm64/mm/kasan_init.c | 11 ++++++++++-
 include/linux/kasan.h      |  8 ++++++++
 mm/kasan/common.c          |  7 +++++++
 3 files changed, 25 insertions(+), 1 deletion(-)

arch/arm64/mm/kasan_init.c +10 -1

index dabfc1ecda3d..d4bceba60010 100644
@@ -90,6 +90,10 @@ static void __init kasan_pte_populate(pmd_t *pmdp, unsigned long addr,
        do {
                phys_addr_t page_phys = early ? __pa_symbol(kasan_zero_page)
                                              : kasan_alloc_zeroed_page(node);
+#if KASAN_SHADOW_INIT != 0
+               if (!early)
+                       memset(__va(page_phys), KASAN_SHADOW_INIT, PAGE_SIZE);
+#endif
                next = addr + PAGE_SIZE;
                set_pte(ptep, pfn_pte(__phys_to_pfn(page_phys), PAGE_KERNEL));
        } while (ptep++, addr = next, addr != end && pte_none(READ_ONCE(*ptep)));

@@ -139,6 +143,11 @@ asmlinkage void __init kasan_early_init(void)
                KASAN_SHADOW_END - (1UL << (64 - KASAN_SHADOW_SCALE_SHIFT)));
        BUILD_BUG_ON(!IS_ALIGNED(KASAN_SHADOW_START, PGDIR_SIZE));
        BUILD_BUG_ON(!IS_ALIGNED(KASAN_SHADOW_END, PGDIR_SIZE));
+
+#if KASAN_SHADOW_INIT != 0
+       memset(kasan_zero_page, KASAN_SHADOW_INIT, PAGE_SIZE);
+#endif
+
        kasan_pgd_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, NUMA_NO_NODE,
                           true);
 }

@@ -235,7 +244,7 @@ void __init kasan_init(void)
                set_pte(&kasan_zero_pte[i],
                        pfn_pte(sym_to_pfn(kasan_zero_page), PAGE_KERNEL_RO));
 
-       memset(kasan_zero_page, 0, PAGE_SIZE);
+       memset(kasan_zero_page, KASAN_SHADOW_INIT, PAGE_SIZE);
        cpu_replace_ttbr1(lm_alias(swapper_pg_dir));
 
        /* At this point kasan is fully initialized. Enable error messages */

include/linux/kasan.h +8 -0

index 3c45e273a936..c34f413b0eac 100644
@@ -139,6 +139,8 @@ static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
 
 #ifdef CONFIG_KASAN_CLASSIC
 
+#define KASAN_SHADOW_INIT 0
+
 void kasan_cache_shrink(struct kmem_cache *cache);
 void kasan_cache_shutdown(struct kmem_cache *cache);
 

@@ -149,4 +151,10 @@ static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
 
 #endif /* CONFIG_KASAN_CLASSIC */
 
+#ifdef CONFIG_KASAN_TAGS
+
+#define KASAN_SHADOW_INIT 0xff
+
+#endif /* CONFIG_KASAN_TAGS */
+
 #endif /* LINUX_KASAN_H */

mm/kasan/common.c +7 -1

index 08f6c8cb9f84..f4ccb9425655 100644
@@ -253,6 +253,9 @@ int kasan_module_alloc(void *addr, size_t size)
                        __builtin_return_address(0));
 
        if (ret) {
+#if KASAN_SHADOW_INIT != 0
+               __memset(ret, KASAN_SHADOW_INIT, shadow_size);
+#endif
                find_vm_area(addr)->flags |= VM_KASAN;
                kmemleak_ignore(ret);
                return 0;

@@ -297,6 +300,10 @@ static int __meminit kasan_mem_notifier(struct notifier_block *nb,
                if (!ret)
                        return NOTIFY_BAD;
 
+#if KASAN_SHADOW_INIT != 0
+               __memset(ret, KASAN_SHADOW_INIT, shadow_end - shadow_start);
+#endif
+
                kmemleak_ignore(ret);
                return NOTIFY_OK;
        }
--

2.16.2.395.g2e18187dfd-goog

arch/arm64/mm/proc.S line 48

Quoted 5 lines · Andrey Konovalov

arch/arm64/mm/proc.S +5 -0

+#ifdef CONFIG_KASAN_TAGS
+#define TCR_TBI_FLAGS (TCR_TBI0 | TCR_TBI1)
+#else
+#define TCR_TBI_FLAGS TCR_TBI0
+#endif

Rather than pulling TBI0 into this, I think it'd make more sense to have:

#ifdef CONFIG_KASAN_TAGS
#define KASAN_TCR_FLAGS TCR_TBI1
#else
#define KASAN_TCR_FLAGS
#endif

arch/arm64/mm/proc.S ENTRY(__cpu_setup) · line 441

Quoted 10 lines

arch/arm64/mm/proc.S +2 -1

+
 #define MAIR(attr, mt)       ((attr) << ((mt) * 8))
 
 /*

@@ -432,7 +438,7 @@ ENTRY(__cpu_setup)
       * both user and kernel.
       */
      ldr     x10, =TCR_TxSZ(VA_BITS) | TCR_CACHE_FLAGS | TCR_SMP_FLAGS | \
-                     TCR_TG_FLAGS | TCR_ASID16 | TCR_TBI0 | TCR_A1
+                     TCR_TG_FLAGS | TCR_ASID16 | TCR_TBI_FLAGS | TCR_A1

... and just append KASAN_TCR_FLAGS to the flags here.

That's roughtly what we do with ENDIAN_SET_EL1 for SCTLR_EL1.

KHWASAN uses the Top Byte Ignore feature of arm64 CPUs to store a pointer tag in the top byte of each pointer. This commit enables the TCR_TBI1 bit, which enables Top Byte Ignore for the kernel, when KHWASAN is used.

---
 arch/arm64/include/asm/pgtable-hwdef.h | 1 +
 arch/arm64/mm/proc.S                   | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Quoted 7 lines · Andrey Konovalov

KHWASAN uses the Top Byte Ignore feature of arm64 CPUs to store a pointer tag in the top byte of each pointer. This commit enables the TCR_TBI1 bit, which enables Top Byte Ignore for the kernel, when KHWASAN is used.

---
 arch/arm64/include/asm/pgtable-hwdef.h | 1 +
 arch/arm64/mm/proc.S                   | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Before it's safe to do this, I also think you'll need to fix up at least:

* virt_to_phys()

* access_ok()

... and potentially others which assume that bits [63:56] of kernel addresses are 0xff. For example, bits of the fault handling logic might need fixups.

Quoted 16 lines · Mark Rutland

Quoted 7 lines · Andrey Konovalov

KHWASAN uses the Top Byte Ignore feature of arm64 CPUs to store a pointer tag in the top byte of each pointer. This commit enables the TCR_TBI1 bit, which enables Top Byte Ignore for the kernel, when KHWASAN is used.

---
 arch/arm64/include/asm/pgtable-hwdef.h | 1 +
 arch/arm64/mm/proc.S                   | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Before it's safe to do this, I also think you'll need to fix up at least:

* virt_to_phys()

* access_ok()

Quoted 3 lines

... and potentially others which assume that bits [63:56] of kernel addresses are 0xff. For example, bits of the fault handling logic might need fixups.

Indeed. I have the ugly feeling that KVM (and anything that leaves in a separate address space) will not be very happy with that change, as it derives HYP VAs from the kernel VA, and doesn't expect lingering bits. Nothing that cannot be addressed, but worth keeping in mind.

Quoted 17 lines · Marc Zyngier

Quoted 16 lines · Mark Rutland

Quoted 7 lines · Andrey Konovalov

KHWASAN uses the Top Byte Ignore feature of arm64 CPUs to store a pointer tag in the top byte of each pointer. This commit enables the TCR_TBI1 bit, which enables Top Byte Ignore for the kernel, when KHWASAN is used.

---
 arch/arm64/include/asm/pgtable-hwdef.h | 1 +
 arch/arm64/mm/proc.S                   | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Before it's safe to do this, I also think you'll need to fix up at least:

* virt_to_phys()

* access_ok()

Quoted 9 lines

Quoted 3 lines

... and potentially others which assume that bits [63:56] of kernel addresses are 0xff. For example, bits of the fault handling logic might need fixups.

Indeed. I have the ugly feeling that KVM (and anything that leaves in a separate address space) will not be very happy with that change, as it derives HYP VAs from the kernel VA, and doesn't expect lingering bits. Nothing that cannot be addressed, but worth keeping in mind.

Hi Marc!

Yes, I would expect there would be issues with KVM. I'll see if I can figure them out, but I think I'll just add a depends on !KVM or something like this, and will have to deal with KVM once the main part is committed.

Well, that's not quite how it works. KVM is an integral part of the kernel, and I don't really want to have to deal with regression (not to mention that KVM is an essential tool in our testing infrastructure).

You could try and exclude KVM from the instrumentation (which we already have for invasive things such as KASAN), but I'm afraid that having a debugging option that conflicts with another essential part of the kernel is not an option.

I'm happy to help you with that though.

Quoted 11 lines · Marc Zyngier

Well, that's not quite how it works. KVM is an integral part of the kernel, and I don't really want to have to deal with regression (not to mention that KVM is an essential tool in our testing infrastructure).

You could try and exclude KVM from the instrumentation (which we already have for invasive things such as KASAN), but I'm afraid that having a debugging option that conflicts with another essential part of the kernel is not an option.

I'm happy to help you with that though.

Hm, KHWASAN instruments the very same parts of the kernel that KASAN does (it reuses the same flag). I've checked, I actually have CONFIG_KVM enabled in my test build, however I haven't tried to test KVM yet. I'm planning to perform extensive fuzzing of the kernel with syzkaller, so if there's any crashes caused by KHWASAN in kvm code I'll see them. However if some bugs don't manifest as crashes, that would be a difficult thing to detect for me.

Quoted 13 lines · Andrey Konovalov

Quoted 11 lines · Marc Zyngier

Well, that's not quite how it works. KVM is an integral part of the kernel, and I don't really want to have to deal with regression (not to mention that KVM is an essential tool in our testing infrastructure).

You could try and exclude KVM from the instrumentation (which we already have for invasive things such as KASAN), but I'm afraid that having a debugging option that conflicts with another essential part of the kernel is not an option.

I'm happy to help you with that though.

Quoted 7 lines

Hm, KHWASAN instruments the very same parts of the kernel that KASAN does (it reuses the same flag). I've checked, I actually have CONFIG_KVM enabled in my test build, however I haven't tried to test KVM yet. I'm planning to perform extensive fuzzing of the kernel with syzkaller, so if there's any crashes caused by KHWASAN in kvm code I'll see them. However if some bugs don't manifest as crashes, that would be a difficult thing to detect for me.

Well, if something is wrong in KVM, it usually manifests itself extremely quickly, and takes the whole box with it. I have the ugly feeling that feeding coloured pointers to KVM is going to be a fun ride though.

Also, last time I checked Clang couldn't even compile KVM correctly. Hopefully, things have changed...

Quoted 4 lines · Marc Zyngier

Well, that's not quite how it works. KVM is an integral part of the kernel, and I don't really want to have to deal with regression (not to mention that KVM is an essential tool in our testing infrastructure).

Quoted 4 lines

You could try and exclude KVM from the instrumentation (which we already have for invasive things such as KASAN), but I'm afraid that having a debugging option that conflicts with another essential part of the kernel is not an option.

Hm, KHWASAN instruments the very same parts of the kernel that KASAN does (it reuses the same flag).

Quoted 7 lines · Andrey Konovalov

KHWASAN uses the Top Byte Ignore feature of arm64 CPUs to store a pointer tag in the top byte of each pointer. This commit enables the TCR_TBI1 bit, which enables Top Byte Ignore for the kernel, when KHWASAN is used.

---
 arch/arm64/include/asm/pgtable-hwdef.h | 1 +
 arch/arm64/mm/proc.S                   | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Before it's safe to do this, I also think you'll need to fix up at least:

* virt_to_phys()

* access_ok()

... and potentially others which assume that bits [63:56] of kernel addresses are 0xff. For example, bits of the fault handling logic might need fixups.

Thanks,
Mark.

Quoted 9 lines · Mark Rutland

Quoted 7 lines · Andrey Konovalov

KHWASAN uses the Top Byte Ignore feature of arm64 CPUs to store a pointer tag in the top byte of each pointer. This commit enables the TCR_TBI1 bit, which enables Top Byte Ignore for the kernel, when KHWASAN is used.

---
 arch/arm64/include/asm/pgtable-hwdef.h | 1 +
 arch/arm64/mm/proc.S                   | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Quoted 4 lines

Before it's safe to do this, I also think you'll need to fix up at least:

* access_ok()

This is used for accessing user addresses, and they are not tagged. Am I missing something?

mm/kasan/khwasan.c +11 -0

+static DEFINE_PER_CPU(u32, prng_state);
+
+void khwasan_init(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		per_cpu(prng_state, cpu) = get_random_u32();
+	}
+	WRITE_ONCE(khwasan_enabled, 1);
+}

Quoted 17 lines · Andrey Konovalov

mm/kasan/khwasan.c +17 -0

+static DEFINE_PER_CPU(u32, prng_state);
+
+void khwasan_init(void)
+{
+     int cpu;
+
+     for_each_possible_cpu(cpu) {
+             per_cpu(prng_state, cpu) = get_random_u32();
+     }
+     WRITE_ONCE(khwasan_enabled, 1);
+}
+
+static inline u8 khwasan_random_tag(void)
+{
+     u32 state = this_cpu_read(prng_state);
+
+     state = 1664525 * state + 1013904223;

mm/kasan/khwasan.c line 64

Quoted 4 lines

mm/kasan/khwasan.c +4 -0

+     this_cpu_write(prng_state, state);
+
+     return (u8)state;
+}

Have you considered preemption here? Is the assumption that it happens sufficiently rarely that cross-contaminating the prng state isn't a problem?

Quoted 8 lines

mm/kasan/khwasan.c line 61

Quoted 4 lines

mm/kasan/khwasan.c +4 -0

+     u32 state = this_cpu_read(prng_state);
+
+     state = 1664525 * state + 1013904223;
+     this_cpu_write(prng_state, state);

Have you considered preemption here? Is the assumption that it happens sufficiently rarely that cross-contaminating the prng state isn't a problem?

Hi Mark!

Yes, I have. If a preemption happens between this_cpu_read and this_cpu_write, the only side effect is that we'll give a few allocated in different contexts objects the same tag. Sine KHWASAN is meant to be used a probabilistic bug-detection debug feature, this doesn't seem to have serious negative impact.

I'll add a comment about this though.

Quoted 17 lines · Andrey Konovalov

Quoted 8 lines

mm/kasan/khwasan.c line 61

Quoted 4 lines

mm/kasan/khwasan.c +4 -0

+     u32 state = this_cpu_read(prng_state);
+
+     state = 1664525 * state + 1013904223;
+     this_cpu_write(prng_state, state);

Have you considered preemption here? Is the assumption that it happens sufficiently rarely that cross-contaminating the prng state isn't a problem?

Hi Mark!

Yes, I have. If a preemption happens between this_cpu_read and this_cpu_write, the only side effect is that we'll give a few allocated in different contexts objects the same tag. Sine KHWASAN is meant to be used a probabilistic bug-detection debug feature, this doesn't seem to have serious negative impact.

Quoted 1 line

I'll add a comment about this though.

You could use this_cpu_cmpxchg here to make it a bit better but it probably does not matter.

Quoted 18 lines · Mark Rutland

Quoted 17 lines · Andrey Konovalov

mm/kasan/khwasan.c +17 -0

+static DEFINE_PER_CPU(u32, prng_state);
+
+void khwasan_init(void)
+{
+     int cpu;
+
+     for_each_possible_cpu(cpu) {
+             per_cpu(prng_state, cpu) = get_random_u32();
+     }
+     WRITE_ONCE(khwasan_enabled, 1);
+}
+
+static inline u8 khwasan_random_tag(void)
+{
+     u32 state = this_cpu_read(prng_state);
+
+     state = 1664525 * state + 1013904223;

Quoted 8 lines

mm/kasan/khwasan.c line 64

Quoted 4 lines

mm/kasan/khwasan.c +4 -0

+     this_cpu_write(prng_state, state);
+
+     return (u8)state;
+}

Have you considered preemption here? Is the assumption that it happens sufficiently rarely that cross-contaminating the prng state isn't a problem?

Hi Mark!

Yes, I have. If a preemption happens between this_cpu_read and this_cpu_write, the only side effect is that we'll give a few allocated in different contexts objects the same tag. Sine KHWASAN is meant to be used a probabilistic bug-detection debug feature, this doesn't seem to have serious negative impact.

I'll add a comment about this though.

The krealloc function checks where the same buffer was reused or a new one allocated by comparing kernel pointers. KHWASAN changes memory tag on the krealloc'ed chunk of memory and therefore also changes the pointer tag of the returned pointer. Therefore we need to perform comparison on untagged (with tags reset) pointers to check whether it's the same memory region or not.

---
 mm/slab_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/slab_common.c +0 -0

index a33e61315ca6..7c829cbda1a5 100644

mm/slab_common.c void *krealloc(const void *p, size_t new_size, gfp_t flags) · line 1497

Quoted 3 lines · Andrey Konovalov

      ret = __do_krealloc(p, new_size, flags);

mm/slab_common.c +1 -1

-     if (ret && p != ret)
+     if (ret && khwasan_reset_tag((void *)p) != khwasan_reset_tag(ret))

Why doesn't khwasan_reset_tag() take a const void *, like khwasan_set_tag() does? That way, this cast wouldn't be necessary.

mm/kasan/khwasan.c void *khwasan_reset_tag(void *addr) · line 108

Quoted 8 lines · Andrey Konovalov

void check_memory_region(unsigned long addr, size_t size, bool write,
unsigned long ret_ip)
{

mm/kasan/khwasan.c +5 -0

+     u8 tag;
+     u8 *shadow_first, *shadow_last, *shadow;
+     void *untagged_addr;
+
+     tag = get_tag((void *)addr);

Please make get_tag() take a const void *, then this cast can go.

Quoted 2 lines

+ untagged_addr = reset_tag((void *)addr);

Likewise for reset_tag().

mm/kasan/khwasan.c void *khwasan_reset_tag(void *addr) · line 111

Quoted 3 lines

mm/kasan/khwasan.c +2 -0

+     shadow_first = (u8 *)kasan_mem_to_shadow(untagged_addr);
+     shadow_last = (u8 *)kasan_mem_to_shadow(untagged_addr + size - 1);

I don't think these u8 * casts are necessary, since kasan_mem_to_shadow() returns a void *.

mm/kasan/khwasan.c void *khwasan_reset_tag(void *addr) · line 116

Quoted 6 lines

mm/kasan/khwasan.c +5 -0

+
+     for (shadow = shadow_first; shadow <= shadow_last; shadow++) {
+             if (*shadow != tag) {
+                     /* Report invalid-access bug here */
+                     return;

Huh? Should that be a TODO?

Thanks,
Mark.

Quoted 7 lines · Mark Rutland

Quoted 5 lines · Andrey Konovalov

mm/kasan/khwasan.c +5 -0

+
+     for (shadow = shadow_first; shadow <= shadow_last; shadow++) {
+             if (*shadow != tag) {
+                     /* Report invalid-access bug here */
+                     return;

Quoted 1 line

Huh? Should that be a TODO?

This is fixed in one of the next commits. I decided to split the main runtime logic and the reporting parts, so this comment is a placeholder, which is replaced with the proper error reporting function call later in the patch series. I can make it a /* TODO:
comment */, if you think that looks better.

Quoted 8 lines · Andrey Konovalov

Quoted 7 lines · Mark Rutland

Quoted 5 lines · Andrey Konovalov

mm/kasan/khwasan.c +5 -0

+
+     for (shadow = shadow_first; shadow <= shadow_last; shadow++) {
+             if (*shadow != tag) {
+                     /* Report invalid-access bug here */
+                     return;

Quoted 7 lines

Quoted 1 line

Huh? Should that be a TODO?

This is fixed in one of the next commits. I decided to split the main runtime logic and the reporting parts, so this comment is a placeholder, which is replaced with the proper error reporting function call later in the patch series. I can make it a /* TODO:
comment */, if you think that looks better.

It might be preferable to introdcue the report functions first (i.e. swap this patch with the next one).

Those will be unused, but since they're not static, you shouldn't get any build warnings. Then the hooks can call the report functions as soon as they're introduced.

This commit adds KHWASAN hooks implementation.

1. When a new slab cache is created, KHWASAN rounds up the size of the objects in this cache to KASAN_SHADOW_SCALE_SIZE (== 16).

2. On each kmalloc KHWASAN generates a random tag, sets the shadow memory, that corresponds to this object to this tag, and embeds this tag value into the top byte of the returned pointer.

3. On each kfree KHWASAN poisons the shadow memory with a random tag to allow detection of use-after-free bugs.

The rest of the logic of the hook implementation is very much similar to the one provided by KASAN. KHWASAN saves allocation and free stack metadata to the slab object the same was KASAN does this.

---
 mm/kasan/khwasan.c | 178 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 175 insertions(+), 3 deletions(-)

mm/kasan/khwasan.c +134 -3

index 21a2221e3368..09d6f0a72266 100644
@@ -78,69 +78,238 @@ void *khwasan_reset_tag(void *addr)
        return reset_tag(addr);
 }
 
+void kasan_poison_shadow(const void *address, size_t size, u8 value)
+{
+       void *shadow_start, *shadow_end;
+
+       /* Perform shadow offset calculation based on untagged address */
+       address = reset_tag((void *)address);
+
+       shadow_start = kasan_mem_to_shadow(address);
+       shadow_end = kasan_mem_to_shadow(address + size);
+
+       memset(shadow_start, value, shadow_end - shadow_start);
+}
+
 void kasan_unpoison_shadow(const void *address, size_t size)
 {
+       /* KHWASAN only allows 16-byte granularity */
+       size = round_up(size, KASAN_SHADOW_SCALE_SIZE);
+       kasan_poison_shadow(address, size, get_tag(address));
 }
 
 void check_memory_region(unsigned long addr, size_t size, bool write,
                                unsigned long ret_ip)
 {
+       u8 tag;
+       u8 *shadow_first, *shadow_last, *shadow;
+       void *untagged_addr;
+
+       tag = get_tag((void *)addr);
+       untagged_addr = reset_tag((void *)addr);
+       shadow_first = (u8 *)kasan_mem_to_shadow(untagged_addr);
+       shadow_last = (u8 *)kasan_mem_to_shadow(untagged_addr + size - 1);
+
+       for (shadow = shadow_first; shadow <= shadow_last; shadow++) {
+               if (*shadow != tag) {
+                       /* Report invalid-access bug here */
+                       return;
+               }
+       }
 }
 
 void kasan_free_pages(struct page *page, unsigned int order)
 {
+       if (likely(!PageHighMem(page)))
+               kasan_poison_shadow(page_address(page),
+                               PAGE_SIZE << order,
+                               khwasan_random_tag());
 }
 
 void kasan_cache_create(struct kmem_cache *cache, size_t *size,
                slab_flags_t *flags)
 {
+       int orig_size = *size;
+
+       cache->kasan_info.alloc_meta_offset = *size;
+       *size += sizeof(struct kasan_alloc_meta);
+
+       if (*size % KASAN_SHADOW_SCALE_SIZE != 0)
+               *size = round_up(*size, KASAN_SHADOW_SCALE_SIZE);
+
+
+       if (*size > KMALLOC_MAX_SIZE) {
+               *size = orig_size;
+               return;
+       }
+
+       cache->align = round_up(cache->align, KASAN_SHADOW_SCALE_SIZE);
+
+       *flags |= SLAB_KASAN;
 }
 
 void kasan_poison_slab(struct page *page)
 {
+       kasan_poison_shadow(page_address(page),
+                       PAGE_SIZE << compound_order(page),
+                       khwasan_random_tag());
 }
 
 void kasan_poison_object_data(struct kmem_cache *cache, void *object)
 {
+       kasan_poison_shadow(object,
+                       round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE),
+                       khwasan_random_tag());
 }
 
 void *kasan_slab_alloc(struct kmem_cache *cache, void *object, gfp_t flags)
 {
+       if (!READ_ONCE(khwasan_enabled))
+               return object;
+       object = kasan_kmalloc(cache, object, cache->object_size, flags);
+       if (unlikely(cache->ctor)) {
+               // Cache constructor might use object's pointer value to
+               // initialize some of its fields.
+               cache->ctor(object);
+       }
        return object;
 }
 
-bool kasan_slab_free(struct kmem_cache *cache, void *object, unsigned long ip)
+static bool __kasan_slab_free(struct kmem_cache *cache, void *object,
+                               unsigned long ip)
 {
+       u8 shadow_byte;
+       u8 tag;
+       unsigned long rounded_up_size;
+       void *untagged_addr = reset_tag(object);
+
+       if (unlikely(nearest_obj(cache, virt_to_head_page(untagged_addr),
+                       untagged_addr) != untagged_addr)) {
+               /* Report invalid-free here */
+               return true;
+       }
+
+       /* RCU slabs could be legally used after free within the RCU period */
+       if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
+               return false;
+
+       shadow_byte = READ_ONCE(*(u8 *)kasan_mem_to_shadow(untagged_addr));
+       tag = get_tag(object);
+       if (tag != shadow_byte) {
+               /* Report invalid-free here */
+               return true;
+       }
+
+       rounded_up_size = round_up(cache->object_size, KASAN_SHADOW_SCALE_SIZE);
+       kasan_poison_shadow(object, rounded_up_size, khwasan_random_tag());
+
+       if (unlikely(!(cache->flags & SLAB_KASAN)))
+               return false;
+
+       set_track(&get_alloc_info(cache, object)->free_track, GFP_NOWAIT);
        return false;
 }
 
+bool kasan_slab_free(struct kmem_cache *cache, void *object, unsigned long ip)
+{
+       return __kasan_slab_free(cache, object, ip);
+}
+
 void *kasan_kmalloc(struct kmem_cache *cache, const void *object,
                        size_t size, gfp_t flags)
 {
-       return (void *)object;
+       unsigned long redzone_start, redzone_end;
+       u8 tag;
+
+       if (!READ_ONCE(khwasan_enabled))
+               return (void *)object;
+
+       if (unlikely(object == NULL))
+               return NULL;
+
+       redzone_start = round_up((unsigned long)(object + size),
+                               KASAN_SHADOW_SCALE_SIZE);
+       redzone_end = round_up((unsigned long)(object + cache->object_size),
+                               KASAN_SHADOW_SCALE_SIZE);
+
+       tag = khwasan_random_tag();
+       kasan_poison_shadow(object, redzone_start - (unsigned long)object, tag);
+       kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start,
+               khwasan_random_tag());
+
+       if (cache->flags & SLAB_KASAN)
+               set_track(&get_alloc_info(cache, object)->alloc_track, flags);
+
+       return set_tag((void *)object, tag);
 }
 EXPORT_SYMBOL(kasan_kmalloc);
 
 void *kasan_kmalloc_large(const void *ptr, size_t size, gfp_t flags)
 {
-       return (void *)ptr;
+       unsigned long redzone_start, redzone_end;
+       u8 tag;
+       struct page *page;
+
+       if (!READ_ONCE(khwasan_enabled))
+               return (void *)ptr;
+
+       if (unlikely(ptr == NULL))
+               return NULL;

mm/kasan/khwasan.c +30 -1

+       return set_tag((void *)ptr, tag);
 }
 
 void kasan_poison_kfree(void *ptr, unsigned long ip)
 {
+       struct page *page;
+
+       page = virt_to_head_page(ptr);
+
+       if (unlikely(!PageSlab(page))) {
+               if (reset_tag(ptr) != page_address(page)) {
+                       /* Report invalid-free here */
+                       return;
+               }
+               kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page),
+                                       khwasan_random_tag());
+       } else {
+               __kasan_slab_free(page->slab_cache, ptr, ip);
+       }
 }
 
 void kasan_kfree_large(void *ptr, unsigned long ip)
 {
+       struct page *page = virt_to_page(ptr);
+       struct page *head_page = virt_to_head_page(ptr);
+
+       if (reset_tag(ptr) != page_address(head_page)) {
+               /* Report invalid-free here */
+               return;
+       }
+
+       kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page),
+                       khwasan_random_tag());
 }
 
 #define DEFINE_HWASAN_LOAD_STORE(size)                                 \
        void __hwasan_load##size##_noabort(unsigned long addr)          \
        {                                                               \
+               check_memory_region(addr, size, false, _RET_IP_);       \
        }                                                               \
        EXPORT_SYMBOL(__hwasan_load##size##_noabort);                   \
        void __hwasan_store##size##_noabort(unsigned long addr)         \
        {                                                               \
+               check_memory_region(addr, size, true, _RET_IP_);        \
        }                                                               \
        EXPORT_SYMBOL(__hwasan_store##size##_noabort)
 

@@ -152,15 +321,18 @@ DEFINE_HWASAN_LOAD_STORE(16);
 
 void __hwasan_loadN_noabort(unsigned long addr, unsigned long size)
 {
+       check_memory_region(addr, size, false, _RET_IP_);
 }
 EXPORT_SYMBOL(__hwasan_loadN_noabort);
 
 void __hwasan_storeN_noabort(unsigned long addr, unsigned long size)
 {
+       check_memory_region(addr, size, true, _RET_IP_);
 }
 EXPORT_SYMBOL(__hwasan_storeN_noabort);
 
 void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)
 {
+       kasan_poison_shadow((void *)addr, size, tag);
 }
 EXPORT_SYMBOL(__hwasan_tag_memory);
--

2.16.2.395.g2e18187dfd-goog

Quoted 17 lines · Andrey Konovalov

void *kasan_kmalloc_large(const void *ptr, size_t size, gfp_t flags) {

mm/kasan/khwasan.c +14 -1

-       return (void *)ptr;
+       unsigned long redzone_start, redzone_end;
+       u8 tag;
+       struct page *page;
+
+       if (!READ_ONCE(khwasan_enabled))
+               return (void *)ptr;
+
+       if (unlikely(ptr == NULL))
+               return NULL;
+
+       page = virt_to_page(ptr);
+       redzone_start = round_up((unsigned long)(ptr + size),
+                               KASAN_SHADOW_SCALE_SIZE);
+       redzone_end = (unsigned long)ptr + (PAGE_SIZE << compound_order(page));

mm/kasan/khwasan.c void *khwasan_reset_tag(void *addr) · line 267

Quoted 5 lines

mm/kasan/khwasan.c +5 -0

+
+       tag = khwasan_random_tag();
+       kasan_poison_shadow(ptr, redzone_start - (unsigned long)ptr, tag);
+       kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start,
+               khwasan_random_tag());

Am I understanding right that the object and the redzone may receive identical tags here?

Does it make sense to generate the redzone tag from the object tag (e.g. by addding 1 to it)?

Quoted 18 lines · Alexander Potapenko' via kasan-dev

Quoted 17 lines · Andrey Konovalov

void *kasan_kmalloc_large(const void *ptr, size_t size, gfp_t flags) {

mm/kasan/khwasan.c +14 -1

-       return (void *)ptr;
+       unsigned long redzone_start, redzone_end;
+       u8 tag;
+       struct page *page;
+
+       if (!READ_ONCE(khwasan_enabled))
+               return (void *)ptr;
+
+       if (unlikely(ptr == NULL))
+               return NULL;
+
+       page = virt_to_page(ptr);
+       redzone_start = round_up((unsigned long)(ptr + size),
+                               KASAN_SHADOW_SCALE_SIZE);
+       redzone_end = (unsigned long)ptr + (PAGE_SIZE << compound_order(page));

Quoted 8 lines

mm/kasan/khwasan.c void *khwasan_reset_tag(void *addr) · line 267

Quoted 5 lines

mm/kasan/khwasan.c +5 -0

+
+       tag = khwasan_random_tag();
+       kasan_poison_shadow(ptr, redzone_start - (unsigned long)ptr, tag);
+       kasan_poison_shadow((void *)redzone_start, redzone_end - redzone_start,
+               khwasan_random_tag());

Am I understanding right that the object and the redzone may receive identical tags here?

Correct.

Quoted 2 lines

Does it make sense to generate the redzone tag from the object tag (e.g. by addding 1 to it)?

Yes, I think so, will do!

Quoted 7 lines · Andrey Konovalov

Quoted 2 lines · Alexander Potapenko' via kasan-dev

Does it make sense to generate the redzone tag from the object tag (e.g. by addding 1 to it)?

Yes, I think so, will do!

Wouldn't be better to have some reserved tag value for invalid memory (redzones/free), so that we catch access to such memory with 100% probability?

Quoted 9 lines · Andrey Ryabinin

Quoted 7 lines · Andrey Konovalov

Quoted 2 lines · Alexander Potapenko' via kasan-dev

Does it make sense to generate the redzone tag from the object tag (e.g. by addding 1 to it)?

Yes, I think so, will do!

Quoted 2 lines

Wouldn't be better to have some reserved tag value for invalid memory (redzones/free), so that we catch access to such memory with 100% probability?

We could do that. That would reduce the chance to detect a use-after-free though, since we're using fewer different tag values for the objects themselves. I don't have a strong opinion about which one is better though.

Quoted 8 lines · Andrey Konovalov

On Thu, Mar 15, 2018 at 5:52 PM, Andrey Ryabinin

Quoted 2 lines

Wouldn't be better to have some reserved tag value for invalid memory (redzones/free), so that we catch access to such memory with 100% probability?

We could do that. That would reduce the chance to detect a use-after-free though, since we're using fewer different tag values for the objects themselves. I don't have a strong opinion about which one is better though.

hwasan does not need redzones.

As for use-after-free, to catch it with
100% probability one would need infinite memory for the quarantine. It is possible to guarantee 100% detection of linear buffer overflow by giving live adjacent chunks distinct tags.

Quoted 10 lines · Evgenii Stepanov

Quoted 8 lines · Andrey Konovalov

On Thu, Mar 15, 2018 at 5:52 PM, Andrey Ryabinin

Quoted 2 lines

Wouldn't be better to have some reserved tag value for invalid memory (redzones/free), so that we catch access to such memory with 100% probability?

We could do that. That would reduce the chance to detect a use-after-free though, since we're using fewer different tag values for the objects themselves. I don't have a strong opinion about which one is better though.

Note: I misread the message and didn't notice the "/free" part there, so I was considering marking only redzones with a reserved tag value.

Quoted 1 line

Quoted 1 line

hwasan does not need redzones.

Right, by redzones in this case I meant the metadata that is stored right after the object (which includes alloc and free stack handles and perhaps some other allocator stuff).

Quoted 5 lines

As for use-after-free, to catch it with
100% probability one would need infinite memory for the quarantine. It is possible to guarantee 100% detection of linear buffer overflow by giving live adjacent chunks distinct tags.

Quoted 3 lines · Andrey Konovalov

Right, by redzones in this case I meant the metadata that is stored right after the object (which includes alloc and free stack handles and perhaps some other allocator stuff).

Oh, I did not realize we have free (as in beer, not as in use-after-free) redzones between allocations. Yes, reserving a color sounds
like a good idea.

Quoted 4 lines

Quoted 4 lines

As for use-after-free, to catch it with
100% probability one would need infinite memory for the quarantine.

Quoted 3 lines

Quoted 3 lines

It
is possible to guarantee 100% detection of linear buffer overflow by giving live adjacent chunks distinct tags.

Quoted 10 lines · Evgenii Stepanov

Quoted 3 lines · Andrey Konovalov

Right, by redzones in this case I meant the metadata that is stored right after the object (which includes alloc and free stack handles and perhaps some other allocator stuff).

Oh, I did not realize we have free (as in beer, not as in use-after-free) redzones between allocations. Yes, reserving a color sounds
like a good idea.

OK, I'll do that then.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

As for use-after-free, to catch it with
100% probability one would need infinite memory for the quarantine.

As for the second part of Andrey's suggestion (as far as I understand it): reserve a color for freed objects. Without quarantine, this should give us a precise use-after-free-but-without-someone-else-allocating-the-same-object detection. What do you think about that?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

It
is possible to guarantee 100% detection of linear buffer overflow by giving live adjacent chunks distinct tags.

I'll add that to the TODO list as well.

mm/kasan/khwasan.c +7 -0

+
+	if (unlikely(!PageSlab(page))) {
+		if (reset_tag(ptr) != page_address(page)) {
+			/* Report invalid-free here */
+			return;
+		}
+		kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page),

mm/kasan/khwasan.c +8 -0

+
+	if (reset_tag(ptr) != page_address(head_page)) {
+		/* Report invalid-free here */
+		return;
+	}
+
+	kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page),
+			khwasan_random_tag());
 }

Hi Andrey,

mm/kasan/khwasan.c void *khwasan_reset_tag(void *addr)

Quoted 5 lines · Andrey Konovalov

void kasan_poison_kfree(void *ptr, unsigned long ip)
{

mm/kasan/khwasan.c +3 -0

+     struct page *page;
+
+     page = virt_to_head_page(ptr)

An untagged addr should be passed to virt_to_head_page(), no?

Quoted 8 lines

mm/kasan/khwasan.c +7 -0

+
+     if (unlikely(!PageSlab(page))) {
+             if (reset_tag(ptr) != page_address(page)) {
+                     /* Report invalid-free here */
+                     return;
+             }
+             kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page),

mm/kasan/khwasan.c void *khwasan_reset_tag(void *addr) · line 293

Quoted 10 lines

mm/kasan/khwasan.c +6 -0

+                                     khwasan_random_tag());
+     } else {
+             __kasan_slab_free(page->slab_cache, ptr, ip);
+     }
 }
 
 void kasan_kfree_large(void *ptr, unsigned long ip)
 {
+     struct page *page = virt_to_page(ptr);
+     struct page *head_page = virt_to_head_page(ptr);

Same as above and for virt_to_page() as well.

Anthony

Quoted 9 lines

mm/kasan/khwasan.c +8 -0

+
+     if (reset_tag(ptr) != page_address(head_page)) {
+             /* Report invalid-free here */
+             return;
+     }
+
+     kasan_poison_shadow(ptr, PAGE_SIZE << compound_order(page),
+                     khwasan_random_tag());
 }

KHWASAN inline instrumentation mode (which embeds checks of shadow memory into the generated code, instead of inserting a callback) generates a brk instruction when a tag mismatch is detected.

This commit add a KHWASAN brk handler, that decodes the immediate value passed to the brk instructions (to extract information about the memory access that triggered the mismatch), reads the register values (x0 contains the guilty address) and reports the bug.

---
 arch/arm64/include/asm/brk-imm.h |  2 ++
 arch/arm64/kernel/traps.c        | 40 ++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

arch/arm64/include/asm/brk-imm.h +2 -0

index ed693c5bcec0..e4a7013321dc 100644
@@ -16,10 +16,12 @@
  * 0x400: for dynamic BRK instruction
  * 0x401: for compile time BRK instruction
  * 0x800: kernel-mode BUG() and WARN() traps
+ * 0x9xx: KHWASAN trap (allowed values 0x900 - 0x9ff)
  */
 #define FAULT_BRK_IMM			0x100
 #define KGDB_DYN_DBG_BRK_IMM		0x400
 #define KGDB_COMPILED_DBG_BRK_IMM	0x401
 #define BUG_BRK_IMM			0x800
+#define KHWASAN_BRK_IMM			0x900
 
 #endif

arch/arm64/kernel/traps.c +1 -0

index eb2d15147e8d..5df8cdf5af13 100644
@@ -35,6 +35,7 @@
 #include <linux/sizes.h>
 #include <linux/syscalls.h>
 #include <linux/mm_types.h>
+#include <linux/kasan.h>
 
 #include <asm/atomic.h>
 #include <asm/bug.h>

Quoted 3 lines · Andrey Konovalov

KHWASAN inline instrumentation mode (which embeds checks of shadow memory into the generated code, instead of inserting a callback) generates a brk instruction when a tag mismatch is detected.

The compiler generates the BRK?

I'm a little worried about the ABI implications of that. So far we've assumed that for hte kernel-side, the BRK space is completely under our control.

How much does this save, compared to having a callback?

Quoted 40 lines

This commit add a KHWASAN brk handler, that decodes the immediate value passed to the brk instructions (to extract information about the memory access that triggered the mismatch), reads the register values (x0 contains the guilty address) and reports the bug.

---
 arch/arm64/include/asm/brk-imm.h |  2 ++
 arch/arm64/kernel/traps.c        | 40 ++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

arch/arm64/include/asm/brk-imm.h +2 -0

index ed693c5bcec0..e4a7013321dc 100644
@@ -16,10 +16,12 @@
  * 0x400: for dynamic BRK instruction
  * 0x401: for compile time BRK instruction
  * 0x800: kernel-mode BUG() and WARN() traps
+ * 0x9xx: KHWASAN trap (allowed values 0x900 - 0x9ff)
  */
 #define FAULT_BRK_IMM                        0x100
 #define KGDB_DYN_DBG_BRK_IMM         0x400
 #define KGDB_COMPILED_DBG_BRK_IMM    0x401
 #define BUG_BRK_IMM                  0x800
+#define KHWASAN_BRK_IMM                      0x900
 
 #endif

arch/arm64/kernel/traps.c +1 -0

index eb2d15147e8d..5df8cdf5af13 100644
@@ -35,6 +35,7 @@
 #include <linux/sizes.h>
 #include <linux/syscalls.h>
 #include <linux/mm_types.h>
+#include <linux/kasan.h>
 
 #include <asm/atomic.h>
 #include <asm/bug.h>

arch/arm64/kernel/traps.c static struct break_hook bug_break_hook = { · line 779

Quoted 8 lines

arch/arm64/kernel/traps.c +5 -0

@@ -771,6 +772,38 @@ static struct break_hook bug_break_hook = {
      .fn = bug_handler,
 };
 
+#ifdef CONFIG_KASAN_TAGS
+static int khwasan_handler(struct pt_regs *regs, unsigned int esr)
+{
+     bool recover = esr & 0x20;
+     bool write = esr & 0x10;

Can you please add mnemonics for these, e.g.

#define KHWASAN_ESR_RECOVER 0x20
#define KHWASAN_ESR_WRITE 0x10

arch/arm64/kernel/traps.c static struct break_hook bug_break_hook = { · line 780

Quoted 1 line

arch/arm64/kernel/traps.c +1 -0

+     size_t size = 1 << (esr & 0xf);

#define KHWASAN_ESR_SIZE_MASK 0x4
#define KHWASAN_ESR_SIZE(esr) (1 << (esr) & KHWASAN_ESR_SIZE_MASK)

arch/arm64/kernel/traps.c static struct break_hook bug_break_hook = { · line 790

Quoted 11 lines

arch/arm64/kernel/traps.c +10 -0

+     u64 addr = regs->regs[0];
+     u64 pc = regs->pc;
+
+     if (user_mode(regs))
+             return DBG_HOOK_ERROR;
+
+     khwasan_report(addr, size, write, pc);
+
+     if (!recover)
+             die("Oops - KHWASAN", regs, 0);

Could you elaborate on what "recover" means, and why it's up the the compiler to decide if the kernel should die()?

arch/arm64/kernel/traps.c static struct break_hook bug_break_hook = { · line 793

Quoted 4 lines

arch/arm64/kernel/traps.c +3 -0

+
+     /* If thread survives, skip over the BUG instruction and continue: */
+     arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);

This is for fast-forwarding user instruction streams, and isn't correct to call for kernel faults (as it'll mess up the userspace single step logic).

Thanks,
Mark.

Quoted 6 lines · Mark Rutland

Quoted 3 lines · Andrey Konovalov

KHWASAN inline instrumentation mode (which embeds checks of shadow memory into the generated code, instead of inserting a callback) generates a brk instruction when a tag mismatch is detected.

The compiler generates the BRK?

Correct.

Quoted 5 lines

I'm a little worried about the ABI implications of that. So far we've assumed that for hte kernel-side, the BRK space is completely under our control.

Quoted 1 line

How much does this save, compared to having a callback?

Around 7% of code size is what I see (you can have the same single instruction for a call, but it may cost some register allocation troubles).

Quoted 51 lines

Quoted 40 lines

This commit add a KHWASAN brk handler, that decodes the immediate value passed to the brk instructions (to extract information about the memory access that triggered the mismatch), reads the register values (x0 contains the guilty address) and reports the bug.

---
 arch/arm64/include/asm/brk-imm.h |  2 ++
 arch/arm64/kernel/traps.c        | 40 ++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

arch/arm64/include/asm/brk-imm.h +2 -0

index ed693c5bcec0..e4a7013321dc 100644
@@ -16,10 +16,12 @@
  * 0x400: for dynamic BRK instruction
  * 0x401: for compile time BRK instruction
  * 0x800: kernel-mode BUG() and WARN() traps
+ * 0x9xx: KHWASAN trap (allowed values 0x900 - 0x9ff)
  */
 #define FAULT_BRK_IMM                        0x100
 #define KGDB_DYN_DBG_BRK_IMM         0x400
 #define KGDB_COMPILED_DBG_BRK_IMM    0x401
 #define BUG_BRK_IMM                  0x800
+#define KHWASAN_BRK_IMM                      0x900
 
 #endif

arch/arm64/kernel/traps.c +1 -0

index eb2d15147e8d..5df8cdf5af13 100644
@@ -35,6 +35,7 @@
 #include <linux/sizes.h>
 #include <linux/syscalls.h>
 #include <linux/mm_types.h>
+#include <linux/kasan.h>
 
 #include <asm/atomic.h>
 #include <asm/bug.h>

arch/arm64/kernel/traps.c static struct break_hook bug_break_hook = { · line 779

Quoted 8 lines

arch/arm64/kernel/traps.c +5 -0

@@ -771,6 +772,38 @@ static struct break_hook bug_break_hook = {
      .fn = bug_handler,
 };
 
+#ifdef CONFIG_KASAN_TAGS
+static int khwasan_handler(struct pt_regs *regs, unsigned int esr)
+{
+     bool recover = esr & 0x20;
+     bool write = esr & 0x10;

Can you please add mnemonics for these, e.g.

Quoted 7 lines

#define KHWASAN_ESR_RECOVER 0x20
#define KHWASAN_ESR_WRITE 0x10

arch/arm64/kernel/traps.c static struct break_hook bug_break_hook = { · line 780

Quoted 1 line

arch/arm64/kernel/traps.c +1 -0

+     size_t size = 1 << (esr & 0xf);

#define KHWASAN_ESR_SIZE_MASK 0x4
#define KHWASAN_ESR_SIZE(esr) (1 << (esr) & KHWASAN_ESR_SIZE_MASK)

Will do!

Quoted 14 lines

arch/arm64/kernel/traps.c static struct break_hook bug_break_hook = { · line 790

Quoted 11 lines

arch/arm64/kernel/traps.c +10 -0

+     u64 addr = regs->regs[0];
+     u64 pc = regs->pc;
+
+     if (user_mode(regs))
+             return DBG_HOOK_ERROR;
+
+     khwasan_report(addr, size, write, pc);
+
+     if (!recover)
+             die("Oops - KHWASAN", regs, 0);

Could you elaborate on what "recover" means, and why it's up the the compiler to decide if the kernel should die()?

The instrumentation allows to control whether we can proceed after a crash was detected. This is done by passing the -recover flag to the compiler. Disabling recovery allows to generate more compact code.

Unfortunately disabling recovery doesn't work for the kernel right now. KHWASAN reporting is disabled in some contexts (for example when the allocator accesses slab object metadata; same is true for KASAN; this is controlled by current->kasan_depth). All these accesses are detected by the tool, even though the reports for them are not printed.

This is something that might be fixed at some point in the future, so I think it makes sense to leave this check as is.

I'll add a comment with explanations though.

Quoted 8 lines

arch/arm64/kernel/traps.c static struct break_hook bug_break_hook = { · line 793

Quoted 4 lines

arch/arm64/kernel/traps.c +3 -0

+
+     /* If thread survives, skip over the BUG instruction and continue: */
+     arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);

This is for fast-forwarding user instruction streams, and isn't correct to call for kernel faults (as it'll mess up the userspace single step logic).

I saw BUG handler using this (which also inserts a brk), so I used it as well. What should I do instead to jump over the faulting brk instruction?

Thanks!

Quoted 3 lines

Thanks,
Mark.

Quoted 7 lines · Mark Rutland

Quoted 5 lines · Andrey Konovalov

(quoted diff) +4 -0

+static int khwasan_handler(struct pt_regs *regs, unsigned int esr)
+{
 
+     /* If thread survives, skip over the BUG instruction and continue: */
+     arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);

Quoted 3 lines

This is for fast-forwarding user instruction streams, and isn't correct to call for kernel faults (as it'll mess up the userspace single step logic).

I saw BUG handler using this (which also inserts a brk), so I used it as well.

What should I do instead to jump over the faulting brk instruction?

Quoted 2 lines · Andrey Konovalov

I saw BUG handler using this (which also inserts a brk), so I used it as well.

Ah; I think that's broken today.

Quoted 1 line

What should I do instead to jump over the faulting brk instruction?

I don't think we have anything to do this properly today.

The simplest fix would be to split arm64_skip_faulting_instruction() into separate functions for user/kernel, something like the below.

It would be nice to drop _user_ in the name of the userspace-specific helper, though.

Thanks
Mark.

---->8----

arch/arm64/kernel/traps.c +7 -2

index eb2d15147e8d..101e3d4ed6c8 100644
@@ -235,9 +235,14 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
        }
 }
 
-void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
+void __arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
 {
        regs->pc += size;
+}
+
+void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
+{
+       __arm64_skip_faulting_instruction(regs, size);
 
        /*
         * If we were single stepping, we want to get the step exception after

@@ -761,7 +766,7 @@ static int bug_handler(struct pt_regs *regs, unsigned int esr)
        }
 
        /* If thread survives, skip over the BUG instruction and continue: */
-       arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
+       __arm64_skip_faulting_instruction(regs, AARCH64_INSN_SIZE);
        return DBG_HOOK_HANDLED;
 }
 

There are two reasons to use outline instrumentation:
1. Outline instrumentation reduces the size of the kernel text, and should be used where this size matters.
2. Outline instrumentation is less invasive and can be used for debugging for KASAN developers, when it's not clear whether some issue is caused by KASAN or by something else.

For the rest cases inline instrumentation is preferrable, since it's faster.

This patch changes the default instrumentation mode to inline.

---
 lib/Kconfig.kasan | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/Kconfig.kasan +0 -0

index ab34e7d7d3a7..8ea6ae26b4a3 100644