by Andy Lutomirski

[permalink] [raw]

Subject: Re: [RESEND PATCH v6 08/12] x86/fsgsbase/64: Use the per-CPU base as GSBASE at the paranoid_entry

> On May 1, 2019, at 2:04 PM, Bae, Chang Seok <[email protected]> wrote:
>
>
>> On May 1, 2019, at 13:25, Andy Lutomirski <[email protected]> wrote:
>>
>>
>>
>>> On May 1, 2019, at 1:21 PM, Bae, Chang Seok <[email protected]> wrote:
>>>
>>>
>>>>> On May 1, 2019, at 11:01, Bae, Chang Seok <[email protected]> wrote:
>>>>>
>>>>> On May 1, 2019, at 10:40, Andy Lutomirski <[email protected]> wrote:
>>>>>
>>>>>> On Wed, May 1, 2019 at 6:52 AM Bae, Chang Seok <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>>> On Apr 5, 2019, at 06:50, Andy Lutomirski <[email protected]> wrote:
>>>>>>>
>>>>>>> Furthermore, if you folks even want me to review this series, the ptrace tests need to be in place. On inspection of the current code (after the debacle a few releases back), it appears the SETREGSET’s effect depends on the current values in the registers — it does not actually seem to reliably load the whole state. So my confidence will be greatly increased if your series first adds a test that detects that bug (and fails!), then fixes the bug in a tiny little patch, then adds FSGSBASE, and keeps the test working.
>>>>>>>
>>>>>>
>>>>>> I think I need to understand the issue. Appreciate if you can elaborate a little bit.
>>>>>>
>>>>>
>>>>> This patch series gives a particular behavior to PTRACE_SETREGS and
>>>>> PTRACE_POKEUSER. There should be a test case that validates that
>>>>> behavior, including testing the weird cases where gs != 0 and gsbase
>>>>> contains unusual values. Some existing tests might be pretty close to
>>>>> doing what's needed.
>>>>>
>>>>> Beyond that, the current putreg() code does this:
>>>>>
>>>>> case offsetof(struct user_regs_struct,gs_base):
>>>>> /*
>>>>> * Exactly the same here as the %fs handling above.
>>>>> */
>>>>> if (value >= TASK_SIZE_MAX)
>>>>> return -EIO;
>>>>> if (child->thread.gsbase != value)
>>>>> return do_arch_prctl_64(child, ARCH_SET_GS, value);
>>>>> return 0;
>>>>>
>>>>> and do_arch_prctl_64(), in turn, does this:
>>>>>
>>>>> case ARCH_SET_GS: {
>>>>> if (unlikely(arg2 >= TASK_SIZE_MAX))
>>>>> return -EPERM;
>>>>>
>>>>> preempt_disable();
>>>>> /*
>>>>> * ARCH_SET_GS has always overwritten the index
>>>>> * and the base. Zero is the most sensible value
>>>>> * to put in the index, and is the only value that
>>>>> * makes any sense if FSGSBASE is unavailable.
>>>>> */
>>>>> if (task == current) {
>>>>> [not used for ptrace]
>>>>> } else {
>>>>> task->thread.gsindex = 0;
>>>>> x86_gsbase_write_task(task, arg2);
>>>>> }
>>>>>
>>>>> ...
>>>>>
>>>>> So writing the value that was already there to gsbase via putreg()
>>>>> does nothing, but writing a *different* value implicitly clears gs,
>>>>> but writing a different value will clear gs.
>>>>>
>>>>> This behavior is, AFAICT, complete nonsense. It happens to work
>>>>> because usually gdb writes the same value back, and, in any case, gs
>>>>> comes *after* gsbase in user_regs_struct, so gs gets replaced anyway.
>>>>> But I think that this behavior should be fixed up and probably tested.
>>>>> Certainly the behavior should *not* be the same on a fsgsbase kernel,
>>>>> and and the fsgsbase behavior definitely needs a selftest.
>>>>
>>>> Okay, got the point; now crystal clear.
>>>>
>>>> I have my own test case for that though, need to find a very simple and
>>>> acceptable solution.
>>>>
>>>
>>> One solution that I recall, HPA once suggested, is:
>>> Write registers in a reverse order from user_regs_struct, for SETREGS
>>>
>>> Assuming these for clarification, first:
>>> * old and new index != 0
>>> * taking GS as an example though, should be the same with FS
>>>
>>> Then, interesting cases would be something like these, without FSGSBASE:
>>> Case (a), when index only changed to (new index):
>>> (Then, the result after SETREGS would be)
>>> GS = (new index), GSBASE = the base fetched from (new index)
>>> Case (b), when base only changed to (new base):
>>> Case (c), when both are changed:
>>> GS = 0, GSBASE = (new base)
>>>
>>> Now, with FSGSBASE:
>>> Case (a):
>>> GS = (new index), GSBASE = (old base)
>>> Case (b):
>>> GS = (old index), GSBASE = (new base)
>>> Case (c):
>>> GS = (new index), GSBASE = (new base)
>>>
>>> As a reference, today's kernel behavior, without FSGSBASE:
>>> Case (a):
>>> GS = (new index), GSBASE = the base fetched from (new index)
>>> Case (b):
>>> GS = (old index), GSBASE = (old base)
>>> Case (c):
>>> GS = (new index), GSBASE = the base fetched from (new index)
>>>
>>> Now, with that reverse ordering and taking that "GSBASE is important" [1],
>>> it looks like to be working in terms of its base value:
>>> Case (b) and (c) will behave the same as with FSGSBASE
>>> Case (a) still differs between w/ and w/o FSGSBASE.
>>> Well, I'd say this bit comes from the 'new model' vs. the 'leagcy
>>> model'. So, then okay with that. Any thoughts?
>>>
>>>
>>>
>>
>> This seems more complicated than needed. How about we just remove all the magic and make putreg on the base registers never change the selector.
>>
>
> Hmm, just wonder what's benefit in terms of making a non-FSGSBASE system
> behave more similar to one with FSGSBASE (although I would buy that removal).

Simplicity. The current behavior is IMO nuts.

> Well, if we're okay with such divergence, maybe that's it.
>
>> As far as I can tell, the only downside is that, on a non-FSGSBASE kernel, setting only the base if the selector already has a nonzero value won’t work, but I would be quite surprised if this breaks anything.
>
>
>

2019-05-06 22:58:02

by Chang S. Bae

[permalink] [raw]

Subject: Re: [RESEND PATCH v6 08/12] x86/fsgsbase/64: Use the per-CPU base as GSBASE at the paranoid_entry

> On Mar 25, 2019, at 02:44, Thomas Gleixner <[email protected]> wrote:
>
> On Fri, 15 Mar 2019, Chang S. Bae wrote:
>
>> The FSGSBASE instructions allow fast accesses on GSBASE. Now, at the
>> paranoid_entry, the per-CPU base value can be always copied to GSBASE.
>> And the original GSBASE value will be restored at the exit.
>
> Again you are describing WHAT but not the WHY.
>
>> So far, GSBASE modification has not been directly allowed from userspace.
>> So, swapping GSBASE has been conditionally executed according to the
>> kernel-enforced convention that a negative GSBASE indicates a kernel value.
>> But when FSGSBASE is enabled, userspace can put an arbitrary value in
>> GSBASE. The change will secure a correct GSBASE value with FSGSBASE.
>
> So that's some WHY, but it should be explained _BEFORE_ explaining the
> change. This changelog style is as bad as top posting. Why?
>
> 1) FSGSBASE is fast
>
> 2) Copy GSBASE always on paranoid exit and restore on entry
>
> 3) Explain the context
>
> No. You want to explain context first and then explain why this needs a
> change when FSGSBASE is enabled and how that change looks like at the
> conceptual level.
>
>> Also, factor out the RDMSR-based GSBASE read into a new macro,
>> READ_MSR_GSBASE.
>
> This new macro is related to this change in what way? None AFAICT. I'm fine
> with the macro itself, but the benefit for a single usage site is dubious.
>
> Adding this macro and using it should be done with a separate patch before
> this one, so this patch becomes simpler to review.
>
>> /*
>> @@ -1178,9 +1185,38 @@ ENTRY(paranoid_entry)
>> * This is also why CS (stashed in the "iret frame" by the
>> * hardware at entry) can not be used: this may be a return
>> * to kernel code, but with a user CR3 value.
>> + *
>> + * As long as this PTI macro doesn't depend on kernel GSBASE,
>> + * we can do it early. This is because FIND_PERCPU_BASE
>> + * references data in kernel space.
>
> It's not about 'can do it early'. The FSGSBASE handling requires that the
> kernel page tables are switched in.
>
> And for review and bisectability sake moving the CR3 switch in front of the
> GS handling should be done as a separate preparatory patch.
>
>> */
>> SAVE_AND_SWITCH_TO_KERNEL_CR3 scratch_reg=%rax save_reg=%r14
>>
>> + /*
>> + * Read GSBASE by RDGSBASE. Kernel GSBASE is found
>> + * from the per-CPU offset table with a CPU NR.
>
> That CPU NR comes out of thin air, right? This code is complex enough by
> itself and does not need further confusion by comments which need a crystal
> ball for decoding.
>
>> + */
>
> Sigh. I can't see how that comment explains the ALTERNATIVE jump.
>
>> + ALTERNATIVE "jmp .Lparanoid_entry_no_fsgsbase", "",\
>> + X86_FEATURE_FSGSBASE
>
> Please separate the above from the below with a new line for readability
> sake.
>
>> + rdgsbase %rbx
>> + FIND_PERCPU_BASE %rax
>> + wrgsbase %rax
>
> So this really should be wrapped in a macro like:
>
> SAVE_AND_SET_GSBASE %rbx, %rax
>
> which makes it entirely clear what this is about.
>
>> + ret
>> +
>
>> @@ -1194,12 +1230,21 @@ END(paranoid_entry)
>> * be complicated. Fortunately, we there's no good reason
>> * to try to handle preemption here.
>> *
>> - * On entry, ebx is "no swapgs" flag (1: don't need swapgs, 0: need it)
>> + * On entry,
>> + * With FSGSBASE,
>> + * %rbx is original GSBASE that needs to be restored on the exit
>> + * Without that,
>> + * %ebx is "no swapgs" flag (1: don't need swapgs, 0: need it)
>> */
>> ENTRY(paranoid_exit)
>> UNWIND_HINT_REGS
>> DISABLE_INTERRUPTS(CLBR_ANY)
>> TRACE_IRQS_OFF_DEBUG
>> + ALTERNATIVE "jmp .Lparanoid_exit_no_fsgsbase", "nop",\
>> + X86_FEATURE_FSGSBASE
>> + wrgsbase %rbx
>> + jmp .Lparanoid_exit_no_swapgs;
>
> Again. A few newlines would make it more readable.
>
> This modifies the semantics of paranoid_entry and paranoid_exit. Looking at
> the usage sites there is the following code in the nmi maze:
>
> /*
> * Use paranoid_entry to handle SWAPGS, but no need to use paranoid_exit
> * as we should not be calling schedule in NMI context.
> * Even with normal interrupts enabled. An NMI should not be
> * setting NEED_RESCHED or anything that normal interrupts and
> * exceptions might do.
> */
> call paranoid_entry
> UNWIND_HINT_REGS
>
> /* paranoidentry do_nmi, 0; without TRACE_IRQS_OFF */
> movq %rsp, %rdi
> movq $-1, %rsi
> call do_nmi
>
> /* Always restore stashed CR3 value (see paranoid_entry) */
> RESTORE_CR3 scratch_reg=%r15 save_reg=%r14
>
> testl %ebx, %ebx /* swapgs needed? */
> jnz nmi_restore
> nmi_swapgs:
> SWAPGS_UNSAFE_STACK
> nmi_restore:
> POP_REGS
>

Oh!, almost miss this bit. Will be terrifying if leave them like this way.

> I might be missing something, but how is that supposed to work when
> paranoid_entry uses FSGSBASE? I think it's broken, but if it's not then
> there is a big fat comment missing explaining why.
>

You will see a revision shortly. Thanks

Chang