Re: [RFC][PATCH] x86/mm: warn on W+x mappings

[Kees Cook]

On Thu, Oct 1, 2015 at 9:28 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> Warn on any residual W+x mappings if X86_PTDUMP is enabled.
>
> Sample dmesg output:
> Checking for W+x mappings
> 0xffffffff81755000-0xffffffff81800000         684K     RW                 GLB x  pte
> Found W+x mappings.  Please fix.
>
> Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> ---
> Not sure if this is the best place to put this check.
> It must occur after free_init_pages() or it won't catch the
> W+x case for the gap between __ex_table and rodata.

Yeah. Hmm. I want this test for sure, but I'd like to be able to do
with without needing PTDUMP, since that puts a very sensitive file in
debugfs. I wonder if we can reuse the same code, but only expose the
page tables to userspace with PTDUMP?

-Kees

>
>  arch/x86/include/asm/pgtable.h |  6 ++++++
>  arch/x86/mm/dump_pagetables.c  | 31 ++++++++++++++++++++++++++++++-
>  arch/x86/mm/init_64.c          |  2 ++
>  3 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 867da5b..8e771c1 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -20,6 +20,12 @@
>
>  void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
>
> +#ifdef CONFIG_X86_PTDUMP
> +void ptdump_walk_pgd_level_checkwx(void);
> +#else
> +#define ptdump_walk_pgd_level_checkwx() do { } while (0)
> +#endif
> +
>  /*
>   * ZERO_PAGE is a global shared page that is always zero: used
>   * for zero-mapped memory areas etc..
> diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> index f0cedf3..986903b 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -32,6 +32,8 @@ struct pg_state {
>         const struct addr_marker *marker;
>         unsigned long lines;
>         bool to_dmesg;
> +       bool check_wx;
> +       bool found_wx;
>  };
>
>  struct addr_marker {
> @@ -214,6 +216,13 @@ static void note_page(struct seq_file *m, struct pg_state *st,
>                 const char *unit = units;
>                 unsigned long delta;
>                 int width = sizeof(unsigned long) * 2;
> +               pgprotval_t pr = pgprot_val(st->current_prot);
> +               bool savedmesg = st->to_dmesg;
> +
> +               if (st->check_wx && (pr & _PAGE_RW) && !(pr & _PAGE_NX)) {
> +                       st->to_dmesg = true;
> +                       st->found_wx = true;
> +               }
>
>                 /*
>                  * Now print the actual finished series
> @@ -261,6 +270,7 @@ static void note_page(struct seq_file *m, struct pg_state *st,
>                 st->start_address = st->current_address;
>                 st->current_prot = new_prot;
>                 st->level = level;
> +               st->to_dmesg = savedmesg;
>         }
>  }
>
> @@ -344,7 +354,8 @@ static void walk_pud_level(struct seq_file *m, struct pg_state *st, pgd_t addr,
>  #define pgd_none(a)  pud_none(__pud(pgd_val(a)))
>  #endif
>
> -void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
> +static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
> +                                      bool checkwx)
>  {
>  #ifdef CONFIG_X86_64
>         pgd_t *start = (pgd_t *) &init_level4_pgt;
> @@ -359,6 +370,12 @@ void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
>                 st.to_dmesg = true;
>         }
>
> +       st.check_wx = checkwx;
> +       if (checkwx) {
> +               pr_info("Checking for W+x mappings\n");
> +               st.found_wx = false;
> +       }
> +
>         for (i = 0; i < PTRS_PER_PGD; i++) {
>                 st.current_address = normalize_addr(i * PGD_LEVEL_MULT);
>                 if (!pgd_none(*start)) {
> @@ -378,6 +395,18 @@ void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
>         /* Flush out the last page */
>         st.current_address = normalize_addr(PTRS_PER_PGD*PGD_LEVEL_MULT);
>         note_page(m, &st, __pgprot(0), 0);
> +       if (checkwx && st.found_wx)
> +               pr_warn("Found W+x mappings.  Please fix.\n");
> +}
> +
> +void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
> +{
> +       ptdump_walk_pgd_level_core(m, pgd, false);
> +}
> +
> +void ptdump_walk_pgd_level_checkwx(void)
> +{
> +       ptdump_walk_pgd_level_core(NULL, NULL, true);
>  }
>
>  static int ptdump_show(struct seq_file *m, void *v)
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index df48430..7e704da 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -1150,6 +1150,8 @@ void mark_rodata_ro(void)
>         free_init_pages("unused kernel",
>                         (unsigned long) __va(__pa_symbol(rodata_end)),
>                         (unsigned long) __va(__pa_symbol(_sdata)));
> +
> +       ptdump_walk_pgd_level_checkwx();
>  }
>
>  #endif
> --
> 2.1.0
>



-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/