From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot0-f200.google.com (mail-ot0-f200.google.com [74.125.82.200]) by kanga.kvack.org (Postfix) with ESMTP id CC1636B0006 for ; Mon, 5 Mar 2018 09:51:24 -0500 (EST) Received: by mail-ot0-f200.google.com with SMTP id i28so3260521otf.21 for ; Mon, 05 Mar 2018 06:51:24 -0800 (PST) Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com. [217.140.101.70]) by mx.google.com with ESMTP id 145si3709693oia.79.2018.03.05.06.51.23 for ; Mon, 05 Mar 2018 06:51:23 -0800 (PST) Date: Mon, 5 Mar 2018 14:51:12 +0000 From: Mark Rutland Subject: Re: [RFC PATCH 11/14] khwasan: add brk handler for inline instrumentation Message-ID: <20180305145111.bbycnzpgzkir2dz4@lakrids.cambridge.arm.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Andrey Konovalov Cc: 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 , Ard Biesheuvel , Yury Norov , Nick Desaulniers , Marc Zyngier , Bob Picco , 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@googlegroups.com, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-ext4@vger.kernel.org, linux-sparse@vger.kernel.org, linux-mm@kvack.org, linux-kbuild@vger.kernel.org, Kostya Serebryany , Evgeniy Stepanov , Lee Smith , Ramana Radhakrishnan , Jacob Bramley , Ruben Ayrapetyan , Kees Cook , Jann Horn , Mark Brand 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? 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. How much does this save, compared to having a callback? > 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) > + 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()? > + > + /* 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). Thanks, Mark. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org