On Thu, Feb 28, 2019 at 4:05 PM Peter Zijlstra <pet...@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. > Cc: Andrey Ryabinin <aryabi...@virtuozzo.com> > Cc: Dmitry Vyukov <dvyu...@google.com> > Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org> > --- > arch/x86/include/asm/bug.h | 28 ++++++++++++++-------------- > arch/x86/include/asm/kasan.h | 15 +++++++++++++++ > include/asm-generic/bug.h | 1 + > include/linux/kasan.h | 12 +++++++++--- > lib/bug.c | 9 ++++++++- > mm/kasan/generic.c | 4 ++-- > mm/kasan/kasan.h | 2 +- > mm/kasan/report.c | 2 +- > 8 files changed, 51 insertions(+), 22 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,6 +2,8 @@ > #ifndef _ASM_X86_KASAN_H > #define _ASM_X86_KASAN_H > > +#include <asm/bug.h> > + > #include <linux/const.h> > #define KASAN_SHADOW_OFFSET _AC(CONFIG_KASAN_SHADOW_OFFSET, UL) > #define KASAN_SHADOW_SCALE_SHIFT 3 > @@ -26,8 +28,21 @@ > #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) > +{ > + 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)); Can BUG return? This should be able to return. We also have other tools coming (KMSAN/KTSAN) where distinction between fast path that does nothing and slower-paths are very blurred and there are dozens of them, I don't think this BUG thunk will be sustainable. What does BUG do what a normal call can't do? > + 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/generic.c > +++ b/mm/kasan/generic.c > @@ -228,7 +228,7 @@ void __asan_unregister_globals(struct ka > EXPORT_SYMBOL(__asan_unregister_globals); > > #define DEFINE_ASAN_LOAD_STORE(size) \ > - void __asan_load##size(unsigned long addr) \ > + notrace void __asan_load##size(unsigned long addr) \ We already have: CFLAGS_REMOVE_generic.o = -pg Doesn't it imply notrace for all functions? > { \ > check_memory_region_inline(addr, size, false, _RET_IP_);\ > } \ > @@ -236,7 +236,7 @@ EXPORT_SYMBOL(__asan_unregister_globals) > __alias(__asan_load##size) \ > void __asan_load##size##_noabort(unsigned long); \ > EXPORT_SYMBOL(__asan_load##size##_noabort); \ > - void __asan_store##size(unsigned long addr) \ > + notrace void __asan_store##size(unsigned long addr) \ > { \ > check_memory_region_inline(addr, size, true, _RET_IP_); \ > } \ > --- 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; > >