Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Fri, 18 Aug 2017 11:07:08 +0300
From: Alexander Popov <alex.popov@...ux.com>
To: Kees Cook <keescook@...omium.org>
Cc: Laura Abbott <labbott@...hat.com>, Mark Rutland <mark.rutland@....com>,
 "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
 Ard Biesheuvel <ard.biesheuvel@...aro.org>, Tycho Andersen
 <tycho@...ker.com>, PaX Team <pageexec@...email.hu>
Subject: Re: [RFC][PATCH 2/2] arm64: Clear the stack

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@...ux.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

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.