Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 18 Oct 2016 14:29:15 -0700
From: Kees Cook <keescook@...omium.org>
To: Colin Vidal <colin@...dal.org>
Cc: "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, 
	"Reshetova, Elena" <elena.reshetova@...el.com>, AKASHI Takahiro <takahiro.akashi@...aro.org>, 
	David Windsor <dave@...gbits.org>, Hans Liljestrand <ishkamiel@...il.com>
Subject: Re: [RFC 2/2] arm: implementation for HARDENED_ATOMIC

On Tue, Oct 18, 2016 at 7:59 AM, Colin Vidal <colin@...dal.org> wrote:
> 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?

>
> Signed-off-by: Colin Vidal <colin@...dal.org>
> ---
>  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 <linux/irqflags.h>
>  #include <asm/barrier.h>
>  #include <asm/cmpxchg.h>
> +#include <linux/bug.h>
>
>  #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.

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?

-Kees

-- 
Kees Cook
Nexus Security

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.