[v3] arm64: enable per-task stack canaries

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(-)

-- 
2.19.2

s

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

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