[kernel-hardening] Re: [RFC][PATCH 2/2] arm64: Clear the stack

[Alexander Popov <alex.popov@linux.com>]

Hello Kees and Laura,

On 25.07.2017 06:34, Kees Cook wrote:
> On Mon, Jul 24, 2017 at 1:19 AM, Alexander Popov <alex.popov@linux.com> wrote:
>> On 22.07.2017 03:23, Laura Abbott wrote:
>>> On 07/21/2017 09:56 AM, Alexander Popov wrote:
>>>> So let's keep it not to break CONFIG_SCHED_STACK_END_CHECK.
>>>
>>> That makes sense, good find! I wonder if CONFIG_SCHED_STACK_END_CHECK
>>> should go on the list of hardening options and/or if we can enhance
>>> it somehow?
>>
>> Do you mean this list?
>> http://www.kernsec.org/wiki/index.php/Kernel_Self_Protection_Project/Recommended_Settings
>>
>>> I'm not sure why it requires two words though since the
>>> poison only seems to be 32-bits?
>>
>> On x86_64 end_of_stack() returns the pointer to unsigned long, so we need at
>> least 8 bytes to avoid breaking CONFIG_SCHED_STACK_END_CHECK. Don't know about 8
>> more bytes.
> 
> Seems like this should be a random value like the per-frame stack
> canary, but it looks like a relatively cheap check. It's probably not
> in the cache, though, since most stack operations should be pretty far
> from the end of the stack...

It seems that the idea you describe is not implemented in Grsecurity/PaX patch.

On x86_64 the bottom of the stack for the mainline kernel looks like that:
0xffffc900004c8000: 0x57ac6e9d 0x00000000 0x00000000 0x00000000
0xffffc900004c8010: 0xffff4111 0xffffffff 0xffff4111 0xffffffff
...

But for the Grsecurity kernel it looks like that:
0xffffc90000324000: 0x00000000 0x00000000 0x57ac6e9d 0x00000000
0xffffc90000324010: 0xffff4111 0xffffffff 0xffff4111 0xffffffff
...

Because Grsecurity/PaX patch has that change:
 static inline unsigned long *end_of_stack(const struct task_struct *task)
 {
-	return task->stack;
+	return (unsigned long *)task->stack + 1;
 }

So that is their reason of having 16 bytes reserved at the bottom of the kernel
stack.

However, right now I don't understand the purpose of patching end_of_stack().
What do you think? Should mainline have it?

Best regards,
Alexander