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@xxxxxxxxx> wrote:
> On Thu, Feb 25, 2016 at 8:47 AM, Brian Gerst <brgerst@xxxxxxxxx> wrote:
>> On Thu, Feb 25, 2016 at 3:03 AM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
>>>
>>> On Feb 24, 2016 10:01 PM, "H. Peter Anvin" <hpa@xxxxxxxxx> 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@xxxxxxxxxx>
>>>> > AuthorDate: Tue, 23 Feb 2016 13:19:29 -0800
>>>> > Committer:  Ingo Molnar <mingo@xxxxxxxxxx>
>>>> > 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