Re: [PATCH v1 3/8] x86/entry/clearregs: Clear registers for 64bit SYSCALL

[Josh Poimboeuf]

On Tue, Jan 09, 2018 at 05:03:23PM -0800, Andi Kleen wrote:
> From: Andi Kleen 
> 
> We clear all the non argument registers for 64bit SYSCALLs
> to minimize any risk of bad speculation using user values.
> 
> So far unused argument registers still leak. To be addressed
> in future patches.
> 
> Signed-off-by: Andi Kleen 
> ---
>  arch/x86/entry/entry_64.S | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index bbdfbdd817d6..632081fd7086 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -236,6 +236,14 @@ GLOBAL(entry_SYSCALL_64_after_hwframe)
>   pushq   %r11/* pt_regs->r11 */
>   sub $(6*8), %rsp
>   SAVE_EXTRA_REGS
> + /* Sanitize registers against speculation attacks */

This comment isn't necessary, though it would be good to add comments
above the CLEAR macros themselves explaining why they're needed.

> + /* r10 is cleared later, arguments are handled in san_args* */

What is san_args?

> + CLEAR_R11_TO_R15
> +#ifndef CONFIG_FRAME_POINTER
> + xor %ebp, %ebp
> +#endif

Why is %rbp not cleared with CONFIG_FRAME_POINTER?  Is it because it
will get clobbered by the first called function?

> + xor %ebx, %ebx
> + xor %ecx, %ecx

I think clearing %ecx isn't needed, it gets clobbered below for the fast
path, and gets clobbered by do_syscall_64() for the slow path.

>  
>   UNWIND_HINT_REGS extra=0
>  
> @@ -263,6 +271,7 @@ entry_SYSCALL_64_fastpath:
>  #endif
>   ja  1f  /* return -ENOSYS (already in 
> pt_regs->ax) */
>   movq%r10, %rcx
> + xor %r10, %r10
>  
>  #ifdef CONFIG_RETPOLINE
>   movqsys_call_table(, %rax, 8), %rax

Now that the fast path is getting slower, I wonder if it still makes
sense to have a "fast path"?  It would be good to see measurements
comparing the fast and slow paths.

-- 
Josh

Re: [PATCH v1 3/8] x86/entry/clearregs: Clear registers for 64bit SYSCALL

[Josh Poimboeuf]

On Tue, Jan 09, 2018 at 05:03:23PM -0800, Andi Kleen wrote:
> From: Andi Kleen 
> 
> We clear all the non argument registers for 64bit SYSCALLs
> to minimize any risk of bad speculation using user values.
> 
> So far unused argument registers still leak. To be addressed
> in future patches.
> 
> Signed-off-by: Andi Kleen 
> ---
>  arch/x86/entry/entry_64.S | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index bbdfbdd817d6..632081fd7086 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -236,6 +236,14 @@ GLOBAL(entry_SYSCALL_64_after_hwframe)
>   pushq   %r11/* pt_regs->r11 */
>   sub $(6*8), %rsp
>   SAVE_EXTRA_REGS
> + /* Sanitize registers against speculation attacks */

This comment isn't necessary, though it would be good to add comments
above the CLEAR macros themselves explaining why they're needed.

> + /* r10 is cleared later, arguments are handled in san_args* */

What is san_args?

> + CLEAR_R11_TO_R15
> +#ifndef CONFIG_FRAME_POINTER
> + xor %ebp, %ebp
> +#endif

Why is %rbp not cleared with CONFIG_FRAME_POINTER?  Is it because it
will get clobbered by the first called function?

> + xor %ebx, %ebx
> + xor %ecx, %ecx

I think clearing %ecx isn't needed, it gets clobbered below for the fast
path, and gets clobbered by do_syscall_64() for the slow path.

>  
>   UNWIND_HINT_REGS extra=0
>  
> @@ -263,6 +271,7 @@ entry_SYSCALL_64_fastpath:
>  #endif
>   ja  1f  /* return -ENOSYS (already in 
> pt_regs->ax) */
>   movq%r10, %rcx
> + xor %r10, %r10
>  
>  #ifdef CONFIG_RETPOLINE
>   movqsys_call_table(, %rax, 8), %rax

Now that the fast path is getting slower, I wonder if it still makes
sense to have a "fast path"?  It would be good to see measurements
comparing the fast and slow paths.

-- 
Josh

Re: [PATCH v1 3/8] x86/entry/clearregs: Clear registers for 64bit SYSCALL

[Andi Kleen]

On Wed, Jan 10, 2018 at 10:35:58PM -0500, Brian Gerst wrote:
> > @@ -263,6 +271,7 @@ entry_SYSCALL_64_fastpath:
> >  #endif
> > ja  1f  /* return -ENOSYS (already 
> > in pt_regs->ax) */
> > movq%r10, %rcx
> > +   xor %r10, %r10
> 
> RCX is already clear, so xchgq %r10, %rcx will be simpler.

XOR is special cased by the hardware, so it's always more
efficient.

-Andi
> 

Re: [PATCH v1 3/8] x86/entry/clearregs: Clear registers for 64bit SYSCALL

[Andi Kleen]

On Wed, Jan 10, 2018 at 10:35:58PM -0500, Brian Gerst wrote:
> > @@ -263,6 +271,7 @@ entry_SYSCALL_64_fastpath:
> >  #endif
> > ja  1f  /* return -ENOSYS (already 
> > in pt_regs->ax) */
> > movq%r10, %rcx
> > +   xor %r10, %r10
> 
> RCX is already clear, so xchgq %r10, %rcx will be simpler.

XOR is special cased by the hardware, so it's always more
efficient.

-Andi
> 

Re: [PATCH v1 3/8] x86/entry/clearregs: Clear registers for 64bit SYSCALL

[Brian Gerst]

On Tue, Jan 9, 2018 at 8:03 PM, Andi Kleen  wrote:
> From: Andi Kleen 
>
> We clear all the non argument registers for 64bit SYSCALLs
> to minimize any risk of bad speculation using user values.
>
> So far unused argument registers still leak. To be addressed
> in future patches.
>
> Signed-off-by: Andi Kleen 
> ---
>  arch/x86/entry/entry_64.S | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index bbdfbdd817d6..632081fd7086 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -236,6 +236,14 @@ GLOBAL(entry_SYSCALL_64_after_hwframe)
> pushq   %r11/* pt_regs->r11 */
> sub $(6*8), %rsp
> SAVE_EXTRA_REGS
> +   /* Sanitize registers against speculation attacks */
> +   /* r10 is cleared later, arguments are handled in san_args* */
> +   CLEAR_R11_TO_R15

Don't need to explicitly clear R11 here.  It is clobbered with current_task.

> +#ifndef CONFIG_FRAME_POINTER
> +   xor %ebp, %ebp
> +#endif
> +   xor %ebx, %ebx
> +   xor %ecx, %ecx
>
> UNWIND_HINT_REGS extra=0
>
> @@ -263,6 +271,7 @@ entry_SYSCALL_64_fastpath:
>  #endif
> ja  1f  /* return -ENOSYS (already in 
> pt_regs->ax) */
> movq%r10, %rcx
> +   xor %r10, %r10

RCX is already clear, so xchgq %r10, %rcx will be simpler.

--
Brian Gerst

Re: [PATCH v1 3/8] x86/entry/clearregs: Clear registers for 64bit SYSCALL

[Brian Gerst]

On Tue, Jan 9, 2018 at 8:03 PM, Andi Kleen  wrote:
> From: Andi Kleen 
>
> We clear all the non argument registers for 64bit SYSCALLs
> to minimize any risk of bad speculation using user values.
>
> So far unused argument registers still leak. To be addressed
> in future patches.
>
> Signed-off-by: Andi Kleen 
> ---
>  arch/x86/entry/entry_64.S | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index bbdfbdd817d6..632081fd7086 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -236,6 +236,14 @@ GLOBAL(entry_SYSCALL_64_after_hwframe)
> pushq   %r11/* pt_regs->r11 */
> sub $(6*8), %rsp
> SAVE_EXTRA_REGS
> +   /* Sanitize registers against speculation attacks */
> +   /* r10 is cleared later, arguments are handled in san_args* */
> +   CLEAR_R11_TO_R15

Don't need to explicitly clear R11 here.  It is clobbered with current_task.

> +#ifndef CONFIG_FRAME_POINTER
> +   xor %ebp, %ebp
> +#endif
> +   xor %ebx, %ebx
> +   xor %ecx, %ecx
>
> UNWIND_HINT_REGS extra=0
>
> @@ -263,6 +271,7 @@ entry_SYSCALL_64_fastpath:
>  #endif
> ja  1f  /* return -ENOSYS (already in 
> pt_regs->ax) */
> movq%r10, %rcx
> +   xor %r10, %r10

RCX is already clear, so xchgq %r10, %rcx will be simpler.

--
Brian Gerst

[PATCH v1 3/8] x86/entry/clearregs: Clear registers for 64bit SYSCALL

[Andi Kleen]

From: Andi Kleen 

We clear all the non argument registers for 64bit SYSCALLs
to minimize any risk of bad speculation using user values.

So far unused argument registers still leak. To be addressed
in future patches.

Signed-off-by: Andi Kleen 
---
 arch/x86/entry/entry_64.S | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index bbdfbdd817d6..632081fd7086 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -236,6 +236,14 @@ GLOBAL(entry_SYSCALL_64_after_hwframe)
pushq   %r11/* pt_regs->r11 */
sub $(6*8), %rsp
SAVE_EXTRA_REGS
+   /* Sanitize registers against speculation attacks */
+   /* r10 is cleared later, arguments are handled in san_args* */
+   CLEAR_R11_TO_R15
+#ifndef CONFIG_FRAME_POINTER
+   xor %ebp, %ebp
+#endif
+   xor %ebx, %ebx
+   xor %ecx, %ecx
 
UNWIND_HINT_REGS extra=0
 
@@ -263,6 +271,7 @@ entry_SYSCALL_64_fastpath:
 #endif
ja  1f  /* return -ENOSYS (already in 
pt_regs->ax) */
movq%r10, %rcx
+   xor %r10, %r10
 
 #ifdef CONFIG_RETPOLINE
movqsys_call_table(, %rax, 8), %rax
-- 
2.14.3

[PATCH v1 3/8] x86/entry/clearregs: Clear registers for 64bit SYSCALL

[Andi Kleen]

From: Andi Kleen 

We clear all the non argument registers for 64bit SYSCALLs
to minimize any risk of bad speculation using user values.

So far unused argument registers still leak. To be addressed
in future patches.

Signed-off-by: Andi Kleen 
---
 arch/x86/entry/entry_64.S | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index bbdfbdd817d6..632081fd7086 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -236,6 +236,14 @@ GLOBAL(entry_SYSCALL_64_after_hwframe)
pushq   %r11/* pt_regs->r11 */
sub $(6*8), %rsp
SAVE_EXTRA_REGS
+   /* Sanitize registers against speculation attacks */
+   /* r10 is cleared later, arguments are handled in san_args* */
+   CLEAR_R11_TO_R15
+#ifndef CONFIG_FRAME_POINTER
+   xor %ebp, %ebp
+#endif
+   xor %ebx, %ebx
+   xor %ecx, %ecx
 
UNWIND_HINT_REGS extra=0
 
@@ -263,6 +271,7 @@ entry_SYSCALL_64_fastpath:
 #endif
ja  1f  /* return -ENOSYS (already in 
pt_regs->ax) */
movq%r10, %rcx
+   xor %r10, %r10
 
 #ifdef CONFIG_RETPOLINE
movqsys_call_table(, %rax, 8), %rax
-- 
2.14.3