Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753910AbeAFRWi (ORCPT + 1 other); Sat, 6 Jan 2018 12:22:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50326 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753663AbeAFRWh (ORCPT ); Sat, 6 Jan 2018 12:22:37 -0500 Date: Sat, 6 Jan 2018 18:22:32 +0100 From: Andrea Arcangeli To: Dave Hansen Cc: Hanjun Guo , Jiri Kosina , Yisheng Xie , linux-kernel@vger.kernel.org, linux-mm@kvack.org, richard.fellner@student.tugraz.at, moritz.lipp@iaik.tugraz.at, daniel.gruss@iaik.tugraz.at, michael.schwarz@iaik.tugraz.at, luto@kernel.org, Linus Torvalds , keescook@google.com, hughd@google.com, x86@kernel.org Subject: Re: [PATCH 05/23] x86, kaiser: unmap kernel from userspace page tables (core patch) Message-ID: <20180106172232.GC25546@redhat.com> References: <20171123003447.1DB395E3@viggo.jf.intel.com> <93776eb2-b6d4-679a-280c-8ba558a69c34@linux.intel.com> <20a54a5f-f4e5-2126-fb73-6a995d13d52d@linux.intel.com>

<332f4eab-8a3d-8b29-04f2-7c075f81b85b@linux.intel.com> <10ed0bc4-5f98-b771-5020-12838923b0ca@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <10ed0bc4-5f98-b771-5020-12838923b0ca@linux.intel.com> User-Agent: Mutt/1.9.2 (2017-12-15) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Sat, 06 Jan 2018 17:22:37 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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. > > 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