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

Kees Cook
2015-10-01T19:24:27Z

On Thu, Oct 1, 2015 at 9:28 AM, Stephen Smalley <sds@tycho.nsa.gov> 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@tycho.nsa.gov>
> ---
> 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