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:17:34 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Andy Lutomirski <luto@...nel.org>, x86@...nel.org
Cc:     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

>From a high level, what finally allowed this to happen?  Because
kpti_add_user_map() all went away, including the LDT one?


> +	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))

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?