On Mon, Mar 05, 2018 at 11:43:19AM -0800, Laura Abbott wrote:
> On 03/05/2018 08:41 AM, Dave Hansen wrote:
> > On 03/03/2018 12:00 PM, Alexander Popov wrote:
> > > Documentation/x86/x86_64/mm.txt | 2 +
> > > arch/Kconfig | 27 ++++++++++
> > > arch/x86/Kconfig | 1 +
> > > arch/x86/entry/entry_32.S | 88 +++++++++++++++++++++++++++++++
> > > arch/x86/entry/entry_64.S | 108 +++++++++++++++++++++++++++++++++++++++
> > > arch/x86/entry/entry_64_compat.S | 11 ++++
> >
> > This is a *lot* of assembly. I wonder if you tried at all to get more
> > of this into C or whether you just inherited the assembly from the
> > original code?
> >
>
> This came up previously http://www.openwall.com/lists/kernel-hardening/2017/10/23/5
> there were concerns about trusting C to do the right thing as well as
> speed.
And therefore the answer to this obvious question should've been part of
the Changelog :-)
Dave is last in a long line of people asking this same question.
On 05.03.2018 23:25, Peter Zijlstra wrote:
> On Mon, Mar 05, 2018 at 11:43:19AM -0800, Laura Abbott wrote:
>> On 03/05/2018 08:41 AM, Dave Hansen wrote:
>>> On 03/03/2018 12:00 PM, Alexander Popov wrote:
>>>> Documentation/x86/x86_64/mm.txt | 2 +
>>>> arch/Kconfig | 27 ++++++++++
>>>> arch/x86/Kconfig | 1 +
>>>> arch/x86/entry/entry_32.S | 88 +++++++++++++++++++++++++++++++
>>>> arch/x86/entry/entry_64.S | 108 +++++++++++++++++++++++++++++++++++++++
>>>> arch/x86/entry/entry_64_compat.S | 11 ++++
>>>
>>> This is a *lot* of assembly. I wonder if you tried at all to get more
>>> of this into C or whether you just inherited the assembly from the
>>> original code?
>>>
>>
>> This came up previously http://www.openwall.com/lists/kernel-hardening/2017/10/23/5
>> there were concerns about trusting C to do the right thing as well as
>> speed.
>
> And therefore the answer to this obvious question should've been part of
> the Changelog :-)
>
> Dave is last in a long line of people asking this same question.
Yes, actually the changelog in the cover letter contains that:
After some experiments, kept the asm implementation of erase_kstack(),
because it gives a full control over the stack for clearing it neatly
and doesn't offend KASAN.
Moreover, later erase_kstack() on x86_64 became different from one on x86_32.
Best regards,
Alexander
On Mon, Mar 5, 2018 at 1:21 PM, Alexander Popov <[email protected]> wrote:
> On 05.03.2018 23:25, Peter Zijlstra wrote:
>> On Mon, Mar 05, 2018 at 11:43:19AM -0800, Laura Abbott wrote:
>>> On 03/05/2018 08:41 AM, Dave Hansen wrote:
>>>> On 03/03/2018 12:00 PM, Alexander Popov wrote:
>>>>> Documentation/x86/x86_64/mm.txt | 2 +
>>>>> arch/Kconfig | 27 ++++++++++
>>>>> arch/x86/Kconfig | 1 +
>>>>> arch/x86/entry/entry_32.S | 88 +++++++++++++++++++++++++++++++
>>>>> arch/x86/entry/entry_64.S | 108 +++++++++++++++++++++++++++++++++++++++
>>>>> arch/x86/entry/entry_64_compat.S | 11 ++++
>>>>
>>>> This is a *lot* of assembly. I wonder if you tried at all to get more
>>>> of this into C or whether you just inherited the assembly from the
>>>> original code?
>>>>
>>>
>>> This came up previously http://www.openwall.com/lists/kernel-hardening/2017/10/23/5
>>> there were concerns about trusting C to do the right thing as well as
>>> speed.
>>
>> And therefore the answer to this obvious question should've been part of
>> the Changelog :-)
>>
>> Dave is last in a long line of people asking this same question.
>
> Yes, actually the changelog in the cover letter contains that:
>
> After some experiments, kept the asm implementation of erase_kstack(),
> because it gives a full control over the stack for clearing it neatly
> and doesn't offend KASAN.
>
> Moreover, later erase_kstack() on x86_64 became different from one on x86_32.
Maybe explicitly mention the C experiments in future change log?
-Kees
--
Kees Cook
Pixel Security
On 05.03.2018 23:25, Peter Zijlstra wrote:
> On Mon, Mar 05, 2018 at 11:43:19AM -0800, Laura Abbott wrote:
>> On 03/05/2018 08:41 AM, Dave Hansen wrote:
>>> On 03/03/2018 12:00 PM, Alexander Popov wrote:
>>>> Documentation/x86/x86_64/mm.txt | 2 +
>>>> arch/Kconfig | 27 ++++++++++
>>>> arch/x86/Kconfig | 1 +
>>>> arch/x86/entry/entry_32.S | 88 +++++++++++++++++++++++++++++++
>>>> arch/x86/entry/entry_64.S | 108 +++++++++++++++++++++++++++++++++++++++
>>>> arch/x86/entry/entry_64_compat.S | 11 ++++
>>>
>>> This is a *lot* of assembly. I wonder if you tried at all to get more
>>> of this into C or whether you just inherited the assembly from the
>>> original code?
>>>
>>
>> This came up previously http://www.openwall.com/lists/kernel-hardening/2017/10/23/5
>> there were concerns about trusting C to do the right thing as well as
>> speed.
>
> And therefore the answer to this obvious question should've been part of
> the Changelog :-)
>
> Dave is last in a long line of people asking this same question.
Hello! I've decided to share the details (and ask for advice) regardless of the
destiny of this patch series.
I've rewritten the assembly part in C, please see the code below. That is
erase_kstack() function, which is called at the end of syscall just before
returning to the userspace.
The generated asm doesn't look nice (and might be somewhat slower), but I don't
care now.
The main obstacle:
erase_kstack() must save and restore any modified registers, because it is
called from the trampoline stack (introduced by Andy Lutomirski), when all
registers except RDI are live.
Laura had a similar issue with C code on ARM:
http://www.openwall.com/lists/kernel-hardening/2017/10/10/3
I've solved that with no_caller_saved_registers attribute, which makes all
registers callee-saved. But that attribute was introduced only in gcc-7.
Does kernel have a solution for similar issues?
Thanks!
-------- >8 --------
#include <linux/bug.h>
#include <linux/sched.h>
#include <asm/current.h>
#include <asm/linkage.h>
#include <asm/processor.h>
/* This function must save and restore any modified registers */
__attribute__ ((no_caller_saved_registers)) asmlinkage void erase_kstack(void)
{
register unsigned long p = current->thread.lowest_stack;
register unsigned long boundary = p & ~(THREAD_SIZE - 1);
unsigned long poison = 0;
unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
sizeof(unsigned long);
/*
* Two qwords at the bottom of the thread stack are reserved and
* should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
*/
boundary += 2 * sizeof(unsigned long);
/*
* Let's search for the poison value in the stack.
* Start from the lowest_stack and go to the bottom.
*/
while (p >= boundary && poison <= check_depth) {
if (*(unsigned long *)p == STACKLEAK_POISON)
poison++;
else
poison = 0;
p -= sizeof(unsigned long);
}
#ifdef CONFIG_STACKLEAK_METRICS
current->thread.prev_lowest_stack = p;
#endif
/*
* So let's write the poison value to the kernel stack. Start from
* the address in p and move up till the new boundary.
*/
if (on_thread_stack())
boundary = current_stack_pointer;
else
boundary = current_top_of_stack();
BUG_ON(boundary - p >= THREAD_SIZE);
while (p < boundary) {
*(unsigned long *)p = STACKLEAK_POISON;
p += sizeof(unsigned long);
}
/* Reset the lowest_stack value for the next syscall */
current->thread.lowest_stack = current_top_of_stack() - 256;
}
On 03/21/2018 04:04 AM, Alexander Popov wrote:
> The main obstacle:
> erase_kstack() must save and restore any modified registers, because it is
> called from the trampoline stack (introduced by Andy Lutomirski), when all
> registers except RDI are live.
Wow, cool, thanks for doing this!
PTI might also cause you some problems here because it probably won't
map your function. Did you have to put it in one of the sections that
gets mapped by the user page tables?
On 21.03.2018 18:33, Dave Hansen wrote:
> On 03/21/2018 04:04 AM, Alexander Popov wrote:
>> The main obstacle:
>> erase_kstack() must save and restore any modified registers, because it is
>> called from the trampoline stack (introduced by Andy Lutomirski), when all
>> registers except RDI are live.
>
> Wow, cool, thanks for doing this!
>
> PTI might also cause you some problems here because it probably won't
> map your function. Did you have to put it in one of the sections that
> gets mapped by the user page tables?
No, I didn't have to do that: erase_kstack() works fine, it is called just
before SWITCH_TO_USER_CR3_STACK.
There is also a way not to offend KASAN. erase_kstack() C code can be put in a
separate source file and compiled with "KASAN_SANITIZE_erase.o := n".
So, as I wrote, the only critical drawback of the C implementation is that it
needs no_caller_saved_registers attribute, which is provided by gcc since version 7.
Can you recommend any solution?
By the way, during my work on STACKLEAK, I've found one case when we get to the
userspace directly from the thread stack. Please see sysret32_from_system_call
in entry_64_compat.S. I checked that.
IMO it seems odd, can the adversary use that to bypass PTI?
Best regards,
Alexander