[PATCH 0/2] powerpc: stack protector (-fstack-protector) support

Add HAVE_CC_STACKPROTECTOR to powerpc. This is copied from ARM.

Not tested on PPC64, compile ok with ppc64_defconfig

Christophe Leroy (2):
  powerpc: initial stack protector (-fstack-protector) support
  powerpc/32: stack protector: change the canary value per task

 arch/powerpc/Kconfig                      |  1 +
 arch/powerpc/include/asm/stackprotector.h | 38 +++++++++++++++++++++++++++++++
 arch/powerpc/kernel/Makefile              |  5 ++++
 arch/powerpc/kernel/asm-offsets.c         |  3 +++
 arch/powerpc/kernel/entry_32.S            |  6 ++++-
 arch/powerpc/kernel/process.c             |  6 +++++
 6 files changed, 58 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/include/asm/stackprotector.h

-- 
2.1.0

Partially copied from commit df0698be14c66 ("ARM: stack protector:
change the canary value per task")

A new random value for the canary is stored in the task struct whenever
a new task is forked.  This is meant to allow for different canary values
per task.  On powerpc, GCC expects the canary value to be found in a global
variable called __stack_chk_guard.  So this variable has to be updated
with the value stored in the task struct whenever a task switch occurs.

Because the variable GCC expects is global, this cannot work on SMP
unfortunately.  So, on SMP, the same initial canary value is kept
throughout, making this feature a bit less effective although it is still
useful.

Cc: Nicolas Pitre <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/kernel/asm-offsets.c | 3 +++
 arch/powerpc/kernel/entry_32.S    | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index a51ae9b..ede2fc4 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c

@@ -91,6 +91,9 @@ int main(void)
 	DEFINE(TI_livepatch_sp, offsetof(struct thread_info, livepatch_sp));
 #endif
 
+#ifdef CONFIG_CC_STACKPROTECTOR
+	DEFINE(TSK_STACK_CANARY, offsetof(struct task_struct, stack_canary));
+#endif
 	DEFINE(KSP, offsetof(struct thread_struct, ksp));
 	DEFINE(PT_REGS, offsetof(struct thread_struct, regs));
 #ifdef CONFIG_BOOKE

diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 3841d74..5742dbd 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S

@@ -674,7 +674,11 @@ BEGIN_FTR_SECTION
 	mtspr	SPRN_SPEFSCR,r0		/* restore SPEFSCR reg */
 END_FTR_SECTION_IFSET(CPU_FTR_SPE)
 #endif /* CONFIG_SPE */
-
+#if defined(CONFIG_CC_STACKPROTECTOR) && !defined(CONFIG_SMP)
+	lwz	r0,TSK_STACK_CANARY(r2)
+	lis	r4,__stack_chk_guard@ha
+	stw	r0,__stack_chk_guard@l(r4)
+#endif
 	lwz	r0,_CCR(r1)
 	mtcrf	0xFF,r0
 	/* r3-r12 are destroyed -- Cort */

-- 
2.1.0

Partialy copied from commit c743f38013aef ("ARM: initial stack protector
(-fstack-protector) support")

This is the very basic stuff without the changing canary upon
task switch yet.  Just the Kconfig option and a constant canary
value initialized at boot time.

Cc: Nicolas Pitre <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/Kconfig                      |  1 +
 arch/powerpc/include/asm/stackprotector.h | 38 +++++++++++++++++++++++++++++++
 arch/powerpc/kernel/Makefile              |  5 ++++
 arch/powerpc/kernel/process.c             |  6 +++++
 4 files changed, 50 insertions(+)
 create mode 100644 arch/powerpc/include/asm/stackprotector.h

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 59e53f4..2947dab 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -162,6 +162,7 @@ config PPC
 	select HAVE_VIRT_CPU_ACCOUNTING
 	select HAVE_ARCH_HARDENED_USERCOPY
 	select HAVE_KERNEL_GZIP
+	select HAVE_CC_STACKPROTECTOR
 
 config GENERIC_CSUM
 	def_bool CPU_LITTLE_ENDIAN

diff --git a/arch/powerpc/include/asm/stackprotector.h b/arch/powerpc/include/asm/stackprotector.h
new file mode 100644
index 0000000..de00332
--- /dev/null
+++ b/arch/powerpc/include/asm/stackprotector.h

@@ -0,0 +1,38 @@
+/*
+ * GCC stack protector support.
+ *
+ * Stack protector works by putting predefined pattern at the start of
+ * the stack frame and verifying that it hasn't been overwritten when
+ * returning from the function.  The pattern is called stack canary
+ * and gcc expects it to be defined by a global variable called
+ * "__stack_chk_guard" on ARM.  This unfortunately means that on SMP
+ * we cannot have a different canary value per task.
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H 1
+
+#include <linux/random.h>
+#include <linux/version.h>
+
+extern unsigned long __stack_chk_guard;
+
+/*
+ * Initialize the stackprotector canary value.
+ *
+ * NOTE: this must only be called from functions that never return,
+ * and it must always be inlined.
+ */
+static __always_inline void boot_init_stack_canary(void)
+{
+	unsigned long canary;
+
+	/* Try to get a semi random initial value. */
+	get_random_bytes(&canary, sizeof(canary));
+	canary ^= LINUX_VERSION_CODE;
+
+	current->stack_canary = canary;
+	__stack_chk_guard = current->stack_canary;
+}
+
+#endif	/* _ASM_STACKPROTECTOR_H */

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index e59ed6a..4a62179 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile

@@ -19,6 +19,11 @@ CFLAGS_init.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
 CFLAGS_btext.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
 CFLAGS_prom.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
 
+# -fstack-protector triggers protection checks in this code,
+# but it is being used too early to link to meaningful stack_chk logic.
+nossp_flags := $(call cc-option, -fno-stack-protector)
+CFLAGS_prom_init.o := $(nossp_flags)
+
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace early boot code
 CFLAGS_REMOVE_cputable.o = -mno-sched-epilog $(CC_FLAGS_FTRACE)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index ce8a26a..ba8f32a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c

@@ -64,6 +64,12 @@
 #include <linux/kprobes.h>
 #include <linux/kdebug.h>
 
+#ifdef CONFIG_CC_STACKPROTECTOR
+#include <linux/stackprotector.h>
+unsigned long __stack_chk_guard __read_mostly;
+EXPORT_SYMBOL(__stack_chk_guard);
+#endif
+
 /* Transactional Memory debug */
 #ifdef TM_DEBUG_SW
 #define TM_DEBUG(x...) printk(KERN_INFO x)

-- 
2.1.0

On Friday, September 30, 2016, Christophe Leroy [off-list ref]
wrote:

Add HAVE_CC_STACKPROTECTOR to powerpc. This is copied from ARM.

Not tested on PPC64, compile ok with ppc64_

Hi Christophe,

are you going to test it on ppc64? If not, I can take it

Christophe Leroy (2):
  powerpc: initial stack protector (-fstack-protector) support
  powerpc/32: stack protector: change the canary value per task

 arch/powerpc/Kconfig                      |  1 +
 arch/powerpc/include/asm/stackprotector.h | 38
+++++++++++++++++++++++++++++++
 arch/powerpc/kernel/Makefile              |  5 ++++
 arch/powerpc/kernel/asm-offsets.c         |  3 +++
 arch/powerpc/kernel/entry_32.S            |  6 ++++-
 arch/powerpc/kernel/process.c             |  6 +++++
 6 files changed, 58 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/include/asm/stackprotector.h

--
2.1.0

Le 30/09/2016 à 18:26, Denis Kirjanov a écrit :

On Friday, September 30, 2016, Christophe Leroy <christophe.leroy@c-s.fr
<mailto:christophe.leroy@c-s.fr>> wrote:

    Add HAVE_CC_STACKPROTECTOR to powerpc. This is copied from ARM.

    Not tested on PPC64, compile ok with ppc64_


Hi Christophe,

are you going to test it on ppc64? If not, I can take it

Thanks Denis, you are welcome to test it.

I don't have any ppc64 target, I only have mpc8xx and mpc83xx which both 
are ppc32

Christophe

    Christophe Leroy (2):
      powerpc: initial stack protector (-fstack-protector) support
      powerpc/32: stack protector: change the canary value per task

     arch/powerpc/Kconfig                      |  1 +
     arch/powerpc/include/asm/stackprotector.h | 38
    +++++++++++++++++++++++++++++++
     arch/powerpc/kernel/Makefile              |  5 ++++
     arch/powerpc/kernel/asm-offsets.c         |  3 +++
     arch/powerpc/kernel/entry_32.S            |  6 ++++-
     arch/powerpc/kernel/process.c             |  6 +++++
     6 files changed, 58 insertions(+), 1 deletion(-)
     create mode 100644 arch/powerpc/include/asm/stackprotector.h

    --
    2.1.0

On Fri, 2016-09-30 at 18:38 +0200, Christophe LEROY wrote:

I don't have any ppc64 target, I only have mpc8xx and mpc83xx which
both 
are ppc32

That's what qemu is for :-)

Cheers,
Ben.

Christophe Leroy [off-list ref] writes:

                             ^
                             PPC
                             

+ * we cannot have a different canary value per task.
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H 1

We usually just define it, not define it to 1.

+
+#include <linux/random.h>
+#include <linux/version.h>
+
+extern unsigned long __stack_chk_guard;
+
+/*
+ * Initialize the stackprotector canary value.
+ *
+ * NOTE: this must only be called from functions that never return,
+ * and it must always be inlined.
+ */
+static __always_inline void boot_init_stack_canary(void)
+{
+	unsigned long canary;
+
+	/* Try to get a semi random initial value. */
+	get_random_bytes(&canary, sizeof(canary));
+	canary ^= LINUX_VERSION_CODE;

What about mixing in an mftb() as well ?

We've already assigned to CFLAGS_prom_init.o so I think you should be
using += not := shouldn't you?

Also it could just be a single line:

CFLAGS_prom_init.o += $(call cc-option, -fno-stack-protector)


cheers

Denis Kirjanov [off-list ref] writes:

On Friday, September 30, 2016, Christophe Leroy [off-list ref]
wrote:

quoted

Add HAVE_CC_STACKPROTECTOR to powerpc. This is copied from ARM.

Not tested on PPC64, compile ok with ppc64_

Hi Christophe,

are you going to test it on ppc64? If not, I can take it

Did you get around to testing it?

It seems to be working here:

  root@mpe-ubuntu-le:/sys/kernel/debug/provoke-crash# echo CORRUPT_STACK > DIRECT
  Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: c000000000671ed8
  
  CPU: 1 PID: 3835 Comm: bash Not tainted 4.9.0-rc5-compiler_gcc-6.2.0-00075-gf41db6c7a3c8-dirty #464
  Call Trace:
  [c0000000dbf73a30] [c000000000541728] dump_stack+0xb8/0x100 (unreliable)
  
  [c0000000dbf73a70] [c000000000219ba8] panic+0x14c/0x320
  [c0000000dbf73b10] [c0000000000c27ec] __stack_chk_fail+0x2c/0x30
  [c0000000dbf73b70] [c000000000671ed8] lkdtm_CORRUPT_STACK+0x88/0x90
  [c0000000dbf73bf0] [6161616161616161] 0x6161616161616161
  Rebooting in 10 seconds..


cheers

On 11/17/16, Michael Ellerman [off-list ref] wrote:

Denis Kirjanov [off-list ref] writes:

quoted

On Friday, September 30, 2016, Christophe Leroy [off-list ref]
wrote:

quoted

Add HAVE_CC_STACKPROTECTOR to powerpc. This is copied from ARM.

Not tested on PPC64, compile ok with ppc64_

Hi Christophe,

are you going to test it on ppc64? If not, I can take it

Not yet, I've broken my test machine :/ I need some time to recover

Did you get around to testing it?

It seems to be working here:

  root@mpe-ubuntu-le:/sys/kernel/debug/provoke-crash# echo CORRUPT_STACK >
DIRECT
  Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in:
c000000000671ed8

  CPU: 1 PID: 3835 Comm: bash Not tainted
4.9.0-rc5-compiler_gcc-6.2.0-00075-gf41db6c7a3c8-dirty #464
  Call Trace:
  [c0000000dbf73a30] [c000000000541728] dump_stack+0xb8/0x100 (unreliable)

  [c0000000dbf73a70] [c000000000219ba8] panic+0x14c/0x320
  [c0000000dbf73b10] [c0000000000c27ec] __stack_chk_fail+0x2c/0x30
  [c0000000dbf73b70] [c000000000671ed8] lkdtm_CORRUPT_STACK+0x88/0x90
  [c0000000dbf73bf0] [6161616161616161] 0x6161616161616161
  Rebooting in 10 seconds..


cheers

Le 17/11/2016 à 12:05, Michael Ellerman a écrit :

Hi Michael,

I took your comments into account in v2. Shame on me, I forgot to add 
the list of changes from v1 to v2 in the commit log.

Christophe

Christophe Leroy [off-list ref] writes:

quoted

diff --git a/arch/powerpc/include/asm/stackprotector.h b/arch/powerpc/include/asm/stackprotector.h
new file mode 100644
index 0000000..de00332
--- /dev/null
+++ b/arch/powerpc/include/asm/stackprotector.h

@@ -0,0 +1,38 @@
+/*
+ * GCC stack protector support.
+ *
+ * Stack protector works by putting predefined pattern at the start of
+ * the stack frame and verifying that it hasn't been overwritten when
+ * returning from the function.  The pattern is called stack canary
+ * and gcc expects it to be defined by a global variable called
+ * "__stack_chk_guard" on ARM.  This unfortunately means that on SMP

                             ^
                             PPC

quoted

+ * we cannot have a different canary value per task.
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H 1

We usually just define it, not define it to 1.

quoted

+
+#include <linux/random.h>
+#include <linux/version.h>
+
+extern unsigned long __stack_chk_guard;
+
+/*
+ * Initialize the stackprotector canary value.
+ *
+ * NOTE: this must only be called from functions that never return,
+ * and it must always be inlined.
+ */
+static __always_inline void boot_init_stack_canary(void)
+{
+	unsigned long canary;
+
+	/* Try to get a semi random initial value. */
+	get_random_bytes(&canary, sizeof(canary));
+	canary ^= LINUX_VERSION_CODE;

What about mixing in an mftb() as well ?

quoted

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index e59ed6a..4a62179 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile

@@ -19,6 +19,11 @@ CFLAGS_init.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
 CFLAGS_btext.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)
 CFLAGS_prom.o += $(DISABLE_LATENT_ENTROPY_PLUGIN)

+# -fstack-protector triggers protection checks in this code,
+# but it is being used too early to link to meaningful stack_chk logic.
+nossp_flags := $(call cc-option, -fno-stack-protector)
+CFLAGS_prom_init.o := $(nossp_flags)

We've already assigned to CFLAGS_prom_init.o so I think you should be
using += not := shouldn't you?

Also it could just be a single line:

CFLAGS_prom_init.o += $(call cc-option, -fno-stack-protector)


cheers