[PATCH 0/3] PTI for x86-32 Fixes

[PATCH 0/3] PTI for x86-32 Fixes

[Joerg Roedel]

Hi,

here is a small patch-set to fix two small issues in the
PTI implementation for 32 bit x86. The issues are:

	1) Fix the 32 bit PCID check. I used the wrong
	   operator there and this caused false-positive
	   warnings.

	2) The other two patches make sure the init-hole is
	   not mapped into the user page-table. It is the
	   32 bit counterpart to commit

	   c40a56a7818c ('x86/mm/init: Remove freed kernel image areas from alias mapping')

	   for the 64 bit PTI implementation.

I tested that no-PAE, PAE and 64 bit kernel all boot and
have correct user page-tables with identical global mappings
between user and kernel.

Regards,

	Joerg

Joerg Roedel (3):
  x86/mm/pti: Fix 32 bit PCID check
  x86/mm/pti: Don't clear permissions in pti_clone_pmd()
  x86/mm/pti: Clone kernel-image on PTE level for 32 bit

 arch/x86/mm/pti.c | 143 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 100 insertions(+), 43 deletions(-)

-- 
2.7.4

[PATCH 1/3] x86/mm/pti: Fix 32 bit PCID check

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The check uses the wrong operator and causes false positive
warnings in the kernel log on some systems.

Fixes: 5e8105950a8b3 ('x86/mm/pti: Add Warning when booting on a PCID capable CPU')
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/mm/pti.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index ef8db6f..113ba14 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -549,7 +549,7 @@ void __init pti_init(void)
 	 * supported on 32 bit anyway. To print the warning we need to
 	 * check with cpuid directly again.
 	 */
-	if (cpuid_ecx(0x1) && BIT(17)) {
+	if (cpuid_ecx(0x1) & BIT(17)) {
 		/* Use printk to work around pr_fmt() */
 		printk(KERN_WARNING "\n");
 		printk(KERN_WARNING "************************************************************\n");
-- 
2.7.4

[PATCH 2/3] x86/mm/pti: Don't clear permissions in pti_clone_pmd()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The function sets the global-bit on cloned PMD entries,
which only makes sense when the permissions are identical
between the user and the kernel page-table.

Further, only write-permissions are cleared for entry-text
and kernel-text sections, which are not writeable anyway.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/mm/pti.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 113ba14..5164c98 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -291,7 +291,7 @@ static void __init pti_setup_vsyscall(void) { }
 #endif
 
 static void
-pti_clone_pmds(unsigned long start, unsigned long end, pmdval_t clear)
+pti_clone_pmds(unsigned long start, unsigned long end)
 {
 	unsigned long addr;
 
@@ -352,7 +352,7 @@ pti_clone_pmds(unsigned long start, unsigned long end, pmdval_t clear)
 		 * tables will share the last-level page tables of this
 		 * address range
 		 */
-		*target_pmd = pmd_clear_flags(*pmd, clear);
+		*target_pmd = *pmd;
 	}
 }
 
@@ -398,7 +398,7 @@ static void __init pti_clone_user_shared(void)
 	start = CPU_ENTRY_AREA_BASE;
 	end   = start + (PAGE_SIZE * CPU_ENTRY_AREA_PAGES);
 
-	pti_clone_pmds(start, end, 0);
+	pti_clone_pmds(start, end);
 }
 #endif /* CONFIG_X86_64 */
 
@@ -418,8 +418,7 @@ static void __init pti_setup_espfix64(void)
 static void pti_clone_entry_text(void)
 {
 	pti_clone_pmds((unsigned long) __entry_text_start,
-			(unsigned long) __irqentry_text_end,
-		       _PAGE_RW);
+		       (unsigned long) __irqentry_text_end);
 }
 
 /*
@@ -501,7 +500,7 @@ static void pti_clone_kernel_text(void)
 	 * pti_set_kernel_image_nonglobal() did to clear the
 	 * global bit.
 	 */
-	pti_clone_pmds(start, end_clone, _PAGE_RW);
+	pti_clone_pmds(start, end_clone);
 
 	/*
 	 * pti_clone_pmds() will set the global bit in any PMDs
-- 
2.7.4

Re: [PATCH 2/3] x86/mm/pti: Don't clear permissions in pti_clone_pmd()

[Dave Hansen]

On 08/07/2018 03:24 AM, Joerg Roedel wrote:
> The function sets the global-bit on cloned PMD entries,
> which only makes sense when the permissions are identical
> between the user and the kernel page-table.
> 
> Further, only write-permissions are cleared for entry-text
> and kernel-text sections, which are not writeable anyway.

I think this patch is correct, but I'd be curious if Andy remembers why
we chose to clear _PAGE_RW on these things.  It might have been that we
were trying to say that the *entry* code shouldn't write to this stuff,
regardless of whether the normal kernel can.

But, either way, I agree with the logic here that Global pages must
share permissions between both mappings, so feel free to add my Ack.  I
just want to make sure Andy doesn't remember some detail I'm forgetting.

Re: [PATCH 2/3] x86/mm/pti: Don't clear permissions in pti_clone_pmd()

[Andy Lutomirski]

On Tue, Aug 7, 2018 at 11:34 AM, Dave Hansen <dave.hansen@intel.com> wrote:
> On 08/07/2018 03:24 AM, Joerg Roedel wrote:
>> The function sets the global-bit on cloned PMD entries,
>> which only makes sense when the permissions are identical
>> between the user and the kernel page-table.
>>
>> Further, only write-permissions are cleared for entry-text
>> and kernel-text sections, which are not writeable anyway.
>
> I think this patch is correct, but I'd be curious if Andy remembers why
> we chose to clear _PAGE_RW on these things.  It might have been that we
> were trying to say that the *entry* code shouldn't write to this stuff,
> regardless of whether the normal kernel can.
>
> But, either way, I agree with the logic here that Global pages must
> share permissions between both mappings, so feel free to add my Ack.  I
> just want to make sure Andy doesn't remember some detail I'm forgetting.

I suspect it's because we used to (and maybe still do) initialize the
user tables before mark_read_only().

Re: [PATCH 2/3] x86/mm/pti: Don't clear permissions in pti_clone_pmd()

[Thomas Gleixner]

On Tue, 7 Aug 2018, Andy Lutomirski wrote:

> On Tue, Aug 7, 2018 at 11:34 AM, Dave Hansen <dave.hansen@intel.com> wrote:
> > On 08/07/2018 03:24 AM, Joerg Roedel wrote:
> >> The function sets the global-bit on cloned PMD entries,
> >> which only makes sense when the permissions are identical
> >> between the user and the kernel page-table.
> >>
> >> Further, only write-permissions are cleared for entry-text
> >> and kernel-text sections, which are not writeable anyway.
> >
> > I think this patch is correct, but I'd be curious if Andy remembers why
> > we chose to clear _PAGE_RW on these things.  It might have been that we
> > were trying to say that the *entry* code shouldn't write to this stuff,
> > regardless of whether the normal kernel can.
> >
> > But, either way, I agree with the logic here that Global pages must
> > share permissions between both mappings, so feel free to add my Ack.  I
> > just want to make sure Andy doesn't remember some detail I'm forgetting.
> 
> I suspect it's because we used to (and maybe still do) initialize the
> user tables before mark_read_only().

We still do that because we need the entry stuff working for interrupts
early on. We now repeat the clone after mark_ro so the mask RW is not
longer required.

Thanks,

	tglx

Re: [PATCH 2/3] x86/mm/pti: Don't clear permissions in pti_clone_pmd()

[Andy Lutomirski]

On Tue, Aug 7, 2018 at 1:21 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 7 Aug 2018, Andy Lutomirski wrote:
>
>> On Tue, Aug 7, 2018 at 11:34 AM, Dave Hansen <dave.hansen@intel.com> wrote:
>> > On 08/07/2018 03:24 AM, Joerg Roedel wrote:
>> >> The function sets the global-bit on cloned PMD entries,
>> >> which only makes sense when the permissions are identical
>> >> between the user and the kernel page-table.
>> >>
>> >> Further, only write-permissions are cleared for entry-text
>> >> and kernel-text sections, which are not writeable anyway.
>> >
>> > I think this patch is correct, but I'd be curious if Andy remembers why
>> > we chose to clear _PAGE_RW on these things.  It might have been that we
>> > were trying to say that the *entry* code shouldn't write to this stuff,
>> > regardless of whether the normal kernel can.
>> >
>> > But, either way, I agree with the logic here that Global pages must
>> > share permissions between both mappings, so feel free to add my Ack.  I
>> > just want to make sure Andy doesn't remember some detail I'm forgetting.
>>
>> I suspect it's because we used to (and maybe still do) initialize the
>> user tables before mark_read_only().
>
> We still do that because we need the entry stuff working for interrupts
> early on. We now repeat the clone after mark_ro so the mask RW is not
> longer required.

Agreed.

[PATCH 3/3] x86/mm/pti: Clone kernel-image on PTE level for 32 bit

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

On 32 bit the kernel sections are not huge-page aligned.
When we clone them on PMD-level we unevitably map some areas
that are normal kernel memory and may contain secrets to
user-space. To prevent that we need to clone the
kernel-image on PTE-level for 32 bit.

Also make the page-table cloning clode more general so that
it can handle PMD and PTE level cloning. This can be
generalized further in the future to also handle clones on
the P4D-level.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/mm/pti.c | 140 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 99 insertions(+), 41 deletions(-)

diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 5164c98..1dc5c68 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -54,6 +54,16 @@
 #define __GFP_NOTRACK	0
 #endif
 
+/*
+ * Define the page-table levels we clone for user-space on 32
+ * and 64 bit.
+ */
+#ifdef CONFIG_X86_64
+#define	PTI_LEVEL_KERNEL_IMAGE	PTI_CLONE_PMD
+#else
+#define	PTI_LEVEL_KERNEL_IMAGE	PTI_CLONE_PTE
+#endif
+
 static void __init pti_print_if_insecure(const char *reason)
 {
 	if (boot_cpu_has_bug(X86_BUG_CPU_MELTDOWN))
@@ -228,7 +238,6 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
 	return pmd_offset(pud, address);
 }
 
-#ifdef CONFIG_X86_VSYSCALL_EMULATION
 /*
  * Walk the shadow copy of the page tables (optionally) trying to allocate
  * page table pages on the way down.  Does not support large pages.
@@ -270,6 +279,7 @@ static __init pte_t *pti_user_pagetable_walk_pte(unsigned long address)
 	return pte;
 }
 
+#ifdef CONFIG_X86_VSYSCALL_EMULATION
 static void __init pti_setup_vsyscall(void)
 {
 	pte_t *pte, *target_pte;
@@ -290,8 +300,14 @@ static void __init pti_setup_vsyscall(void)
 static void __init pti_setup_vsyscall(void) { }
 #endif
 
+enum pti_clone_level {
+	PTI_CLONE_PMD,
+	PTI_CLONE_PTE,
+};
+
 static void
-pti_clone_pmds(unsigned long start, unsigned long end)
+pti_clone_pgtable(unsigned long start, unsigned long end,
+		  enum pti_clone_level level)
 {
 	unsigned long addr;
 
@@ -299,7 +315,8 @@ pti_clone_pmds(unsigned long start, unsigned long end)
 	 * Clone the populated PMDs which cover start to end. These PMD areas
 	 * can have holes.
 	 */
-	for (addr = start; addr < end; addr += PMD_SIZE) {
+	for (addr = start; addr < end;) {
+		pte_t *pte, *target_pte;
 		pmd_t *pmd, *target_pmd;
 		pgd_t *pgd;
 		p4d_t *p4d;
@@ -315,44 +332,84 @@ pti_clone_pmds(unsigned long start, unsigned long end)
 		p4d = p4d_offset(pgd, addr);
 		if (WARN_ON(p4d_none(*p4d)))
 			return;
+
 		pud = pud_offset(p4d, addr);
-		if (pud_none(*pud))
+		if (pud_none(*pud)) {
+			addr += PUD_SIZE;
 			continue;
+		}
+
 		pmd = pmd_offset(pud, addr);
-		if (pmd_none(*pmd))
+		if (pmd_none(*pmd)) {
+			addr += PMD_SIZE;
 			continue;
+		}
 
-		target_pmd = pti_user_pagetable_walk_pmd(addr);
-		if (WARN_ON(!target_pmd))
-			return;
-
-		/*
-		 * Only clone present PMDs.  This ensures only setting
-		 * _PAGE_GLOBAL on present PMDs.  This should only be
-		 * called on well-known addresses anyway, so a non-
-		 * present PMD would be a surprise.
-		 */
-		if (WARN_ON(!(pmd_flags(*pmd) & _PAGE_PRESENT)))
-			return;
-
-		/*
-		 * Setting 'target_pmd' below creates a mapping in both
-		 * the user and kernel page tables.  It is effectively
-		 * global, so set it as global in both copies.  Note:
-		 * the X86_FEATURE_PGE check is not _required_ because
-		 * the CPU ignores _PAGE_GLOBAL when PGE is not
-		 * supported.  The check keeps consistentency with
-		 * code that only set this bit when supported.
-		 */
-		if (boot_cpu_has(X86_FEATURE_PGE))
-			*pmd = pmd_set_flags(*pmd, _PAGE_GLOBAL);
-
-		/*
-		 * Copy the PMD.  That is, the kernelmode and usermode
-		 * tables will share the last-level page tables of this
-		 * address range
-		 */
-		*target_pmd = *pmd;
+		if (pmd_large(*pmd) || level == PTI_CLONE_PMD) {
+			target_pmd = pti_user_pagetable_walk_pmd(addr);
+			if (WARN_ON(!target_pmd))
+				return;
+
+			/*
+			 * Only clone present PMDs.  This ensures only setting
+			 * _PAGE_GLOBAL on present PMDs.  This should only be
+			 * called on well-known addresses anyway, so a non-
+			 * present PMD would be a surprise.
+			 */
+			if (WARN_ON(!(pmd_flags(*pmd) & _PAGE_PRESENT)))
+				return;
+
+			/*
+			 * Setting 'target_pmd' below creates a mapping in both
+			 * the user and kernel page tables.  It is effectively
+			 * global, so set it as global in both copies.  Note:
+			 * the X86_FEATURE_PGE check is not _required_ because
+			 * the CPU ignores _PAGE_GLOBAL when PGE is not
+			 * supported.  The check keeps consistentency with
+			 * code that only set this bit when supported.
+			 */
+			if (boot_cpu_has(X86_FEATURE_PGE))
+				*pmd = pmd_set_flags(*pmd, _PAGE_GLOBAL);
+
+			/*
+			 * Copy the PMD.  That is, the kernelmode and usermode
+			 * tables will share the last-level page tables of this
+			 * address range
+			 */
+			*target_pmd = *pmd;
+
+			addr += PMD_SIZE;
+
+		} else if (level == PTI_CLONE_PTE) {
+
+			/* Walk the page-table down to the pte level */
+			pte = pte_offset_kernel(pmd, addr);
+			if (pte_none(*pte)) {
+				addr += PAGE_SIZE;
+				continue;
+			}
+
+			/* Only clone present PTEs */
+			if (WARN_ON(!(pte_flags(*pte) & _PAGE_PRESENT)))
+				return;
+
+			/* Allocate PTE in the user page-table */
+			target_pte = pti_user_pagetable_walk_pte(addr);
+			if (WARN_ON(!target_pte))
+				return;
+
+			/* Set GLOBAL bit in both PTEs */
+			if (boot_cpu_has(X86_FEATURE_PGE))
+				*pte = pte_set_flags(*pte, _PAGE_GLOBAL);
+
+			/* Clone the PTE */
+			*target_pte = *pte;
+
+			addr += PAGE_SIZE;
+
+		} else {
+			BUG();
+		}
 	}
 }
 
@@ -398,7 +455,7 @@ static void __init pti_clone_user_shared(void)
 	start = CPU_ENTRY_AREA_BASE;
 	end   = start + (PAGE_SIZE * CPU_ENTRY_AREA_PAGES);
 
-	pti_clone_pmds(start, end);
+	pti_clone_pgtable(start, end, PTI_CLONE_PMD);
 }
 #endif /* CONFIG_X86_64 */
 
@@ -417,8 +474,9 @@ static void __init pti_setup_espfix64(void)
  */
 static void pti_clone_entry_text(void)
 {
-	pti_clone_pmds((unsigned long) __entry_text_start,
-		       (unsigned long) __irqentry_text_end);
+	pti_clone_pgtable((unsigned long) __entry_text_start,
+			  (unsigned long) __irqentry_text_end,
+			  PTI_CLONE_PMD);
 }
 
 /*
@@ -500,10 +558,10 @@ static void pti_clone_kernel_text(void)
 	 * pti_set_kernel_image_nonglobal() did to clear the
 	 * global bit.
 	 */
-	pti_clone_pmds(start, end_clone);
+	pti_clone_pgtable(start, end_clone, PTI_LEVEL_KERNEL_IMAGE);
 
 	/*
-	 * pti_clone_pmds() will set the global bit in any PMDs
+	 * pti_clone_pgtable() will set the global bit in any PMDs
 	 * that it clones, but we also need to get any PTEs in
 	 * the last level for areas that are not huge-page-aligned.
 	 */
-- 
2.7.4

Re: [PATCH 0/3] PTI for x86-32 Fixes

[David H. Gutteridge]

On Tue, 2018-08-07 at 12:24 +0200, Joerg Roedel wrote:
> Hi,
> 
> here is a small patch-set to fix two small issues in the
> PTI implementation for 32 bit x86. The issues are:
> 
> 	1) Fix the 32 bit PCID check. I used the wrong
> 	   operator there and this caused false-positive
> 	   warnings.
> 
> 	2) The other two patches make sure the init-hole is
> 	   not mapped into the user page-table. It is the
> 	   32 bit counterpart to commit
> 
> 	   c40a56a7818c ('x86/mm/init: Remove freed kernel image areas
> from alias mapping')
> 
> 	   for the 64 bit PTI implementation.
> 
> I tested that no-PAE, PAE and 64 bit kernel all boot and
> have correct user page-tables with identical global mappings
> between user and kernel.
> 
> Regards,
> 
> 	Joerg
> 
> Joerg Roedel (3):
>   x86/mm/pti: Fix 32 bit PCID check
>   x86/mm/pti: Don't clear permissions in pti_clone_pmd()
>   x86/mm/pti: Clone kernel-image on PTE level for 32 bit
> 
>  arch/x86/mm/pti.c | 143 ++++++++++++++++++++++++++++++++++++++-------
> ---------
>  1 file changed, 100 insertions(+), 43 deletions(-)

I've tested this in a VM and on an Atom laptop, as usual. No
regressions noted.

Tested-by: David H. Gutteridge <dhgutteridge@sympatico.ca>

Regards,

Dave