riscv: enable per-task stack canaries

RFC PATCH Superseded [email protected] 2 participants 5 messages Jul 5, 2020 – Jul 6, 2020

Subsystems (2)

RISC-V C-sky architecture

lore

Share & tools

lore

[email protected]

author maintainer

2020-07-05 14:15 UTC #0 lore raw

RFC PATCH 1/1 riscv: enable per-task stack canaries

From: Guo Ren <[email protected]>

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 <[email protected]>

Cc Kees Cook <[email protected]>

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

arch/riscv/Kconfig +7 -0

index 4b0e308..4b4e833 100644
@@ -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

arch/riscv/Makefile +10 -0

index fb6e37d..880a288 100644
@@ -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

arch/riscv/include/asm/stackprotector.h +2 -1

index 5962f88..09093af 100644
@@ -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 */

arch/riscv/kernel/asm-offsets.c +3 -0

index 07cb9c1..999b465 100644
@@ -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);

arch/riscv/kernel/process.c +1 -1

index 6548929..cb4ac65 100644
@@ -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);

Signature · 1 line

2.7.4

Kees Cook

2020-07-05 20:40 UTC #1 lore raw

Quoted 8 lines · On Sun, Jul 05, 2020 at 02:13:17PM +0000, [email protected] wrote:

From: Guo Ren <[email protected]>
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.

Quoted 9 lines

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.

Quoted 5 lines

arm64: Use -mstack-protector-guard-offset & guard-reg
ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
prepare: stack_protector_prepare
stack_protector_prepare: prepare0

Quoted 8 lines

       $(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! :)

Quoted 2 lines

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.

Quoted 3 lines

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?

Quoted 16 lines

Signed-off-by Guo Ren <[email protected]>

Cc Kees Cook <[email protected]>

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

arch/riscv/Kconfig +0 -0

index 4b0e308..4b4e833 100644

arch/riscv/Kconfig config CMDLINE_FORCE · line 398

Quoted 5 lines

arch/riscv/Kconfig +2 -0

@@ -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'?

Signature · 1 line

Kees Cook

Guo Ren

author maintainer

2020-07-06 01:02 UTC #2 lore raw

Quoted 37 lines · Kees Cook

Quoted 8 lines · On Sun, Jul 05, 2020 at 02:13:17PM +0000, [email protected] wrote:
From: Guo Ren <[email protected]>
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.
Quoted 9 lines
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.
Quoted 5 lines
arm64: Use -mstack-protector-guard-offset & guard-reg
ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
prepare: stack_protector_prepare
stack_protector_prepare: prepare0
Quoted 8 lines
       $(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 ?

Quoted 7 lines

I don't know riscv internals, so I leave that to y'all! :)
Quoted 2 lines
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!

Quoted 7 lines

Quoted 4 lines
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.

Quoted 18 lines

Signed-off-by Guo Ren <[email protected]>

Cc Kees Cook <[email protected]>

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

arch/riscv/Kconfig +0 -0

index 4b0e308..4b4e833 100644
@@ -394,6 +394,13 @@ config CMDLINE_FORCE

Quoted 3 lines

arch/riscv/Kconfig config CMDLINE_FORCE · line 397
Quoted 3 lines
arch/riscv/Kconfig +1 -0
 endchoice
 
+config CC_HAVE_STACKPROTECTOR_SYSREG

Should change to CC_HAVE_STACKPROTECTOR_GPR

Quoted 8 lines

arch/riscv/Kconfig config CMDLINE_FORCE · line 398
Quoted 1 line
arch/riscv/Kconfig +1 -0
+     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.

Signature · 4 lines

Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Kees Cook

2020-07-06 01:21 UTC #3 lore raw

Quoted 4 lines · Guo Ren

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?

Signature · 1 line

Kees Cook

Guo Ren

author maintainer

2020-07-06 02:42 UTC #4 lore raw

Quoted 9 lines · Kees Cook

Quoted 4 lines · Guo Ren
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.

Signature · 4 lines

Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/