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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3f5d1937-393c-1105-30a0-72d568a1c190@c-s.fr>
Date:   Wed, 19 Sep 2018 16:22:52 +0200
From:   Christophe LEROY <christophe.leroy@....fr>
To:     Segher Boessenkool <segher@...nel.crashing.org>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2 2/2] powerpc/32: add stack protector support



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