Message-ID: <CAErMHp_zm2UbUyd=E3gKOLUbjq+t+gnphUsxbUYjCMqOQN86qA@mail.gmail.com>
Date:   Wed, 3 Jan 2018 22:36:44 +0900
From:   Jinbum Park <jinb.park7@...il.com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Dave Martin <Dave.Martin@....com>
Cc:     linux-arm-kernel@...ts.infradead.org,
        LKML <linux-kernel@...r.kernel.org>,
        Kernel Hardening <kernel-hardening@...ts.openwall.com>,
        Russell King <linux@...linux.org.uk>,
        Will Deacon <will.deacon@....com>,
        Peter Zijlstra <peterz@...radead.org>, boqun.feng@...il.com,
        Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Nicolas Pitre <nicolas.pitre@...aro.org>, mickael.guene@...com,
        Kees Cook <keescook@...omium.org>,
        Laura Abbott <labbott@...hat.com>
Subject: Re: [kernel-hardening] [PATCH] arm: kernel: implement fast refcount checking

>> This is a nice result. However, without any insight into the presence
>> of actual refcount hot spots, it is not obvious that we need this
>> patch. This is the reason we ended up enabling CONFIG_REFCOUNT_FULL
>> for arm64. I will let others comment on whether we want this patch in
>> the first place,

Dear Ard, Dave,

I wanna hear some comment on above point.
Is CONFIG_REFCOUNT_FULL much better for arm?
If it is, I don't need to prepare v2 patch. (then, just needed to add
"select REFCOUNT_FULL")



2017-12-21 18:20 GMT+09:00 Ard Biesheuvel <ard.biesheuvel@...aro.org>:
> (add Dave)
>
> On 21 December 2017 at 09:18, Ard Biesheuvel <ard.biesheuvel@...aro.org> wrote:
>> On 21 December 2017 at 07:50, Jinbum Park <jinb.park7@...il.com> wrote:
>>> This adds support to arm for fast refcount checking.
>>> It's heavily based on x86, arm64 implementation.
>>> (7a46ec0e2f48 ("locking/refcounts,
>>> x86/asm: Implement fast refcount overflow protection)
>>>
>>> This doesn't support under-ARMv6, thumb2-kernel yet.
>>>
>>> Test result of this patch is as follows.
>>>
>>> 1. LKDTM test
>>>
>>> - All REFCOUNT_* tests in LKDTM have passed.
>>>
>>> 2. Performance test
>>>
>>> - Cortex-A7, Quad Core, 450 MHz
>>> - Case with CONFIG_ARCH_HAS_REFCOUNT,
>>>
>>> perf stat -B -- echo ATOMIC_TIMING \
>>>                 >/sys/kernel/debug/provoke-crash/DIRECT
>>> 204.364247057 seconds time elapsed
>>>
>>> perf stat -B -- echo REFCOUNT_TIMING \
>>>                 >/sys/kernel/debug/provoke-crash/DIRECT
>>> 208.006062212 seconds time elapsed
>>>
>>> - Case with CONFIG_REFCOUNT_FULL,
>>>
>>> perf stat -B -- echo REFCOUNT_TIMING \
>>>                 >/sys/kernel/debug/provoke-crash/DIRECT
>>> 369.256523453 seconds time elapsed
>>>
>>
>> This is a nice result. However, without any insight into the presence
>> of actual refcount hot spots, it is not obvious that we need this
>> patch. This is the reason we ended up enabling CONFIG_REFCOUNT_FULL
>> for arm64. I will let others comment on whether we want this patch in
>> the first place, and I will give some feedback on the implementation
>> below.
>>
>>> Signed-off-by: Jinbum Park <jinb.park7@...il.com>
>>> ---
>>>  arch/arm/Kconfig                |  1 +
>>>  arch/arm/include/asm/atomic.h   | 82 +++++++++++++++++++++++++++++++++++++++++
>>>  arch/arm/include/asm/refcount.h | 55 +++++++++++++++++++++++++++
>>>  arch/arm/kernel/traps.c         | 44 ++++++++++++++++++++++
>>>  4 files changed, 182 insertions(+)
>>>  create mode 100644 arch/arm/include/asm/refcount.h
>>>
>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>>> index 3ea00d6..e07b986 100644
>>> --- a/arch/arm/Kconfig
>>> +++ b/arch/arm/Kconfig
>>> @@ -7,6 +7,7 @@ config ARM
>>>         select ARCH_HAS_DEBUG_VIRTUAL
>>>         select ARCH_HAS_DEVMEM_IS_ALLOWED
>>>         select ARCH_HAS_ELF_RANDOMIZE
>>> +       select ARCH_HAS_REFCOUNT if __LINUX_ARM_ARCH__ >= 6 && (!THUMB2_KERNEL)
>>>         select ARCH_HAS_SET_MEMORY
>>>         select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
>>>         select ARCH_HAS_STRICT_MODULE_RWX if MMU
>>> diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h
>>> index 66d0e21..b203396 100644
>>> --- a/arch/arm/include/asm/atomic.h
>>> +++ b/arch/arm/include/asm/atomic.h
>>> @@ -17,6 +17,7 @@
>>>  #include <linux/irqflags.h>
>>>  #include <asm/barrier.h>
>>>  #include <asm/cmpxchg.h>
>>> +#include <asm/opcodes.h>
>>>
>>>  #define ATOMIC_INIT(i) { (i) }
>>>
>>> @@ -32,6 +33,87 @@
>>>
>>>  #if __LINUX_ARM_ARCH__ >= 6
>>>
>>> +#ifdef CONFIG_ARCH_HAS_REFCOUNT
>>> +
>>> +#define REFCOUNT_ARM_BKPT_INSTR 0xfff001fc
>>> +
>>> +/*
>>> + * 1) encode call site that detect overflow in dummy b instruction.
>>> + * 2) overflow handler can decode dummy b, and get call site.
>>> + * 3) "(call site + 4) == strex" is always true.
>>> + * 4) the handler can get counter address via decoding strex.
>>> + */
>>> +#define REFCOUNT_TRIGGER_BKPT \
>>> +       __inst_arm(REFCOUNT_ARM_BKPT_INSTR) \
>>> +"      b       22b\n"
>>
>> In my arm64 implementation, I used a cbz instruction so I could encode
>> both a register number and a relative offset easily. In your case, you
>> only need to encode the offset, so it is much better to use '.long 22b
>> - .' instead.
>>
>>> +
>>> +/* If bkpt is triggered, skip strex routines after handling overflow */
>>> +#define REFCOUNT_CHECK_TAIL \
>>> +"      strex   %1, %0, [%3]\n" \
>>> +"      teq     %1, #0\n" \
>>> +"      bne     1b\n" \
>>> +"      .subsection     1\n" \
>>> +"33:\n" \
>>> +       REFCOUNT_TRIGGER_BKPT \
>>> +"      .previous\n"
>>> +
>>> +#define REFCOUNT_POST_CHECK_NEG \
>>> +"22:   bmi       33f\n" \
>>
>> It may be better to put the label on the 'strex' instruction directly,
>> to make things less confusing.
>>
>>> +       REFCOUNT_CHECK_TAIL
>>> +
>>> +#define REFCOUNT_POST_CHECK_NEG_OR_ZERO \
>>> +"      beq     33f\n" \
>>> +       REFCOUNT_POST_CHECK_NEG
>>> +
>>> +#define REFCOUNT_SMP_MB smp_mb()
>>> +#define REFCOUNT_SMP_NONE_MB
>>> +
>>> +#define REFCOUNT_OP(op, c_op, asm_op, post, mb) \
>>> +static inline int __refcount_##op(int i, atomic_t *v) \
>>> +{ \
>>> +       unsigned long tmp; \
>>> +       int result; \
>>> +\
>>> +       REFCOUNT_SMP_ ## mb; \
>>> +       prefetchw(&v->counter); \
>>> +       __asm__ __volatile__("@ __refcount_" #op "\n" \
>>> +"1:    ldrex   %0, [%3]\n" \
>>> +"      " #asm_op "     %0, %0, %4\n" \
>>> +       REFCOUNT_POST_CHECK_ ## post \
>>> +       : "=&r" (result), "=&r" (tmp), "+Qo" (v->counter) \
>>> +       : "r" (&v->counter), "Ir" (i) \
>>> +       : "cc"); \
>>> +\
>>> +       REFCOUNT_SMP_ ## mb; \
>>> +       return result; \
>>> +} \
>>> +
>>> +REFCOUNT_OP(add_lt, +=, adds, NEG_OR_ZERO, NONE_MB);
>>> +REFCOUNT_OP(sub_lt, -=, subs, NEG, MB);
>>> +REFCOUNT_OP(sub_le, -=, subs, NEG_OR_ZERO, NONE_MB);
>>> +
>>> +static inline int __refcount_add_not_zero(int i, atomic_t *v)
>>> +{
>>> +       unsigned long tmp;
>>> +       int result;
>>> +
>>> +       prefetchw(&v->counter);
>>> +       __asm__ __volatile__("@ __refcount_add_not_zero\n"
>>> +"1:    ldrex   %0, [%3]\n"
>>> +"      teq             %0, #0\n"
>>> +"      beq             2f\n"
>>> +"      adds    %0, %0, %4\n"
>>> +       REFCOUNT_POST_CHECK_NEG
>>> +"2:"
>>> +       : "=&r" (result), "=&r" (tmp), "+Qo" (v->counter)
>>> +       : "r" (&v->counter), "Ir" (i)
>>> +       : "cc");
>>> +
>>> +       return result;
>>> +}
>>> +
>>> +#endif /* CONFIG_ARCH_HAS_REFCOUNT */
>>> +
>>>  /*
>>>   * ARMv6 UP and SMP safe atomic ops.  We use load exclusive and
>>>   * store exclusive to ensure that these are atomic.  We may loop
>>> diff --git a/arch/arm/include/asm/refcount.h b/arch/arm/include/asm/refcount.h
>>> new file mode 100644
>>> index 0000000..300a2d5
>>> --- /dev/null
>>> +++ b/arch/arm/include/asm/refcount.h
>>> @@ -0,0 +1,55 @@
>>> +/*
>>> + * arm-specific implementation of refcount_t. Based on x86 version and
>>> + * PAX_REFCOUNT from PaX/grsecurity.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#ifndef __ASM_REFCOUNT_H
>>> +#define __ASM_REFCOUNT_H
>>> +
>>> +#include <linux/refcount.h>
>>> +
>>> +#include <asm/atomic.h>
>>> +#include <asm/uaccess.h>
>>> +
>>> +static __always_inline void refcount_add(int i, refcount_t *r)
>>> +{
>>> +       __refcount_add_lt(i, &r->refs);
>>> +}
>>> +
>>> +static __always_inline void refcount_inc(refcount_t *r)
>>> +{
>>> +       __refcount_add_lt(1, &r->refs);
>>> +}
>>> +
>>> +static __always_inline void refcount_dec(refcount_t *r)
>>> +{
>>> +       __refcount_sub_le(1, &r->refs);
>>> +}
>>> +
>>> +static __always_inline __must_check bool refcount_sub_and_test(unsigned int i,
>>> +                                                               refcount_t *r)
>>> +{
>>> +       return __refcount_sub_lt(i, &r->refs) == 0;
>>> +}
>>> +
>>> +static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
>>> +{
>>> +       return __refcount_sub_lt(1, &r->refs) == 0;
>>> +}
>>> +
>>> +static __always_inline __must_check bool refcount_add_not_zero(unsigned int i,
>>> +                                                               refcount_t *r)
>>> +{
>>> +       return __refcount_add_not_zero(i, &r->refs) != 0;
>>> +}
>>> +
>>> +static __always_inline __must_check bool refcount_inc_not_zero(refcount_t *r)
>>> +{
>>> +       return __refcount_add_not_zero(1, &r->refs) != 0;
>>> +}
>>> +
>>> +#endif
>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>>> index 5cf0488..a309215 100644
>>> --- a/arch/arm/kernel/traps.c
>>> +++ b/arch/arm/kernel/traps.c
>>> @@ -795,8 +795,52 @@ void abort(void)
>>>  }
>>>  EXPORT_SYMBOL(abort);
>>>
>>> +#ifdef CONFIG_ARCH_HAS_REFCOUNT
>>> +
>>> +static int refcount_overflow_handler(struct pt_regs *regs, unsigned int instr)
>>> +{
>>> +       u32 dummy_b = le32_to_cpup((__le32 *)(regs->ARM_pc + 4));
>>> +       u32 strex;
>>> +       u32 rt;
>>> +       bool zero = regs->ARM_cpsr & PSR_Z_BIT;
>>> +
>>> +       /* point pc to the branch instruction that detected the overflow */
>>> +       regs->ARM_pc += 4 + (((s32)dummy_b << 8) >> 6) + 8;
>>> +
>>
>> This would become something like
>>
>> s32 offset = *(s32 *)(regs->ARM_pc + 4);
>>
>> /* point pc to the strex instruction that will overflow the refcount */
>> regs->ARM_pc += offset + 4;
>>
>>
>>> +       /* decode strex to set refcount */
>>> +       strex = le32_to_cpup((__le32 *)(regs->ARM_pc + 4));
>>> +       rt = (strex << 12) >> 28;
>>> +
>>
>> Don't we have better ways to decode an instruction? Also, could you
>> add a Thumb2 variant here? (and for the breakpoint instruction)
>>
>>
>>> +       /* unconditionally saturate the refcount */
>>> +       *(int *)regs->uregs[rt] = INT_MIN / 2;
>>> +
>>> +       /* report error */
>>> +       refcount_error_report(regs, zero ? "hit zero" : "overflow");
>>> +
>>> +       /* advance pc and proceed, skip "strex" routine */
>>> +       regs->ARM_pc += 16;
>>
>> Please use a macro here to clarify that you are skipping the remaining
>> instructions in REFCOUNT_CHECK_TAIL
>>
>>> +       return 0;
>>> +}
>>> +
>>> +static struct undef_hook refcount_break_hook = {
>>> +       .instr_mask     = 0xffffffff,
>>> +       .instr_val      = REFCOUNT_ARM_BKPT_INSTR,
>>> +       .cpsr_mask      = 0,
>>> +       .cpsr_val       = 0,
>>> +       .fn             = refcount_overflow_handler,
>>> +};
>>> +
>>> +#define register_refcount_break_hook() register_undef_hook(&refcount_break_hook)
>>> +
>>> +#else /* !CONFIG_ARCH_HAS_REFCOUNT */
>>> +
>>> +#define register_refcount_break_hook()
>>> +
>>> +#endif /* CONFIG_ARCH_HAS_REFCOUNT */
>>> +
>>>  void __init trap_init(void)
>>>  {
>>> +       register_refcount_break_hook();
>>>         return;
>>>  }
>>>
>>> --
>>> 1.9.1
>>>

Powered by blists - more mailing lists