Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC 2/2] x86/pti/64: Remove the SYSCALL64 entry trampoline
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <CA+55aFz1ne3KTzni2Yvsp8ZRFzk+s78ZhKyGeLZvmRivBhFMfA@mail.gmail.com>
Date:   Sun, 22 Jul 2018 13:59:21 -0700
Cc:     Andrew Lutomirski <luto@kernel.org>,
        the arch/x86 maintainers <x86@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <422DF5AC-6B45-406F-B3FC-DD1AA9BC18F6@amacapital.net>
References: <cover.1532281180.git.luto@kernel.org> <1e3d01ce04315218d3f8ee269528bb774a4d1d60.1532281180.git.luto@kernel.org> <CA+55aFz1ne3KTzni2Yvsp8ZRFzk+s78ZhKyGeLZvmRivBhFMfA@mail.gmail.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Jul 22, 2018, at 11:27 AM, Linus Torvalds <torvalds@linux-foundation.or=
g> wrote:
>=20
>> On Sun, Jul 22, 2018 at 10:45 AM Andy Lutomirski <luto@kernel.org> wrote:=

>>=20
>> 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.
>=20
> Me likey.
>=20
> However:
>=20
>> This does not add a new direct information leak, since the TSS is
>> readable by Meltdown from the cpu_entry_area alias regardless.
>=20
> Afaik, it does now potentially expose through meltdown the per-thread
> entry stack info, which is new.

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

>=20
> But I don't think that's a show-stopper.
>=20
>> static void __init pti_clone_user_shared(void)
>> {
>> +       for_each_possible_cpu(cpu) {
>=20
> But this code is pretty disgusting and seems wrong.
>=20
> 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=E2=80=99s awk=
ward because cpu_entry_area is mapped one PMD at a time right now.

It=E2=80=99s also awkward to expose a percpu page dynamically, because (I th=
ink) percpu data isn=E2=80=99t guaranteed to all be in the same PGD-sized ar=
ea. A vmalloc fault in the early SYSCALL64 path is fatal.

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

Oops.

>=20
> 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 beca=
use the real kernel mapping uses large pages. The vsyscall code wants to cop=
y a PTE because it=E2=80=99s really a PTE and it has unusual permissions.

>=20
> Beause not being shared, I react to this:
>=20
>> +               set_pte(target_pte, pfn_pte(pa >> PAGE_SHIFT, PAGE_KERNEL=
));
>=20
> Hmm. The vsyscall code just does
>=20
>        *target_pte =3D ..
>=20
> 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=E2=80=99t need it. I could use plain assignment.

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

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

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

What we *should* do IMO is defer allocation of percpu space for not-present C=
PUs to save a bunch of memory.  But that=E2=80=99s a major change and will p=
robably break things.=