Re: [PATCH 1/8] kasan,x86: Frob kasan_report() in an exception

[Dmitry Vyukov <dvyukov@google.com>]

On Fri, Mar 1, 2019 at 3:46 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Feb 28, 2019 at 04:22:04PM +0100, Dmitry Vyukov wrote:
> > On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> > > UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> > > be called from there, move it into an exception much like BUG/WARN.
> > >
> > > *compile tested only*
> >
> >
> > Please test it by booting KASAN kernel and then loading module
> > produced by CONFIG_TEST_KASAN=y. There are too many subtle aspects to
> > rely on "compile tested only", reviewers can't catch all of them
> > either.
>
> The below boots and survives test_kasan.
>
> I'll now try that annotation you preferred. But I think this is a pretty
> neat hack :-)

Yes, it's "neat" but it's also a "hack" :)

It involves asm, hardware exceptions, UD2 instructions. It also seems
to use arch-dependent code in arch-independent files: there is no RSI
on other arches, does this compile on non-x86? I understand you are
pretty comfortable with such code, but all else being equal normal C
code is preferable.
And it's not that we gain much due to this, we are merely working
around things. We tried to use UD2 for asan reports, but we emitted it
into the generated code where it had a chance of speeding up things
which could potentially justify hacks. But even there the final
decision was to go with normal calls.


> ---
> Subject: kasan,x86: Frob kasan_report() in an exception
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu Feb 28 15:52:03 CET 2019
>
> Because __asan_{load,store}{N,1,2,4,8,16}_noabort() get called from
> UACCESS context, and kasan_report() is most definitely _NOT_ safe to
> be called from there, move it into an exception much like BUg/WARN.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/bug.h   |   28 ++++++++++++++--------------
>  arch/x86/include/asm/kasan.h |   17 +++++++++++++++++
>  include/asm-generic/bug.h    |    1 +
>  include/linux/kasan.h        |   12 +++++++++---
>  lib/bug.c                    |    9 ++++++++-
>  mm/kasan/kasan.h             |    2 +-
>  mm/kasan/report.c            |    2 +-
>  7 files changed, 51 insertions(+), 20 deletions(-)
>
> --- a/arch/x86/include/asm/bug.h
> +++ b/arch/x86/include/asm/bug.h
> @@ -30,33 +30,33 @@
>
>  #ifdef CONFIG_DEBUG_BUGVERBOSE
>
> -#define _BUG_FLAGS(ins, flags)                                         \
> +#define _BUG_FLAGS(ins, flags, ...)                                    \
>  do {                                                                   \
>         asm volatile("1:\t" ins "\n"                                    \
>                      ".pushsection __bug_table,\"aw\"\n"                \
> -                    "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n"   \
> -                    "\t"  __BUG_REL(%c0) "\t# bug_entry::file\n"       \
> -                    "\t.word %c1"        "\t# bug_entry::line\n"       \
> -                    "\t.word %c2"        "\t# bug_entry::flags\n"      \
> -                    "\t.org 2b+%c3\n"                                  \
> +                    "2:\t" __BUG_REL(1b)      "\t# bug_entry::bug_addr\n" \
> +                    "\t"  __BUG_REL(%c[file]) "\t# bug_entry::file\n"  \
> +                    "\t.word %c[line]"        "\t# bug_entry::line\n"  \
> +                    "\t.word %c[flag]"        "\t# bug_entry::flags\n" \
> +                    "\t.org 2b+%c[size]\n"                             \
>                      ".popsection"                                      \
> -                    : : "i" (__FILE__), "i" (__LINE__),                \
> -                        "i" (flags),                                   \
> -                        "i" (sizeof(struct bug_entry)));               \
> +                    : : [file] "i" (__FILE__), [line] "i" (__LINE__),  \
> +                        [flag] "i" (flags),                            \
> +                        [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__); \
>  } while (0)
>
>  #else /* !CONFIG_DEBUG_BUGVERBOSE */
>
> -#define _BUG_FLAGS(ins, flags)                                         \
> +#define _BUG_FLAGS(ins, flags, ...)                                    \
>  do {                                                                   \
>         asm volatile("1:\t" ins "\n"                                    \
>                      ".pushsection __bug_table,\"aw\"\n"                \
>                      "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n"   \
> -                    "\t.word %c0"        "\t# bug_entry::flags\n"      \
> -                    "\t.org 2b+%c1\n"                                  \
> +                    "\t.word %c[flag]"   "\t# bug_entry::flags\n"      \
> +                    "\t.org 2b+%c[size]\n"                             \
>                      ".popsection"                                      \
> -                    : : "i" (flags),                                   \
> -                        "i" (sizeof(struct bug_entry)));               \
> +                    : : [flag] "i" (flags),                            \
> +                        [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__); \
>  } while (0)
>
>  #endif /* CONFIG_DEBUG_BUGVERBOSE */
> --- a/arch/x86/include/asm/kasan.h
> +++ b/arch/x86/include/asm/kasan.h
> @@ -2,7 +2,10 @@
>  #ifndef _ASM_X86_KASAN_H
>  #define _ASM_X86_KASAN_H
>
> +#include <asm/bug.h>
> +
>  #include <linux/const.h>
> +#include <linux/sched.h>
>  #define KASAN_SHADOW_OFFSET _AC(CONFIG_KASAN_SHADOW_OFFSET, UL)
>  #define KASAN_SHADOW_SCALE_SHIFT 3
>
> @@ -26,8 +29,22 @@
>  #ifndef __ASSEMBLY__
>
>  #ifdef CONFIG_KASAN
> +
>  void __init kasan_early_init(void);
>  void __init kasan_init(void);
> +
> +static __always_inline void
> +kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
> +{
> +       if (!current->kasan_depth) {
> +               unsigned long rdi = addr, rsi = size, rdx = is_write, rcx = ip;
> +               _BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN,
> +                               "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
> +               annotate_reachable();
> +       }
> +}
> +#define kasan_report kasan_report
> +
>  #else
>  static inline void kasan_early_init(void) { }
>  static inline void kasan_init(void) { }
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -10,6 +10,7 @@
>  #define BUGFLAG_WARNING                (1 << 0)
>  #define BUGFLAG_ONCE           (1 << 1)
>  #define BUGFLAG_DONE           (1 << 2)
> +#define BUGFLAG_KASAN          (1 << 3)
>  #define BUGFLAG_TAINT(taint)   ((taint) << 8)
>  #define BUG_GET_TAINT(bug)     ((bug)->flags >> 8)
>  #endif
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -83,6 +83,9 @@ size_t kasan_metadata_size(struct kmem_c
>  bool kasan_save_enable_multi_shot(void);
>  void kasan_restore_multi_shot(bool enabled);
>
> +void __kasan_report(unsigned long addr, size_t size,
> +               bool is_write, unsigned long ip);
> +
>  #else /* CONFIG_KASAN */
>
>  static inline void kasan_unpoison_shadow(const void *address, size_t size) {}
> @@ -153,8 +156,14 @@ static inline void kasan_remove_zero_sha
>  static inline void kasan_unpoison_slab(const void *ptr) { }
>  static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
>
> +static inline void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip) { }
> +
>  #endif /* CONFIG_KASAN */
>
> +#ifndef kasan_report
> +#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
> +#endif
> +
>  #ifdef CONFIG_KASAN_GENERIC
>
>  #define KASAN_SHADOW_INIT 0
> @@ -177,9 +186,6 @@ void kasan_init_tags(void);
>
>  void *kasan_reset_tag(const void *addr);
>
> -void kasan_report(unsigned long addr, size_t size,
> -               bool is_write, unsigned long ip);
> -
>  #else /* CONFIG_KASAN_SW_TAGS */
>
>  static inline void kasan_init_tags(void) { }
> --- a/lib/bug.c
> +++ b/lib/bug.c
> @@ -47,6 +47,7 @@
>  #include <linux/bug.h>
>  #include <linux/sched.h>
>  #include <linux/rculist.h>
> +#include <linux/kasan.h>
>
>  extern struct bug_entry __start___bug_table[], __stop___bug_table[];
>
> @@ -144,7 +145,7 @@ enum bug_trap_type report_bug(unsigned l
>  {
>         struct bug_entry *bug;
>         const char *file;
> -       unsigned line, warning, once, done;
> +       unsigned line, warning, once, done, kasan;
>
>         if (!is_valid_bugaddr(bugaddr))
>                 return BUG_TRAP_TYPE_NONE;
> @@ -167,6 +168,7 @@ enum bug_trap_type report_bug(unsigned l
>                 line = bug->line;
>  #endif
>                 warning = (bug->flags & BUGFLAG_WARNING) != 0;
> +               kasan = (bug->flags & BUGFLAG_KASAN) != 0;
>                 once = (bug->flags & BUGFLAG_ONCE) != 0;
>                 done = (bug->flags & BUGFLAG_DONE) != 0;
>
> @@ -188,6 +190,11 @@ enum bug_trap_type report_bug(unsigned l
>                 return BUG_TRAP_TYPE_WARN;
>         }
>
> +       if (kasan) {
> +               __kasan_report(regs->di, regs->si, regs->dx, regs->cx);
> +               return BUG_TRAP_TYPE_WARN;
> +       }
> +
>         printk(KERN_DEFAULT CUT_HERE);
>
>         if (file)
> --- a/mm/kasan/kasan.h
> +++ b/mm/kasan/kasan.h
> @@ -130,7 +130,7 @@ void check_memory_region(unsigned long a
>  void *find_first_bad_addr(void *addr, size_t size);
>  const char *get_bug_type(struct kasan_access_info *info);
>
> -void kasan_report(unsigned long addr, size_t size,
> +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);
>
> --- a/mm/kasan/report.c
> +++ b/mm/kasan/report.c
> @@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *obj
>         end_report(&flags);
>  }
>
> -void kasan_report(unsigned long addr, size_t size,
> +void __kasan_report(unsigned long addr, size_t size,
>                 bool is_write, unsigned long ip)
>  {
>         struct kasan_access_info info;