Re: [PATCH v3] arm64: enable per-task stack canaries

From: Kees Cook
Date: Wed Dec 12 2018 - 12:50:55 EST


On Wed, Dec 12, 2018 at 4:08 AM Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> 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@xxxxxxxxxx>

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

-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