[PATCH 0/3] x86/pti: Call pti_init() after mark_readonly()

[PATCH 0/3] x86/pti: Call pti_init() after mark_readonly()

[Joerg Roedel]

Hi,

here is a small patch-set to move the call to pti_init()
after mark_readonly() has run. The purpose of pti_inti() is to
initialize the kernel-mappings in the user-space page-table
by mapping kernel-text, entry-text, espfix and vsyscall
mappings into the user-space page-table.

These mappings only make sense when they have exactly the
same permissions as in the kernel page-table wrt.
read/write/execute with the global bit set (which we set in
shared mappings for performance reasons).

Since the mappings are copied only once and are not updated
later, we need to copy them when they are finished, which is
not before mark_readonly() has run.

Calling pti_init() earlier worked for now on x86-64 because
the sections that are cloned are at least 2M aligned and not
changed by later code. But that is still fragile because
pti_init() always needs special care when kernel mappings or
the elf-layout is changed or extended. Further it doesn't
work on x86-32 because the elf sections are not 2M aligned
there.

So move the call to pti_init() after all the kernel-mappings
have been finished.

Any useful feedback appreciated.


Thanks,

	Joerg

Joerg Roedel (3):
  x86/pti: Move pti_init() code out of __init
  x86/mm/pti: Call pti_init() after mark_readonly()
  x86/pti: Call pti_clone_kernel_text() from pti_init()

 arch/x86/entry/vsyscall/vsyscall_64.c |  2 +-
 arch/x86/include/asm/pti.h            |  2 --
 arch/x86/mm/init_64.c                 |  6 ------
 arch/x86/mm/pti.c                     | 19 ++++++++++---------
 init/main.c                           |  8 ++++++--
 5 files changed, 17 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH 1/3] x86/pti: Move pti_init() code out of __init

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This removes the __init annotations from pti_init() and everything it
calls on x86. The pti_init() function sets up the kernel-mappings
visible in the user-space page-table when PTI is enabled, which only
makes sense after the relevant kernel mappings have been finished.

The kernel mappings are finished when the appropriate read-only and
no-execute protections are established for the kernel text, rodata and
data sections, which happens after the init-code/data has been freed by
the kernel.

So to call pti_init() at the right place it can't be __init anymore.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/entry/vsyscall/vsyscall_64.c |  2 +-
 arch/x86/mm/pti.c                     | 16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c
index 82ed001..6cd5cbf 100644
--- a/arch/x86/entry/vsyscall/vsyscall_64.c
+++ b/arch/x86/entry/vsyscall/vsyscall_64.c
@@ -340,7 +340,7 @@ int in_gate_area_no_mm(unsigned long addr)
  * vsyscalls but leave the page not present.  If so, we skip calling
  * this.
  */
-void __init set_vsyscall_pgtable_user_bits(pgd_t *root)
+void set_vsyscall_pgtable_user_bits(pgd_t *root)
 {
 	pgd_t *pgd;
 	p4d_t *p4d;
diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 4d418e7..8a80522 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -234,7 +234,7 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
  *
  * Returns a pointer to a PTE on success, or NULL on failure.
  */
-static __init pte_t *pti_user_pagetable_walk_pte(unsigned long address)
+static pte_t *pti_user_pagetable_walk_pte(unsigned long address)
 {
 	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
 	pmd_t *pmd = pti_user_pagetable_walk_pmd(address);
@@ -262,7 +262,7 @@ static __init pte_t *pti_user_pagetable_walk_pte(unsigned long address)
 	return pte;
 }
 
-static void __init pti_setup_vsyscall(void)
+static void pti_setup_vsyscall(void)
 {
 	pte_t *pte, *target_pte;
 	unsigned int level;
@@ -279,7 +279,7 @@ static void __init pti_setup_vsyscall(void)
 	set_vsyscall_pgtable_user_bits(kernel_to_user_pgdp(swapper_pg_dir));
 }
 #else
-static void __init pti_setup_vsyscall(void) { }
+static void pti_setup_vsyscall(void) { }
 #endif
 
 static void
@@ -348,7 +348,7 @@ pti_clone_pmds(unsigned long start, unsigned long end, pmdval_t clear)
  * Clone a single p4d (i.e. a top-level entry on 4-level systems and a
  * next-level entry on 5-level systems.
  */
-static void __init pti_clone_p4d(unsigned long addr)
+static void pti_clone_p4d(unsigned long addr)
 {
 	p4d_t *kernel_p4d, *user_p4d;
 	pgd_t *kernel_pgd;
@@ -362,7 +362,7 @@ static void __init pti_clone_p4d(unsigned long addr)
 /*
  * Clone the CPU_ENTRY_AREA into the user space visible page table.
  */
-static void __init pti_clone_user_shared(void)
+static void pti_clone_user_shared(void)
 {
 	pti_clone_p4d(CPU_ENTRY_AREA_BASE);
 }
@@ -370,7 +370,7 @@ static void __init pti_clone_user_shared(void)
 /*
  * Clone the ESPFIX P4D into the user space visible page table
  */
-static void __init pti_setup_espfix64(void)
+static void pti_setup_espfix64(void)
 {
 #ifdef CONFIG_X86_ESPFIX64
 	pti_clone_p4d(ESPFIX_BASE_ADDR);
@@ -380,7 +380,7 @@ static void __init pti_setup_espfix64(void)
 /*
  * Clone the populated PMDs of the entry and irqentry text and force it RO.
  */
-static void __init pti_clone_entry_text(void)
+static void pti_clone_entry_text(void)
 {
 	pti_clone_pmds((unsigned long) __entry_text_start,
 			(unsigned long) __irqentry_text_end,
@@ -486,7 +486,7 @@ void pti_set_kernel_image_nonglobal(void)
 /*
  * Initialize kernel page table isolation
  */
-void __init pti_init(void)
+void pti_init(void)
 {
 	if (!static_cpu_has(X86_FEATURE_PTI))
 		return;
-- 
2.7.4

[PATCH 2/3] x86/pti: Call pti_init() after mark_readonly()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

PTI init code clones some parts of the kernel mappings to the user-space
page-table. For the kernel and user-space page-table to be consistent,
the cloning should happen when the relevant parts of the kernel
page-table are finished, which is right after mark_readonly() returns.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 init/main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/init/main.c b/init/main.c
index 3b4ada1..0b5d0f1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -524,8 +524,6 @@ static void __init mm_init(void)
 	ioremap_huge_init();
 	/* Should be run before the first non-init thread is created */
 	init_espfix_bsp();
-	/* Should be run after espfix64 is set up. */
-	pti_init();
 }
 
 asmlinkage __visible void __init start_kernel(void)
@@ -1065,6 +1063,12 @@ static int __ref kernel_init(void *unused)
 	jump_label_invalidate_initmem();
 	free_initmem();
 	mark_readonly();
+	/*
+	 * Kernel text/rodata/data sections have the right protections
+	 * now. If necessary, init PTI and clone the relevant pieces
+	 * to the user-space page-table.
+	 */
+	pti_init();
 	system_state = SYSTEM_RUNNING;
 	numa_default_policy();
 
-- 
2.7.4

[PATCH 3/3] x86/pti: Call pti_clone_kernel_text() from pti_init()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The pti_init() function is now called late enough for
pti_clone_kernel_text(), so call it directly from there.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/include/asm/pti.h | 2 --
 arch/x86/mm/init_64.c      | 6 ------
 arch/x86/mm/pti.c          | 3 ++-
 3 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/pti.h b/arch/x86/include/asm/pti.h
index 38a17f1..0b5ef05 100644
--- a/arch/x86/include/asm/pti.h
+++ b/arch/x86/include/asm/pti.h
@@ -6,10 +6,8 @@
 #ifdef CONFIG_PAGE_TABLE_ISOLATION
 extern void pti_init(void);
 extern void pti_check_boottime_disable(void);
-extern void pti_clone_kernel_text(void);
 #else
 static inline void pti_check_boottime_disable(void) { }
-static inline void pti_clone_kernel_text(void) { }
 #endif
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index a688617..9b19f9a 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1291,12 +1291,6 @@ void mark_rodata_ro(void)
 			(unsigned long) __va(__pa_symbol(_sdata)));
 
 	debug_checkwx();
-
-	/*
-	 * Do this after all of the manipulation of the
-	 * kernel text page tables are complete.
-	 */
-	pti_clone_kernel_text();
 }
 
 int kern_addr_valid(unsigned long addr)
diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 8a80522..dd344f7 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -438,7 +438,7 @@ static inline bool pti_kernel_image_global_ok(void)
  * For some configurations, map all of kernel text into the user page
  * tables.  This reduces TLB misses, especially on non-PCID systems.
  */
-void pti_clone_kernel_text(void)
+static void pti_clone_kernel_text(void)
 {
 	/*
 	 * rodata is part of the kernel image and is normally
@@ -499,6 +499,7 @@ void pti_init(void)
 	pti_set_kernel_image_nonglobal();
 	/* Replace some of the global bits just for shared entry text: */
 	pti_clone_entry_text();
+	pti_clone_kernel_text();
 	pti_setup_espfix64();
 	pti_setup_vsyscall();
 }
-- 
2.7.4

Re: [PATCH 0/3] x86/pti: Call pti_init() after mark_readonly()

[Joerg Roedel]

Hey Thomas,

On Tue, Jul 03, 2018 at 01:52:23PM +0200, Joerg Roedel wrote:
> Joerg Roedel (3):
>   x86/pti: Move pti_init() code out of __init
>   x86/mm/pti: Call pti_init() after mark_readonly()
>   x86/pti: Call pti_clone_kernel_text() from pti_init()

Please ignore this patch-set. It turned out that some code in the kernel
executes user-space even before /init is started. I just ran into a
problem where request_module calls usermode-helper and then
triple-faults when it tries to go to user-space and finds the
user-pagetable still empty.

I need to look into another solution for this then.


Thanks,

	Joerg

Re: [PATCH 0/3] x86/pti: Call pti_init() after mark_readonly()

[Thomas Gleixner]

On Tue, 10 Jul 2018, Joerg Roedel wrote:

> Hey Thomas,
> 
> On Tue, Jul 03, 2018 at 01:52:23PM +0200, Joerg Roedel wrote:
> > Joerg Roedel (3):
> >   x86/pti: Move pti_init() code out of __init
> >   x86/mm/pti: Call pti_init() after mark_readonly()
> >   x86/pti: Call pti_clone_kernel_text() from pti_init()
> 
> Please ignore this patch-set. It turned out that some code in the kernel
> executes user-space even before /init is started. I just ran into a
> problem where request_module calls usermode-helper and then
> triple-faults when it tries to go to user-space and finds the
> user-pagetable still empty.
> 
> I need to look into another solution for this then.

Thanks for the clue. I was about to stare at it.

Thanks,

	tglx