Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1345503 > unrolled thread

[PATCH 00/10] x86: Various SYSENTER/SYSEXIT/#DB fixes and cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/10] x86: Various SYSENTER/SYSEXIT/#DB fixes and cleanups Andy Lutomirski <luto@kernel.org> - 2016-02-29 06:40 +0100
    [PATCH 06/10] x86/traps: Clear DR6 early in do_debug and improve the comment Andy Lutomirski <luto@kernel.org> - 2016-02-29 06:40 +0100
    [PATCH 08/10] x86/entry: Only allocate space for SYSENTER_stack if needed Andy Lutomirski <luto@kernel.org> - 2016-02-29 06:40 +0100
    [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing below the existing FLAGS test Andy Lutomirski <luto@kernel.org> - 2016-02-29 06:40 +0100
      Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing  below the existing FLAGS test Borislav Petkov <bp@alien8.de> - 2016-02-29 21:40 +0100
        Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing  below the existing FLAGS test Andy Lutomirski <luto@amacapital.net> - 2016-02-29 21:50 +0100
          Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing  below the existing FLAGS test Borislav Petkov <bp@alien8.de> - 2016-02-29 23:10 +0100
            Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing  below the existing FLAGS test Borislav Petkov <bp@alien8.de> - 2016-02-29 23:40 +0100
            Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing  below the existing FLAGS test Brian Gerst <brgerst@gmail.com> - 2016-02-29 23:40 +0100
    Re: [PATCH 00/10] x86: Various SYSENTER/SYSEXIT/#DB fixes and cleanups Andy Lutomirski <luto@amacapital.net> - 2016-02-29 20:00 +0100

#1345503 — [PATCH 00/10] x86: Various SYSENTER/SYSEXIT/#DB fixes and cleanups

hpa asked me to get rid of the ASM_CLAC at the beginning of the SYSENTER
path.  Little did he know...

This series makes the observed behavior of SYSENTER wrt flags the same
for all sane flags and kernel bitnesses.  That is, SYSENTER preserves
flags now unless you do a syscall that explicitly changes flags, and
the HW flags that the syscall executes with are sanitized.  This
includes NT, TF, AC and all arithmetic flags.  Prior to this series,
32-bit kernels clobbered TF and the arithmetic flags and behaved
highly erratically if NT was set.  (If IF is cleared by evil userspace
when SYSENTER starts, IF will be set again on return.  There's nothing
the kernel can do about this -- SYSENTER inherently forgets the state
of IF.)

This series speeds up SYSENTER on all kernels by a surprisingly large
amount on Skylake because it eliminates an unconditional CLAC.

While SYSENTER used to handle TF correctly as far as I can tell on
64-bit kernels, the means by which it did so was heavily tangled up in
the ptrace single-step logic.  It now works just like all the other
kernel entries except insofar as do_debug has a simple special case
for it.  Relatedly, the bizarre and poorly explained old fixup in
do_debug is now hidden behind a WARN_ON_ONCE in preparation for
deleting it at some point.

The code that fixed up NMI and #DB early in SYSENTER in 32-bit kernels
used to be both terrifying and incorrect.  (It doesn't appear to have
been exploitably bad, but the reason for that is subtle, and the code
was certainy more fragile than it deserved to me.)  We still need a
special fixup, but it's much simpler now.

While I was doing all this, I also noticed that DR6 and BTF handling
in do_debug was a bit off.  Two of the patches in here try to fix it
up.

Have fun!

tl;dr: Cleanups and sanity fixes here, but no security fixes, and I
don't think anything needs to be backported or put in x86/urgent.

This series applies to the result of merging tip:x86/asm and
tip:x86/urgent.  I've been testing on a somewhat bastardized base,
because tip currently doesn't work on my laptop in 32-bit mode.  (That
bug is fixed in Linus' tree.)

Andy Lutomirski (10):
  selftests/x86: In syscall_nt, test NT|TF as well
  x86/entry/compat: In SYSENTER, sink AC clearing below the existing
    FLAGS test
  x86/entry/32: Filter NT and speed up AC filtering in SYSENTER
  x86/entry/32: Restore FLAGS on SYSEXIT
  x86/traps: Clear TIF_BLOCKSTEP on all debug exceptions
  x86/traps: Clear DR6 early in do_debug and improve the comment
  x86/entry: Vastly simplify SYSENTER TF handling
  x86/entry: Only allocate space for SYSENTER_stack if needed
  x86/entry/32: Simplify and fix up the SYSENTER stack #DB/NMI fixup
  x86/entry/32: Add and check a stack canary for the SYSENTER stack

 arch/x86/entry/entry_32.S                | 182 ++++++++++++++++++-------------
 arch/x86/entry/entry_64_compat.S         |  15 ++-
 arch/x86/include/asm/processor.h         |   5 +-
 arch/x86/include/asm/proto.h             |  15 ++-
 arch/x86/kernel/asm-offsets_32.c         |   5 +
 arch/x86/kernel/process.c                |   3 +
 arch/x86/kernel/traps.c                  |  87 ++++++++++++---
 tools/testing/selftests/x86/syscall_nt.c |  57 ++++++++--
 8 files changed, 263 insertions(+), 106 deletions(-)

-- 
2.5.0

[toc] | [next] | [standalone]

#1345504 — [PATCH 06/10] x86/traps: Clear DR6 early in do_debug and improve the comment

Leaving any bits set in DR6 on return from a debug exception is
asking for trouble.  Prevent it by writing zero right away and
clarify the comment.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/traps.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 19e6cfa501e3..6dddc220e3ed 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -593,6 +593,18 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	ist_enter(regs);
 
 	get_debugreg(dr6, 6);
+	/*
+	 * The Intel SDM says:
+	 *
+	 *   Certain debug exceptions may clear bits 0-3. The remaining
+	 *   contents of the DR6 register are never cleared by the
+	 *   processor. To avoid confusion in identifying debug
+	 *   exceptions, debug handlers should clear the register before
+	 *   returning to the interrupted task.
+	 *
+	 * Keep it simple: clear DR6 immediately.
+	 */
+	set_debugreg(0, 6);
 
 	/* Filter out all the reserved bits which are preset to 1 */
 	dr6 &= ~DR6_RESERVED;
@@ -616,9 +628,6 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	if ((dr6 & DR_STEP) && kmemcheck_trap(regs))
 		goto exit;
 
-	/* DR6 may or may not be cleared by the CPU */
-	set_debugreg(0, 6);
-
 	/* Store the virtualized DR6 value */
 	tsk->thread.debugreg6 = dr6;
 
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1345505 — [PATCH 08/10] x86/entry: Only allocate space for SYSENTER_stack if needed

The SYSENTER stack is only used on 32-bit kernels.  Remove it in
64-bit kernels.

(We may end up using it down the road on 64-bit kernels.  If so,
 we'll re-enable it for CONFIG_IA32_EMULATION.)

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/processor.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index ecb410310e70..7cd01b71b5bd 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -297,10 +297,12 @@ struct tss_struct {
 	 */
 	unsigned long		io_bitmap[IO_BITMAP_LONGS + 1];
 
+#ifdef CONFIG_X86_32
 	/*
 	 * Space for the temporary SYSENTER stack:
 	 */
 	unsigned long		SYSENTER_stack[64];
+#endif
 
 } ____cacheline_aligned;
 
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1345506 — [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing below the existing FLAGS test

CLAC is slow, and the SYSENTER code already has an unlikely path
that runs if unusual flags are set.  Drop the CLAC and instead rely
on the unlikely path to clear AC.

This seems to save ~24 cycles on my Skylake laptop.  (Hey, Intel,
make this faster please!)

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/entry/entry_64_compat.S | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 89bcb4979e7a..7c8e72da7654 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -66,8 +66,6 @@ ENTRY(entry_SYSENTER_compat)
 	 */
 	pushfq				/* pt_regs->flags (except IF = 0) */
 	orl	$X86_EFLAGS_IF, (%rsp)	/* Fix saved flags */
-	ASM_CLAC			/* Clear AC after saving FLAGS */
-
 	pushq	$__USER32_CS		/* pt_regs->cs */
 	xorq    %r8,%r8
 	pushq	%r8			/* pt_regs->ip = 0 (placeholder) */
@@ -90,9 +88,9 @@ ENTRY(entry_SYSENTER_compat)
 	cld
 
 	/*
-	 * Sysenter doesn't filter flags, so we need to clear NT
+	 * Sysenter doesn't filter flags, so we need to clear NT and AC
 	 * ourselves.  To save a few cycles, we can check whether
-	 * NT was set instead of doing an unconditional popfq.
+	 * either was set instead of doing an unconditional popfq.
 	 * This needs to happen before enabling interrupts so that
 	 * we don't get preempted with NT set.
 	 *
@@ -102,7 +100,7 @@ ENTRY(entry_SYSENTER_compat)
 	 * we're keeping that code behind a branch which will predict as
 	 * not-taken and therefore its instructions won't be fetched.
 	 */
-	testl	$X86_EFLAGS_NT, EFLAGS(%rsp)
+	testl	$X86_EFLAGS_NT|X86_EFLAGS_AC, EFLAGS(%rsp)
 	jnz	.Lsysenter_fix_flags
 .Lsysenter_flags_fixed:
 
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1346144 — Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing below the existing FLAGS test

On Sun, Feb 28, 2016 at 09:28:47PM -0800, Andy Lutomirski wrote:
> CLAC is slow, and the SYSENTER code already has an unlikely path
> that runs if unusual flags are set.  Drop the CLAC and instead rely
> on the unlikely path to clear AC.
> 
> This seems to save ~24 cycles on my Skylake laptop.  (Hey, Intel,
> make this faster please!)
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/entry/entry_64_compat.S | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
> index 89bcb4979e7a..7c8e72da7654 100644
> --- a/arch/x86/entry/entry_64_compat.S
> +++ b/arch/x86/entry/entry_64_compat.S
> @@ -66,8 +66,6 @@ ENTRY(entry_SYSENTER_compat)
>  	 */
>  	pushfq				/* pt_regs->flags (except IF = 0) */
>  	orl	$X86_EFLAGS_IF, (%rsp)	/* Fix saved flags */
> -	ASM_CLAC			/* Clear AC after saving FLAGS */
> -
>  	pushq	$__USER32_CS		/* pt_regs->cs */
>  	xorq    %r8,%r8
>  	pushq	%r8			/* pt_regs->ip = 0 (placeholder) */
> @@ -90,9 +88,9 @@ ENTRY(entry_SYSENTER_compat)
>  	cld
>  
>  	/*
> -	 * Sysenter doesn't filter flags, so we need to clear NT
> +	 * Sysenter doesn't filter flags, so we need to clear NT and AC
>  	 * ourselves.  To save a few cycles, we can check whether
> -	 * NT was set instead of doing an unconditional popfq.
> +	 * either was set instead of doing an unconditional popfq.
>  	 * This needs to happen before enabling interrupts so that
>  	 * we don't get preempted with NT set.
>  	 *
> @@ -102,7 +100,7 @@ ENTRY(entry_SYSENTER_compat)
>  	 * we're keeping that code behind a branch which will predict as
>  	 * not-taken and therefore its instructions won't be fetched.
>  	 */
> -	testl	$X86_EFLAGS_NT, EFLAGS(%rsp)
> +	testl	$X86_EFLAGS_NT|X86_EFLAGS_AC, EFLAGS(%rsp)
>  	jnz	.Lsysenter_fix_flags
>  .Lsysenter_flags_fixed:

Do I see it correctly that with this change, that .Lsysenter_fix_flags:
is going to be visited each time on SMAP machines and then we can get
rid of it? The reason for it was not to pollute I$ as the comment says
but that happening now anyway...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1346155 — Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing below the existing FLAGS test

On Mon, Feb 29, 2016 at 12:39 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Sun, Feb 28, 2016 at 09:28:47PM -0800, Andy Lutomirski wrote:
>> CLAC is slow, and the SYSENTER code already has an unlikely path
>> that runs if unusual flags are set.  Drop the CLAC and instead rely
>> on the unlikely path to clear AC.
>>
>> This seems to save ~24 cycles on my Skylake laptop.  (Hey, Intel,
>> make this faster please!)
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/entry/entry_64_compat.S | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
>> index 89bcb4979e7a..7c8e72da7654 100644
>> --- a/arch/x86/entry/entry_64_compat.S
>> +++ b/arch/x86/entry/entry_64_compat.S
>> @@ -66,8 +66,6 @@ ENTRY(entry_SYSENTER_compat)
>>        */
>>       pushfq                          /* pt_regs->flags (except IF = 0) */
>>       orl     $X86_EFLAGS_IF, (%rsp)  /* Fix saved flags */
>> -     ASM_CLAC                        /* Clear AC after saving FLAGS */
>> -
>>       pushq   $__USER32_CS            /* pt_regs->cs */
>>       xorq    %r8,%r8
>>       pushq   %r8                     /* pt_regs->ip = 0 (placeholder) */
>> @@ -90,9 +88,9 @@ ENTRY(entry_SYSENTER_compat)
>>       cld
>>
>>       /*
>> -      * Sysenter doesn't filter flags, so we need to clear NT
>> +      * Sysenter doesn't filter flags, so we need to clear NT and AC
>>        * ourselves.  To save a few cycles, we can check whether
>> -      * NT was set instead of doing an unconditional popfq.
>> +      * either was set instead of doing an unconditional popfq.
>>        * This needs to happen before enabling interrupts so that
>>        * we don't get preempted with NT set.
>>        *
>> @@ -102,7 +100,7 @@ ENTRY(entry_SYSENTER_compat)
>>        * we're keeping that code behind a branch which will predict as
>>        * not-taken and therefore its instructions won't be fetched.
>>        */
>> -     testl   $X86_EFLAGS_NT, EFLAGS(%rsp)
>> +     testl   $X86_EFLAGS_NT|X86_EFLAGS_AC, EFLAGS(%rsp)
>>       jnz     .Lsysenter_fix_flags
>>  .Lsysenter_flags_fixed:
>
> Do I see it correctly that with this change, that .Lsysenter_fix_flags:
> is going to be visited each time on SMAP machines and then we can get
> rid of it? The reason for it was not to pollute I$ as the comment says
> but that happening now anyway...
>

I don't think so.  Sensible user programs shouldn't set AC in the first place.

--Andy

[toc] | [prev] | [next] | [standalone]

#1346193 — Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing below the existing FLAGS test

On Mon, Feb 29, 2016 at 12:45:58PM -0800, Andy Lutomirski wrote:
> I don't think so.  Sensible user programs shouldn't set AC in the first place.

Then I'm most likely missing something: so before this patch, we did
unconditionally CLAC thus disallowing kernel access to user pages. Why
don't we need it anymore and need to pay attention only to user rFLAGS?

Especially since we do:

do_fast_syscall_32
|-> __get_user
   |-> __get_user_nocheck
      |-> __uaccess_begin which is stac()

Or are we saying, we don't need that CLAC in the beginning of
entry_SYSENTER_compat() at all because we're going to STAC anyway in
__get_user() ?

Hmmm...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1346221 — Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing below the existing FLAGS test

On Mon, Feb 29, 2016 at 05:33:16PM -0500, Brian Gerst wrote:
> SYSENTER doesn't save EFLAGS so we have to fudge it by pushing the
> kernel's flags.  Most user programs never set AC, so by adding it to
> the test for flags to clear we can avoid the CLAC or POPF in the
> common case that it is already clear.

Yeah, Andy just explained it to me on IRC. Thanks guys.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1346225 — Re: [PATCH 02/10] x86/entry/compat: In SYSENTER, sink AC clearing below the existing FLAGS test

On Mon, Feb 29, 2016 at 5:09 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Feb 29, 2016 at 12:45:58PM -0800, Andy Lutomirski wrote:
>> I don't think so.  Sensible user programs shouldn't set AC in the first place.
>
> Then I'm most likely missing something: so before this patch, we did
> unconditionally CLAC thus disallowing kernel access to user pages. Why
> don't we need it anymore and need to pay attention only to user rFLAGS?

SYSENTER doesn't save EFLAGS so we have to fudge it by pushing the
kernel's flags.  Most user programs never set AC, so by adding it to
the test for flags to clear we can avoid the CLAC or POPF in the
common case that it is already clear.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1346092

On Sun, Feb 28, 2016 at 9:28 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> While I was doing all this, I also noticed that DR6 and BTF handling
> in do_debug was a bit off.  Two of the patches in here try to fix it
> up.

It's worth noting that do_debug is really quite screwed up with or
without this patchset applied.  For example:

    /*
     * Let others (NMI) know that the debug stack is in use
     * as we may switch to the interrupt stack.
     */
    debug_stack_usage_inc();

    /* It's safe to allow irq's after DR6 has been saved */
    preempt_disable();
    cond_local_irq_enable(regs);

This has never really been valid.  It should be guarded by an
if(user_mode(regs)).  And we need to kill the die_notifier garbage in
here -- it makes it basically impossible to understand what's going
on.

--Andy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web