lkml.org 
[lkml]   [2018]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC 2/2] x86/pti/64: Remove the SYSCALL64 entry trampoline
From
Date

> On Jul 22, 2018, at 11:27 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Sun, Jul 22, 2018 at 10:45 AM Andy Lutomirski <luto@kernel.org> wrote:
>>
>> This patch changes the code to map the percpu TSS into the user page
>> tables to allow the non-trampoline SYSCALL64 path to work under PTI.
>
> Me likey.
>
> However:
>
>> This does not add a new direct information leak, since the TSS is
>> readable by Meltdown from the cpu_entry_area alias regardless.
>
> Afaik, it does now potentially expose through meltdown the per-thread
> entry stack info, which is new.

It’s always been exposed through the RO alias. The only new exposure is the *address* of the RW alias, I think.

>
> But I don't think that's a show-stopper.
>
>> static void __init pti_clone_user_shared(void)
>> {
>> + for_each_possible_cpu(cpu) {
>
> But this code is pretty disgusting and seems wrong.
>
> Do you really want to do all trhe _possible_ cpu's, not just the
> online ones? I'd rather expose less (think MAXCPU) and then have the
> CPU hotplug code expose the page as the CPU comes up?

We already have exactly the same issue for cpu_entry_area. If we change it, I think we should do cpu_entry_area at the same time. But that’s awkward because cpu_entry_area is mapped one PMD at a time right now.

It’s also awkward to expose a percpu page dynamically, because (I think) percpu data isn’t guaranteed to all be in the same PGD-sized area. A vmalloc fault in the early SYSCALL64 path is fatal.

>
>> + unsigned long va = (unsigned long)&per_cpu(cpu_tss_rw, cpu);
>> + phys_addr_t pa = per_cpu_ptr_to_phys((void *)va);
>> + pte_t *target_pte;
>> +
>> + target_pte = pti_user_pagetable_walk_pte(va);
>
> This function only exists if CONFIG_X86_VSYSCALL_EMULATION, so it
> won't even compile under (very unusual) configurations.

Oops.

>
> The "disgusting" part is that I think it could/should share more code
> with the vsyscall case, and the whole target-pte checking and setting
> should be shared too.

I tried that. It was uglier. The percpu code wants to make up a new PTE because the real kernel mapping uses large pages. The vsyscall code wants to copy a PTE because it’s really a PTE and it has unusual permissions.

>
> Beause not being shared, I react to this:
>
>> + set_pte(target_pte, pfn_pte(pa >> PAGE_SHIFT, PAGE_KERNEL));
>
> Hmm. The vsyscall code just does
>
> *target_pte = ..
>
> without any set_pte() stuff. Do we want/need the PVOP cases, and if
> so, why doesn't the vsyscall case need it?

It doesn’t need it. I could use plain assignment.

>
> Anyway, I love the approach, and how this gets rid of the nasty
> trampoline, so no real complaints, just "this needs some fixups".
>
>

I’ll do the fixups. I think that, if we want to unmap the pages for CPUs that aren’t present, that should be a separate patch. I’m also not convinced it adds much value.

In general, PTI is fairly crappy, and it leaks all kinds of information. I suspect the worst leak is the NMI stack for local and remote CPUs. Fixing *that* is going to be fugly, but may actually be important, because I can easily imagine malicious user code that causes arbitrary kernel memory to get read and spilled on the NMI stack.

What we *should* do IMO is defer allocation of percpu space for not-present CPUs to save a bunch of memory. But that’s a major change and will probably break things.
\
 
 \ /
  Last update: 2018-07-22 22:59    [W:0.067 / U:6.976 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site