diff mbox series

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

Message ID d60ce7dd74704b0ca0a857186f30de4006b63534.1537355312.git.christophe.leroy@c-s.fr (mailing list archive)
State Superseded
Headers show
Series [v2,1/2] sched: move stack_canary field at the top of task_struct | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied
snowpatch_ozlabs/checkpatch success Test checkpatch on branch next

Commit Message

Christophe Leroy Sept. 19, 2018, 11:14 a.m. UTC
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(-)

Comments

Peter Zijlstra Sept. 19, 2018, 11:58 a.m. UTC | #1
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.
Christophe Leroy Sept. 19, 2018, 12:25 p.m. UTC | #2
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
Peter Zijlstra Sept. 19, 2018, 12:52 p.m. UTC | #3
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.
Michael Ellerman Sept. 19, 2018, 11:54 p.m. UTC | #4
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
diff mbox series

Patch

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