mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/2] powerpc: stack protector (-fstack-protector) support
@ 2016-09-30 14:26 Christophe Leroy
  2016-09-30 14:26 ` [PATCH 1/2] powerpc: initial " Christophe Leroy
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Christophe Leroy @ 2016-09-30 14:26 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Scott Wood
  Cc: linux-kernel, linuxppc-dev, Nicolas Pitre

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
  2016-09-30 14:26 [PATCH 0/2] powerpc: stack protector (-fstack-protector) support Christophe Leroy
@ 2016-09-30 14:26 ` Christophe Leroy
  2016-11-17 11:05   ` Michael Ellerman
  2016-09-30 14:27 ` [PATCH 2/2] powerpc/32: stack protector: change the canary value per task Christophe Leroy
       [not found] ` <CAOJe8K2bYSGe5ciY75eSbthkXNtQwJ2LBdgWcyqqo_jwhmoUsg@mail.gmail.com>
  2 siblings, 1 reply; 12+ messages in thread
From: Christophe Leroy @ 2016-09-30 14:26 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Scott Wood
  Cc: linux-kernel, linuxppc-dev, Nicolas Pitre

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 <nicolas.pitre@linaro.org>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/2] powerpc/32: stack protector: change the canary value per task
  2016-09-30 14:26 [PATCH 0/2] powerpc: stack protector (-fstack-protector) support Christophe Leroy
  2016-09-30 14:26 ` [PATCH 1/2] powerpc: initial " Christophe Leroy
@ 2016-09-30 14:27 ` Christophe Leroy
       [not found] ` <CAOJe8K2bYSGe5ciY75eSbthkXNtQwJ2LBdgWcyqqo_jwhmoUsg@mail.gmail.com>
  2 siblings, 0 replies; 12+ messages in thread
From: Christophe Leroy @ 2016-09-30 14:27 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Scott Wood
  Cc: linux-kernel, linuxppc-dev, Nicolas Pitre

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 <nicolas.pitre@linaro.org>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] powerpc: stack protector (-fstack-protector) support
       [not found] ` <CAOJe8K2bYSGe5ciY75eSbthkXNtQwJ2LBdgWcyqqo_jwhmoUsg@mail.gmail.com>
@ 2016-09-30 16:38   ` Christophe LEROY
  2016-09-30 23:17     ` Benjamin Herrenschmidt
  2016-11-17 11:27   ` Michael Ellerman
  1 sibling, 1 reply; 12+ messages in thread
From: Christophe LEROY @ 2016-09-30 16:38 UTC (permalink / raw)
  To: Denis Kirjanov
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Scott Wood, Nicolas Pitre, linuxppc-dev, linux-kernel



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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] powerpc: stack protector (-fstack-protector) support
  2016-09-30 16:38   ` [PATCH 0/2] powerpc: stack protector (-fstack-protector) support Christophe LEROY
@ 2016-09-30 23:17     ` Benjamin Herrenschmidt
  0 siblings, 0 replies; 12+ messages in thread
From: Benjamin Herrenschmidt @ 2016-09-30 23:17 UTC (permalink / raw)
  To: Christophe LEROY, Denis Kirjanov
  Cc: Paul Mackerras, Michael Ellerman, Scott Wood, Nicolas Pitre,
	linuxppc-dev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
  2016-09-30 14:26 ` [PATCH 1/2] powerpc: initial " Christophe Leroy
@ 2016-11-17 11:05   ` Michael Ellerman
  2016-11-22 10:51     ` Christophe LEROY
  0 siblings, 1 reply; 12+ messages in thread
From: Michael Ellerman @ 2016-11-17 11:05 UTC (permalink / raw)
  To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras, Scott Wood
  Cc: linux-kernel, linuxppc-dev, Nicolas Pitre

Christophe Leroy <christophe.leroy@c-s.fr> writes:

> 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
                             
> + * 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 ?

> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] powerpc: stack protector (-fstack-protector) support
       [not found] ` <CAOJe8K2bYSGe5ciY75eSbthkXNtQwJ2LBdgWcyqqo_jwhmoUsg@mail.gmail.com>
  2016-09-30 16:38   ` [PATCH 0/2] powerpc: stack protector (-fstack-protector) support Christophe LEROY
@ 2016-11-17 11:27   ` Michael Ellerman
  2016-11-18 12:43     ` Denis Kirjanov
  1 sibling, 1 reply; 12+ messages in thread
From: Michael Ellerman @ 2016-11-17 11:27 UTC (permalink / raw)
  To: Denis Kirjanov, Christophe Leroy
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Scott Wood,
	Nicolas Pitre, linuxppc-dev, linux-kernel

Denis Kirjanov <kda@linux-powerpc.org> writes:

> On Friday, September 30, 2016, Christophe Leroy <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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] powerpc: stack protector (-fstack-protector) support
  2016-11-17 11:27   ` Michael Ellerman
@ 2016-11-18 12:43     ` Denis Kirjanov
  0 siblings, 0 replies; 12+ messages in thread
From: Denis Kirjanov @ 2016-11-18 12:43 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
	Scott Wood, Nicolas Pitre, linuxppc-dev, linux-kernel

On 11/17/16, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Denis Kirjanov <kda@linux-powerpc.org> writes:
>
>> On Friday, September 30, 2016, Christophe Leroy <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

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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
  2016-11-17 11:05   ` Michael Ellerman
@ 2016-11-22 10:51     ` Christophe LEROY
  0 siblings, 0 replies; 12+ messages in thread
From: Christophe LEROY @ 2016-11-22 10:51 UTC (permalink / raw)
  To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras, Scott Wood
  Cc: linux-kernel, linuxppc-dev, Nicolas Pitre



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 <christophe.leroy@c-s.fr> writes:
>
>> 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
>
>> + * 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 ?
>
>> 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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
  2018-09-17 12:15 [PATCH 1/2] powerpc: initial " Christophe Leroy
  2018-09-17 16:21 ` kbuild test robot
@ 2018-09-17 16:46 ` Segher Boessenkool
  1 sibling, 0 replies; 12+ messages in thread
From: Segher Boessenkool @ 2018-09-17 16:46 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	linuxppc-dev, linux-kernel

Hi!

On Mon, Sep 17, 2018 at 12:15:05PM +0000, Christophe Leroy wrote:
> Now, GCC offers the possibility to manually set the
> stack-protector mode (global or tls) regardless of libc support.

Yup :-)

> This time, the patch selects HAVE_STACKPROTECTOR only if
> -mstack-protector-guard=global is supported by GCC.

"global" is weaker than "tls" (it is easier to read the cookie in an
exploit).  It is better to use tls if you can.


Segher

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
  2018-09-17 12:15 [PATCH 1/2] powerpc: initial " Christophe Leroy
@ 2018-09-17 16:21 ` kbuild test robot
  2018-09-17 16:46 ` Segher Boessenkool
  1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2018-09-17 16:21 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: kbuild-all, Benjamin Herrenschmidt, Paul Mackerras,
	Michael Ellerman, linux-kernel, linuxppc-dev

[-- Attachment #1: Type: text/plain, Size: 1868 bytes --]

Hi Christophe,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.19-rc4 next-20180913]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-initial-stack-protector-fstack-protector-support/20180917-202227
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-ppc6xx_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_printf':
>> bootx_init.c:(.init.text+0x2bc): undefined reference to `__stack_chk_fail_local'
   arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_add_display_props.isra.1':
   bootx_init.c:(.init.text+0x750): undefined reference to `__stack_chk_fail_local'
   arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_scan_dt_build_struct':
   bootx_init.c:(.init.text+0xa84): undefined reference to `__stack_chk_fail_local'
   arch/powerpc/platforms/powermac/bootx_init.o: In function `bootx_init':
   bootx_init.c:(.init.text+0xf48): undefined reference to `__stack_chk_fail_local'
   powerpc-linux-gnu-ld: .tmp_vmlinux1: hidden symbol `__stack_chk_fail_local' isn't defined
   powerpc-linux-gnu-ld: final link failed: Bad value

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29220 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] powerpc: initial stack protector (-fstack-protector) support
@ 2018-09-17 12:15 Christophe Leroy
  2018-09-17 16:21 ` kbuild test robot
  2018-09-17 16:46 ` Segher Boessenkool
  0 siblings, 2 replies; 12+ messages in thread
From: Christophe Leroy @ 2018-09-17 12:15 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
  Cc: linux-kernel, linuxppc-dev

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.

This patch was tentatively added in the past (commit 6533b7c16ee5
("powerpc: Initial stack protector (-fstack-protector) support"))
but had to be reverted (commit f2574030b0e3 ("powerpc: Revert the
initial stack protector support") because GCC implementing it
differently whether it had been built with libc support or not.

Now, GCC offers the possibility to manually set the
stack-protector mode (global or tls) regardless of libc support.

This time, the patch selects HAVE_STACKPROTECTOR only if
-mstack-protector-guard=global is supported by GCC.

 $ echo CORRUPT_STACK > /sys/kernel/debug/provoke-crash/DIRECT
[  134.943666] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: lkdtm_CORRUPT_STACK+0x64/0x64
[  134.943666]
[  134.955414] CPU: 0 PID: 283 Comm: sh Not tainted 4.18.0-s3k-dev-12143-ga3272be41209 #835
[  134.963380] Call Trace:
[  134.965860] [c6615d60] [c001f76c] panic+0x118/0x260 (unreliable)
[  134.971775] [c6615dc0] [c001f654] panic+0x0/0x260
[  134.976435] [c6615dd0] [c032c368] lkdtm_CORRUPT_STACK_STRONG+0x0/0x64
[  134.982769] [c6615e00] [ffffffff] 0xffffffff

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/Kconfig                      |  1 +
 arch/powerpc/Makefile                     |  4 +++
 arch/powerpc/include/asm/stackprotector.h | 41 +++++++++++++++++++++++++++++++
 arch/powerpc/kernel/Makefile              |  4 +++
 arch/powerpc/kernel/process.c             |  6 +++++
 5 files changed, 56 insertions(+)
 create mode 100644 arch/powerpc/include/asm/stackprotector.h

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index db0b6eebbfa5..3f5776ed99d3 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -181,6 +181,7 @@ config PPC
 	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_CBPF_JIT			if !PPC64
+	select HAVE_STACKPROTECTOR		if $(cc-option,-mstack-protector-guard=global)
 	select HAVE_CONTEXT_TRACKING		if PPC64
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DEBUG_STACKOVERFLOW
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 8397c7bd5880..0dbfdb6a145d 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -112,6 +112,10 @@ LDFLAGS		+= -m elf$(BITS)$(LDEMULATION)
 KBUILD_ARFLAGS	+= --target=elf$(BITS)-$(GNUTARGET)
 endif
 
+ifdef CONFIG_STACKPROTECTOR
+KBUILD_CFLAGS	+= -mstack-protector-guard=global
+endif
+
 LDFLAGS_vmlinux-y := -Bstatic
 LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
 LDFLAGS_vmlinux	:= $(LDFLAGS_vmlinux-y)
diff --git a/arch/powerpc/include/asm/stackprotector.h b/arch/powerpc/include/asm/stackprotector.h
new file mode 100644
index 000000000000..2556e227cdb2
--- /dev/null
+++ b/arch/powerpc/include/asm/stackprotector.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * 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 PPC.  This unfortunately means that on SMP
+ * we cannot have a different canary value per task.
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H
+
+#include <linux/random.h>
+#include <linux/version.h>
+#include <asm/reg.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 ^= mftb();
+	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 3b66f2c19c84..0556a7243d2a 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -20,6 +20,10 @@ CFLAGS_prom_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.
+CFLAGS_prom_init.o += $(call cc-option, -fno-stack-protector)
+
 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 913c5725cdb2..8e9e90e8d773 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -69,6 +69,12 @@
 #include <linux/kprobes.h>
 #include <linux/kdebug.h>
 
+#ifdef CONFIG_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.13.3


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-09-17 17:13 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-30 14:26 [PATCH 0/2] powerpc: stack protector (-fstack-protector) support Christophe Leroy
2016-09-30 14:26 ` [PATCH 1/2] powerpc: initial " Christophe Leroy
2016-11-17 11:05   ` Michael Ellerman
2016-11-22 10:51     ` Christophe LEROY
2016-09-30 14:27 ` [PATCH 2/2] powerpc/32: stack protector: change the canary value per task Christophe Leroy
     [not found] ` <CAOJe8K2bYSGe5ciY75eSbthkXNtQwJ2LBdgWcyqqo_jwhmoUsg@mail.gmail.com>
2016-09-30 16:38   ` [PATCH 0/2] powerpc: stack protector (-fstack-protector) support Christophe LEROY
2016-09-30 23:17     ` Benjamin Herrenschmidt
2016-11-17 11:27   ` Michael Ellerman
2016-11-18 12:43     ` Denis Kirjanov
2018-09-17 12:15 [PATCH 1/2] powerpc: initial " Christophe Leroy
2018-09-17 16:21 ` kbuild test robot
2018-09-17 16:46 ` Segher Boessenkool

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®