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

[Ingo Molnar <mingo@kernel.org>]

* 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