[PATCH v2 1/2] sched: move stack_canary field at the top of task_struct

[PATCH v2 1/2] sched: move stack_canary field at the top of task_struct

[Christophe Leroy]

In order to allow the use of non global stack protector canary,
the stack canary needs to be located at a know offset defined
in Makefile via -mstack-protector-guard-offset.

On powerpc/32, register r2 points to current task_struct at
all time, the stack_canary located inside task_struct can be
used directly if it is located in a known place.

In order to allow that, this patch moves the stack_canary field
out of the randomized area of task_struct.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 include/linux/sched.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 977cb57d7bc9..1d977b8a4bac 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -601,6 +601,10 @@ struct task_struct {
 	/* -1 unrunnable, 0 runnable, >0 stopped: */
 	volatile long			state;
 
+#ifdef CONFIG_STACKPROTECTOR
+	/* Canary value for the -fstack-protector GCC feature: */
+	unsigned long			stack_canary;
+#endif
 	/*
 	 * This begins the randomizable portion of task_struct. Only
 	 * scheduling-critical items should be added above here.
@@ -746,10 +750,6 @@ struct task_struct {
 	pid_t				pid;
 	pid_t				tgid;
 
-#ifdef CONFIG_STACKPROTECTOR
-	/* Canary value for the -fstack-protector GCC feature: */
-	unsigned long			stack_canary;
-#endif
 	/*
 	 * Pointers to the (original) parent process, youngest child, younger sibling,
 	 * older sibling, respectively.  (p->father can be replaced with
-- 
2.13.3

[PATCH v2 2/2] powerpc/32: add stack protector support

[Christophe Leroy]

This functionality 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 of 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.

On PPC32, as register r2 points to current task_struct at
all time, the stack_canary located inside task_struct can be
used directly by using the following GCC options:
-mstack-protector-guard=tls
-mstack-protector-guard-reg=r2
-mstack-protector-guard-offset=4
	(= offsetof(struct task_struct, stack_canary))

 $ 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 | 38 +++++++++++++++++++++++++++++++
 arch/powerpc/kernel/Makefile              |  4 ++++
 4 files changed, 47 insertions(+)
 create mode 100644 arch/powerpc/include/asm/stackprotector.h

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a80669209155..538bb6e2fd70 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -180,6 +180,7 @@ config PPC
 	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_CBPF_JIT			if !PPC64
+	select HAVE_STACKPROTECTOR		if PPC32 && $(cc-option,-mstack-protector-guard=tls)
 	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 11a1acba164a..1108981292e9 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -112,6 +112,10 @@ KBUILD_LDFLAGS	+= -m elf$(BITS)$(LDEMULATION)
 KBUILD_ARFLAGS	+= --target=elf$(BITS)-$(GNUTARGET)
 endif
 
+cflags-$(CONFIG_STACKPROTECTOR)	+= -mstack-protector-guard=tls
+cflags-$(CONFIG_STACKPROTECTOR)	+= -mstack-protector-guard-reg=r2
+cflags-$(CONFIG_STACKPROTECTOR)	+= -mstack-protector-guard-offset=4
+
 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..4fdecbdd78e7
--- /dev/null
+++ b/arch/powerpc/include/asm/stackprotector.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * GCC stack protector support.
+ *
+ */
+
+#ifndef _ASM_STACKPROTECTOR_H
+#define _ASM_STACKPROTECTOR_H
+
+#include <linux/random.h>
+#include <linux/version.h>
+#include <asm/reg.h>
+
+/*
+ * 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;
+
+	/*
+	 * The stack_canary must be located at the offset given to
+	 * -mstack-protector-guard-offset in the Makefile
+	 */
+	BUILD_BUG_ON(offsetof(struct task_struct, stack_canary) != sizeof(long));
+
+	/* Try to get a semi random initial value. */
+	get_random_bytes(&canary, sizeof(canary));
+	canary ^= mftb();
+	canary ^= LINUX_VERSION_CODE;
+
+	current->stack_canary = 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)
-- 
2.13.3

Re: [PATCH v2 1/2] sched: move stack_canary field at the top of task_struct

[Peter Zijlstra]

On Wed, Sep 19, 2018 at 11:14:43AM +0000, Christophe Leroy wrote:
> In order to allow the use of non global stack protector canary,
> the stack canary needs to be located at a know offset defined
> in Makefile via -mstack-protector-guard-offset.
> 
> On powerpc/32, register r2 points to current task_struct at
> all time, the stack_canary located inside task_struct can be
> used directly if it is located in a known place.
> 
> In order to allow that, this patch moves the stack_canary field
> out of the randomized area of task_struct.

And you cannot use something like asm-offsets to extract this?

> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  include/linux/sched.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 977cb57d7bc9..1d977b8a4bac 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -601,6 +601,10 @@ struct task_struct {
>  	/* -1 unrunnable, 0 runnable, >0 stopped: */
>  	volatile long			state;
>  
> +#ifdef CONFIG_STACKPROTECTOR
> +	/* Canary value for the -fstack-protector GCC feature: */
> +	unsigned long			stack_canary;
> +#endif
>  	/*
>  	 * This begins the randomizable portion of task_struct. Only
>  	 * scheduling-critical items should be added above here.

Might as well put it before state, right after the task_info thing.

Re: [PATCH v2 1/2] sched: move stack_canary field at the top of task_struct

[Christophe LEROY]

Le 19/09/2018 à 13:58, Peter Zijlstra a écrit :
> On Wed, Sep 19, 2018 at 11:14:43AM +0000, Christophe Leroy wrote:
>> In order to allow the use of non global stack protector canary,
>> the stack canary needs to be located at a know offset defined
>> in Makefile via -mstack-protector-guard-offset.
>>
>> On powerpc/32, register r2 points to current task_struct at
>> all time, the stack_canary located inside task_struct can be
>> used directly if it is located in a known place.
>>
>> In order to allow that, this patch moves the stack_canary field
>> out of the randomized area of task_struct.
> 
> And you cannot use something like asm-offsets to extract this?

I have not been able to find a way to define the compilation flags AFTER 
building asm-offsets.h, see https://patchwork.ozlabs.org/patch/971521/

If you have a suggestion, it is welcomed.

> 
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   include/linux/sched.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 977cb57d7bc9..1d977b8a4bac 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -601,6 +601,10 @@ struct task_struct {
>>   	/* -1 unrunnable, 0 runnable, >0 stopped: */
>>   	volatile long			state;
>>   
>> +#ifdef CONFIG_STACKPROTECTOR
>> +	/* Canary value for the -fstack-protector GCC feature: */
>> +	unsigned long			stack_canary;
>> +#endif
>>   	/*
>>   	 * This begins the randomizable portion of task_struct. Only
>>   	 * scheduling-critical items should be added above here.
> 
> Might as well put it before state, right after the task_info thing.
> 

Yes, it doesn't make much difference, don't any arch expect state at 
offset 0 ?

Christophe

Re: [PATCH v2 1/2] sched: move stack_canary field at the top of task_struct

[Peter Zijlstra]

On Wed, Sep 19, 2018 at 02:25:00PM +0200, Christophe LEROY wrote:
> I have not been able to find a way to define the compilation flags AFTER
> building asm-offsets.h, see https://patchwork.ozlabs.org/patch/971521/
> 
> If you have a suggestion, it is welcomed.

Not really; I always get lost in that stuff :/

> > Might as well put it before state, right after the task_info thing.
> > 
> 
> Yes, it doesn't make much difference, don't any arch expect state at offset
> 0 ?

Uhmm.. dunno. I would not expect so, but then I didn't check.

Re: [PATCH v2 2/2] powerpc/32: add stack protector support

[Segher Boessenkool]

On Wed, Sep 19, 2018 at 11:14:45AM +0000, Christophe Leroy wrote:
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -112,6 +112,10 @@ KBUILD_LDFLAGS	+= -m elf$(BITS)$(LDEMULATION)
>  KBUILD_ARFLAGS	+= --target=elf$(BITS)-$(GNUTARGET)
>  endif
>  
> +cflags-$(CONFIG_STACKPROTECTOR)	+= -mstack-protector-guard=tls
> +cflags-$(CONFIG_STACKPROTECTOR)	+= -mstack-protector-guard-reg=r2
> +cflags-$(CONFIG_STACKPROTECTOR)	+= -mstack-protector-guard-offset=4

This last line is only correct if !CONFIG_THREAD_INFO_IN_TASK; is that
always true?  Add an assert somewhere maybe?

> +	/*
> +	 * The stack_canary must be located at the offset given to
> +	 * -mstack-protector-guard-offset in the Makefile
> +	 */
> +	BUILD_BUG_ON(offsetof(struct task_struct, stack_canary) != sizeof(long));

Well this will help :-)

It looks like it will be easy to enable on 64 bit as well.

> +	/* Try to get a semi random initial value. */
> +	get_random_bytes(&canary, sizeof(canary));
> +	canary ^= mftb();
> +	canary ^= LINUX_VERSION_CODE;

These last two lines are useless (or worse, they may give people the idea
that they are not!)

You should use wait_for_random_bytes I think.


Segher

Re: [PATCH v2 2/2] powerpc/32: add stack protector support

[Christophe LEROY]

Le 19/09/2018 à 15:26, Segher Boessenkool a écrit :
> On Wed, Sep 19, 2018 at 11:14:45AM +0000, Christophe Leroy wrote:
>> --- a/arch/powerpc/Makefile
>> +++ b/arch/powerpc/Makefile
>> @@ -112,6 +112,10 @@ KBUILD_LDFLAGS	+= -m elf$(BITS)$(LDEMULATION)
>>   KBUILD_ARFLAGS	+= --target=elf$(BITS)-$(GNUTARGET)
>>   endif
>>   
>> +cflags-$(CONFIG_STACKPROTECTOR)	+= -mstack-protector-guard=tls
>> +cflags-$(CONFIG_STACKPROTECTOR)	+= -mstack-protector-guard-reg=r2
>> +cflags-$(CONFIG_STACKPROTECTOR)	+= -mstack-protector-guard-offset=4
> 
> This last line is only correct if !CONFIG_THREAD_INFO_IN_TASK; is that
> always true?  Add an assert somewhere maybe?

At the time being powerpc doesn't select CONFIG_THREAD_INFO_IN_TASK.

A BUILD_BUG_ON() is added in stackprotector.h

> 
>> +	/*
>> +	 * The stack_canary must be located at the offset given to
>> +	 * -mstack-protector-guard-offset in the Makefile
>> +	 */
>> +	BUILD_BUG_ON(offsetof(struct task_struct, stack_canary) != sizeof(long));
> 
> Well this will help :-)
> 
> It looks like it will be easy to enable on 64 bit as well.

Will it ? It seems that PPC64 doesn't have r2 pointing to current task 
struct, but instead it has r13 pointing to the paca struct. Which means 
we should add a canary in the paca struct, and populate it at task 
switch from current->stack_canary. Or am I missing something ?

> 
>> +	/* Try to get a semi random initial value. */
>> +	get_random_bytes(&canary, sizeof(canary));
>> +	canary ^= mftb();
>> +	canary ^= LINUX_VERSION_CODE;
> 
> These last two lines are useless (or worse, they may give people the idea
> that they are not!)

Well, the last line is in all arches except x86
The mftb() was suggested by Michael to add some entropy.
x86 does the same sort of thing with their rdtsc()

> 
> You should use wait_for_random_bytes I think.

On the 8xx, it takes several minutes before crnd_is_ready(), while 
boot_init_stack_canary() is called quite early in start_kernel()

Christophe

Re: [PATCH v2 2/2] powerpc/32: add stack protector support

[Segher Boessenkool]

On Wed, Sep 19, 2018 at 04:22:52PM +0200, Christophe LEROY wrote:
> >It looks like it will be easy to enable on 64 bit as well.
> 
> Will it ? It seems that PPC64 doesn't have r2 pointing to current task 
> struct, but instead it has r13 pointing to the paca struct. Which means 
> we should add a canary in the paca struct, and populate it at task 
> switch from current->stack_canary. Or am I missing something ?

No, I am just forgetting things :-)

> >>+	/* Try to get a semi random initial value. */
> >>+	get_random_bytes(&canary, sizeof(canary));
> >>+	canary ^= mftb();
> >>+	canary ^= LINUX_VERSION_CODE;
> >
> >These last two lines are useless (or worse, they may give people the idea
> >that they are not!)
> 
> Well, the last line is in all arches except x86
> The mftb() was suggested by Michael to add some entropy.
> x86 does the same sort of thing with their rdtsc()
> 
> >
> >You should use wait_for_random_bytes I think.
> 
> On the 8xx, it takes several minutes before crnd_is_ready(), while 
> boot_init_stack_canary() is called quite early in start_kernel()

If you do not provide real entropy to the canary, the canary doesn't help
providing protection as much as you may hope.


Segher

Re: [PATCH v2 1/2] sched: move stack_canary field at the top of task_struct

[Michael Ellerman]

Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 19/09/2018 à 13:58, Peter Zijlstra a écrit :
>> On Wed, Sep 19, 2018 at 11:14:43AM +0000, Christophe Leroy wrote:
>>> In order to allow the use of non global stack protector canary,
>>> the stack canary needs to be located at a know offset defined
>>> in Makefile via -mstack-protector-guard-offset.
>>>
>>> On powerpc/32, register r2 points to current task_struct at
>>> all time, the stack_canary located inside task_struct can be
>>> used directly if it is located in a known place.
>>>
>>> In order to allow that, this patch moves the stack_canary field
>>> out of the randomized area of task_struct.
>> 
>> And you cannot use something like asm-offsets to extract this?
>
> I have not been able to find a way to define the compilation flags AFTER 
> building asm-offsets.h, see https://patchwork.ozlabs.org/patch/971521/
>
> If you have a suggestion, it is welcomed.

Hmm, that's something of a hard problem.

But the stack canary is one of the things we really *do* want to be
randomised, so we should probably try to come up with a solution.

cheers

Re: [PATCH v2 1/2] sched: move stack_canary field at the top of task_struct

[Michael Ellerman]

Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 19/09/2018 =C3=A0 13:58, Peter Zijlstra a =C3=A9crit=C2=A0:
>> On Wed, Sep 19, 2018 at 11:14:43AM +0000, Christophe Leroy wrote:
>>> In order to allow the use of non global stack protector canary,
>>> the stack canary needs to be located at a know offset defined
>>> in Makefile via -mstack-protector-guard-offset.
>>>
>>> On powerpc/32, register r2 points to current task_struct at
>>> all time, the stack_canary located inside task_struct can be
>>> used directly if it is located in a known place.
>>>
>>> In order to allow that, this patch moves the stack_canary field
>>> out of the randomized area of task_struct.
>>=20
>> And you cannot use something like asm-offsets to extract this?
>
> I have not been able to find a way to define the compilation flags AFTER=
=20
> building asm-offsets.h, see https://patchwork.ozlabs.org/patch/971521/
>
> If you have a suggestion, it is welcomed.

Hmm, that's something of a hard problem.

But the stack canary is one of the things we really *do* want to be
randomised, so we should probably try to come up with a solution.

cheers