* [RFC PATCH] riscv: enable per-task stack canaries
@ 2020-07-05 14:13 guoren
2020-07-05 20:40 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: guoren @ 2020-07-05 14:13 UTC (permalink / raw)
To: guoren; +Cc: linux-riscv, linux-kernel, linux-csky, Guo Ren, Kees Cook
From: Guo Ren <guoren@linux.alibaba.com>
After compare arm64 and x86 implementations, seems arm64's is more
flexible and readable. The key point is how gcc get the offset of
stack_canary from gs/el0_sp.
x86: Use a fix offset from gs, not flexible.
struct fixed_percpu_data {
/*
* GCC hardcodes the stack canary as %gs:40. Since the
* irq_stack is the object at %gs:0, we reserve the bottom
* 48 bytes of the irq stack for the canary.
*/
char gs_base[40]; // :(
unsigned long stack_canary;
};
arm64: Use -mstack-protector-guard-offset & guard-reg
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
I prefer arm64, but x86 percpu_data design needs to be considered ?
After the discussion, let's continue the work for riscv gcc
stack-protector.
Here is arm64 gcc's work [1].
[1] https://github.com/gcc-mirror/gcc/commit/cd0b2d361df82c848dc7e1c3078651bb0624c3c6
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Cc: Kees Cook <keescook@chromium.org>
---
arch/riscv/Kconfig | 7 +++++++
arch/riscv/Makefile | 10 ++++++++++
arch/riscv/include/asm/stackprotector.h | 3 ++-
arch/riscv/kernel/asm-offsets.c | 3 +++
arch/riscv/kernel/process.c | 2 +-
5 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 4b0e308..4b4e833 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -394,6 +394,13 @@ config CMDLINE_FORCE
endchoice
+config CC_HAVE_STACKPROTECTOR_SYSREG
+ def_bool $(cc-option,-mstack-protector-guard=gpr -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
+
+config STACKPROTECTOR_PER_TASK
+ def_bool y
+ depends on STACKPROTECTOR && CC_HAVE_STACKPROTECTOR_SYSREG
+
endmenu
config BUILTIN_DTB
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index fb6e37d..880a288 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -47,6 +47,16 @@ KBUILD_AFLAGS += -march=$(riscv-march-y)
KBUILD_CFLAGS += -mno-save-restore
KBUILD_CFLAGS += -DCONFIG_PAGE_OFFSET=$(CONFIG_PAGE_OFFSET)
+ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
+prepare: stack_protector_prepare
+stack_protector_prepare: prepare0
+ $(eval KBUILD_CFLAGS += -mstack-protector-guard=gpr \
+ -mstack-protector-guard-reg=tp \
+ -mstack-protector-guard-offset=$(shell \
+ awk '{if ($$2 == "TSK_STACK_CANARY") print $$3;}' \
+ include/generated/asm-offsets.h))
+endif
+
ifeq ($(CONFIG_CMODEL_MEDLOW),y)
KBUILD_CFLAGS += -mcmodel=medlow
endif
diff --git a/arch/riscv/include/asm/stackprotector.h b/arch/riscv/include/asm/stackprotector.h
index 5962f88..09093af 100644
--- a/arch/riscv/include/asm/stackprotector.h
+++ b/arch/riscv/include/asm/stackprotector.h
@@ -24,6 +24,7 @@ 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_RISCV_STACKPROTECTOR_H */
diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index 07cb9c1..999b465 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -29,6 +29,9 @@ void asm_offsets(void)
OFFSET(TASK_THREAD_S11, task_struct, thread.s[11]);
OFFSET(TASK_THREAD_SP, task_struct, thread.sp);
OFFSET(TASK_STACK, task_struct, stack);
+#ifdef CONFIG_STACKPROTECTOR
+ OFFSET(TSK_STACK_CANARY, task_struct, stack_canary);
+#endif
OFFSET(TASK_TI, task_struct, thread_info);
OFFSET(TASK_TI_FLAGS, task_struct, thread_info.flags);
OFFSET(TASK_TI_PREEMPT_COUNT, task_struct, thread_info.preempt_count);
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 6548929..cb4ac65 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -24,7 +24,7 @@
register unsigned long gp_in_global __asm__("gp");
-#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.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] riscv: enable per-task stack canaries
2020-07-05 14:13 [RFC PATCH] riscv: enable per-task stack canaries guoren
@ 2020-07-05 20:40 ` Kees Cook
2020-07-06 1:01 ` Guo Ren
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2020-07-05 20:40 UTC (permalink / raw)
To: guoren
Cc: linux-riscv, linux-kernel, linux-csky, Guo Ren, Ramana Radhakrishnan
On Sun, Jul 05, 2020 at 02:13:17PM +0000, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
>
> After compare arm64 and x86 implementations, seems arm64's is more
> flexible and readable. The key point is how gcc get the offset of
> stack_canary from gs/el0_sp.
>
> x86: Use a fix offset from gs, not flexible.
>
> struct fixed_percpu_data {
> /*
> * GCC hardcodes the stack canary as %gs:40. Since the
> * irq_stack is the object at %gs:0, we reserve the bottom
> * 48 bytes of the irq stack for the canary.
> */
> char gs_base[40]; // :(
> unsigned long stack_canary;
> };
Yes, x86's compiler's implementation of "thread local" stack canary
isn't great for the kernel.
> arm64: Use -mstack-protector-guard-offset & guard-reg
>
> 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
>
> I prefer arm64, but x86 percpu_data design needs to be considered ?
I don't know riscv internals, so I leave that to y'all! :)
> After the discussion, let's continue the work for riscv gcc
> stack-protector.
I think you'll need some buy-in from GCC before this kernel patch can
land.
> Here is arm64 gcc's work [1].
>
> [1] https://github.com/gcc-mirror/gcc/commit/cd0b2d361df82c848dc7e1c3078651bb0624c3c6
Can this kind of thing be made general-purposes, instead of having to
reimplement it each time there's a new arch wanting to do it?
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Cc: Kees Cook <keescook@chromium.org>
> ---
> arch/riscv/Kconfig | 7 +++++++
> arch/riscv/Makefile | 10 ++++++++++
> arch/riscv/include/asm/stackprotector.h | 3 ++-
> arch/riscv/kernel/asm-offsets.c | 3 +++
> arch/riscv/kernel/process.c | 2 +-
> 5 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 4b0e308..4b4e833 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -394,6 +394,13 @@ config CMDLINE_FORCE
>
> endchoice
>
> +config CC_HAVE_STACKPROTECTOR_SYSREG
> + def_bool $(cc-option,-mstack-protector-guard=gpr -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
And, as I'm sure you realize, it's not supported by the riscv backend
yet:
riscv64-unknown-linux-gnu-gcc: error: unrecognized command line option '-mstack-protector-guard=gpr'; did you mean '-fstack-protector-strong'?
riscv64-unknown-linux-gnu-gcc: error: unrecognized command line option '-mstack-protector-guard-reg=tp'; did you mean '-fstack-protector-strong'?
riscv64-unknown-linux-gnu-gcc: error: unrecognized command line option '-mstack-protector-guard-offset=0'; did you mean '-fstack-protector-strong'?
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] riscv: enable per-task stack canaries
2020-07-05 20:40 ` Kees Cook
@ 2020-07-06 1:01 ` Guo Ren
2020-07-06 1:21 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Guo Ren @ 2020-07-06 1:01 UTC (permalink / raw)
To: Kees Cook
Cc: linux-riscv, Linux Kernel Mailing List, linux-csky, Guo Ren,
Ramana Radhakrishnan
On Mon, Jul 6, 2020 at 4:40 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Sun, Jul 05, 2020 at 02:13:17PM +0000, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > After compare arm64 and x86 implementations, seems arm64's is more
> > flexible and readable. The key point is how gcc get the offset of
> > stack_canary from gs/el0_sp.
> >
> > x86: Use a fix offset from gs, not flexible.
> >
> > struct fixed_percpu_data {
> > /*
> > * GCC hardcodes the stack canary as %gs:40. Since the
> > * irq_stack is the object at %gs:0, we reserve the bottom
> > * 48 bytes of the irq stack for the canary.
> > */
> > char gs_base[40]; // :(
> > unsigned long stack_canary;
> > };
>
> Yes, x86's compiler's implementation of "thread local" stack canary
> isn't great for the kernel.
>
> > arm64: Use -mstack-protector-guard-offset & guard-reg
> >
> > 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
> >
> > I prefer arm64, but x86 percpu_data design needs to be considered ?
>
> I don't know riscv internals, so I leave that to y'all! :)
>
> > After the discussion, let's continue the work for riscv gcc
> > stack-protector.
>
> I think you'll need some buy-in from GCC before this kernel patch can
> land.
exactly!
>
> > Here is arm64 gcc's work [1].
> >
> > [1] https://github.com/gcc-mirror/gcc/commit/cd0b2d361df82c848dc7e1c3078651bb0624c3c6
>
> Can this kind of thing be made general-purposes, instead of having to
> reimplement it each time there's a new arch wanting to do it?
Great idea. Now only x86 arm64 support, It's the right time point.
>
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > ---
> > arch/riscv/Kconfig | 7 +++++++
> > arch/riscv/Makefile | 10 ++++++++++
> > arch/riscv/include/asm/stackprotector.h | 3 ++-
> > arch/riscv/kernel/asm-offsets.c | 3 +++
> > arch/riscv/kernel/process.c | 2 +-
> > 5 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 4b0e308..4b4e833 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -394,6 +394,13 @@ config CMDLINE_FORCE
> >
> > endchoice
> >
> > +config CC_HAVE_STACKPROTECTOR_SYSREG
Should change to CC_HAVE_STACKPROTECTOR_GPR
> > + def_bool $(cc-option,-mstack-protector-guard=gpr -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0)
>
> And, as I'm sure you realize, it's not supported by the riscv backend
> yet:
>
> riscv64-unknown-linux-gnu-gcc: error: unrecognized command line option '-mstack-protector-guard=gpr'; did you mean '-fstack-protector-strong'?
> riscv64-unknown-linux-gnu-gcc: error: unrecognized command line option '-mstack-protector-guard-reg=tp'; did you mean '-fstack-protector-strong'?
> riscv64-unknown-linux-gnu-gcc: error: unrecognized command line option '-mstack-protector-guard-offset=0'; did you mean '-fstack-protector-strong'?
Yeah! :) I just want to show you, how about the format: use tp in gpr
to do that. The format is similar to arm64.
tp is the task_struct point in riscv.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] riscv: enable per-task stack canaries
2020-07-06 1:01 ` Guo Ren
@ 2020-07-06 1:21 ` Kees Cook
2020-07-06 2:42 ` Guo Ren
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2020-07-06 1:21 UTC (permalink / raw)
To: Guo Ren
Cc: linux-riscv, Linux Kernel Mailing List, linux-csky, Guo Ren,
Ramana Radhakrishnan
On Mon, Jul 06, 2020 at 09:01:51AM +0800, Guo Ren wrote:
> Yeah! :) I just want to show you, how about the format: use tp in gpr
> to do that. The format is similar to arm64.
>
> tp is the task_struct point in riscv.
Sounds good to me, yes. Thanks! Is there anyone looking at the GCC and
Clang sides of this?
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] riscv: enable per-task stack canaries
2020-07-06 1:21 ` Kees Cook
@ 2020-07-06 2:42 ` Guo Ren
0 siblings, 0 replies; 5+ messages in thread
From: Guo Ren @ 2020-07-06 2:42 UTC (permalink / raw)
To: Kees Cook
Cc: linux-riscv, Linux Kernel Mailing List, linux-csky, Guo Ren,
Ramana Radhakrishnan
On Mon, Jul 6, 2020 at 9:21 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Jul 06, 2020 at 09:01:51AM +0800, Guo Ren wrote:
> > Yeah! :) I just want to show you, how about the format: use tp in gpr
> > to do that. The format is similar to arm64.
> >
> > tp is the task_struct point in riscv.
>
> Sounds good to me, yes. Thanks! Is there anyone looking at the GCC and
> Clang sides of this?
My colleague is working on gcc's.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-06 2:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-05 14:13 [RFC PATCH] riscv: enable per-task stack canaries guoren
2020-07-05 20:40 ` Kees Cook
2020-07-06 1:01 ` Guo Ren
2020-07-06 1:21 ` Kees Cook
2020-07-06 2:42 ` Guo Ren
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).