x86/mm/pti: Move user W+X check into pti_finalize()
diff mbox series

Message ID 1533727000-9172-1-git-send-email-joro@8bytes.org
State New
Headers show
Series
  • x86/mm/pti: Move user W+X check into pti_finalize()
Related show

Commit Message

Joerg Roedel Aug. 8, 2018, 11:16 a.m. UTC
From: Joerg Roedel <jroedel@suse.de>

The user page-table gets the updated kernel mappings in
pti_finalize(), which runs after the RO+X permissions got
applied to the kernel page-table in mark_readonly().

But with CONFIG_DEBUG_WX enabled, the user page-table is
already checked in mark_readonly() for insecure mappings.
This causes false-positive warnings, because the user
page-table did not get the updated mappings yet.

Move the W+X check for the user page-table into
pti_finalize() after it updated all required mappings.

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

Comments

Dave Hansen Aug. 8, 2018, 3:54 p.m. UTC | #1
On 08/08/2018 04:16 AM, Joerg Roedel wrote:
> But with CONFIG_DEBUG_WX enabled, the user page-table is
> already checked in mark_readonly() for insecure mappings.
> This causes false-positive warnings, because the user
> page-table did not get the updated mappings yet.

One bit of information missing from the changelog: Could you clarify how
there are any entries in the user page tables for the code to complain?
Before pti_init(), I would have expected the user page tables to be empty.

That causes a different problem, but it would not have resulted in
warnings, so I think I'm missing something.
Kees Cook Aug. 8, 2018, 8:33 p.m. UTC | #2
On Wed, Aug 8, 2018 at 4:16 AM, Joerg Roedel <joro@8bytes.org> wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> The user page-table gets the updated kernel mappings in
> pti_finalize(), which runs after the RO+X permissions got
> applied to the kernel page-table in mark_readonly().
>
> But with CONFIG_DEBUG_WX enabled, the user page-table is
> already checked in mark_readonly() for insecure mappings.
> This causes false-positive warnings, because the user
> page-table did not get the updated mappings yet.
>
> Move the W+X check for the user page-table into
> pti_finalize() after it updated all required mappings.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/include/asm/pgtable.h | 7 +++++--
>  arch/x86/mm/dump_pagetables.c  | 3 +--
>  arch/x86/mm/pti.c              | 2 ++
>  3 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index e39088cb..a1cb333 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -30,11 +30,14 @@ int __init __early_make_pgtable(unsigned long address, pmdval_t pmd);
>  void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
>  void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user);
>  void ptdump_walk_pgd_level_checkwx(void);
> +void ptdump_walk_user_pgd_level_checkwx(void);
>
>  #ifdef CONFIG_DEBUG_WX
> -#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
> +#define debug_checkwx()                ptdump_walk_pgd_level_checkwx()
> +#define debug_checkwx_user()   ptdump_walk_user_pgd_level_checkwx()
>  #else
> -#define debug_checkwx() do { } while (0)
> +#define debug_checkwx()                do { } while (0)
> +#define debug_checkwx_user()   do { } while (0)
>  #endif
>
>  /*
> diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> index ccd92c4..b8ab901 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -569,7 +569,7 @@ void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user)
>  }
>  EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);
>
> -static void ptdump_walk_user_pgd_level_checkwx(void)
> +void ptdump_walk_user_pgd_level_checkwx(void)
>  {
>  #ifdef CONFIG_PAGE_TABLE_ISOLATION
>         pgd_t *pgd = INIT_PGD;
> @@ -586,7 +586,6 @@ static void ptdump_walk_user_pgd_level_checkwx(void)
>  void ptdump_walk_pgd_level_checkwx(void)
>  {
>         ptdump_walk_pgd_level_core(NULL, NULL, true, false);
> -       ptdump_walk_user_pgd_level_checkwx();
>  }
>
>  static int __init pt_dump_init(void)
> diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
> index 69a9d60..026a89a 100644
> --- a/arch/x86/mm/pti.c
> +++ b/arch/x86/mm/pti.c
> @@ -628,4 +628,6 @@ void pti_finalize(void)
>          */
>         pti_clone_entry_text();
>         pti_clone_kernel_text();
> +
> +       debug_checkwx_user();
>  }

I'm slightly nervous about complicating this and splitting up the
check. I have a mild preference that all the checks get moved later,
so that all architectures have the checks happening at the same time
during boot. Splitting this up could give us some weird differences
between architectures, etc.

-Kees
Joerg Roedel Aug. 9, 2018, 11:16 a.m. UTC | #3
Hi Dave,

On Wed, Aug 08, 2018 at 08:54:37AM -0700, Dave Hansen wrote:
> One bit of information missing from the changelog: Could you clarify how
> there are any entries in the user page tables for the code to complain?
> Before pti_init(), I would have expected the user page tables to be empty.

The W+X check runs at the end of mark_readonly() in x86, which is after
pti_init() already put kernel mappings into the user page-table. Problem
is that the cloned entries are still W+X mapped, which is fixed in
pti_finalize() running _after_ mark_readonly().

Regards,

	Joerg
Joerg Roedel Aug. 9, 2018, 11:26 a.m. UTC | #4
Hi Kees,

On Wed, Aug 08, 2018 at 01:33:01PM -0700, Kees Cook wrote:
> I'm slightly nervous about complicating this and splitting up the
> check. I have a mild preference that all the checks get moved later,
> so that all architectures have the checks happening at the same time
> during boot. Splitting this up could give us some weird differences
> between architectures, etc.

As fas as I can see the checks are implemented on x86, arm, and arm64. I
agree that it would be better to run the checks at a unified place
across architectures and can send a patch-set for set once the dust
around the 32-bit PTI implementation for x86 has settled.

But currently the call-places are architecture specific and with that in
mind the split-up on x86 is the right thing to do. I'll change that back
when I implement your idea above.

Regards,

	Joerg
David H. Gutteridge Aug. 17, 2018, 2:42 a.m. UTC | #5
On Wed, 2018-08-08 at 13:16 +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> The user page-table gets the updated kernel mappings in
> pti_finalize(), which runs after the RO+X permissions got
> applied to the kernel page-table in mark_readonly().
> 
> But with CONFIG_DEBUG_WX enabled, the user page-table is
> already checked in mark_readonly() for insecure mappings.
> This causes false-positive warnings, because the user
> page-table did not get the updated mappings yet.
> 
> Move the W+X check for the user page-table into
> pti_finalize() after it updated all required mappings.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/include/asm/pgtable.h | 7 +++++--
>  arch/x86/mm/dump_pagetables.c  | 3 +--
>  arch/x86/mm/pti.c              | 2 ++
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pgtable.h
> b/arch/x86/include/asm/pgtable.h
> index e39088cb..a1cb333 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -30,11 +30,14 @@ int __init __early_make_pgtable(unsigned long
> address, pmdval_t pmd);
>  void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
>  void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd,
> bool user);
>  void ptdump_walk_pgd_level_checkwx(void);
> +void ptdump_walk_user_pgd_level_checkwx(void);
>  
>  #ifdef CONFIG_DEBUG_WX
> -#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
> +#define debug_checkwx()		ptdump_walk_pgd_level_checkwx()
> +#define debug_checkwx_user()	ptdump_walk_user_pgd_level_checkwx()
>  #else
> -#define debug_checkwx() do { } while (0)
> +#define debug_checkwx()		do { } while (0)
> +#define debug_checkwx_user()	do { } while (0)
>  #endif
>  
>  /*
> diff --git a/arch/x86/mm/dump_pagetables.c
> b/arch/x86/mm/dump_pagetables.c
> index ccd92c4..b8ab901 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -569,7 +569,7 @@ void ptdump_walk_pgd_level_debugfs(struct seq_file
> *m, pgd_t *pgd, bool user)
>  }
>  EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);
>  
> -static void ptdump_walk_user_pgd_level_checkwx(void)
> +void ptdump_walk_user_pgd_level_checkwx(void)
>  {
>  #ifdef CONFIG_PAGE_TABLE_ISOLATION
>  	pgd_t *pgd = INIT_PGD;
> @@ -586,7 +586,6 @@ static void
> ptdump_walk_user_pgd_level_checkwx(void)
>  void ptdump_walk_pgd_level_checkwx(void)
>  {
>  	ptdump_walk_pgd_level_core(NULL, NULL, true, false);
> -	ptdump_walk_user_pgd_level_checkwx();
>  }
>  
>  static int __init pt_dump_init(void)
> diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
> index 69a9d60..026a89a 100644
> --- a/arch/x86/mm/pti.c
> +++ b/arch/x86/mm/pti.c
> @@ -628,4 +628,6 @@ void pti_finalize(void)
>  	 */
>  	pti_clone_entry_text();
>  	pti_clone_kernel_text();
> +
> +	debug_checkwx_user();
>  }

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

(The version I tested was the latter pulled into tip:
[ tglx: Folded !NX supported fix ])

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

Regards,

Dave

Patch
diff mbox series

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index e39088cb..a1cb333 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -30,11 +30,14 @@  int __init __early_make_pgtable(unsigned long address, pmdval_t pmd);
 void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
 void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user);
 void ptdump_walk_pgd_level_checkwx(void);
+void ptdump_walk_user_pgd_level_checkwx(void);
 
 #ifdef CONFIG_DEBUG_WX
-#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
+#define debug_checkwx()		ptdump_walk_pgd_level_checkwx()
+#define debug_checkwx_user()	ptdump_walk_user_pgd_level_checkwx()
 #else
-#define debug_checkwx() do { } while (0)
+#define debug_checkwx()		do { } while (0)
+#define debug_checkwx_user()	do { } while (0)
 #endif
 
 /*
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index ccd92c4..b8ab901 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -569,7 +569,7 @@  void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool user)
 }
 EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);
 
-static void ptdump_walk_user_pgd_level_checkwx(void)
+void ptdump_walk_user_pgd_level_checkwx(void)
 {
 #ifdef CONFIG_PAGE_TABLE_ISOLATION
 	pgd_t *pgd = INIT_PGD;
@@ -586,7 +586,6 @@  static void ptdump_walk_user_pgd_level_checkwx(void)
 void ptdump_walk_pgd_level_checkwx(void)
 {
 	ptdump_walk_pgd_level_core(NULL, NULL, true, false);
-	ptdump_walk_user_pgd_level_checkwx();
 }
 
 static int __init pt_dump_init(void)
diff --git a/arch/x86/mm/pti.c b/arch/x86/mm/pti.c
index 69a9d60..026a89a 100644
--- a/arch/x86/mm/pti.c
+++ b/arch/x86/mm/pti.c
@@ -628,4 +628,6 @@  void pti_finalize(void)
 	 */
 	pti_clone_entry_text();
 	pti_clone_kernel_text();
+
+	debug_checkwx_user();
 }