linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] arm64: enable per-task stack canaries
@ 2018-12-12 12:08 Ard Biesheuvel
  2018-12-12 17:50 ` Kees Cook
  0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2018-12-12 12:08 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, will.deacon, mark.rutland, catalin.marinas,
	Ramana.Radhakrishnan, linux-hardened, keescook, labbott,
	Ard Biesheuvel

This enables the use of per-task stack canary values if GCC has
support for emitting the stack canary reference relative to the
value of sp_el0, which holds the task struct pointer in the arm64
kernel.

The $(eval) extends KBUILD_CFLAGS at the moment the make rule is
applied, which means asm-offsets.o (which we rely on for the offset
value) is built without the arguments, and everything built afterwards
has the options set.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
v3: add all 3 options to the Kconfig cc-option check, to avoid relying on
    implementation defined logic in the compiler regarding which options
    need to appear together.

 arch/arm64/Kconfig                      |  7 +++++++
 arch/arm64/Makefile                     | 10 ++++++++++
 arch/arm64/include/asm/stackprotector.h |  3 ++-
 arch/arm64/kernel/asm-offsets.c         |  3 +++
 arch/arm64/kernel/process.c             |  2 +-
 5 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index ea2ab0330e3a..e355946cde97 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1272,6 +1272,13 @@ config RANDOMIZE_MODULE_REGION_FULL
 	  a limited range that contains the [_stext, _etext] interval of the
 	  core kernel, so branch relocations are always in range.
 
+config CC_HAVE_STACKPROTECTOR_SYSREG
+	def_bool $(cc-option,-mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=0)
+
+config STACKPROTECTOR_PER_TASK
+	def_bool y
+	depends on STACKPROTECTOR && CC_HAVE_STACKPROTECTOR_SYSREG
+
 endmenu
 
 menu "Boot options"
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 6cb9fc7e9382..79d927542322 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -56,6 +56,16 @@ KBUILD_AFLAGS	+= $(lseinstr) $(brokengasinst)
 KBUILD_CFLAGS	+= $(call cc-option,-mabi=lp64)
 KBUILD_AFLAGS	+= $(call cc-option,-mabi=lp64)
 
+ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
+prepare: stack_protector_prepare
+stack_protector_prepare: prepare0
+	$(eval KBUILD_CFLAGS += -mstack-protector-guard=sysreg		  \
+				-mstack-protector-guard-reg=sp_el0	  \
+				-mstack-protector-guard-offset=$(shell	  \
+			awk '{if ($$2 == "TSK_STACK_CANARY") print $$3;}' \
+					include/generated/asm-offsets.h))
+endif
+
 ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
 KBUILD_CPPFLAGS	+= -mbig-endian
 CHECKFLAGS	+= -D__AARCH64EB__
diff --git a/arch/arm64/include/asm/stackprotector.h b/arch/arm64/include/asm/stackprotector.h
index 58d15be11c4d..5884a2b02827 100644
--- a/arch/arm64/include/asm/stackprotector.h
+++ b/arch/arm64/include/asm/stackprotector.h
@@ -34,7 +34,8 @@ static __always_inline void boot_init_stack_canary(void)
 	canary &= CANARY_MASK;
 
 	current->stack_canary = canary;
-	__stack_chk_guard = current->stack_canary;
+	if (!IS_ENABLED(CONFIG_STACKPROTECTOR_PER_TASK))
+		__stack_chk_guard = current->stack_canary;
 }
 
 #endif	/* _ASM_STACKPROTECTOR_H */
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 323aeb5f2fe6..65b8afc84466 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -46,6 +46,9 @@ int main(void)
   DEFINE(TSK_TI_TTBR0,		offsetof(struct task_struct, thread_info.ttbr0));
 #endif
   DEFINE(TSK_STACK,		offsetof(struct task_struct, stack));
+#ifdef CONFIG_STACKPROTECTOR
+  DEFINE(TSK_STACK_CANARY,	offsetof(struct task_struct, stack_canary));
+#endif
   BLANK();
   DEFINE(THREAD_CPU_CONTEXT,	offsetof(struct task_struct, thread.cpu_context));
   BLANK();
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index d9a4c2d6dd8b..8a2d68f04e0d 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -59,7 +59,7 @@
 #include <asm/processor.h>
 #include <asm/stacktrace.h>
 
-#ifdef CONFIG_STACKPROTECTOR
+#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
 #include <linux/stackprotector.h>
 unsigned long __stack_chk_guard __read_mostly;
 EXPORT_SYMBOL(__stack_chk_guard);
-- 
2.19.2

s

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v3] arm64: enable per-task stack canaries
  2018-12-12 12:08 [PATCH v3] arm64: enable per-task stack canaries Ard Biesheuvel
@ 2018-12-12 17:50 ` Kees Cook
  2018-12-12 19:28   ` Will Deacon
  0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2018-12-12 17:50 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-arm-kernel, LKML, Will Deacon, Mark Rutland,
	Catalin Marinas, Ramana Radhakrishnan, linux-hardened,
	Laura Abbott

On Wed, Dec 12, 2018 at 4:08 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> This enables the use of per-task stack canary values if GCC has
> support for emitting the stack canary reference relative to the
> value of sp_el0, which holds the task struct pointer in the arm64
> kernel.
>
> The $(eval) extends KBUILD_CFLAGS at the moment the make rule is
> applied, which means asm-offsets.o (which we rely on for the offset
> value) is built without the arguments, and everything built afterwards
> has the options set.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> v3: add all 3 options to the Kconfig cc-option check, to avoid relying on
>     implementation defined logic in the compiler regarding which options
>     need to appear together.
>
>  arch/arm64/Kconfig                      |  7 +++++++
>  arch/arm64/Makefile                     | 10 ++++++++++
>  arch/arm64/include/asm/stackprotector.h |  3 ++-
>  arch/arm64/kernel/asm-offsets.c         |  3 +++
>  arch/arm64/kernel/process.c             |  2 +-
>  5 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index ea2ab0330e3a..e355946cde97 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1272,6 +1272,13 @@ config RANDOMIZE_MODULE_REGION_FULL
>           a limited range that contains the [_stext, _etext] interval of the
>           core kernel, so branch relocations are always in range.
>
> +config CC_HAVE_STACKPROTECTOR_SYSREG
> +       def_bool $(cc-option,-mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=0)
> +
> +config STACKPROTECTOR_PER_TASK
> +       def_bool y
> +       depends on STACKPROTECTOR && CC_HAVE_STACKPROTECTOR_SYSREG
> +
>  endmenu
>
>  menu "Boot options"
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 6cb9fc7e9382..79d927542322 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -56,6 +56,16 @@ KBUILD_AFLAGS        += $(lseinstr) $(brokengasinst)
>  KBUILD_CFLAGS  += $(call cc-option,-mabi=lp64)
>  KBUILD_AFLAGS  += $(call cc-option,-mabi=lp64)
>
> +ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
> +prepare: stack_protector_prepare
> +stack_protector_prepare: prepare0
> +       $(eval KBUILD_CFLAGS += -mstack-protector-guard=sysreg            \
> +                               -mstack-protector-guard-reg=sp_el0        \
> +                               -mstack-protector-guard-offset=$(shell    \
> +                       awk '{if ($$2 == "TSK_STACK_CANARY") print $$3;}' \
> +                                       include/generated/asm-offsets.h))
> +endif
> +
>  ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
>  KBUILD_CPPFLAGS        += -mbig-endian
>  CHECKFLAGS     += -D__AARCH64EB__
> diff --git a/arch/arm64/include/asm/stackprotector.h b/arch/arm64/include/asm/stackprotector.h
> index 58d15be11c4d..5884a2b02827 100644
> --- a/arch/arm64/include/asm/stackprotector.h
> +++ b/arch/arm64/include/asm/stackprotector.h
> @@ -34,7 +34,8 @@ static __always_inline void boot_init_stack_canary(void)
>         canary &= CANARY_MASK;
>
>         current->stack_canary = canary;
> -       __stack_chk_guard = current->stack_canary;
> +       if (!IS_ENABLED(CONFIG_STACKPROTECTOR_PER_TASK))
> +               __stack_chk_guard = current->stack_canary;
>  }
>
>  #endif /* _ASM_STACKPROTECTOR_H */
> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 323aeb5f2fe6..65b8afc84466 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -46,6 +46,9 @@ int main(void)
>    DEFINE(TSK_TI_TTBR0,         offsetof(struct task_struct, thread_info.ttbr0));
>  #endif
>    DEFINE(TSK_STACK,            offsetof(struct task_struct, stack));
> +#ifdef CONFIG_STACKPROTECTOR
> +  DEFINE(TSK_STACK_CANARY,     offsetof(struct task_struct, stack_canary));
> +#endif
>    BLANK();
>    DEFINE(THREAD_CPU_CONTEXT,   offsetof(struct task_struct, thread.cpu_context));
>    BLANK();
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index d9a4c2d6dd8b..8a2d68f04e0d 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -59,7 +59,7 @@
>  #include <asm/processor.h>
>  #include <asm/stacktrace.h>
>
> -#ifdef CONFIG_STACKPROTECTOR
> +#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
>  #include <linux/stackprotector.h>
>  unsigned long __stack_chk_guard __read_mostly;
>  EXPORT_SYMBOL(__stack_chk_guard);
> --
> 2.19.2
>
> s



-- 
Kees Cook

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v3] arm64: enable per-task stack canaries
  2018-12-12 17:50 ` Kees Cook
@ 2018-12-12 19:28   ` Will Deacon
  0 siblings, 0 replies; 3+ messages in thread
From: Will Deacon @ 2018-12-12 19:28 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ard Biesheuvel, linux-arm-kernel, LKML, Mark Rutland,
	Catalin Marinas, Ramana Radhakrishnan, linux-hardened,
	Laura Abbott

On Wed, Dec 12, 2018 at 09:50:37AM -0800, Kees Cook wrote:
> On Wed, Dec 12, 2018 at 4:08 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > This enables the use of per-task stack canary values if GCC has
> > support for emitting the stack canary reference relative to the
> > value of sp_el0, which holds the task struct pointer in the arm64
> > kernel.
> >
> > The $(eval) extends KBUILD_CFLAGS at the moment the make rule is
> > applied, which means asm-offsets.o (which we rely on for the offset
> > value) is built without the arguments, and everything built afterwards
> > has the options set.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>

I've bitten the bullet and queued this for 4.21. We can always revert it
if the compiler support falls apart, but at this point the worst I can see
happening is that we have to change the name of an option.

Will

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-12-12 19:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-12 12:08 [PATCH v3] arm64: enable per-task stack canaries Ard Biesheuvel
2018-12-12 17:50 ` Kees Cook
2018-12-12 19:28   ` Will Deacon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).