[PATCH] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[PATCH] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Andy Lutomirski]

Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
SYSENTER using the new C path"), we relied on a uaccess very early
in the SYSENTER path to clear AC.  After that change, though, we can
potentially make it all the way into C code with AC set, which
enlarges the attack surface for SMAP bypass by doing SYSENTER with
AC set.

Strengthen the SMAP protection by addding the missing ASM_CLAC right
at the beginning.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

This is probably an x86/urgent candidate.  It fixes a minor
hardening regression in 4.4.

It's lightly tested.  It's hard to test well right now because the
4.5 series is completely broken for 32-bit SMAP sytems.

arch/x86/entry/entry_32.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index f3facd40fd2d..9d6165c171eb 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -294,6 +294,7 @@ sysenter_past_esp:
 	pushl	$__USER_DS		/* pt_regs->ss */
 	pushl	%ebp			/* pt_regs->sp (stashed in bp) */
 	pushfl				/* pt_regs->flags (except IF = 0) */
+	ASM_CLAC			/* Clear AC after saving FLAGS */
 	orl	$X86_EFLAGS_IF, (%esp)	/* Fix IF */
 	pushl	$__USER_CS		/* pt_regs->cs */
 	pushl	$0			/* pt_regs->ip = 0 (placeholder) */
-- 
2.5.0

Re: [PATCH] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Brian Gerst]

On Tue, Feb 23, 2016 at 4:19 PM, Andy Lutomirski <luto@kernel.org> wrote:
> Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
> SYSENTER using the new C path"), we relied on a uaccess very early
> in the SYSENTER path to clear AC.  After that change, though, we can
> potentially make it all the way into C code with AC set, which
> enlarges the attack surface for SMAP bypass by doing SYSENTER with
> AC set.
>
> Strengthen the SMAP protection by addding the missing ASM_CLAC right
> at the beginning.
>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>
> This is probably an x86/urgent candidate.  It fixes a minor
> hardening regression in 4.4.
>
> It's lightly tested.  It's hard to test well right now because the
> 4.5 series is completely broken for 32-bit SMAP sytems.
>
> arch/x86/entry/entry_32.S | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index f3facd40fd2d..9d6165c171eb 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -294,6 +294,7 @@ sysenter_past_esp:
>         pushl   $__USER_DS              /* pt_regs->ss */
>         pushl   %ebp                    /* pt_regs->sp (stashed in bp) */
>         pushfl                          /* pt_regs->flags (except IF = 0) */
> +       ASM_CLAC                        /* Clear AC after saving FLAGS */
>         orl     $X86_EFLAGS_IF, (%esp)  /* Fix IF */
>         pushl   $__USER_CS              /* pt_regs->cs */
>         pushl   $0                      /* pt_regs->ip = 0 (placeholder) */
> --
> 2.5.0
>

It looks like entry_INT80_compat is also missing a CLAC.

--
Brian Gerst

Re: [PATCH] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Andy Lutomirski]

On Wed, Feb 24, 2016 at 7:46 AM, Brian Gerst <brgerst@gmail.com> wrote:
> On Tue, Feb 23, 2016 at 4:19 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
>> SYSENTER using the new C path"), we relied on a uaccess very early
>> in the SYSENTER path to clear AC.  After that change, though, we can
>> potentially make it all the way into C code with AC set, which
>> enlarges the attack surface for SMAP bypass by doing SYSENTER with
>> AC set.
>>
>> Strengthen the SMAP protection by addding the missing ASM_CLAC right
>> at the beginning.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>
>> This is probably an x86/urgent candidate.  It fixes a minor
>> hardening regression in 4.4.
>>
>> It's lightly tested.  It's hard to test well right now because the
>> 4.5 series is completely broken for 32-bit SMAP sytems.
>>
>> arch/x86/entry/entry_32.S | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
>> index f3facd40fd2d..9d6165c171eb 100644
>> --- a/arch/x86/entry/entry_32.S
>> +++ b/arch/x86/entry/entry_32.S
>> @@ -294,6 +294,7 @@ sysenter_past_esp:
>>         pushl   $__USER_DS              /* pt_regs->ss */
>>         pushl   %ebp                    /* pt_regs->sp (stashed in bp) */
>>         pushfl                          /* pt_regs->flags (except IF = 0) */
>> +       ASM_CLAC                        /* Clear AC after saving FLAGS */
>>         orl     $X86_EFLAGS_IF, (%esp)  /* Fix IF */
>>         pushl   $__USER_CS              /* pt_regs->cs */
>>         pushl   $0                      /* pt_regs->ip = 0 (placeholder) */
>> --
>> 2.5.0
>>
>
> It looks like entry_INT80_compat is also missing a CLAC.
>

Indeed, and that's a much worse bug.  For better or for worse, I think
it's been buggy since the beginning (3.10).

Patch coming.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

[tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[tip-bot for Andy Lutomirski]

Commit-ID:  04d1d281dcfe683a53cddfab8371fc8bb302b069
Gitweb:     http://git.kernel.org/tip/04d1d281dcfe683a53cddfab8371fc8bb302b069
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Tue, 23 Feb 2016 13:19:29 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 24 Feb 2016 08:43:04 +0100

x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
SYSENTER using the new C path"), we relied on a uaccess very early
in the SYSENTER path to clear AC.  After that change, though, we can
potentially make it all the way into C code with AC set, which
enlarges the attack surface for SMAP bypass by doing SYSENTER with
AC set.

Strengthen the SMAP protection by addding the missing ASM_CLAC right
at the beginning.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/3e36be110724896e32a4a1fe73bacb349d3cba94.1456262295.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/entry_32.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 77d8c51..bb3e376 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -294,6 +294,7 @@ sysenter_past_esp:
 	pushl	$__USER_DS		/* pt_regs->ss */
 	pushl	%ebp			/* pt_regs->sp (stashed in bp) */
 	pushfl				/* pt_regs->flags (except IF = 0) */
+	ASM_CLAC			/* Clear AC after saving FLAGS */
 	orl	$X86_EFLAGS_IF, (%esp)	/* Fix IF */
 	pushl	$__USER_CS		/* pt_regs->cs */
 	pushl	$0			/* pt_regs->ip = 0 (placeholder) */

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[H. Peter Anvin]

On 02/24/16 21:53, tip-bot for Andy Lutomirski wrote:
> Commit-ID:  04d1d281dcfe683a53cddfab8371fc8bb302b069
> Gitweb:     http://git.kernel.org/tip/04d1d281dcfe683a53cddfab8371fc8bb302b069
> Author:     Andy Lutomirski <luto@kernel.org>
> AuthorDate: Tue, 23 Feb 2016 13:19:29 -0800
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Wed, 24 Feb 2016 08:43:04 +0100
> 
> x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32
> 
> Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
> SYSENTER using the new C path"), we relied on a uaccess very early
> in the SYSENTER path to clear AC.  After that change, though, we can
> potentially make it all the way into C code with AC set, which
> enlarges the attack surface for SMAP bypass by doing SYSENTER with
> AC set.
> 
> Strengthen the SMAP protection by addding the missing ASM_CLAC right
> at the beginning.
> 

Hmmm... this potentially adds a *lot* of unnecessary cycles to this
path.  Could we reinstate the early uaccess?

	-hpa

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Andy Lutomirski]

[resend -- thank you Gmail for sucking]

On Wed, Feb 24, 2016 at 10:00 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 02/24/16 21:53, tip-bot for Andy Lutomirski wrote:
>> Commit-ID:  04d1d281dcfe683a53cddfab8371fc8bb302b069
>> Gitweb:     http://git.kernel.org/tip/04d1d281dcfe683a53cddfab8371fc8bb302b069
>> Author:     Andy Lutomirski <luto@kernel.org>
>> AuthorDate: Tue, 23 Feb 2016 13:19:29 -0800
>> Committer:  Ingo Molnar <mingo@kernel.org>
>> CommitDate: Wed, 24 Feb 2016 08:43:04 +0100
>>
>> x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32
>>
>> Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
>> SYSENTER using the new C path"), we relied on a uaccess very early
>> in the SYSENTER path to clear AC.  After that change, though, we can
>> potentially make it all the way into C code with AC set, which
>> enlarges the attack surface for SMAP bypass by doing SYSENTER with
>> AC set.
>>
>> Strengthen the SMAP protection by addding the missing ASM_CLAC right
>> at the beginning.
>>
>
> Hmmm... this potentially adds a *lot* of unnecessary cycles to this
> path.  Could we reinstate the early uaccess?
>

I think that's more trouble than it's worth, and it'll undo a bunch of
the context tracking cleanups that deferring it made possible,
especially since this only matters in a configuration (32-bit SMAP)
that no one uses. [1]

*However*, I just realized that I have no idea why the 32-bit sysenter
path is safe against NT being set.  I fixed it on compat, and now I'm
confused as to the status on 32-bit.  If we need to fix up NT, I think
we can fold AC into that.

Also, I'll try to benchmark this soon.

[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=de9e478b9d49f3a0214310d921450cf5bb4a21e6
(it didn't even boot through most of 4.5-rc)

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Andy Lutomirski]

On Thu, Feb 25, 2016 at 12:07 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> [resend -- thank you Gmail for sucking]
>
> On Wed, Feb 24, 2016 at 10:00 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 02/24/16 21:53, tip-bot for Andy Lutomirski wrote:
>>> Commit-ID:  04d1d281dcfe683a53cddfab8371fc8bb302b069
>>> Gitweb:     http://git.kernel.org/tip/04d1d281dcfe683a53cddfab8371fc8bb302b069
>>> Author:     Andy Lutomirski <luto@kernel.org>
>>> AuthorDate: Tue, 23 Feb 2016 13:19:29 -0800
>>> Committer:  Ingo Molnar <mingo@kernel.org>
>>> CommitDate: Wed, 24 Feb 2016 08:43:04 +0100
>>>
>>> x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32
>>>
>>> Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
>>> SYSENTER using the new C path"), we relied on a uaccess very early
>>> in the SYSENTER path to clear AC.  After that change, though, we can
>>> potentially make it all the way into C code with AC set, which
>>> enlarges the attack surface for SMAP bypass by doing SYSENTER with
>>> AC set.
>>>
>>> Strengthen the SMAP protection by addding the missing ASM_CLAC right
>>> at the beginning.
>>>
>>
>> Hmmm... this potentially adds a *lot* of unnecessary cycles to this
>> path.  Could we reinstate the early uaccess?
>>
>
> I think that's more trouble than it's worth, and it'll undo a bunch of
> the context tracking cleanups that deferring it made possible,
> especially since this only matters in a configuration (32-bit SMAP)
> that no one uses. [1]
>
> *However*, I just realized that I have no idea why the 32-bit sysenter
> path is safe against NT being set.  I fixed it on compat, and now I'm
> confused as to the status on 32-bit.  If we need to fix up NT, I think
> we can fold AC into that.
>

It is, indeed, broken.  My test case doesn't notice because
opportunistic sysexit papers over the issue.  Grump.

> Also, I'll try to benchmark this soon.
>
> [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=de9e478b9d49f3a0214310d921450cf5bb4a21e6
> (it didn't even boot through most of 4.5-rc)



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> >> Strengthen the SMAP protection by addding the missing ASM_CLAC right at the 
> >> beginning.
> >
> > Hmmm... this potentially adds a *lot* of unnecessary cycles to this path.  
> > Could we reinstate the early uaccess?
> 
> I think that's more trouble than it's worth, and it'll undo a bunch of the 
> context tracking cleanups that deferring it made possible, especially since this 
> only matters in a configuration (32-bit SMAP) that no one uses. [1]

But but ... 'context tracking' is not really something that a regular distro 
kernel cares about much - it's a nohz-full special AFAICS.

So if the only reason to keep this overhead is to simplify context tracking then 
I'm pretty sure we want to burden context-tracking with that, not the common fast 
path.

Anyway, maybe we are 'lucky':

> *However*, I just realized that I have no idea why the 32-bit sysenter
> path is safe against NT being set.  I fixed it on compat, and now I'm
> confused as to the status on 32-bit.  If we need to fix up NT, I think
> we can fold AC into that.
> 
> Also, I'll try to benchmark this soon.

Sounds good, thanks!

	Ingo

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Mike Galbraith]

On Thu, 2016-02-25 at 09:14 +0100, Ingo Molnar wrote:

> But but ... 'context tracking' is not really something that a regular distro 
> kernel cares about much - it's a nohz-full special AFAICS.

(psst.. distros are shipping it)

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Ingo Molnar]

* Mike Galbraith <umgwanakikbuti@gmail.com> wrote:

> On Thu, 2016-02-25 at 09:14 +0100, Ingo Molnar wrote:
> 
> > But but ... 'context tracking' is not really something that a regular distro 
> > kernel cares about much - it's a nohz-full special AFAICS.

Let me qualify that: with the timer code maintenance hat on I really love all nohz 
variants (the deeper the better), but now I have my x86 maintainer hat on, and as 
such I'm really annoyed at those nohz folks adding overhead to the syscall hot 
path! ;-)

> (psst.. distros are shipping it)

Yeah, indeed, Fedora does - but AFAICS:

 fomalhaut:~> grep NO_HZ /boot/config-4.1.13-100.fc21.x86_64 
 CONFIG_NO_HZ_COMMON=y
 # CONFIG_NO_HZ_IDLE is not set
 CONFIG_NO_HZ_FULL=y
 # CONFIG_NO_HZ_FULL_ALL is not set
 # CONFIG_NO_HZ_FULL_SYSIDLE is not set
 CONFIG_NO_HZ=y
 CONFIG_RCU_FAST_NO_HZ=y

... which won't result in actual full-nohz CPUs unless you boot it with a special 
boot parameter, right?

What is the easiest way to query which/how many CPUs are in nohz-full mode and do 
context tracking? I somehow thought /proc/timer_* had that info, but that does not 
appear to be the case.

Thanks,

	Ingo

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Mike Galbraith]

On Thu, 2016-02-25 at 09:40 +0100, Ingo Molnar wrote:
> * Mike Galbraith <umgwanakikbuti@gmail.com> wrote:
> 
> > On Thu, 2016-02-25 at 09:14 +0100, Ingo Molnar wrote:
> > 
> > > But but ... 'context tracking' is not really something that a regular distro 
> > > kernel cares about much - it's a nohz-full special AFAICS.
> 
> Let me qualify that: with the timer code maintenance hat on I really love all nohz 
> variants (the deeper the better), but now I have my x86 maintainer hat on, and as 
> such I'm really annoyed at those nohz folks adding overhead to the syscall hot 
> path! ;-)
> 
> > (psst.. distros are shipping it)
> 
> Yeah, indeed, Fedora does - but AFAICS:
> 
>  fomalhaut:~> grep NO_HZ /boot/config-4.1.13-100.fc21.x86_64 
>  CONFIG_NO_HZ_COMMON=y
>  # CONFIG_NO_HZ_IDLE is not set
>  CONFIG_NO_HZ_FULL=y
>  # CONFIG_NO_HZ_FULL_ALL is not set
>  # CONFIG_NO_HZ_FULL_SYSIDLE is not set
>  CONFIG_NO_HZ=y
>  CONFIG_RCU_FAST_NO_HZ=y
> 
> ... which won't result in actual full-nohz CPUs unless you boot it with a special 
> boot parameter, right?

Yeah, you have to manually enable it unless you (in a suicidal moment)
enable CONFIG_NO_HZ_FULL_ALL.

> What is the easiest way to query which/how many CPUs are in nohz-full mode and do 
> context tracking? I somehow thought /proc/timer_* had that info, but that does not 
> appear to be the case.

/sys/devices/system/cpu/nohz_full

	-Mike

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Brian Gerst]

On Thu, Feb 25, 2016 at 3:03 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> On Feb 24, 2016 10:01 PM, "H. Peter Anvin" <hpa@zytor.com> wrote:
>>
>> On 02/24/16 21:53, tip-bot for Andy Lutomirski wrote:
>> > Commit-ID:  04d1d281dcfe683a53cddfab8371fc8bb302b069
>> > Gitweb:
>> > http://git.kernel.org/tip/04d1d281dcfe683a53cddfab8371fc8bb302b069
>> > Author:     Andy Lutomirski <luto@kernel.org>
>> > AuthorDate: Tue, 23 Feb 2016 13:19:29 -0800
>> > Committer:  Ingo Molnar <mingo@kernel.org>
>> > CommitDate: Wed, 24 Feb 2016 08:43:04 +0100
>> >
>> > x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32
>> >
>> > Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
>> > SYSENTER using the new C path"), we relied on a uaccess very early
>> > in the SYSENTER path to clear AC.  After that change, though, we can
>> > potentially make it all the way into C code with AC set, which
>> > enlarges the attack surface for SMAP bypass by doing SYSENTER with
>> > AC set.
>> >
>> > Strengthen the SMAP protection by addding the missing ASM_CLAC right
>> > at the beginning.
>> >
>>
>> Hmmm... this potentially adds a *lot* of unnecessary cycles to this
>> path.  Could we reinstate the early uaccess?
>
> I think that's more trouble than it's worth, and it'll undo a bunch of the
> context tracking cleanups that deferring it made possible, especially since
> this only matters in a configuration (32-bit SMAP) that no one uses. [1]
>
> *However*, I just realized that I have no idea why the 32-bit sysenter path
> is safe against NT being set.  I fixed it on compat, and now I'm confused as
> to the status on 32-bit.  If we need to fix up NT, I think we can fold AC
> into that.

32-bit still saves eflags in switch_to(), so NT can't leak to other
tasks.  But for consistency it should get the same treatment as 64-bit
(clear NT in sysenter entry and drop saving eflags in switch_to).

--
Brian Gerst

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Brian Gerst]

On Thu, Feb 25, 2016 at 8:47 AM, Brian Gerst <brgerst@gmail.com> wrote:
> On Thu, Feb 25, 2016 at 3:03 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> On Feb 24, 2016 10:01 PM, "H. Peter Anvin" <hpa@zytor.com> wrote:
>>>
>>> On 02/24/16 21:53, tip-bot for Andy Lutomirski wrote:
>>> > Commit-ID:  04d1d281dcfe683a53cddfab8371fc8bb302b069
>>> > Gitweb:
>>> > http://git.kernel.org/tip/04d1d281dcfe683a53cddfab8371fc8bb302b069
>>> > Author:     Andy Lutomirski <luto@kernel.org>
>>> > AuthorDate: Tue, 23 Feb 2016 13:19:29 -0800
>>> > Committer:  Ingo Molnar <mingo@kernel.org>
>>> > CommitDate: Wed, 24 Feb 2016 08:43:04 +0100
>>> >
>>> > x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32
>>> >
>>> > Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
>>> > SYSENTER using the new C path"), we relied on a uaccess very early
>>> > in the SYSENTER path to clear AC.  After that change, though, we can
>>> > potentially make it all the way into C code with AC set, which
>>> > enlarges the attack surface for SMAP bypass by doing SYSENTER with
>>> > AC set.
>>> >
>>> > Strengthen the SMAP protection by addding the missing ASM_CLAC right
>>> > at the beginning.
>>> >
>>>
>>> Hmmm... this potentially adds a *lot* of unnecessary cycles to this
>>> path.  Could we reinstate the early uaccess?
>>
>> I think that's more trouble than it's worth, and it'll undo a bunch of the
>> context tracking cleanups that deferring it made possible, especially since
>> this only matters in a configuration (32-bit SMAP) that no one uses. [1]
>>
>> *However*, I just realized that I have no idea why the 32-bit sysenter path
>> is safe against NT being set.  I fixed it on compat, and now I'm confused as
>> to the status on 32-bit.  If we need to fix up NT, I think we can fold AC
>> into that.
>
> 32-bit still saves eflags in switch_to(), so NT can't leak to other
> tasks.  But for consistency it should get the same treatment as 64-bit
> (clear NT in sysenter entry and drop saving eflags in switch_to).

Looking deeper, the sysexit path doesn't restore eflags, so we'd need
to manually restore AC and IOPL.

--
Brian Gerst

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Andy Lutomirski]

On Thu, Feb 25, 2016 at 7:42 AM, Brian Gerst <brgerst@gmail.com> wrote:
> On Thu, Feb 25, 2016 at 8:47 AM, Brian Gerst <brgerst@gmail.com> wrote:
>> On Thu, Feb 25, 2016 at 3:03 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>
>>> On Feb 24, 2016 10:01 PM, "H. Peter Anvin" <hpa@zytor.com> wrote:
>>>>
>>>> On 02/24/16 21:53, tip-bot for Andy Lutomirski wrote:
>>>> > Commit-ID:  04d1d281dcfe683a53cddfab8371fc8bb302b069
>>>> > Gitweb:
>>>> > http://git.kernel.org/tip/04d1d281dcfe683a53cddfab8371fc8bb302b069
>>>> > Author:     Andy Lutomirski <luto@kernel.org>
>>>> > AuthorDate: Tue, 23 Feb 2016 13:19:29 -0800
>>>> > Committer:  Ingo Molnar <mingo@kernel.org>
>>>> > CommitDate: Wed, 24 Feb 2016 08:43:04 +0100
>>>> >
>>>> > x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32
>>>> >
>>>> > Both before and after 5f310f739b4c ("x86/entry/32: Re-implement
>>>> > SYSENTER using the new C path"), we relied on a uaccess very early
>>>> > in the SYSENTER path to clear AC.  After that change, though, we can
>>>> > potentially make it all the way into C code with AC set, which
>>>> > enlarges the attack surface for SMAP bypass by doing SYSENTER with
>>>> > AC set.
>>>> >
>>>> > Strengthen the SMAP protection by addding the missing ASM_CLAC right
>>>> > at the beginning.
>>>> >
>>>>
>>>> Hmmm... this potentially adds a *lot* of unnecessary cycles to this
>>>> path.  Could we reinstate the early uaccess?
>>>
>>> I think that's more trouble than it's worth, and it'll undo a bunch of the
>>> context tracking cleanups that deferring it made possible, especially since
>>> this only matters in a configuration (32-bit SMAP) that no one uses. [1]
>>>
>>> *However*, I just realized that I have no idea why the 32-bit sysenter path
>>> is safe against NT being set.  I fixed it on compat, and now I'm confused as
>>> to the status on 32-bit.  If we need to fix up NT, I think we can fold AC
>>> into that.
>>
>> 32-bit still saves eflags in switch_to(), so NT can't leak to other
>> tasks.  But for consistency it should get the same treatment as 64-bit
>> (clear NT in sysenter entry and drop saving eflags in switch_to).
>
> Looking deeper, the sysexit path doesn't restore eflags, so we'd need
> to manually restore AC and IOPL.

Ick.

First things first, though -- there's no immediate reason to stop
saving flags on 32-bit context switches.  So we don't need to worry
about IOPL except for sanity and/or consistency.

Ideally we'd fix this up and restore flags on sysexit.  At least
failing to restore arithmetic flags isn't an info leak because the
exit code clobbers them with entirely predictable data.  I doubt
anyone cares all that much if we clobber AC.

I wrote a test for NT and the test fails for a different reason: our
TF handling appears broken as well.  (Our sysenter TF handling is
*crap*, but it seems to work on 64-bit kernels at least.)

My personal preference would be to add the missing popf.

--Andy

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Linus Torvalds]

On Thu, Feb 25, 2016 at 10:20 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Ideally we'd fix this up and restore flags on sysexit.  At least
> failing to restore arithmetic flags isn't an info leak because the
> exit code clobbers them with entirely predictable data.  I doubt
> anyone cares all that much if we clobber AC.

As long as the "clobber AC" is purely about clearing it, it's probably fine.

Although there may be programs that set AC in order to actually get
notified about alignment issues (perhaps for portability reasons,
perhaps for small performance reasons). Clearing it will make those
programs still work, but they lose the checking.

> I wrote a test for NT and the test fails for a different reason: our
> TF handling appears broken as well.  (Our sysenter TF handling is
> *crap*, but it seems to work on 64-bit kernels at least.)

TF should be entirely immaterial for system calls. Why would we care?
We need it for correct handling of real traps, but not for the system
call case afaik. Returning with TF clear is the right thing, since
we're not returning *to* the system call instruction, but the
instruction after.

> My personal preference would be to add the missing popf.

I don't mind adding the popf, but it won't help for iopl. Only iret
restores iopl, if I recall correctly (but maybe I don't, and I'm too
lazy to take the 30 seconds to look it up).

               Linus

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Andy Lutomirski]

On Thu, Feb 25, 2016 at 10:30 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Feb 25, 2016 at 10:20 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Ideally we'd fix this up and restore flags on sysexit.  At least
>> failing to restore arithmetic flags isn't an info leak because the
>> exit code clobbers them with entirely predictable data.  I doubt
>> anyone cares all that much if we clobber AC.
>
> As long as the "clobber AC" is purely about clearing it, it's probably fine.
>
> Although there may be programs that set AC in order to actually get
> notified about alignment issues (perhaps for portability reasons,
> perhaps for small performance reasons). Clearing it will make those
> programs still work, but they lose the checking.
>
>> I wrote a test for NT and the test fails for a different reason: our
>> TF handling appears broken as well.  (Our sysenter TF handling is
>> *crap*, but it seems to work on 64-bit kernels at least.)
>
> TF should be entirely immaterial for system calls. Why would we care?
> We need it for correct handling of real traps, but not for the system
> call case afaik. Returning with TF clear is the right thing, since
> we're not returning *to* the system call instruction, but the
> instruction after.

TF is very material to SYSENTER because Intel completely fucked up.

SYSENTER with TF set causes SYSENTER to trap in the sense that an
interrupt is delivered after SYSENTER, *in kernel mode*, *from CPL 0*,
with whatever probably-bullshit stack pointer we have set up.

We have overcomplicated code to fix up the resulting mess, but it
doesn't seem to work right on 32-bit.  So I'm thinking of rewriting it
from scratch to make sense.

>
>> My personal preference would be to add the missing popf.
>
> I don't mind adding the popf, but it won't help for iopl. Only iret
> restores iopl, if I recall correctly (but maybe I don't, and I'm too
> lazy to take the 30 seconds to look it up).
>

OK, I'll make sure to check this.

--Andy

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Brian Gerst]

On Thu, Feb 25, 2016 at 1:30 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Feb 25, 2016 at 10:20 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Ideally we'd fix this up and restore flags on sysexit.  At least
>> failing to restore arithmetic flags isn't an info leak because the
>> exit code clobbers them with entirely predictable data.  I doubt
>> anyone cares all that much if we clobber AC.
>
> As long as the "clobber AC" is purely about clearing it, it's probably fine.
>
> Although there may be programs that set AC in order to actually get
> notified about alignment issues (perhaps for portability reasons,
> perhaps for small performance reasons). Clearing it will make those
> programs still work, but they lose the checking.
>
>> I wrote a test for NT and the test fails for a different reason: our
>> TF handling appears broken as well.  (Our sysenter TF handling is
>> *crap*, but it seems to work on 64-bit kernels at least.)
>
> TF should be entirely immaterial for system calls. Why would we care?
> We need it for correct handling of real traps, but not for the system
> call case afaik. Returning with TF clear is the right thing, since
> we're not returning *to* the system call instruction, but the
> instruction after.
>
>> My personal preference would be to add the missing popf.
>
> I don't mind adding the popf, but it won't help for iopl. Only iret
> restores iopl, if I recall correctly (but maybe I don't, and I'm too
> lazy to take the 30 seconds to look it up).
>
>                Linus

According to the SDM, popf will change IOPL only at CPL0, which is why
Xen (which runs at CPL1 on 32-bit) has a paravirt hook for it.

--
Brian Gerst

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Andy Lutomirski]

On Thu, Feb 25, 2016 at 11:31 AM, Brian Gerst <brgerst@gmail.com> wrote:
> On Thu, Feb 25, 2016 at 1:30 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Thu, Feb 25, 2016 at 10:20 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>
>>> Ideally we'd fix this up and restore flags on sysexit.  At least
>>> failing to restore arithmetic flags isn't an info leak because the
>>> exit code clobbers them with entirely predictable data.  I doubt
>>> anyone cares all that much if we clobber AC.
>>
>> As long as the "clobber AC" is purely about clearing it, it's probably fine.
>>
>> Although there may be programs that set AC in order to actually get
>> notified about alignment issues (perhaps for portability reasons,
>> perhaps for small performance reasons). Clearing it will make those
>> programs still work, but they lose the checking.
>>
>>> I wrote a test for NT and the test fails for a different reason: our
>>> TF handling appears broken as well.  (Our sysenter TF handling is
>>> *crap*, but it seems to work on 64-bit kernels at least.)
>>
>> TF should be entirely immaterial for system calls. Why would we care?
>> We need it for correct handling of real traps, but not for the system
>> call case afaik. Returning with TF clear is the right thing, since
>> we're not returning *to* the system call instruction, but the
>> instruction after.
>>
>>> My personal preference would be to add the missing popf.
>>
>> I don't mind adding the popf, but it won't help for iopl. Only iret
>> restores iopl, if I recall correctly (but maybe I don't, and I'm too
>> lazy to take the 30 seconds to look it up).
>>
>>                Linus
>
> According to the SDM, popf will change IOPL only at CPL0, which is why
> Xen (which runs at CPL1 on 32-bit) has a paravirt hook for it.

But maybe we can ditch that paravirt hook and just modify regs->flags
in sys_iopl.  Xen never uses sysexit at all:

    /* XEN PV guests always use IRET path */
    ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \
            "jmp .Lsyscall_32_done", X86_FEATURE_XENPV

and if we add the missing popf, we should be good to go.

--Andy

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Brian Gerst]

On Thu, Feb 25, 2016 at 2:39 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Feb 25, 2016 at 11:31 AM, Brian Gerst <brgerst@gmail.com> wrote:
>> On Thu, Feb 25, 2016 at 1:30 PM, Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>> On Thu, Feb 25, 2016 at 10:20 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>>
>>>> Ideally we'd fix this up and restore flags on sysexit.  At least
>>>> failing to restore arithmetic flags isn't an info leak because the
>>>> exit code clobbers them with entirely predictable data.  I doubt
>>>> anyone cares all that much if we clobber AC.
>>>
>>> As long as the "clobber AC" is purely about clearing it, it's probably fine.
>>>
>>> Although there may be programs that set AC in order to actually get
>>> notified about alignment issues (perhaps for portability reasons,
>>> perhaps for small performance reasons). Clearing it will make those
>>> programs still work, but they lose the checking.
>>>
>>>> I wrote a test for NT and the test fails for a different reason: our
>>>> TF handling appears broken as well.  (Our sysenter TF handling is
>>>> *crap*, but it seems to work on 64-bit kernels at least.)
>>>
>>> TF should be entirely immaterial for system calls. Why would we care?
>>> We need it for correct handling of real traps, but not for the system
>>> call case afaik. Returning with TF clear is the right thing, since
>>> we're not returning *to* the system call instruction, but the
>>> instruction after.
>>>
>>>> My personal preference would be to add the missing popf.
>>>
>>> I don't mind adding the popf, but it won't help for iopl. Only iret
>>> restores iopl, if I recall correctly (but maybe I don't, and I'm too
>>> lazy to take the 30 seconds to look it up).
>>>
>>>                Linus
>>
>> According to the SDM, popf will change IOPL only at CPL0, which is why
>> Xen (which runs at CPL1 on 32-bit) has a paravirt hook for it.
>
> But maybe we can ditch that paravirt hook and just modify regs->flags
> in sys_iopl.  Xen never uses sysexit at all:
>
>     /* XEN PV guests always use IRET path */
>     ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \
>             "jmp .Lsyscall_32_done", X86_FEATURE_XENPV
>
> and if we add the missing popf, we should be good to go.

IRET won't change IOPL either at CPL != 0, so Xen still needs that hook.

--
Brian Gerst

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Andy Lutomirski]

On Thu, Feb 25, 2016 at 11:49 AM, Brian Gerst <brgerst@gmail.com> wrote:
> On Thu, Feb 25, 2016 at 2:39 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Thu, Feb 25, 2016 at 11:31 AM, Brian Gerst <brgerst@gmail.com> wrote:
>>> On Thu, Feb 25, 2016 at 1:30 PM, Linus Torvalds
>>> <torvalds@linux-foundation.org> wrote:
>>>> On Thu, Feb 25, 2016 at 10:20 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>>>
>>>>> Ideally we'd fix this up and restore flags on sysexit.  At least
>>>>> failing to restore arithmetic flags isn't an info leak because the
>>>>> exit code clobbers them with entirely predictable data.  I doubt
>>>>> anyone cares all that much if we clobber AC.
>>>>
>>>> As long as the "clobber AC" is purely about clearing it, it's probably fine.
>>>>
>>>> Although there may be programs that set AC in order to actually get
>>>> notified about alignment issues (perhaps for portability reasons,
>>>> perhaps for small performance reasons). Clearing it will make those
>>>> programs still work, but they lose the checking.
>>>>
>>>>> I wrote a test for NT and the test fails for a different reason: our
>>>>> TF handling appears broken as well.  (Our sysenter TF handling is
>>>>> *crap*, but it seems to work on 64-bit kernels at least.)
>>>>
>>>> TF should be entirely immaterial for system calls. Why would we care?
>>>> We need it for correct handling of real traps, but not for the system
>>>> call case afaik. Returning with TF clear is the right thing, since
>>>> we're not returning *to* the system call instruction, but the
>>>> instruction after.
>>>>
>>>>> My personal preference would be to add the missing popf.
>>>>
>>>> I don't mind adding the popf, but it won't help for iopl. Only iret
>>>> restores iopl, if I recall correctly (but maybe I don't, and I'm too
>>>> lazy to take the 30 seconds to look it up).
>>>>
>>>>                Linus
>>>
>>> According to the SDM, popf will change IOPL only at CPL0, which is why
>>> Xen (which runs at CPL1 on 32-bit) has a paravirt hook for it.
>>
>> But maybe we can ditch that paravirt hook and just modify regs->flags
>> in sys_iopl.  Xen never uses sysexit at all:
>>
>>     /* XEN PV guests always use IRET path */
>>     ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \
>>             "jmp .Lsyscall_32_done", X86_FEATURE_XENPV
>>
>> and if we add the missing popf, we should be good to go.
>
> IRET won't change IOPL either at CPL != 0, so Xen still needs that hook.

But xen_iret isn't the same thing as IRET at all.  We don't use a real
IRET instruction to switch between kernel and user mode on Xen PV.

--Andy

>
> --
> Brian Gerst



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [tip:x86/urgent] x86/entry/32: Add an ASM_CLAC to entry_SYSENTER_32

[Linus Torvalds]

On Thu, Feb 25, 2016 at 11:31 AM, Brian Gerst <brgerst@gmail.com> wrote:
>
> According to the SDM, popf will change IOPL only at CPL0,

Ok, so that's good enough for the native case, and adding a "popf"
would at least restore AC and IOPL.

                 Linus