Thread (30 messages) 30 messages, 8 authors, 2018-12-13

Re: [PATCH v7 13/14] module: Do not set nx for module memory before freeing

From: Andrea Parri <hidden>
Date: 2018-12-06 11:13:29
Also in: linux-integrity, lkml

On Tue, Dec 04, 2018 at 05:34:07PM -0800, Nadav Amit wrote:
When module memory is about to be freed, there is no apparent reason to
make it (and its data) executable, but that's exactly what is done
today. This is not efficient and not secure.
Looks to me like you forgot to Cc the maintainer of this file: doing it
now.  The same consideration would hold for 14/14.

  Andrea

quoted hunk ↗ jump to hunk
There are various theories why it was done, but none of them seem as
something that really require it today. nios2 uses kmalloc for module
memory, but anyhow it does not change the PTEs of the module memory.  In
x86, changing vmalloc'd memory mappings also modifies the direct mapping
alias, but the NX-bit is not modified in such way.

So let's remove it. Andy suggested that the changes of the PTEs can be
avoided (excluding the direct-mapping alias), which is true. However,
in x86 it requires some cleanup of the contiguous page allocator, which
is outside of the scope of this patch-set.

Cc: Rick P Edgecombe <rick.p.edgecombe@intel.com>
Cc: Will Deacon <redacted>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Nadav Amit <redacted>
---
 kernel/module.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 7cb207249437..57c5b23746e7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2027,20 +2027,29 @@ void set_all_modules_text_ro(void)
 	mutex_unlock(&module_mutex);
 }
 
-static void disable_ro_nx(const struct module_layout *layout)
+static void module_restore_mappings(const struct module_layout *layout)
 {
-	if (rodata_enabled) {
-		frob_text(layout, set_memory_rw);
-		frob_rodata(layout, set_memory_rw);
-		frob_ro_after_init(layout, set_memory_rw);
-	}
-	frob_rodata(layout, set_memory_x);
-	frob_ro_after_init(layout, set_memory_x);
-	frob_writable_data(layout, set_memory_x);
+	/*
+	 * First, make the mappings of the code non-executable to prevent
+	 * transient W+X mappings from being set when the text is set as RW.
+	 */
+	frob_text(layout, set_memory_nx);
+
+	if (!rodata_enabled)
+		return;
+
+	/*
+	 * Second, set the memory as writable. Although the module memory is
+	 * about to be freed, these calls are required (at least on x86) to
+	 * restore the direct map to its "correct" state.
+	 */
+	frob_text(layout, set_memory_rw);
+	frob_rodata(layout, set_memory_rw);
+	frob_ro_after_init(layout, set_memory_rw);
 }
 
 #else
-static void disable_ro_nx(const struct module_layout *layout) { }
+static void module_restore_mappings(const struct module_layout *layout) { }
 static void module_enable_nx(const struct module *mod) { }
 static void module_disable_nx(const struct module *mod) { }
 #endif
@@ -2173,7 +2182,7 @@ static void free_module(struct module *mod)
 	mutex_unlock(&module_mutex);
 
 	/* This may be empty, but that's OK */
-	disable_ro_nx(&mod->init_layout);
+	module_restore_mappings(&mod->init_layout);
 	module_arch_freeing_init(mod);
 	module_memfree(mod->init_layout.base);
 	kfree(mod->args);
@@ -2183,7 +2192,7 @@ static void free_module(struct module *mod)
 	lockdep_free_key_range(mod->core_layout.base, mod->core_layout.size);
 
 	/* Finally, free the core (containing the module structure) */
-	disable_ro_nx(&mod->core_layout);
+	module_restore_mappings(&mod->core_layout);
 	module_memfree(mod->core_layout.base);
 }
 
@@ -3507,7 +3516,7 @@ static noinline int do_init_module(struct module *mod)
 #endif
 	module_enable_ro(mod, true);
 	mod_tree_remove_init(mod);
-	disable_ro_nx(&mod->init_layout);
+	module_restore_mappings(&mod->init_layout);
 	module_arch_freeing_init(mod);
 	mod->init_layout.base = NULL;
 	mod->init_layout.size = 0;
-- 
2.17.1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help