Linux-csky Archive on lore.kernel.org
 help / color / Atom feed
* [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	[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, back to index

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

Linux-csky Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-csky/0 linux-csky/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-csky linux-csky/ https://lore.kernel.org/linux-csky \
		linux-csky@vger.kernel.org
	public-inbox-index linux-csky

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-csky


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git