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

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

[Stephen Smalley]

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.

 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

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

[Kees Cook]

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

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

[Borislav Petkov]

On Thu, Oct 01, 2015 at 12:24:25PM -0700, Kees Cook wrote:
> 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?

So make it a debugging option like CONFIG_EFI_PGT_DUMP and let it dump
the pagetable in dmesg during boot, at the exact point you want it to.
Then one can grep dmesg for W+x bits or whatever else...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Oct 01, 2015 at 12:24:25PM -0700, Kees Cook wrote:
> > 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?
> 
> So make it a debugging option like CONFIG_EFI_PGT_DUMP and let it dump the 
> pagetable in dmesg during boot, at the exact point you want it to. Then one can 
> grep dmesg for W+x bits or whatever else...

It's better to generate a WARN()ing programmatically if the W+X condition occurs, 
that gets noticed by tools and people alike. I'd like to start treating that 
condition as a hard kernel bug.

A dump in dmesg is subject to random noise by printk crusaders and is also subject 
to general bitrot, nor does it provide any ready warning to act upon.

Adding an extra debug option is a good idea (just please don't put 'EFI' into the 
name - this isn't really EFI related), to not generate the debugfs node.

I'd even add this debug check as default-enabled in the x86 defconfigs, so that my 
own continuous kernel testing kit picks up any new warnings from it.

Thanks,

	Ingo

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

[Ingo Molnar]

* Stephen Smalley <sds@tycho.nsa.gov> wrote:

> +	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");

So I like the patch, except the way it presents failures:

please generate any printks() only after the check has been performed, not before 
it. There should be a single line printed in the 'test successful' case:

	pr_info("x86/mm: Checked W+X mappings: passed, no W+X pages found.\n");

In the failure case there should be printk()s generated at the first point of 
violation:

+               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;
+               }

via something like:

	WARN_ONCE(1, "x86/mm: Found insecure W+X mapping at address %p/%pS\n", addr, addr);

i.e. output the address and the symbolic name as well. Only generate a single 
warning - we expect the normal kernel to be entirely warnings-free.

and then print the number of pages with bad mappings in the 'result' line:

	pr_info("x86/mm: Checked W+X mappings: FAILED, %ld W+X pages found.\n", nr_failures);

Thanks,

	Ingo

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

[Borislav Petkov]

On Fri, Oct 02, 2015 at 09:26:44AM +0200, Ingo Molnar wrote:
> It's better to generate a WARN()ing programmatically if the W+X condition occurs, 
> that gets noticed by tools and people alike. I'd like to start treating that 
> condition as a hard kernel bug.
> 
> A dump in dmesg is subject to random noise by printk crusaders and is also subject 
> to general bitrot, nor does it provide any ready warning to act upon.

You're not going to enable this option in production anyway. So when it
is enabled, you're expected to stare at dmesg anyway. The only advantage
of the WARN()'s is that they're bigger. :)

> Adding an extra debug option is a good idea (just please don't put 'EFI' into the 
> name - this isn't really EFI related), to not generate the debugfs node.

Of course not - it was just an example how the EFI code uses PTDUMP. There it is
off by default too.

> I'd even add this debug check as default-enabled in the x86 defconfigs, so that my 
> own continuous kernel testing kit picks up any new warnings from it.

There's the problem with exposing sensitive info in debugfs if you do
that. And nowadays we're trying hard not to leak any of that.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Oct 02, 2015 at 09:26:44AM +0200, Ingo Molnar wrote:
> > It's better to generate a WARN()ing programmatically if the W+X condition occurs, 
> > that gets noticed by tools and people alike. I'd like to start treating that 
> > condition as a hard kernel bug.
> > 
> > A dump in dmesg is subject to random noise by printk crusaders and is also subject 
> > to general bitrot, nor does it provide any ready warning to act upon.
> 
> You're not going to enable this option in production anyway. [...]

Why not? I'd suggest distros do it too, it's not too much code to run during 
bootup. That way if we one some weird configuration forget about a W+X mapping, 
the distro is warned that there's a security problem.

> > I'd even add this debug check as default-enabled in the x86 defconfigs, so 
> > that my own continuous kernel testing kit picks up any new warnings from it.
> 
> There's the problem with exposing sensitive info in debugfs if you do that. And 
> nowadays we're trying hard not to leak any of that.

Ah, I think you missed the following detail: the patch I suggested would separate 
the debugfs bits from the checking bits and would thus allow a 'security check 
only' .config setting.

Distros would normally not want to enable the debugfs file, agreed about that.

Thanks,

	Ingo

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

[Borislav Petkov]

On Sat, Oct 03, 2015 at 09:50:45AM +0200, Ingo Molnar wrote:
> Ah, I think you missed the following detail: the patch I suggested would separate 
> the debugfs bits from the checking bits and would thus allow a 'security check 
> only' .config setting.
> 
> Distros would normally not want to enable the debugfs file, agreed about that.

Right, exactly. Ok, we're on the same page. I see v2 on lkml doing that.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.