[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