Re: [PATCH v6 03/18] khwasan: add CONFIG_KASAN_GENERIC and CONFIG_KASAN_HW

[Dmitry Vyukov <dvyukov@google.com>]

On Wed, Sep 12, 2018 at 4:47 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Aug 29, 2018 at 1:35 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
>> This commit splits the current CONFIG_KASAN config option into two:
>> 1. CONFIG_KASAN_GENERIC, that enables the generic software-only KASAN
>>    version (the one that exists now);
>> 2. CONFIG_KASAN_HW, that enables KHWASAN.
>>
>> With CONFIG_KASAN_HW enabled, compiler options are changed to instrument
>> kernel files wiht -fsantize=hwaddress (except the ones for which
>> KASAN_SANITIZE := n is set).
>>
>> Both CONFIG_KASAN_GENERIC and CONFIG_KASAN_HW support both
>> CONFIG_KASAN_INLINE and CONFIG_KASAN_OUTLINE instrumentation modes.
>>
>> This commit also adds empty placeholder (for now) implementation of
>> KHWASAN specific hooks inserted by the compiler and adjusts common hooks
>> implementation to compile correctly with each of the config options.
>>
>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> ---
>>  arch/arm64/Kconfig             |  1 +
>>  include/linux/compiler-clang.h |  3 +-
>>  include/linux/compiler-gcc.h   |  4 ++
>>  include/linux/compiler.h       |  3 +-
>>  include/linux/kasan.h          | 16 +++++--
>>  lib/Kconfig.kasan              | 77 ++++++++++++++++++++++++++--------
>>  mm/kasan/Makefile              |  6 ++-
>>  mm/kasan/kasan.h               |  3 +-
>>  mm/kasan/khwasan.c             | 75 +++++++++++++++++++++++++++++++++
>>  mm/slub.c                      |  2 +-
>>  scripts/Makefile.kasan         | 27 +++++++++++-
>>  11 files changed, 188 insertions(+), 29 deletions(-)
>>  create mode 100644 mm/kasan/khwasan.c
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 29e75b47becd..991564148f54 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -105,6 +105,7 @@ config ARM64
>>         select HAVE_ARCH_HUGE_VMAP
>>         select HAVE_ARCH_JUMP_LABEL
>>         select HAVE_ARCH_KASAN if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
>> +       select HAVE_ARCH_KASAN_HW if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
>>         select HAVE_ARCH_KGDB
>>         select HAVE_ARCH_MMAP_RND_BITS
>>         select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
>> index b1ce500fe8b3..2c258a9d4c67 100644
>> --- a/include/linux/compiler-clang.h
>> +++ b/include/linux/compiler-clang.h
>> @@ -17,11 +17,12 @@
>>  #define KASAN_ABI_VERSION 5
>>
>>  /* emulate gcc's __SANITIZE_ADDRESS__ flag */
>> -#if __has_feature(address_sanitizer)
>> +#if __has_feature(address_sanitizer) || __has_feature(hwaddress_sanitizer)
>>  #define __SANITIZE_ADDRESS__
>>  #endif
>>
>>  #define __no_sanitize_address __attribute__((no_sanitize("address")))
>> +#define __no_sanitize_hwaddress __attribute__((no_sanitize("hwaddress")))
>
> It seems that it would be better to have just 1 attribute for both types.
> Currently __no_sanitize_address is used just in a single place. But if
> it ever used more, people will need to always spell both which looks
> unnecessary, or, worse will only fix asan but forget about khwasan.
>
> If we do just:
>
> #define __no_sanitize_address __attribute__((no_sanitize("address",
> "hwaddress")))
>
> Then we don't need any changes in compiler-gcc.h nor in compiler.h,
> and no chance or forgetting one of them.
>
>>  /*
>>   * Not all versions of clang implement the the type-generic versions
>> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
>> index 763bbad1e258..a186b55c8c4c 100644
>> --- a/include/linux/compiler-gcc.h
>> +++ b/include/linux/compiler-gcc.h
>> @@ -227,6 +227,10 @@
>>  #define __no_sanitize_address
>>  #endif
>>
>> +#if !defined(__no_sanitize_hwaddress)
>> +#define __no_sanitize_hwaddress        /* gcc doesn't support KHWASAN */
>> +#endif
>> +
>>  /*
>>   * Turn individual warnings and errors on and off locally, depending
>>   * on version.
>> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> index 681d866efb1e..3f2ba192d57d 100644
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -195,7 +195,8 @@ void __read_once_size(const volatile void *p, void *res, int size)
>>   *     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
>>   * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
>>   */
>> -# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
>> +# define __no_kasan_or_inline __no_sanitize_address __no_sanitize_hwaddress \
>> +                             __maybe_unused
>>  #else
>>  # define __no_kasan_or_inline __always_inline
>>  #endif
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index 54d577ad2181..beb56a26fe9b 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -45,8 +45,6 @@ void kasan_free_pages(struct page *page, unsigned int order);
>>
>>  void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>>                         slab_flags_t *flags);
>> -void kasan_cache_shrink(struct kmem_cache *cache);
>> -void kasan_cache_shutdown(struct kmem_cache *cache);
>>
>>  void kasan_poison_slab(struct page *page);
>>  void kasan_unpoison_object_data(struct kmem_cache *cache, void *object);
>> @@ -97,8 +95,6 @@ static inline void kasan_free_pages(struct page *page, unsigned int order) {}
>>  static inline void kasan_cache_create(struct kmem_cache *cache,
>>                                       unsigned int *size,
>>                                       slab_flags_t *flags) {}
>> -static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
>> -static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
>>
>>  static inline void kasan_poison_slab(struct page *page) {}
>>  static inline void kasan_unpoison_object_data(struct kmem_cache *cache,
>> @@ -152,4 +148,16 @@ static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
>>
>>  #endif /* CONFIG_KASAN */
>>
>> +#ifdef CONFIG_KASAN_GENERIC
>> +
>> +void kasan_cache_shrink(struct kmem_cache *cache);
>> +void kasan_cache_shutdown(struct kmem_cache *cache);
>> +
>> +#else /* CONFIG_KASAN_GENERIC */
>> +
>> +static inline void kasan_cache_shrink(struct kmem_cache *cache) {}
>> +static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
>> +
>> +#endif /* CONFIG_KASAN_GENERIC */
>> +
>>  #endif /* LINUX_KASAN_H */
>> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
>> index befb127507c0..5a22629f30e7 100644
>> --- a/lib/Kconfig.kasan
>> +++ b/lib/Kconfig.kasan
>> @@ -1,34 +1,75 @@
>>  config HAVE_ARCH_KASAN
>>         bool
>>
>> +config HAVE_ARCH_KASAN_HW
>> +       bool
>> +
>>  if HAVE_ARCH_KASAN
>>
>>  config KASAN
>> -       bool "KASan: runtime memory debugger"
>> +       bool "KASAN: runtime memory debugger"
>> +       help
>> +         Enables KASAN (KernelAddressSANitizer) - runtime memory debugger,
>> +         designed to find out-of-bounds accesses and use-after-free bugs.
>
> Perhaps also give link to Documentation/dev-tools/kasan.rst while we are here.
>
>> +
>> +choice
>> +       prompt "KASAN mode"
>> +       depends on KASAN
>> +       default KASAN_GENERIC
>> +       help
>> +         KASAN has two modes: KASAN (a classic version, similar to userspace
>
> In these few sentences we call the old mode with 3 different terms:
> "generic", "classic" and "KASAN" :)
> This is somewhat confusing. Let's call it "generic" throughout (here
> and in the docs patch). "Generic" as in "supported on multiple arch
> and not-dependent on hardware features". "Classic" makes sense for
> people who knew KASAN before, but for future readers in won't make
> sense.
>
>
>> +         ASan, enabled with CONFIG_KASAN_GENERIC) and KHWASAN (a version
>> +         based on pointer tagging, only for arm64, similar to userspace
>> +         HWASan, enabled with CONFIG_KASAN_HW).
>> +
>> +config KASAN_GENERIC
>> +       bool "KASAN: the generic mode"
>>         depends on (SLUB && SYSFS) || (SLAB && !DEBUG_SLAB)
>>         select SLUB_DEBUG if SLUB
>>         select CONSTRUCTORS
>>         select STACKDEPOT
>>         help
>> -         Enables kernel address sanitizer - runtime memory debugger,
>> -         designed to find out-of-bounds accesses and use-after-free bugs.
>> -         This is strictly a debugging feature and it requires a gcc version
>> -         of 4.9.2 or later. Detection of out of bounds accesses to stack or
>> -         global variables requires gcc 5.0 or later.
>> -         This feature consumes about 1/8 of available memory and brings about
>> -         ~x3 performance slowdown.
>> +         Enables the generic mode of KASAN.
>> +         This is strictly a debugging feature and it requires a GCC version
>> +         of 4.9.2 or later. Detection of out-of-bounds accesses to stack or
>> +         global variables requires GCC 5.0 or later.
>> +         This mode consumes about 1/8 of available memory at kernel start
>> +         and introduces an overhead of ~x1.5 for the rest of the allocations.
>> +         The performance slowdown is ~x3.
>>           For better error detection enable CONFIG_STACKTRACE.
>> -         Currently CONFIG_KASAN doesn't work with CONFIG_DEBUG_SLAB
>> +         Currently CONFIG_KASAN_GENERIC doesn't work with CONFIG_DEBUG_SLAB
>>           (the resulting kernel does not boot).
>>
>> +if HAVE_ARCH_KASAN_HW
>
> This choice looks somewhat weird on non-arm64. It's kinda a choice
> menu, but one can't really choose anything. Should we put the whole
> choice under HAVE_ARCH_KASAN_HW, and just select KASAN_GENERIC
> otherwise? I don't know what't the practice here. Andrey R?
>
>> +config KASAN_HW
>> +       bool "KHWASAN: the hardware assisted mode"

Do we need a hyphen here? hardware-assisted?

>> +       depends on (SLUB && SYSFS) || (SLAB && !DEBUG_SLAB)
>> +       select SLUB_DEBUG if SLUB
>> +       select CONSTRUCTORS
>> +       select STACKDEPOT
>> +       help
>> +         Enabled KHWASAN (KASAN mode based on pointer tagging).
>> +         This mode requires Top Byte Ignore support by the CPU and therefore
>> +         only supported for arm64.
>> +         This feature requires clang revision 330044 or later.
>> +         This mode consumes about 1/16 of available memory at kernel start
>> +         and introduces an overhead of ~20% for the rest of the allocations.
>> +         For better error detection enable CONFIG_STACKTRACE.
>> +         Currently CONFIG_KASAN_HW doesn't work with CONFIG_DEBUG_SLAB
>> +         (the resulting kernel does not boot).
>> +
>> +endif
>> +
>> +endchoice
>> +
>>  config KASAN_EXTRA
>> -       bool "KAsan: extra checks"
>> -       depends on KASAN && DEBUG_KERNEL && !COMPILE_TEST
>> +       bool "KASAN: extra checks"
>> +       depends on KASAN_GENERIC && DEBUG_KERNEL && !COMPILE_TEST
>>         help
>> -         This enables further checks in the kernel address sanitizer, for now
>> -         it only includes the address-use-after-scope check that can lead
>> -         to excessive kernel stack usage, frame size warnings and longer
>> -         compile time.
>> +         This enables further checks in KASAN, for now it only includes the
>> +         address-use-after-scope check that can lead to excessive kernel
>> +         stack usage, frame size warnings and longer compile time.
>>           https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715 has more
>>
>>
>> @@ -53,16 +94,16 @@ config KASAN_INLINE
>>           memory accesses. This is faster than outline (in some workloads
>>           it gives about x2 boost over outline instrumentation), but
>>           make kernel's .text size much bigger.
>> -         This requires a gcc version of 5.0 or later.
>> +         For CONFIG_KASAN_GENERIC this requires GCC 5.0 or later.
>>
>>  endchoice
>>
>>  config TEST_KASAN
>> -       tristate "Module for testing kasan for bug detection"
>> +       tristate "Module for testing KASAN for bug detection"
>>         depends on m && KASAN
>>         help
>>           This is a test module doing various nasty things like
>>           out of bounds accesses, use after free. It is useful for testing
>> -         kernel debugging features like kernel address sanitizer.
>> +         kernel debugging features like KASAN.
>>
>>  endif
>> diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
>> index a6df14bffb6b..14955add96d3 100644
>> --- a/mm/kasan/Makefile
>> +++ b/mm/kasan/Makefile
>> @@ -2,6 +2,7 @@
>>  KASAN_SANITIZE := n
>>  UBSAN_SANITIZE_common.o := n
>>  UBSAN_SANITIZE_kasan.o := n
>> +UBSAN_SANITIZE_khwasan.o := n
>>  KCOV_INSTRUMENT := n
>>
>>  CFLAGS_REMOVE_kasan.o = -pg
>> @@ -10,5 +11,8 @@ CFLAGS_REMOVE_kasan.o = -pg
>>
>>  CFLAGS_common.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
>>  CFLAGS_kasan.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
>> +CFLAGS_khwasan.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
>>
>> -obj-y := common.o kasan.o report.o kasan_init.o quarantine.o
>> +obj-$(CONFIG_KASAN) := common.o kasan_init.o report.o
>> +obj-$(CONFIG_KASAN_GENERIC) += kasan.o quarantine.o
>> +obj-$(CONFIG_KASAN_HW) += khwasan.o
>> diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
>> index 659463800f10..19b950eaccff 100644
>> --- a/mm/kasan/kasan.h
>> +++ b/mm/kasan/kasan.h
>> @@ -114,7 +114,8 @@ void kasan_report(unsigned long addr, size_t size,
>>                 bool is_write, unsigned long ip);
>>  void kasan_report_invalid_free(void *object, unsigned long ip);
>>
>> -#if defined(CONFIG_SLAB) || defined(CONFIG_SLUB)
>> +#if defined(CONFIG_KASAN_GENERIC) && \
>> +       (defined(CONFIG_SLAB) || defined(CONFIG_SLUB))
>>  void quarantine_put(struct kasan_free_meta *info, struct kmem_cache *cache);
>>  void quarantine_reduce(void);
>>  void quarantine_remove_cache(struct kmem_cache *cache);
>> diff --git a/mm/kasan/khwasan.c b/mm/kasan/khwasan.c
>> new file mode 100644
>> index 000000000000..e2c3a7f7fd1f
>> --- /dev/null
>> +++ b/mm/kasan/khwasan.c
>> @@ -0,0 +1,75 @@
>> +/*
>> + * This file contains core KHWASAN code.
>> + *
>> + * Copyright (c) 2018 Google, Inc.
>> + * Author: Andrey Konovalov <andreyknvl@google.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +#define DISABLE_BRANCH_PROFILING
>> +
>> +#include <linux/export.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/init.h>
>> +#include <linux/kasan.h>
>> +#include <linux/kernel.h>
>> +#include <linux/kmemleak.h>
>> +#include <linux/linkage.h>
>> +#include <linux/memblock.h>
>> +#include <linux/memory.h>
>> +#include <linux/mm.h>
>> +#include <linux/module.h>
>> +#include <linux/printk.h>
>> +#include <linux/random.h>
>> +#include <linux/sched.h>
>> +#include <linux/sched/task_stack.h>
>> +#include <linux/slab.h>
>> +#include <linux/stacktrace.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/bug.h>
>> +
>> +#include "kasan.h"
>> +#include "../slab.h"
>> +
>> +void check_memory_region(unsigned long addr, size_t size, bool write,
>> +                               unsigned long ret_ip)
>> +{
>> +}
>> +
>> +#define DEFINE_HWASAN_LOAD_STORE(size)                                 \
>> +       void __hwasan_load##size##_noabort(unsigned long addr)          \
>> +       {                                                               \
>> +       }                                                               \
>> +       EXPORT_SYMBOL(__hwasan_load##size##_noabort);                   \
>> +       void __hwasan_store##size##_noabort(unsigned long addr)         \
>> +       {                                                               \
>> +       }                                                               \
>> +       EXPORT_SYMBOL(__hwasan_store##size##_noabort)
>> +
>> +DEFINE_HWASAN_LOAD_STORE(1);
>> +DEFINE_HWASAN_LOAD_STORE(2);
>> +DEFINE_HWASAN_LOAD_STORE(4);
>> +DEFINE_HWASAN_LOAD_STORE(8);
>> +DEFINE_HWASAN_LOAD_STORE(16);
>> +
>> +void __hwasan_loadN_noabort(unsigned long addr, unsigned long size)
>> +{
>> +}
>> +EXPORT_SYMBOL(__hwasan_loadN_noabort);
>> +
>> +void __hwasan_storeN_noabort(unsigned long addr, unsigned long size)
>> +{
>> +}
>> +EXPORT_SYMBOL(__hwasan_storeN_noabort);
>> +
>> +void __hwasan_tag_memory(unsigned long addr, u8 tag, unsigned long size)
>> +{
>> +}
>> +EXPORT_SYMBOL(__hwasan_tag_memory);
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 30b9bf777bab..4206e1b616e7 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -2955,7 +2955,7 @@ static __always_inline void slab_free(struct kmem_cache *s, struct page *page,
>>                 do_slab_free(s, page, head, tail, cnt, addr);
>>  }
>>
>> -#ifdef CONFIG_KASAN
>> +#ifdef CONFIG_KASAN_GENERIC
>>  void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
>>  {
>>         do_slab_free(cache, virt_to_head_page(x), x, NULL, 1, addr);
>> diff --git a/scripts/Makefile.kasan b/scripts/Makefile.kasan
>> index 69552a39951d..49c6e056c697 100644
>> --- a/scripts/Makefile.kasan
>> +++ b/scripts/Makefile.kasan
>> @@ -1,5 +1,5 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> -ifdef CONFIG_KASAN
>> +ifdef CONFIG_KASAN_GENERIC
>>  ifdef CONFIG_KASAN_INLINE
>>         call_threshold := 10000
>>  else
>> @@ -42,6 +42,29 @@ ifdef CONFIG_KASAN_EXTRA
>>  CFLAGS_KASAN += $(call cc-option, -fsanitize-address-use-after-scope)
>>  endif
>>
>> -CFLAGS_KASAN_NOSANITIZE := -fno-builtin
>> +endif
>> +
>> +ifdef CONFIG_KASAN_HW
>> +
>> +ifdef CONFIG_KASAN_INLINE
>> +    instrumentation_flags := -mllvm -hwasan-mapping-offset=$(KASAN_SHADOW_OFFSET)
>> +else
>> +    instrumentation_flags := -mllvm -hwasan-instrument-with-calls=1
>> +endif
>>
>> +CFLAGS_KASAN := -fsanitize=kernel-hwaddress \
>> +               -mllvm -hwasan-instrument-stack=0 \
>> +               $(instrumentation_flags)
>> +
>> +ifeq ($(call cc-option, $(CFLAGS_KASAN) -Werror),)
>> +    ifneq ($(CONFIG_COMPILE_TEST),y)
>> +        $(warning Cannot use CONFIG_KASAN_HW: \
>> +            -fsanitize=hwaddress is not supported by compiler)
>> +    endif
>> +endif
>> +
>> +endif
>> +
>> +ifdef CONFIG_KASAN
>> +CFLAGS_KASAN_NOSANITIZE := -fno-builtin
>>  endif
>> --
>> 2.19.0.rc0.228.g281dcd1b4d0-goog
>>