linux-kernel - Re: [PATCH 05/23] x86, kaiser: unmap kernel from userspace page tables (core patch)

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] [day] [month] [year] [list]

Date:   Sat, 6 Jan 2018 18:22:32 +0100
From:   Andrea Arcangeli <aarcange@...hat.com>
To:     Dave Hansen <dave.hansen@...ux.intel.com>
Cc:     Hanjun Guo <guohanjun@...wei.com>, Jiri Kosina <jikos@...nel.org>,
        Yisheng Xie <xieyisheng1@...wei.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        richard.fellner@...dent.tugraz.at, moritz.lipp@...k.tugraz.at,
        daniel.gruss@...k.tugraz.at, michael.schwarz@...k.tugraz.at,
        luto@...nel.org, Linus Torvalds <torvalds@...ux-foundation.org>,
        keescook@...gle.com, hughd@...gle.com, x86@...nel.org
Subject: Re: [PATCH 05/23] x86, kaiser: unmap kernel from userspace page
 tables (core patch)

On Fri, Jan 05, 2018 at 11:51:38PM -0800, Dave Hansen wrote:
> On 01/05/2018 10:28 PM, Hanjun Guo wrote:
> >> +
> >>  	p4d = p4d_alloc(&tboot_mm, pgd, vaddr);
> > Seems pgd will be re-set after p4d_alloc(), so should
> > we put the code behind (or after pud_alloc())?

Thanks Dave and Jiri for these two tboot and efi_64 fixes.

> 
> <sigh> Yes, it has to go below where the PGD actually gets set which is
> after pud_alloc().  You can put it anywhere later in the function.

Did the exact same oversight yesterday when porting Jiri's fix.

efi_64 booted fine verified yesterday in a respin of what I sent here
by just moving it after pud_alloc too:

		pud = pud_alloc(&init_mm, pgd_efi, addr_pgd);
		if (!pud) {
			pr_err("Failed to allocate pud table!\n");
			break;
		}
+		pgd_efi->pgd &= ~_PAGE_NX;

Now I'm having this tested for tboot too (still untested). With tboot
I expect the first build pass the test. All followups on bz.

diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index 088681d4fc45..09cff5f4f9a4 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -131,6 +131,7 @@ static int map_tboot_page(unsigned long vaddr, unsigned long pfn,
 	pud = pud_alloc(&tboot_mm, pgd, vaddr);
 	if (!pud)
 		return -1;
+	pgd->pgd &= ~_PAGE_NX;
 	pmd = pmd_alloc(&tboot_mm, pud, vaddr);
 	if (!pmd)
 		return -1;

Note your upstream submitted version is less theoretically correct than the
above. It won't make a difference in practice, but it is theoretically
wrong to clear the PAGE_NX only if pte_alloc_map succeeds like your
patch does.

If in the future pte_alloc_map fails and for whatever reason the pgd
will still be used and the whole thing will not abort, your fix will
still end up with NX set in the pgd.

Only the first pud allocation establishes itself in the pgd, follow
ups don't if in __pud_alloc pgd_present() will return true.

This is why I did the strictier backport of Jiri's fix yesterday but I
was a little too strict putting it just before pud_alloc and it had to
go just after it.

Thanks,
Andrea