linux-kernel - Re: [RFC PTI 1/3] x86/pti: Vastly simplify pgd synchronization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Dec 2017 12:49:18 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Borislav Petkov <bp@...en8.de>,
        Brian Gerst <brgerst@...il.com>,
        David Laight <David.Laight@...lab.com>,
        Kees Cook <keescook@...omium.org>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC PTI 1/3] x86/pti: Vastly simplify pgd synchronization

On Fri, Dec 8, 2017 at 12:17 PM, Dave Hansen <dave.hansen@...el.com> wrote:
> From a high level, what finally allowed this to happen?  Because
> kpti_add_user_map() all went away, including the LDT one?
>

Yes, exactly.

>
>> +     if (pgdp_maps_userspace(pgdp)) {
>>               /*
>> +              * The user page tables get the full PGD,
>> +              * accessible from userspace:
>>                */
>> +             kernel_to_user_pgdp(pgdp)->pgd = pgd.pgd;
>> +
>> +             /*
>> +              * If this is normal user memory, make it NX in the kernel
>> +              * pagetables so that, if we somehow screw up and return to
>> +              * usermode with the kernel CR3 loaded, we'll get a page
>> +              * fault instead of allowing user code to execute with
>> +              * the wrong CR3.
>> +              *
>> +              * As exceptions, we don't set NX if:
>> +              *  - this is EFI or similar, the kernel may execute from it
>> +              *  - we don't have NX support
>> +              *  - we're clearing the PGD (i.e. pgd.pgd == 0).
>> +              */
>> +             if ((pgd.pgd & _PAGE_USER) && (__supported_pte_mask & _PAGE_NX))
>> +                     pgd.pgd |= _PAGE_NX;
>
> I scratched my head for a sec to realize why you didn't need an explicit
> pgd.pgd==0 check.  It's part of the _PAGE_USER check, which is a bit
> confusing.  Could we do:
>
> if ((pgd.pgd & (_PAGE_USER|_PAGE_PRESENT)) &&
>     (__supported_pte_mask & _PAGE_NX))

I assume you mean pgd.pgd & (_PAGE_USER|_PAGE_PRESENT) ==
(_PAGE_USER|_PAGE_PRESENT)?

>
> And change the comment:
>
>          * As exceptions, we don't set NX fo:
>          *  - PGDs without _PAGE_USER:  Assume this is for a weird in-kernel
>          *    user like EFI from which we may need to execute.
>          *  - PGDs withoout _PAGE_PRESENT: PGD is being cleared, must
>          *    not set _PAGE_NX
>          *  - we don't have NX support
>
>
>
>
>
>>       } else {
>>               /*
>> +              * Changes to the high (kernel) portion of the kernelmode
>> +              * page tables are not automatically propagated to the
>> +              * usermode tables.
>> +              *
>> +              * Users should keep in mind that, unlike the kernelmode
>> +              * tables, there is no vmalloc_fault equivalent for the
>> +              * usermode tables.  Top-level entries added to init_mm's
>> +              * usermode pgd after boot will not be automatically
>> +              * propagated to other mms.
>>                */
>> -             WARN_ON_ONCE(system_state == SYSTEM_RUNNING);
>>       }
>>  #endif
>
> How does the VSYSCALL page get into the user page tables now?

Fantastic question.  I appear to be bad at testing.  Lemme fix that.