[PATCH 0/5] KAISER fixlets..

[PATCH 0/5] KAISER fixlets..

[Peter Zijlstra]

Thomas asked me to post the various patches I did today so he doesn't need to
go dig them out the various email threads and make em pretty.

They're all tested in so far that they booted and managed to build the next kernel.

[PATCH 1/5] x86/mm/kaiser: Alternative ESPFIX

[Peter Zijlstra]

[-- Attachment #1: peterz-kaiser-espfix.patch --]
[-- Type: text/plain, Size: 2375 bytes --]

Change the asm to do the CR3 switcheroo so we can remove the magic
mappings.

Since RDI is unused after SWAPGS we can use it as a scratch reg for
SWITCH_TO_KERNEL. And once we've computed the new RSP (in RAX) we no
longer need RDI and can again use it as scratch reg for
SWITCH_TO_USER.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/entry_64.S   |   11 ++++++++---
 arch/x86/kernel/espfix_64.c |   10 ++--------
 2 files changed, 10 insertions(+), 11 deletions(-)

--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -825,7 +825,9 @@ ENTRY(native_iret)
 	 */
 
 	pushq	%rdi				/* Stash user RDI */
-	SWAPGS
+	SWAPGS					/* to kernel GS */
+	SWITCH_TO_KERNEL_CR3 scratch_reg=%rdi	/* to kernel CR3 */
+
 	movq	PER_CPU_VAR(espfix_waddr), %rdi
 	movq	%rax, (0*8)(%rdi)		/* user RAX */
 	movq	(1*8)(%rsp), %rax		/* user RIP */
@@ -841,7 +843,6 @@ ENTRY(native_iret)
 	/* Now RAX == RSP. */
 
 	andl	$0xffff0000, %eax		/* RAX = (RSP & 0xffff0000) */
-	popq	%rdi				/* Restore user RDI */
 
 	/*
 	 * espfix_stack[31:16] == 0.  The page tables are set up such that
@@ -852,7 +853,11 @@ ENTRY(native_iret)
 	 * still points to an RO alias of the ESPFIX stack.
 	 */
 	orq	PER_CPU_VAR(espfix_stack), %rax
-	SWAPGS
+
+	SWITCH_TO_USER_CR3 scratch_reg=%rdi	/* to user CR3 */
+	SWAPGS					/* to user GS */
+	popq	%rdi				/* Restore user RDI */
+
 	movq	%rax, %rsp
 	UNWIND_HINT_IRET_REGS offset=8
 
--- a/arch/x86/kernel/espfix_64.c
+++ b/arch/x86/kernel/espfix_64.c
@@ -61,8 +61,8 @@
 #define PGALLOC_GFP (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO)
 
 /* This contains the *bottom* address of the espfix stack */
-DEFINE_PER_CPU_USER_MAPPED(unsigned long, espfix_stack);
-DEFINE_PER_CPU_USER_MAPPED(unsigned long, espfix_waddr);
+DEFINE_PER_CPU_READ_MOSTLY(unsigned long, espfix_stack);
+DEFINE_PER_CPU_READ_MOSTLY(unsigned long, espfix_waddr);
 
 /* Initialization mutex - should this be a spinlock? */
 static DEFINE_MUTEX(espfix_init_mutex);
@@ -226,10 +226,4 @@ void init_espfix_ap(int cpu)
 	per_cpu(espfix_stack, cpu) = addr;
 	per_cpu(espfix_waddr, cpu) = (unsigned long)stack_page
 				      + (addr & ~PAGE_MASK);
-	/*
-	 * _PAGE_GLOBAL is not really required.  This is not a hot
-	 * path, but we do it here for consistency.
-	 */
-	kaiser_add_mapping((unsigned long)stack_page, PAGE_SIZE,
-			__PAGE_KERNEL | _PAGE_GLOBAL);
 }

[PATCH 2/5] x86/mm/kaiser: Add a banner

[Peter Zijlstra]

[-- Attachment #1: peterz-kaiser-banner.patch --]
[-- Type: text/plain, Size: 433 bytes --]

So we can more easily see if the shiny got enabled.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/mm/kaiser.c |    2 ++
 1 file changed, 2 insertions(+)

--- a/arch/x86/mm/kaiser.c
+++ b/arch/x86/mm/kaiser.c
@@ -425,6 +425,8 @@ void __init kaiser_init(void)
 	if (!kaiser_enabled)
 		return;
 
+	printk("All your KAISER are belong to us\n");
+
 	kaiser_init_all_pgds();
 
 	for_each_possible_cpu(cpu) {

[PATCH 3/5] x86/mm/kaiser: Revert ("Map the entry stack variables")

[Peter Zijlstra]

[-- Attachment #1: peterz-kaiser-revert-syscall-entry-vars.patch --]
[-- Type: text/plain, Size: 1175 bytes --]

Because its not needed. The entry_SYSCALL_64 code isn't ran when
KAISER is enabled, we use entry_SYSCALL_64_trampoline instead.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/cpu/common.c |    2 +-
 arch/x86/kernel/process_64.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1515,7 +1515,7 @@ EXPORT_PER_CPU_SYMBOL(__preempt_count);
  * the top of the kernel stack.  Use an extra percpu variable to track the
  * top of the kernel stack directly.
  */
-DEFINE_PER_CPU_USER_MAPPED(unsigned long, cpu_current_top_of_stack) =
+DEFINE_PER_CPU(unsigned long, cpu_current_top_of_stack) =
 	(unsigned long)&init_thread_union + THREAD_SIZE;
 EXPORT_PER_CPU_SYMBOL(cpu_current_top_of_stack);
 
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -59,7 +59,7 @@
 #include <asm/unistd_32_ia32.h>
 #endif
 
-__visible DEFINE_PER_CPU_USER_MAPPED(unsigned long, rsp_scratch);
+__visible DEFINE_PER_CPU(unsigned long, rsp_scratch);
 
 /* Prints also some state that isn't saved in the pt_regs */
 void __show_regs(struct pt_regs *regs, int all)

[PATCH 4/5] x86/mm/kaiser: Remove superfluous SWITCH_TO_KERNEL

[Peter Zijlstra]

[-- Attachment #1: peterz-kaiser-fixup-syscall-switch.patch --]
[-- Type: text/plain, Size: 817 bytes --]

We never use this code-path with KAISER enabled.

Fixes: ("Prepare the x86/entry assembly code for entry/exit CR3 switching")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/entry_64.S |    8 --------
 1 file changed, 8 deletions(-)

--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -201,14 +201,6 @@ ENTRY(entry_SYSCALL_64)
 
 	swapgs
 	movq	%rsp, PER_CPU_VAR(rsp_scratch)
-
-	/*
-	 * The kernel CR3 is needed to map the process stack, but we
-	 * need a scratch register to be able to load CR3.  %rsp is
-	 * clobberable right now, so use it as a scratch register.
-	 * %rsp will look crazy here for a couple instructions.
-	 */
-	SWITCH_TO_KERNEL_CR3 scratch_reg=%rsp
 	movq	PER_CPU_VAR(cpu_current_top_of_stack), %rsp
 
 	/* Construct struct pt_regs on stack */

[PATCH 5/5] x86/mm/kaiser: Disable the SYSCALL-64 trampoline along with KAISER

[Peter Zijlstra]

[-- Attachment #1: peterz-kaiser-disable-syscall-trampoline.patch --]
[-- Type: text/plain, Size: 738 bytes --]

If we don't use KAISER we don't need the trampoline, use the old entry
code.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/cpu/common.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1442,7 +1442,10 @@ void syscall_init(void)
 		(entry_SYSCALL_64_trampoline - _entry_trampoline);
 
 	wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
-	wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
+	if (kaiser_enabled)
+		wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
+	else
+		wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
 
 #ifdef CONFIG_IA32_EMULATION
 	wrmsrl(MSR_CSTAR, (unsigned long)entry_SYSCALL_compat);

Re: [PATCH 1/5] x86/mm/kaiser: Alternative ESPFIX

[Peter Zijlstra]

On Mon, Nov 27, 2017 at 11:31:11PM +0100, Peter Zijlstra wrote:
> Change the asm to do the CR3 switcheroo so we can remove the magic
> mappings.
> 
> Since RDI is unused after SWAPGS we can use it as a scratch reg for
> SWITCH_TO_KERNEL. And once we've computed the new RSP (in RAX) we no
> longer need RDI and can again use it as scratch reg for
> SWITCH_TO_USER.

Forgot to note; this passes tools/testing/selftests/x86/sigreturn_64.

> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH 1/5] x86/mm/kaiser: Alternative ESPFIX

[Dave Hansen]

On 11/27/2017 02:31 PM, Peter Zijlstra wrote:
> Change the asm to do the CR3 switcheroo so we can remove the magic
> mappings.
> 
> Since RDI is unused after SWAPGS we can use it as a scratch reg for
> SWITCH_TO_KERNEL. And once we've computed the new RSP (in RAX) we no
> longer need RDI and can again use it as scratch reg for
> SWITCH_TO_USER.

This definitely looks like the right thing.  Either I missed something
obvious before, or Andy's entry rework made this much more obviously
correct to do the simple thing here.

Re: [PATCH 4/5] x86/mm/kaiser: Remove superfluous SWITCH_TO_KERNEL

[Dave Hansen]

On 11/27/2017 02:31 PM, Peter Zijlstra wrote:
> We never use this code-path with KAISER enabled.
...
> @@ -201,14 +201,6 @@ ENTRY(entry_SYSCALL_64)
>  
>  	swapgs
>  	movq	%rsp, PER_CPU_VAR(rsp_scratch)
> -
> -	/*
> -	 * The kernel CR3 is needed to map the process stack, but we
> -	 * need a scratch register to be able to load CR3.  %rsp is
> -	 * clobberable right now, so use it as a scratch register.
> -	 * %rsp will look crazy here for a couple instructions.
> -	 */
> -	SWITCH_TO_KERNEL_CR3 scratch_reg=%rsp
>  	movq	PER_CPU_VAR(cpu_current_top_of_stack), %rsp

What's the mechanism that we use to switch between the two versions of
the SYSCALL entry?  It wasn't obvious from some grepping.

Re: [PATCH 4/5] x86/mm/kaiser: Remove superfluous SWITCH_TO_KERNEL

[Peter Zijlstra]

On Mon, Nov 27, 2017 at 02:47:08PM -0800, Dave Hansen wrote:
> On 11/27/2017 02:31 PM, Peter Zijlstra wrote:
> > We never use this code-path with KAISER enabled.
> ...
> > @@ -201,14 +201,6 @@ ENTRY(entry_SYSCALL_64)
> >  
> >  	swapgs
> >  	movq	%rsp, PER_CPU_VAR(rsp_scratch)
> > -
> > -	/*
> > -	 * The kernel CR3 is needed to map the process stack, but we
> > -	 * need a scratch register to be able to load CR3.  %rsp is
> > -	 * clobberable right now, so use it as a scratch register.
> > -	 * %rsp will look crazy here for a couple instructions.
> > -	 */
> > -	SWITCH_TO_KERNEL_CR3 scratch_reg=%rsp
> >  	movq	PER_CPU_VAR(cpu_current_top_of_stack), %rsp
> 
> What's the mechanism that we use to switch between the two versions of
> the SYSCALL entry?  It wasn't obvious from some grepping.

the next patch, the code in tip will in fact never use this code.

Re: [PATCH 5/5] x86/mm/kaiser: Disable the SYSCALL-64 trampoline along with KAISER

[Dave Hansen]

On 11/27/2017 02:31 PM, Peter Zijlstra wrote:
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1442,7 +1442,10 @@ void syscall_init(void)
>  		(entry_SYSCALL_64_trampoline - _entry_trampoline);
>  
>  	wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
> -	wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
> +	if (kaiser_enabled)
> +		wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
> +	else
> +		wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);

Heh, ask and ye shall receive, I guess.

We do need a Documentation/ update now.  For this, and other things.
I'll put something together.

Re: [PATCH 2/5] x86/mm/kaiser: Add a banner

[Andy Lutomirski]

On Mon, Nov 27, 2017 at 2:31 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> So we can more easily see if the shiny got enabled.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/mm/kaiser.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> --- a/arch/x86/mm/kaiser.c
> +++ b/arch/x86/mm/kaiser.c
> @@ -425,6 +425,8 @@ void __init kaiser_init(void)
>         if (!kaiser_enabled)
>                 return;
>
> +       printk("All your KAISER are belong to us\n");
> +

All your incomprehensible academic names are belong to us.

On a serious note, can we please banish the name KAISER from all the
user-facing bits?  No one should be setting a boot option that has a
name based on an academic project called "Kernel Address Isolation to
have Side-channels Efficiently Removed".  We're not efficiently
removing side channels.  The side channels are still very much there.
Heck, the series as currently presented doesn't even rescue kASLR.  It
could*, if we were to finish the work that I mostly started and
completely banish all the normal kernel mappings from the shadow**
tables.  We're rather inefficiently (and partially!) mitigating the
fact that certain CPU designers have had their heads up their
collective arses for *years* and have failed to pay attention to
numerous academic papers documenting that fact.

Let's call the user facing bits "separate user pagetables".  If we
want to make it conditioned on a future cpu cap called
X86_BUG_REALLY_DUMB_SIDE_CHANNELS, great, assuming a better CPU ever
shows up.  But please let's not make users look up WTF "KAISER" means.

* No one ever documented the %*!& side channels AFAIK, so everything
we're talking about here is mostly speculation.

** The word "shadow" needs to die, too.  I know what shadow page
tables are, and they have *nothing* to do with KAISER.

Re: [PATCH 2/5] x86/mm/kaiser: Add a banner

[Josh Poimboeuf]

On Mon, Nov 27, 2017 at 07:36:40PM -0800, Andy Lutomirski wrote:
> On Mon, Nov 27, 2017 at 2:31 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > So we can more easily see if the shiny got enabled.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  arch/x86/mm/kaiser.c |    2 ++
> >  1 file changed, 2 insertions(+)
> >
> > --- a/arch/x86/mm/kaiser.c
> > +++ b/arch/x86/mm/kaiser.c
> > @@ -425,6 +425,8 @@ void __init kaiser_init(void)
> >         if (!kaiser_enabled)
> >                 return;
> >
> > +       printk("All your KAISER are belong to us\n");
> > +
> 
> All your incomprehensible academic names are belong to us.
> 
> On a serious note, can we please banish the name KAISER from all the
> user-facing bits?  No one should be setting a boot option that has a
> name based on an academic project called "Kernel Address Isolation to
> have Side-channels Efficiently Removed".  We're not efficiently
> removing side channels.  The side channels are still very much there.
> Heck, the series as currently presented doesn't even rescue kASLR.  It
> could*, if we were to finish the work that I mostly started and
> completely banish all the normal kernel mappings from the shadow**
> tables.  We're rather inefficiently (and partially!) mitigating the
> fact that certain CPU designers have had their heads up their
> collective arses for *years* and have failed to pay attention to
> numerous academic papers documenting that fact.
> 
> Let's call the user facing bits "separate user pagetables".  If we
> want to make it conditioned on a future cpu cap called
> X86_BUG_REALLY_DUMB_SIDE_CHANNELS, great, assuming a better CPU ever
> shows up.  But please let's not make users look up WTF "KAISER" means.
> 
> * No one ever documented the %*!& side channels AFAIK, so everything
> we're talking about here is mostly speculation.
> 
> ** The word "shadow" needs to die, too.  I know what shadow page
> tables are, and they have *nothing* to do with KAISER.

+1.  Somebody please rename KAISER and shadow page tables for more
clarity.

To fix KASLR I think we need to move (at least parts of) .entry.text,
.irqentry.text, and .entry_trampoline into their own fixed section(s).
Is there anything else missing?

-- 
Josh

Re: [PATCH 2/5] x86/mm/kaiser: Add a banner

[Andy Lutomirski]

On Mon, Nov 27, 2017 at 9:03 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Mon, Nov 27, 2017 at 07:36:40PM -0800, Andy Lutomirski wrote:
>> On Mon, Nov 27, 2017 at 2:31 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > So we can more easily see if the shiny got enabled.
>> >
>> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>> > ---
>> >  arch/x86/mm/kaiser.c |    2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > --- a/arch/x86/mm/kaiser.c
>> > +++ b/arch/x86/mm/kaiser.c
>> > @@ -425,6 +425,8 @@ void __init kaiser_init(void)
>> >         if (!kaiser_enabled)
>> >                 return;
>> >
>> > +       printk("All your KAISER are belong to us\n");
>> > +
>>
>> All your incomprehensible academic names are belong to us.
>>
>> On a serious note, can we please banish the name KAISER from all the
>> user-facing bits?  No one should be setting a boot option that has a
>> name based on an academic project called "Kernel Address Isolation to
>> have Side-channels Efficiently Removed".  We're not efficiently
>> removing side channels.  The side channels are still very much there.
>> Heck, the series as currently presented doesn't even rescue kASLR.  It
>> could*, if we were to finish the work that I mostly started and
>> completely banish all the normal kernel mappings from the shadow**
>> tables.  We're rather inefficiently (and partially!) mitigating the
>> fact that certain CPU designers have had their heads up their
>> collective arses for *years* and have failed to pay attention to
>> numerous academic papers documenting that fact.
>>
>> Let's call the user facing bits "separate user pagetables".  If we
>> want to make it conditioned on a future cpu cap called
>> X86_BUG_REALLY_DUMB_SIDE_CHANNELS, great, assuming a better CPU ever
>> shows up.  But please let's not make users look up WTF "KAISER" means.
>>
>> * No one ever documented the %*!& side channels AFAIK, so everything
>> we're talking about here is mostly speculation.
>>
>> ** The word "shadow" needs to die, too.  I know what shadow page
>> tables are, and they have *nothing* to do with KAISER.
>
> +1.  Somebody please rename KAISER and shadow page tables for more
> clarity.
>
> To fix KASLR I think we need to move (at least parts of) .entry.text,
> .irqentry.text, and .entry_trampoline into their own fixed section(s).
> Is there anything else missing?

We need to completely eliminate anything that maps normal kernel
addresses into the usermode tables.

>
> --
> Josh

Re: [PATCH 2/5] x86/mm/kaiser: Add a banner

[Borislav Petkov]

On Mon, Nov 27, 2017 at 07:36:40PM -0800, Andy Lutomirski wrote:
> ** The word "shadow" needs to die, too.  I know what shadow page
> tables are, and they have *nothing* to do with KAISER.

ACK to that. Calling them "shadow" is mishandling an already overloaded
term.

Let's call them the user page tables as we call the other the kernel
page tables already. Nicely balanced.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.