Re: [PATCH] x86/mm/pti: Move user W+X check into pti_finalize()

[Kees Cook]

On Wed, Aug 8, 2018 at 4:16 AM, Joerg Roedel <joro@xxxxxxxxxx> wrote:
> From: Joerg Roedel <jroedel@xxxxxxx>
>
> 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@xxxxxxx>
> ---
>  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

-- 
Kees Cook
Pixel Security