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

[Segher Boessenkool <segher@kernel.crashing.org>]

On Thu, Sep 27, 2018 at 08:20:00AM +0200, Christophe LEROY wrote:
> Le 26/09/2018 à 21:16, Segher Boessenkool a écrit :
> >On Wed, Sep 26, 2018 at 11:40:38AM +0000, Christophe Leroy wrote:
> >>+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;
> >>+}
> >
> >I still think you should wait until there is entropy available.  You
> >haven't answered my questions about that (or I didn't see them): what
> >does the kernel do in other similar cases?
> >
> >Looks great otherwise!
> 
> What do you mean by 'other similar cases' ? All arches have similar 
> boot_init_stack_canary().

Yes, those, and other things that want entropy early.

> x86 uses rdtsc() which is equivalent to our 
> mftb(). Most arches xor it with LINUX_VERSION_CODE.
> 
> The issue is that it is called very early in start_kernel(), however 
> they try to set some entropy anyway:
> 
> 	boot_cpu_init();
> 	page_address_init();
> 	pr_notice("%s", linux_banner);
> 	setup_arch(&command_line);
> 	/*
> 	 * Set up the the initial canary and entropy after arch
> 	 * and after adding latent and command line entropy.
> 	 */
> 	add_latent_entropy();
> 	add_device_randomness(command_line, strlen(command_line));
> 	boot_init_stack_canary();
> 
> Apparently, it is too early for calling wait_for_random_bytes(), see below.

Hrm.  Too early to call wait_event_interruptible?  From there it went
into schedule(), which blew up.  Well you say we have only one context
at this point, so that is not too surprising then :-)

> However this is the canary for initial startup only. Only idle() still 
> uses this canary once the system is running. A new canary is set for any 
> new forked task.

Ah, that makes things a lot better!  Do those new tasks get a canary
from something with sufficient entropy though?

> Maybe should the idle canary be updated later once there is more entropy 

That is tricky to do, but sure, if you can, that should help.

> ? Today there is a new call to boot_init_stack_canary() in 
> cpu_startup_entry(), but it is enclosed inside #ifdef CONFIG_X86.

It needs to know the details of how ssp works on each platform.


Segher