lkml.org 
[lkml]   [2018]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 13/15] x86/fsgsbase/64: With FSGSBASE, compare GS bases on paranoid_entry
On Tue, Mar 20, 2018 at 8:07 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 03/20/18 03:16, David Laight wrote:
>> From: Chang S. Bae
>>> Sent: 19 March 2018 17:49
>> ...
>>> When FSGSBASE is enabled, SWAPGS needs if and only if (current)
>>> GS base is not the kernel's.
>>>
>>> FSGSBASE instructions allow user to write any value on GS base;
>>> even negative. Sign check on the current GS base is not
>>> sufficient. Fortunately, reading GS base is fast. Kernel GS
>>> base is also known from the offset table with the CPU number.
>> ...
>>
>> Use code might want to put a negative value into GSBASE.
>> While it is normal to put a valid address into GSBASE there
>> is no reason why the code can't put an offset into GSBASE,
>> in which case it might be negative.
>>
>> Yes, I know you can't put arbitrary 64bit values into GSBASE.
>> But the difference between 2 user pointers will always be valid.
>>
>
> You don't have a choice: you can't control what userspace puts in there.
> Anything that depends on a specific value is inherently unsafe.
>
> But we also don't need swapgs when we have rdgsbase/wrgsbase available.
> We can indeed just unconditionally save it (via rdgsbase) into the stack
> frame and wrgsbase the correct percpu value. In that case it might be
> necessary in order to avoid insane complexity to also save/restore the
> gs selector.

This is exactly what the old code did. I liked the old code better.

>
> Is it going to be faster? *Probably* not as swapgs is designed to be
> fast; it does, however, eliminate the need to RDMSR/WRMSR inside the
> kernel task switch as the user space gsbase will simply live on the
> stack. (This is assuming we do this unconditionally on every method of
> kernel entry, including non-paranoid. I'm not sure if we ever care
> about the userspace GS/GSBASE inside a paranoid handler, but if we do it
> would be rather messy to find if we do this conditionally.
>
> Now...
>
> + ALTERNATIVE "jmp .Lparanoid_entry_no_fsgsbase", \
> + "RDGSBASE %rdx", X86_FEATURE_FSGSBASE
> + READ_KERNEL_GSBASE %rax
>
> READ_KERNEL_GSBASE here seems like a Really Bad Name[TM] for this macro,
> since it seems to imply reading MSR_KERNEL_GS_BASE, rather than finding
> the current percpu offset. I would prefer calling it something like
> FIND_PERCPU_BASE or something like that.

I think we should revert to what the old patches did here.

\
 
 \ /
  Last update: 2018-03-21 01:37    [W:0.797 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site