From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f71.google.com (mail-it0-f71.google.com [209.85.214.71]) by kanga.kvack.org (Postfix) with ESMTP id 114D66B000C for ; Fri, 23 Mar 2018 23:42:19 -0400 (EDT) Received: by mail-it0-f71.google.com with SMTP id p203-v6so3648457itc.1 for ; Fri, 23 Mar 2018 20:42:19 -0700 (PDT) Received: from mail-sor-f65.google.com (mail-sor-f65.google.com. [209.85.220.65]) by mx.google.com with SMTPS id i66-v6sor3325133itg.118.2018.03.23.20.42.17 for (Google Transport Security); Fri, 23 Mar 2018 20:42:17 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20180305145111.bbycnzpgzkir2dz4@lakrids.cambridge.arm.com> From: Ard Biesheuvel Date: Sat, 24 Mar 2018 03:42:16 +0000 Message-ID: Subject: Re: [RFC PATCH 11/14] khwasan: add brk handler for inline instrumentation Content-Type: text/plain; charset="UTF-8" Sender: owner-linux-mm@kvack.org List-ID: To: Andrey Konovalov Cc: Mark Rutland , Andrey Ryabinin , Alexander Potapenko , Dmitry Vyukov , Jonathan Corbet , Catalin Marinas , Will Deacon , Theodore Ts'o , Jan Kara , Christopher Li , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Masahiro Yamada , Michal Marek , Yury Norov , Nick Desaulniers , Marc Zyngier , Suzuki K Poulose , Kristina Martsenko , Punit Agrawal , Dave Martin , James Morse , Julien Thierry , Michael Weiser , Steve Capper , Ingo Molnar , Thomas Gleixner , Sandipan Das , Paul Lawrence , David Woodhouse , Kees Cook , Geert Uytterhoeven , Josh Poimboeuf , Arnd Bergmann , kasan-dev , Linux Doc Mailing List , LKML , Linux ARM , linux-ext4@vger.kernel.org, Linux-Sparse , Linux Memory Management List , Linux Kbuild mailing list , Kostya Serebryany , Evgeniy Stepanov , Lee Smith , Ramana Radhakrishnan , Jacob Bramley , Ruben Ayrapetyan , Kees Cook , Jann Horn , Mark Brand On 23 March 2018 at 15:59, Andrey Konovalov wrote: > On Mon, Mar 5, 2018 at 3:51 PM, Mark Rutland wrote: >> On Fri, Mar 02, 2018 at 08:44:30PM +0100, Andrey Konovalov wrote: >>> 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. > >> >> 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. >> GCC already generates traps (translating to BRKs in the arm64 world) for other things like integer divide by zero and NULL dereferences. (Arnd may know more, I know he has looked into this in the past.) So we should probably implement a BRK handler for compiler generated traps and reserve it in the brk space, given that this behavior is not specific to khwasan >> 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). > >> >>> 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(+) >>> >>> diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h >>> index ed693c5bcec0..e4a7013321dc 100644 >>> --- a/arch/arm64/include/asm/brk-imm.h >>> +++ b/arch/arm64/include/asm/brk-imm.h >>> @@ -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 >>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c >>> index eb2d15147e8d..5df8cdf5af13 100644 >>> --- a/arch/arm64/kernel/traps.c >>> +++ b/arch/arm64/kernel/traps.c >>> @@ -35,6 +35,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> #include >>> @@ -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 >> >>> + 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! > >> >>> + 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. > >> >>> + >>> + /* 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! > >> >> Thanks, >> Mark.