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

[Dmitry Vyukov]

On Wed, Aug 29, 2018 at 1:35 PM, Andrey Konovalov <andreyknvl@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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"
> +       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@xxxxxxxxxx>
> + *
> + * 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
>