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

Ingo Molnar
2015-10-02T07:37:14Z

* 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