Message ID | 20181203170343.2602-1-ard.biesheuvel@linaro.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] arm64: enable per-task stack canaries | expand |
On Mon, Dec 3, 2018 at 9:04 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. Yay! (Should we include the gcc plugin version of this too, since it may be a while before gcc with this support is available widely?) > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > Note that the cc-option invocation below relies on the fact that Ramana's > current implementation of the GCC support permits -mstack-protector-guard=sysreg > to appear without defining the register name or offset. > > 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. Can we detect this from Kconfig instead and then remove the flag for the asm-offsets.o build step? -Kees > > arch/arm64/Kconfig | 5 +++++ > 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, 21 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index ea2ab0330e3a..0d7fb47fe5e1 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -1272,6 +1272,11 @@ 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 STACKPROTECTOR_PER_TASK > + def_bool y > + depends on STACKPROTECTOR > + depends on $(cc-option,-mstack-protector-guard=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 >
On Mon, 3 Dec 2018 at 21:54, Kees Cook <keescook@chromium.org> wrote: > > On Mon, Dec 3, 2018 at 9:04 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. > > Yay! > > (Should we include the gcc plugin version of this too, since it may be > a while before gcc with this support is available widely?) > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > Note that the cc-option invocation below relies on the fact that Ramana's > > current implementation of the GCC support permits -mstack-protector-guard=sysreg > > to appear without defining the register name or offset. > > > > 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. > > Can we detect this from Kconfig instead and then remove the flag for > the asm-offsets.o build step? > We need to know offsetof(struct task_struct, stack_canary) at Kconfig time, which could be affected by struct randomization as well, I suppose. So it is not clear to me how we could ever drop this step.
On Mon, 3 Dec 2018 at 21:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Mon, 3 Dec 2018 at 21:54, Kees Cook <keescook@chromium.org> wrote: > > > > On Mon, Dec 3, 2018 at 9:04 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. > > > > Yay! > > > > (Should we include the gcc plugin version of this too, since it may be > > a while before gcc with this support is available widely?) > > > > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > --- > > > Note that the cc-option invocation below relies on the fact that Ramana's > > > current implementation of the GCC support permits -mstack-protector-guard=sysreg > > > to appear without defining the register name or offset. > > > > > > 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. > > > > Can we detect this from Kconfig instead and then remove the flag for > > the asm-offsets.o build step? > > > > We need to know offsetof(struct task_struct, stack_canary) at Kconfig > time, which could be affected by struct randomization as well, I > suppose. So it is not clear to me how we could ever drop this step. Ah never mind. I see what you mean now.
On Mon, Dec 03, 2018 at 06:03:43PM +0100, Ard Biesheuvel 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. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > Note that the cc-option invocation below relies on the fact that Ramana's > current implementation of the GCC support permits -mstack-protector-guard=sysreg > to appear without defining the register name or offset. > > 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. > > arch/arm64/Kconfig | 5 +++++ > 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, 21 insertions(+), 2 deletions(-) This looks really good to me, but I'm not sure what we should do next. Ramana -- is your implementation stable, or are we likely to see changes to the way the options are passed? Will
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index ea2ab0330e3a..0d7fb47fe5e1 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1272,6 +1272,11 @@ 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 STACKPROTECTOR_PER_TASK + def_bool y + depends on STACKPROTECTOR + depends on $(cc-option,-mstack-protector-guard=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);
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. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- Note that the cc-option invocation below relies on the fact that Ramana's current implementation of the GCC support permits -mstack-protector-guard=sysreg to appear without defining the register name or offset. 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. arch/arm64/Kconfig | 5 +++++ 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, 21 insertions(+), 2 deletions(-)