linux-kernel - Re: [PATCH v4] 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 for Android: free password hash cracker in your pocket

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

Message-Id: <200907112119.46603.rusty@rustcorp.com.au>
Date:	Sat, 11 Jul 2009 21:19:45 +0930
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Siarhei Liakh <sliakh.lkml@...il.com>
Cc:	Arjan van de Ven <arjan@...radead.org>,
	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.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>, Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH v4] RO/NX protection for loadable kernel modules

On Thu, 9 Jul 2009 08:01:59 am Siarhei Liakh wrote:
> [ Arjen wrote: ]
> > I *think* this can be done as
> >
> > begin_pfn = PFN_UP( (base);
> > end_pfn = PFN_DOWN(base + ro_size);
> >
> > if (end_pfn > begin_pfn)
> >        set_memory_ro(begin_pfn >> PAGE_SHIFT, end_pfn - begin_pfn);
> >
> > (note that I think the +1 you have might be buggy)
>
> The +1 is necessary because of (end_addr - 1). However, I like your
> way better, so I will incorporate it in V5.

In fact, this wasn't quite what you did.  You did begin_pfn = PFN_DOWN(),
end_pfn = PFN_DOWN(), then later use PFN_UP on both of them.

Is this what you intended?

(Note: I removed some DEBUGPs, where action is implied anyway).

diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1517,55 +1517,41 @@ static void set_section_ro_nx(void *base
 	unsigned long begin_pfn;
 	unsigned long end_pfn;
 
+#ifdef CONFIG_DEBUG_RODATA
+	/* Most module_alloc use vmalloc which page-aligns.  If not,
+	 * we could be missing protection on first part of module. */
+	WARN_ON(offset_in_page((unsigned long)base));
+#endif
+
 	/* 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);
+	/* Set RO for module text and RO-data */
+	/* Don't protect partial pages. */
+	begin_pfn = PFN_UP((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");
+	/* 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);
 	}
 
 	/* 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);
+	/* Don't protect partial pages. */
+	begin_pfn = PFN_UP((unsigned long)base + text_size);
+	end_pfn = PFN_DOWN((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");
+	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);
 	}
 }
 

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