Re: [RFC PATCH 11/14] khwasan: add brk handler for inline instrumentation

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 23 March 2018 at 15:59, Andrey Konovalov <andreyknvl@google.com> wrote:
> On Mon, Mar 5, 2018 at 3:51 PM, Mark Rutland <mark.rutland@arm.com> 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 <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.