Re: [PATCH v5] RO/NX protection for loadable kernel modules

[Rusty Russell <rusty@rustcorp.com.au>]

On Sat, 11 Jul 2009 03:37:01 pm Rusty Russell wrote:
> On Fri, 10 Jul 2009 08:54:03 pm Ingo Molnar wrote:
> > please use the customary comment style:
>
> Please don't.  There's one spot in that file which does it, and frankly
> it's a distracting waste of space.

Sorry, Ingo there's more than one.  I don't bother fixing up others' comments
in patches, so the file is now a bit mixed.  It'd be better to make it
uniform, in which case I'd change it to kernel-style.

I have a question about this patch though: I think it's unsafe in general to
mark the last partial page as NX (we asked for executable pages, this could
remove executable from some unrelated allocation).

And here's the cleanup patch I applied on top of yours; mainly removing
pointless zero checks (ignore the whitespace cleanups).

Thanks,
Rusty.

diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1518,55 +1518,30 @@ static void set_section_ro_nx(void *base
 	unsigned long end_pfn;
 
 	/* Initially, all module sections have RWX permissions*/
-
 	DEBUGP("PROTECTING MODULE SECTION: 0x%lx\n"
 			"  text size: %lu\n"
 			"  ro size: %lu\n"
 			"  total size: %lu\n",
 			(unsigned long)base,
-			text_size, ro_size, total_size);
+	       text_size, ro_size, total_size);
 
 	/* Set RO for module text and RO-data*/
-	if (ro_size > 0) {
-		/* Always protect first page.
-		 * Do not protect last partial page */
-		begin_pfn = PFN_DOWN((unsigned long)base);
-		end_pfn = PFN_DOWN((unsigned long)base + ro_size);
+	/* Always protect first page.  Do not protect last partial page */
+	begin_pfn = PFN_DOWN((unsigned long)base);
+	end_pfn = PFN_DOWN((unsigned long)base + ro_size);
 
-		/*Set text RO if there are still pages between begin and end*/
-		if (end_pfn > begin_pfn) {
-			DEBUGP("  RO: 0x%lx %lu\n",
-					begin_pfn << PAGE_SHIFT,
-					end_pfn - begin_pfn);
-			set_memory_ro(begin_pfn << PAGE_SHIFT,
-						end_pfn - begin_pfn);
-		} else {
-			DEBUGP("  RO: less than a page, not enforcing.\n");
-		}
-	} else {
-		DEBUGP("  RO: section not present.\n");
-	}
+	DEBUGP("  RO: 0x%lx %lu\n",
+	       begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
+	set_memory_ro(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
 
 	/* Set NX permissions for module data */
-	if (total_size > text_size) {
-		/* Do not protect first partial page
-		 * Always protect last page. */
-		begin_pfn = PFN_UP((unsigned long)base + text_size);
-		end_pfn = PFN_UP((unsigned long)base + total_size);
+	/* Do not protect first partial page.  Always protect last page. */
+	begin_pfn = PFN_UP((unsigned long)base + text_size);
+	end_pfn = PFN_UP((unsigned long)base + total_size);
 
-		/*Set data NX if there are still pages between begin and end*/
-		if (end_pfn > begin_pfn) {
-			DEBUGP("  NX: 0x%lx %lu\n",
-					begin_pfn << PAGE_SHIFT,
-					end_pfn - begin_pfn);
-			set_memory_nx(begin_pfn << PAGE_SHIFT,
-						end_pfn - begin_pfn);
-		} else {
-			DEBUGP("  NX: less than a page, not enforcing.\n");
-		}
-	} else {
-		DEBUGP("  NX: section not present.\n");
-	}
+	DEBUGP("  NX: 0x%lx %lu\n",
+	       begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
+	set_memory_nx(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
 }
 
 /* Setting memory back to RW+NX before releasing it */