From mboxrd@z Thu Jan 1 00:00:00 1970 Reply-To: kernel-hardening@lists.openwall.com MIME-Version: 1.0 Sender: keescook@google.com In-Reply-To: <1476866702.21069.8.camel@cvidal.org> References: <1476802761-24340-1-git-send-email-colin@cvidal.org> <1476802761-24340-3-git-send-email-colin@cvidal.org> <1476866702.21069.8.camel@cvidal.org> From: Kees Cook Date: Wed, 19 Oct 2016 13:11:46 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: [kernel-hardening] Re: [RFC 2/2] arm: implementation for HARDENED_ATOMIC To: Colin Vidal Cc: "kernel-hardening@lists.openwall.com" , "Reshetova, Elena" , AKASHI Takahiro , David Windsor , Hans Liljestrand , Mark Rutland List-ID: On Wed, Oct 19, 2016 at 1:45 AM, Colin Vidal wrote: > Hi Kees, > >> > This adds arm-specific code in order to support HARDENED_ATOMIC >> > feature. When overflow is detected in atomic_t, atomic64_t or >> > atomic_long_t, an exception is raised and call >> > hardened_atomic_overflow. >> >> Can you include some notes that this was originally in PaX/grsecurity, >> and detail what is different from their implemention? > > Of course. I add it in the next version. > >> > Signed-off-by: Colin Vidal >> > --- >> > arch/arm/Kconfig | 1 + >> > arch/arm/include/asm/atomic.h | 434 +++++++++++++++++++++++++++++------------- >> > arch/arm/mm/fault.c | 15 ++ >> > 3 files changed, 320 insertions(+), 130 deletions(-) >> > >> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig >> > index b5d529f..fcf4a64 100644 >> > --- a/arch/arm/Kconfig >> > +++ b/arch/arm/Kconfig >> > @@ -36,6 +36,7 @@ config ARM >> > select HAVE_ARCH_AUDITSYSCALL if (AEABI && !OABI_COMPAT) >> > select HAVE_ARCH_BITREVERSE if (CPU_32v7M || CPU_32v7) && !CPU_32v6 >> > select HAVE_ARCH_HARDENED_USERCOPY >> > + select HAVE_ARCH_HARDENED_ATOMIC >> > select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU >> > select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32 && MMU >> > select HAVE_ARCH_MMAP_RND_BITS if MMU >> > diff --git a/arch/arm/include/asm/atomic.h b/arch/arm/include/asm/atomic.h >> > index 66d0e21..fdaee17 100644 >> > --- a/arch/arm/include/asm/atomic.h >> > +++ b/arch/arm/include/asm/atomic.h >> > @@ -17,18 +17,52 @@ >> > #include >> > #include >> > #include >> > +#include >> > >> > #define ATOMIC_INIT(i) { (i) } >> > >> > #ifdef __KERNEL__ >> > >> > +#ifdef CONFIG_HARDENED_ATOMIC >> > +#define HARDENED_ATOMIC_INSN "bkpt 0xf103" >> >> In PaX, I see a check for THUMB2 config: >> >> #ifdef CONFIG_THUMB2_KERNEL >> #define REFCOUNT_TRAP_INSN "bkpt 0xf1" >> #else >> #define REFCOUNT_TRAP_INSN "bkpt 0xf103" >> #endif >> >> That should probably stay unless I'm misunderstanding something. Also, >> for your new ISNS define name, I'd leave "TRAP" in the name, since >> that describes more clearly what it does. > > Oh yeah. I will add it. Actually I does not add it at first since I > does not really understand why there is a special case for Thumbs2 (as > far I understand, instructions size can also be 4 bytes). If ARM > experts are around, I would appreciate pointers about it :-) Cool. Perhaps Takahiro or Mark (now on CC) can comment on it. >> Beyond these things, it looks great to me -- though I'm hardly an ARM expert. :) >> >> Were you able to test on ARM with this for overflow with the lkdtm tests? > > Yep. I have to make more thorough tests, but things like > > echo HARDENED_ATOMIC_OVERFLOW > /provoke-crash/DIRECT > > raise the exception as well (with hardened message and stack trace). I > tested it with armv7/qemu. Great! -Kees -- Kees Cook Nexus Security