On Mon, Mar 5, 2018 at 3:51 PM, Mark Rutland <mark.rutland@xxxxxxx> 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.
>
> 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 <linux/sizes.h>
>> #include <linux/syscalls.h>
>> #include <linux/mm_types.h>
>> +#include <linux/kasan.h>
>>
>> #include <asm/atomic.h>
>> #include <asm/bug.h>
>> @@ -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.
