Skip to contentSkip to conversation

Undoing module RONX protection

PATCH Applied Jan Glauber 3 participants 12 messages Apr 18, 2011 – Apr 30, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

While debugging I stumbled over two problems in the code that protects module pages.

First issue is that disabling the protection before freeing init or unload of a module is not symmetric with the enablement. For instance, if pages are set to RO the page range from module_core to module_core + core_ro_size is protected. If a module is unloaded the page range from module_core to module_core + core_size is set back to RW.
So pages that were not set to RO are also changed to RW.
This is not critical but IMHO it should be symmetric.

Second issue is that while set_memory_rw & set_memory_ro are used for RO/RW changes only set_memory_nx is involved for NX/X. One would await that the inverse function is called when the NX protection should be removed, which is not the case here, unless I'm missing something.

The following patch addresses both issues. Works on s390. Boot tested on x86.

Please comment,
Jan

---------

Use the proper function to reset the NX page protection and only reset pages that were previously protected during module load. Export the missing set_memory_x for s390.

---
 arch/s390/include/asm/cacheflush.h |    1 +
 arch/s390/mm/pageattr.c            |    6 ++++++
 kernel/module.c                    |   25 +++++++++++++------------
 3 files changed, 20 insertions(+), 12 deletions(-)
arch/s390/include/asm/cacheflush.h +1 -0
@@ -11,5 +11,6 @@ void kernel_map_pages(struct page *page,
 int set_memory_ro(unsigned long addr, int numpages);
 int set_memory_rw(unsigned long addr, int numpages);
 int set_memory_nx(unsigned long addr, int numpages);
+int set_memory_x(unsigned long addr, int numpages);
 
 #endif /* _S390_CACHEFLUSH_H */
arch/s390/mm/pageattr.c +6 -0
@@ -54,3 +54,9 @@ int set_memory_nx(unsigned long addr, in
 	return 0;
 }
 EXPORT_SYMBOL_GPL(set_memory_nx);
+
+int set_memory_x(unsigned long addr, int numpages)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(set_memory_x);
kernel/module.c +13 -12
@@ -1607,22 +1607,23 @@ static void set_section_ro_nx(void *base
 	}
 }
 
-/* Setting memory back to RW+NX before releasing it */
+/* Setting memory back to W+X before releasing it */
 void unset_section_ro_nx(struct module *mod, void *module_region)
 {
-	unsigned long total_pages;
-
 	if (mod->module_core == module_region) {
-		/* Set core as NX+RW */
-		total_pages = MOD_NUMBER_OF_PAGES(mod->module_core, mod->core_size);
-		set_memory_nx((unsigned long)mod->module_core, total_pages);
-		set_memory_rw((unsigned long)mod->module_core, total_pages);
-
+		set_page_attributes(mod->module_core + mod->core_text_size,
+			mod->module_core + mod->core_size,
+			set_memory_x);
+		set_page_attributes(mod->module_core,
+			mod->module_core + mod->core_ro_size,
+			set_memory_rw);
 	} else if (mod->module_init == module_region) {
-		/* Set init as NX+RW */
-		total_pages = MOD_NUMBER_OF_PAGES(mod->module_init, mod->init_size);
-		set_memory_nx((unsigned long)mod->module_init, total_pages);
-		set_memory_rw((unsigned long)mod->module_init, total_pages);
+		set_page_attributes(mod->module_init + mod->init_text_size,
+			mod->module_init + mod->init_size,
+			set_memory_x);
+		set_page_attributes(mod->module_init,
+			mod->module_init + mod->init_ro_size,
+			set_memory_rw);
 	}
 }
 
#1 lore raw

arch/s390/mm/pageattr.c int set_memory_nx(unsigned long addr, in · line 62

Quoted 5 lines
arch/s390/mm/pageattr.c +5 -0
+int set_memory_x(unsigned long addr, int numpages)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(set_memory_x);

No need to export this one.

#2 lore raw
Quoted 7 lines · Christoph Hellwig

arch/s390/mm/pageattr.c int set_memory_nx(unsigned long addr, in · line 62

Quoted 5 lines
arch/s390/mm/pageattr.c +5 -0
+int set_memory_x(unsigned long addr, int numpages)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(set_memory_x);

No need to export this one.

True. Also for the other exports in that file. Will remove them.

--Jan

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#3 lore raw
In reply to Christoph Hellwig
Quoted 11 lines · Jan Glauber

While debugging I stumbled over two problems in the code that protects module pages.

First issue is that disabling the protection before freeing init or unload of a module is not symmetric with the enablement. For instance, if pages are set to RO the page range from module_core to module_core + core_ro_size is protected. If a module is unloaded the page range from module_core to module_core + core_size is set back to RW.
So pages that were not set to RO are also changed to RW.
This is not critical but IMHO it should be symmetric.

Quoted 8 lines

Second issue is that while set_memory_rw & set_memory_ro are used for RO/RW changes only set_memory_nx is involved for NX/X. One would await that the inverse function is called when the NX protection should be removed, which is not the case here, unless I'm missing something.

The following patch addresses both issues. Works on s390. Boot tested on x86.

Please comment,

Applied, minus the S/390 EXPORT_SYMBOL which Christoph pointed out. I turned your mail into the commit message, since it was clearer and more verbose. I don't see why they would be different.

Might have been nicer as three separate patches, in future.

Thanks!
Rusty.

#4 lore raw
Quoted 17 lines · Rusty Russell
Quoted 16 lines · Jan Glauber

While debugging I stumbled over two problems in the code that protects module pages.

First issue is that disabling the protection before freeing init or unload of a module is not symmetric with the enablement. For instance, if pages are set to RO the page range from module_core to module_core + core_ro_size is protected. If a module is unloaded the page range from module_core to module_core + core_size is set back to RW.
So pages that were not set to RO are also changed to RW.
This is not critical but IMHO it should be symmetric.

Second issue is that while set_memory_rw & set_memory_ro are used for RO/RW changes only set_memory_nx is involved for NX/X. One would await that the inverse function is called when the NX protection should be removed, which is not the case here, unless I'm missing something.

Quoted 7 lines
Quoted 3 lines

The following patch addresses both issues. Works on s390. Boot tested on x86.

Please comment,

Applied, minus the S/390 EXPORT_SYMBOL which Christoph pointed out. I turned your mail into the commit message, since it was clearer and more verbose. I don't see why they would be different.

There's a bug in my patch which just killed one of my s390 machines. Can you merge this with the previuos patch?

thanks, Jan
---

From: Jan Glauber <[email protected]>

A module may not have an init part so set_page_attributes may try to change an invalid page range. Modifying page 0 is fatal on s390. Bail out in case the module_core or module_init is not present.

---
 kernel/module.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/module.c +2 -2
@@ -1610,14 +1610,14 @@ static void set_section_ro_nx(void *base
 /* Setting memory back to W+X before releasing it */
 void unset_section_ro_nx(struct module *mod, void *module_region)
 {
-	if (mod->module_core == module_region) {
+	if (mod->module_core && mod->module_core == module_region) {
 		set_page_attributes(mod->module_core + mod->core_text_size,
 			mod->module_core + mod->core_size,
 			set_memory_x);
 		set_page_attributes(mod->module_core,
 			mod->module_core + mod->core_ro_size,
 			set_memory_rw);
-	} else if (mod->module_init == module_region) {
+	} else if (mod->module_init && mod->module_init == module_region) {
 		set_page_attributes(mod->module_init + mod->init_text_size,
 			mod->module_init + mod->init_size,
 			set_memory_x);
#5 lore raw

Re: Undoing module RONX protection fix

Quoted 20 lines · Jan Glauber
Quoted 19 lines · Rusty Russell
Quoted 18 lines · Jan Glauber

While debugging I stumbled over two problems in the code that protects module pages.

First issue is that disabling the protection before freeing init or unload of a module is not symmetric with the enablement. For instance, if pages are set to RO the page range from module_core to module_core + core_ro_size is protected. If a module is unloaded the page range from module_core to module_core + core_size is set back to RW.
So pages that were not set to RO are also changed to RW.
This is not critical but IMHO it should be symmetric.

Second issue is that while set_memory_rw & set_memory_ro are used for RO/RW changes only set_memory_nx is involved for NX/X. One would await that the inverse function is called when the NX protection should be removed, which is not the case here, unless I'm missing something.

The following patch addresses both issues. Works on s390. Boot tested on x86.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Please comment,

Applied, minus the S/390 EXPORT_SYMBOL which Christoph pointed out. I turned your mail into the commit message, since it was clearer and more verbose. I don't see why they would be different.

There's a bug in my patch which just killed one of my s390 machines. Can you merge this with the previuos patch?

Hmm...

Applied, but that function is really kind of silly. We should probably just split into unset_section_ro_nx() into unset_module_init_ro_nx() and unset_module_core_ro_nx().

(And why isn't that function static anyway?)

Patch appreciated :)
Rusty.

#6 lore raw

Re: Undoing module RONX protection fix

Quoted 32 lines · Rusty Russell
Quoted 20 lines · Jan Glauber
Quoted 19 lines · Rusty Russell
Quoted 18 lines · Jan Glauber

While debugging I stumbled over two problems in the code that protects module pages.

First issue is that disabling the protection before freeing init or unload of a module is not symmetric with the enablement. For instance, if pages are set to RO the page range from module_core to module_core + core_ro_size is protected. If a module is unloaded the page range from module_core to module_core + core_size is set back to RW.
So pages that were not set to RO are also changed to RW.
This is not critical but IMHO it should be symmetric.

Second issue is that while set_memory_rw & set_memory_ro are used for RO/RW changes only set_memory_nx is involved for NX/X. One would await that the inverse function is called when the NX protection should be removed, which is not the case here, unless I'm missing something.

The following patch addresses both issues. Works on s390. Boot tested on x86.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Please comment,

Applied, minus the S/390 EXPORT_SYMBOL which Christoph pointed out. I turned your mail into the commit message, since it was clearer and more verbose. I don't see why they would be different.

There's a bug in my patch which just killed one of my s390 machines. Can you merge this with the previuos patch?

Hmm...

Quoted 8 lines

Applied, but that function is really kind of silly. We should probably just split into unset_section_ro_nx() into unset_module_init_ro_nx() and unset_module_core_ro_nx().

(And why isn't that function static anyway?)

Patch appreciated :)
Rusty.

How about this?

To be honest I don't like the inverse naming like in unset no-execute too much, it makes me feel dizzy. But I wanted to keep the changes minimal.

Jan
------

Split the unprotect function into a function per section to make the code more readable and add the missing static declaration.

---
 kernel/module.c |   47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)
kernel/module.c +26 -21
@@ -1607,24 +1607,28 @@ static void set_section_ro_nx(void *base
 	}
 }
 
-/* Setting memory back to W+X before releasing it */
-void unset_section_ro_nx(struct module *mod, void *module_region)
+static void unset_module_core_ro_nx(struct module *mod)
 {
-	if (mod->module_core && mod->module_core == module_region) {
-		set_page_attributes(mod->module_core + mod->core_text_size,
-			mod->module_core + mod->core_size,
-			set_memory_x);
-		set_page_attributes(mod->module_core,
-			mod->module_core + mod->core_ro_size,
-			set_memory_rw);
-	} else if (mod->module_init && mod->module_init == module_region) {
-		set_page_attributes(mod->module_init + mod->init_text_size,
-			mod->module_init + mod->init_size,
-			set_memory_x);
-		set_page_attributes(mod->module_init,
-			mod->module_init + mod->init_ro_size,
-			set_memory_rw);
-	}
+	if (mod->module_core == NULL)
+		return;
+	set_page_attributes(mod->module_core + mod->core_text_size,
+		mod->module_core + mod->core_size,
+		set_memory_x);
+	set_page_attributes(mod->module_core,
+		mod->module_core + mod->core_ro_size,
+		set_memory_rw);
+}
+
+static void unset_module_init_ro_nx(struct module *mod)
+{
+	if (mod->module_init == NULL)
+		return;
+	set_page_attributes(mod->module_init + mod->init_text_size,
+		mod->module_init + mod->init_size,
+		set_memory_x);
+	set_page_attributes(mod->module_init,
+		mod->module_init + mod->init_ro_size,
+		set_memory_rw);
 }
 
 /* Iterate through all modules and set each module's text as RW */
@@ -1670,7 +1674,8 @@ void set_all_modules_text_ro()
 }
 #else
 static inline void set_section_ro_nx(void *base, unsigned long text_size, unsigned long ro_size, unsigned long total_size) { }
-static inline void unset_section_ro_nx(struct module *mod, void *module_region) { }
+static void unset_module_core_ro_nx(struct module *mod) { }
+static void unset_module_init_ro_nx(struct module *mod) { }
 #endif
 
 /* Free a module, remove from lists, etc. */
@@ -1697,7 +1702,7 @@ static void free_module(struct module *m
 	destroy_params(mod->kp, mod->num_kp);
 
 	/* This may be NULL, but that's OK */
-	unset_section_ro_nx(mod, mod->module_init);
+	unset_module_init_ro_nx(mod);
 	module_free(mod, mod->module_init);
 	kfree(mod->args);
 	percpu_modfree(mod);
@@ -1706,7 +1711,7 @@ static void free_module(struct module *m
 	lockdep_free_key_range(mod->module_core, mod->core_size);
 
 	/* Finally, free the core (containing the module structure) */
-	unset_section_ro_nx(mod, mod->module_core);
+	unset_module_core_ro_nx(mod);
 	module_free(mod, mod->module_core);
 
 #ifdef CONFIG_MPU
@@ -2932,7 +2937,7 @@ SYSCALL_DEFINE3(init_module, void __user
 	mod->symtab = mod->core_symtab;
 	mod->strtab = mod->core_strtab;
 #endif
-	unset_section_ro_nx(mod, mod->module_init);
+	unset_module_init_ro_nx(mod);
 	module_free(mod, mod->module_init);
 	mod->module_init = NULL;
 	mod->init_size = 0;
#7 lore raw

Re: Undoing module RONX protection fix

Quoted 5 lines · Jan Glauber

How about this?

To be honest I don't like the inverse naming like in unset no-execute too much, it makes me feel dizzy. But I wanted to keep the changes minimal.

Yes, it should probably just be called protect_module_pages and unprotect_module_pages. The current names provide far too much information.

But going back a bit, how did we end up with a NULL mod->module_init and yet module->init_text_size, mod->init_size or mod->init_ro_size non-zero?

Because if start == end, set_page_attributes() is a noop, right?

Confused,
Rusty.

#8 lore raw

Re: Undoing module RONX protection fix

Quoted 7 lines · Rusty Russell
Quoted 5 lines · Jan Glauber

How about this?

To be honest I don't like the inverse naming like in unset no-execute too much, it makes me feel dizzy. But I wanted to keep the changes minimal.

Quoted 7 lines

Yes, it should probably just be called protect_module_pages and unprotect_module_pages. The current names provide far too much information.

But going back a bit, how did we end up with a NULL mod->module_init and yet module->init_text_size, mod->init_size or mod->init_ro_size non-zero?

printk'ing this reveals that mod->init_ro_size is not 0 but 0x1000. Therefore the first page was modified.

Looks like init_ro_size is missing the reset to zero at the end of the init_module syscall. Next patch ? ;-

Quoted 1 line

Because if start == end, set_page_attributes() is a noop, right?

Right.

Quoted 8 lines

Confused,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#9 lore raw

Re: Undoing module RONX protection fix

Quoted 16 lines · Jan Glauber
Quoted 7 lines · Rusty Russell
Quoted 5 lines · Jan Glauber

How about this?

To be honest I don't like the inverse naming like in unset no-execute too much, it makes me feel dizzy. But I wanted to keep the changes minimal.

Quoted 7 lines

Yes, it should probably just be called protect_module_pages and unprotect_module_pages. The current names provide far too much information.

But going back a bit, how did we end up with a NULL mod->module_init and yet module->init_text_size, mod->init_size or mod->init_ro_size non-zero?

Quoted 5 lines

printk'ing this reveals that mod->init_ro_size is not 0 but 0x1000. Therefore the first page was modified.

Looks like init_ro_size is missing the reset to zero at the end of the init_module syscall. Next patch ? ;-

Yes, that seems like a much cleaner and clearer fix to me...

Thanks,
Rusty.

#10 lore raw

Re: Undoing module RONX protection fix

Quoted 17 lines · Rusty Russell
Quoted 16 lines · Jan Glauber
Quoted 14 lines · Rusty Russell
Quoted 5 lines · Jan Glauber

How about this?

To be honest I don't like the inverse naming like in unset no-execute too much, it makes me feel dizzy. But I wanted to keep the changes minimal.

Yes, it should probably just be called protect_module_pages and unprotect_module_pages. The current names provide far too much information.

But going back a bit, how did we end up with a NULL mod->module_init and yet module->init_text_size, mod->init_size or mod->init_ro_size non-zero?

Quoted 7 lines
Quoted 5 lines

printk'ing this reveals that mod->init_ro_size is not 0 but 0x1000. Therefore the first page was modified.

Looks like init_ro_size is missing the reset to zero at the end of the init_module syscall. Next patch ? ;-

Yes, that seems like a much cleaner and clearer fix to me...

Rusty, I'm not sure if I should resend a merged patch or not, going for the later so you can apply on top of what you might have. If you would appreciate a merged patch more please let me know...

Cheers,
Jan
------

Reset mod->init_ro_size to zero after the init part of a module is unloaded. That makes the check if a module part exists in the unprotect functions superfluous so they can be removed.

---
 kernel/module.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
kernel/module.c +1 -4
@@ -1609,8 +1609,6 @@ static void set_section_ro_nx(void *base
 
 static void unset_module_core_ro_nx(struct module *mod)
 {
-	if (mod->module_core == NULL)
-		return;
 	set_page_attributes(mod->module_core + mod->core_text_size,
 		mod->module_core + mod->core_size,
 		set_memory_x);
@@ -1621,8 +1619,6 @@ static void unset_module_core_ro_nx(stru
 
 static void unset_module_init_ro_nx(struct module *mod)
 {
-	if (mod->module_init == NULL)
-		return;
 	set_page_attributes(mod->module_init + mod->init_text_size,
 		mod->module_init + mod->init_size,
 		set_memory_x);
@@ -2941,6 +2937,7 @@ SYSCALL_DEFINE3(init_module, void __user
 	module_free(mod, mod->module_init);
 	mod->module_init = NULL;
 	mod->init_size = 0;
+	mod->init_ro_size = 0;
 	mod->init_text_size = 0;
 	mutex_unlock(&module_mutex);
 
#11 lore raw

Re: Undoing module RONX protection fix

Quoted 3 lines · Jan Glauber

Rusty, I'm not sure if I should resend a merged patch or not, going for the later so you can apply on top of what you might have. If you would appreciate a merged patch more please let me know...

Yeah, it was a bit messy. I applied the third chunk of this, shuffled it to the top, threw away the initial fix and fixed up the successors (I use patch queues, so editing the diffs for this stuff makes it pretty easy).

I've appended all three patches that resulted below (there's nothing new, just so you can see the final presentation).

Thanks!
Rusty.
PS. Don't let that stop you from doing more cleanups as discussed though :)

From: Jan Glauber <[email protected]>
Subject: module: zero mod->init_ro_size after init is freed.

Reset mod->init_ro_size to zero after the init part of a module is unloaded. Otherwise we need to check if module->init is NULL in the unprotect functions in the next patch.

---
 kernel/module.c |    1 +
 1 file changed, 1 insertion(+)
kernel/module.c +1 -0
@@ -2941,6 +2937,7 @@ SYSCALL_DEFINE3(init_module, void __user
 	module_free(mod, mod->module_init);
 	mod->module_init = NULL;
 	mod->init_size = 0;
+	mod->init_ro_size = 0;
 	mod->init_text_size = 0;
 	mutex_unlock(&module_mutex);
 

Subject: module: undo module RONX protection correctly.
From: Jan Glauber <[email protected]>

While debugging I stumbled over two problems in the code that protects module pages.

First issue is that disabling the protection before freeing init or unload of a module is not symmetric with the enablement. For instance, if pages are set to RO the page range from module_core to module_core + core_ro_size is protected. If a module is unloaded the page range from module_core to module_core + core_size is set back to RW.
So pages that were not set to RO are also changed to RW.
This is not critical but IMHO it should be symmetric.

Second issue is that while set_memory_rw & set_memory_ro are used for RO/RW changes only set_memory_nx is involved for NX/X. One would await that the inverse function is called when the NX protection should be removed, which is not the case here, unless I'm missing something.

---
 arch/s390/include/asm/cacheflush.h |    1 +
 arch/s390/mm/pageattr.c            |    5 +++++
 kernel/module.c                    |   25 +++++++++++++------------
 3 files changed, 19 insertions(+), 12 deletions(-)
arch/s390/include/asm/cacheflush.h +1 -0
@@ -11,5 +11,6 @@ void kernel_map_pages(struct page *page,
 int set_memory_ro(unsigned long addr, int numpages);
 int set_memory_rw(unsigned long addr, int numpages);
 int set_memory_nx(unsigned long addr, int numpages);
+int set_memory_x(unsigned long addr, int numpages);
 
 #endif /* _S390_CACHEFLUSH_H */
arch/s390/mm/pageattr.c +5 -0
@@ -54,3 +54,8 @@ int set_memory_nx(unsigned long addr, in
 	return 0;
 }
 EXPORT_SYMBOL_GPL(set_memory_nx);
+
+int set_memory_x(unsigned long addr, int numpages)
+{
+	return 0;
+}
kernel/module.c +13 -12
@@ -1607,22 +1607,23 @@ static void set_section_ro_nx(void *base
 	}
 }
 
-/* Setting memory back to RW+NX before releasing it */
+/* Setting memory back to W+X before releasing it */
 void unset_section_ro_nx(struct module *mod, void *module_region)
 {
-	unsigned long total_pages;
-
 	if (mod->module_core == module_region) {
-		/* Set core as NX+RW */
-		total_pages = MOD_NUMBER_OF_PAGES(mod->module_core, mod->core_size);
-		set_memory_nx((unsigned long)mod->module_core, total_pages);
-		set_memory_rw((unsigned long)mod->module_core, total_pages);
-
+		set_page_attributes(mod->module_core + mod->core_text_size,
+			mod->module_core + mod->core_size,
+			set_memory_x);
+		set_page_attributes(mod->module_core,
+			mod->module_core + mod->core_ro_size,
+			set_memory_rw);
 	} else if (mod->module_init == module_region) {
-		/* Set init as NX+RW */
-		total_pages = MOD_NUMBER_OF_PAGES(mod->module_init, mod->init_size);
-		set_memory_nx((unsigned long)mod->module_init, total_pages);
-		set_memory_rw((unsigned long)mod->module_init, total_pages);
+		set_page_attributes(mod->module_init + mod->init_text_size,
+			mod->module_init + mod->init_size,
+			set_memory_x);
+		set_page_attributes(mod->module_init,
+			mod->module_init + mod->init_ro_size,
+			set_memory_rw);
 	}
 }
 

Subject: module: split unset_section_ro_nx function.
From: Jan Glauber <[email protected]>

Split the unprotect function into a function per section to make the code more readable and add the missing static declaration.

---
 kernel/module.c |   43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)
kernel/module.c +22 -21

74 lines not on this page · raw message