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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <200907111821.47769.rusty@rustcorp.com.au>
Date:	Sat, 11 Jul 2009 18:21:46 +0930
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Siarhei Liakh <sliakh.lkml@...il.com>,
	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	Arjan van de Ven <arjan@...radead.org>,
	James Morris <jmorris@...ei.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <ak@....de>, Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v5] RO/NX protection for loadable kernel modules

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 */



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