[PATCH] RO/NX protection for loadable kernel modules

[PATCH] RO/NX protection for loadable kernel modules

[Siarhei Liakh]

This patch is a logical extension of the protection provided by
CONFIG_DEBUG_RODATA to LKMs. The protection is provided by splitting
module_core and module_init into three logical parts each and setting
appropriate page access permissions for each individual section:

   1. Code: RO+X
   2. RO data: RO+NX
   3. RW data: RW+NX

In order to achieve proper protection, layout_sections() have been
modified to align each of the three parts mentioned above onto page
boundary. Next, the corresponding page access permissions are set
right before successful exit from load_module(). Further,
module_free() have been modified to set module_core or module_init as
RW+NX right before calling vfree(). Functionality of this patch is
enabled only when CONFIG_DEBUG_RODATA defined at compile time.

The patch have been developed for Linux 2.6.30 i386.

If there are any issues with the patch formatting and/or kernel
version, please let me know and I will gladly correct the issues.
Thank you.

Signed-off-by: Siarhei Liakh <sliakh.lkml@gmail.com>
---

diff --git a/arch/x86/kernel/module_32.c b/arch/x86/kernel/module_32.c
index 0edd819..8a72ef8 100644
--- a/arch/x86/kernel/module_32.c
+++ b/arch/x86/kernel/module_32.c
@@ -22,6 +22,7 @@
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/bug.h>
+#include <asm/cacheflush.h>

 #if 0
 #define DEBUGP printk
@@ -29,6 +30,14 @@
 #define DEBUGP(fmt...)
 #endif

+#ifdef CONFIG_DEBUG_RODATA
+/* Given BASE and SIZE this macro calculates the number of pages this
+ * memory regions occupies */
+#define NUMBER_OF_PAGES(BASE, SIZE) ( \
+	( ( (unsigned long) BASE + (unsigned long) SIZE ) >> PAGE_SHIFT ) - \
+	( ( (unsigned long) BASE ) >> PAGE_SHIFT) + ( (SIZE>0) ? 1:0 ) )
+#endif
+
 void *module_alloc(unsigned long size)
 {
 	if (size == 0)
@@ -40,6 +49,26 @@ void *module_alloc(unsigned long size)
 /* Free memory returned from module_alloc */
 void module_free(struct module *mod, void *module_region)
 {
+#ifdef CONFIG_DEBUG_RODATA
+	unsigned long total_pages;
+
+	if(mod->module_core == module_region){
+		/* Set core as NX+RW */
+		total_pages = NUMBER_OF_PAGES( mod->module_core, mod->core_size );
+		DEBUGP(KERN_WARNING "RELEASING MODULE CORE: 0x%lx %lu\n",
+				(unsigned long)mod->module_core, total_pages);
+		set_memory_nx( (unsigned long)mod->module_core, total_pages);
+		set_memory_rw( (unsigned long)mod->module_core, total_pages);
+
+	}else if(mod->module_init == module_region){
+		/* Set init as NX+RW */
+		total_pages = NUMBER_OF_PAGES( mod->module_init, mod->init_size );
+		DEBUGP(KERN_WARNING "RELEASING MODULE INIT: 0x%lx %lu\n",
+				(unsigned long)mod->module_init, total_pages);
+		set_memory_nx( (unsigned long)mod->module_init, total_pages);
+		set_memory_rw( (unsigned long)mod->module_init, total_pages);
+	}
+#endif
 	vfree(module_region);
 	/* FIXME: If module_region == mod->init_region, trim exception
 	   table entries. */
diff --git a/include/linux/module.h b/include/linux/module.h
index 627ac08..75e7428 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -293,6 +293,11 @@ struct module
 	/* The size of the executable code in each section.  */
 	unsigned int init_text_size, core_text_size;

+#ifdef CONFIG_DEBUG_RODATA
+	/* Size of RO sections of the module (text+rodata) */
+	unsigned int init_ro_size, core_ro_size;
+#endif
+
 	/* Arch-specific module values */
 	struct mod_arch_specific arch;

diff --git a/kernel/module.c b/kernel/module.c
index e797812..4afcb06 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -63,6 +63,14 @@
 #define ARCH_SHF_SMALL 0
 #endif

+#ifdef CONFIG_DEBUG_RODATA
+/* Given BASE and SIZE this macro calculates the number of pages this
+ * memory regions occupies */
+#define NUMBER_OF_PAGES(BASE, SIZE) ( \
+	( ( (unsigned long) BASE + (unsigned long) SIZE ) >> PAGE_SHIFT ) - \
+	( ( (unsigned long) BASE ) >> PAGE_SHIFT) + ( (SIZE>0) ? 1:0 ) )
+#endif
+
 /* If this is set, the section belongs in the init part of the module */
 #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))

@@ -1661,6 +1669,10 @@ static void layout_sections(struct module *mod,
 		{ ARCH_SHF_SMALL | SHF_ALLOC, 0 }
 	};
 	unsigned int m, i;
+#ifdef CONFIG_DEBUG_RODATA
+	/* always align first section on page boundary */
+	int need_alignment = 1;
+#endif

 	for (i = 0; i < hdr->e_shnum; i++)
 		sechdrs[i].sh_entsize = ~0UL;
@@ -1675,13 +1687,39 @@ static void layout_sections(struct module *mod,
 			    || s->sh_entsize != ~0UL
 			    || strstarts(secstrings + s->sh_name, ".init"))
 				continue;
+#ifdef CONFIG_DEBUG_RODATA
+			if(need_alignment){
+				mod->core_size = ALIGN(mod->core_size, PAGE_SIZE);
+				need_alignment = 0;
+			}
+#endif
 			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
-		if (m == 0)
+
+		if (m == 0) {
+			/* module code size */
 			mod->core_text_size = mod->core_size;
+#ifdef CONFIG_DEBUG_RODATA
+			/* next section should start on new page */
+			need_alignment = 1;
+#endif
+		}
+
+#ifdef CONFIG_DEBUG_RODATA
+		if (m == 1) {
+			/* module RO size (text+rodata) */
+			mod->core_ro_size = mod->core_size;
+			/* next section should start on new page */
+			need_alignment = 1;
+		}
+#endif
 	}

+#ifdef CONFIG_DEBUG_RODATA
+	/* always align first section on page boundary */
+	need_alignment = 1;
+#endif
 	DEBUGP("Init section allocation order:\n");
 	for (m = 0; m < ARRAY_SIZE(masks); ++m) {
 		for (i = 0; i < hdr->e_shnum; ++i) {
@@ -1692,13 +1730,50 @@ static void layout_sections(struct module *mod,
 			    || s->sh_entsize != ~0UL
 			    || !strstarts(secstrings + s->sh_name, ".init"))
 				continue;
+#ifdef CONFIG_DEBUG_RODATA
+			if(need_alignment){
+				mod->init_size = ALIGN(mod->init_size, PAGE_SIZE);
+				need_alignment = 0;
+			}
+#endif			
+
 			s->sh_entsize = (get_offset(mod, &mod->init_size, s, i)
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", secstrings + s->sh_name);
 		}
-		if (m == 0)
+
+		if (m == 0) {
+			/* module code size */
 			mod->init_text_size = mod->init_size;
+#ifdef CONFIG_DEBUG_RODATA
+			/* next section should start on new page */
+			need_alignment = 1;
+#endif
+		}
+
+#ifdef CONFIG_DEBUG_RODATA
+		if (m == 1) {
+			// module RO size (text+rodata)
+			mod->init_ro_size = mod->init_size;
+			// next section should start on new page
+			need_alignment = 1;
+		}
+#endif
 	}
+#ifdef CONFIG_DEBUG_RODATA
+	DEBUGP( "core_text_size: %i\n"
+		"core_ro_size: %i\n"
+		"core_size: %i\n"
+		"init_text_size: %i\n"
+		"init_ro_size: %i\n"
+		"init_size: %i\n",
+		mod->core_text_size,
+		mod->core_ro_size,
+		mod->core_size,
+		mod->init_text_size,
+		mod->init_ro_size,
+		mod->init_size);
+#endif
 }

 static void set_license(struct module *mod, const char *license)
@@ -1898,6 +1973,12 @@ static noinline struct module *load_module(void
__user *umod,
 	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
 	unsigned long *mseg;
 	mm_segment_t old_fs;
+#ifdef CONFIG_DEBUG_RODATA
+	/* these variables are used to set RO and NX regions */
+	unsigned long total_pages;
+	unsigned long text_pages;
+	unsigned long ro_pages;
+#endif

 	DEBUGP("load_module: umod=%p, len=%lu, uargs=%p\n",
 	       umod, len, uargs);
@@ -2291,6 +2372,73 @@ static noinline struct module *load_module(void
__user *umod,
 	/* Get rid of temporary copy */
 	vfree(hdr);

+#ifdef CONFIG_DEBUG_RODATA
+	/* In this section we calculate the number of pages for
+	 * each part of core and init: code, ro data, rw data.
+	 * Then we set code as RO+X, ro data as RO+NX, and rw data as RW+NX
+	 * Siarhei Liakh
+	 * Xuxian Jiang  */
+	DEBUGP(	"module_core: 0x%lx %lu\n"
+		"module_init: 0x%lx %lu\n",
+		(unsigned long)mod->module_core,
+		(unsigned long)mod->core_size,
+		(unsigned long)mod->module_init,
+		(unsigned long)mod->init_size);
+
+	/* calculate RO and NX regions for core */
+	total_pages = NUMBER_OF_PAGES( mod->module_core, mod->core_size);
+	text_pages = NUMBER_OF_PAGES( mod->module_core, mod->core_text_size);
+	ro_pages = NUMBER_OF_PAGES( mod->module_core, mod->core_ro_size);
+
+	DEBUGP(	"CORE:\n  text_pages: %lu\n"
+		"  ro_pages: %lu\n  total_pages: %lu\n",
+		text_pages, ro_pages,total_pages);
+
+	/* Set text and RO data as RO */
+	DEBUGP(	"  RO: 0x%lx %lu\n",
+		(unsigned long)mod->module_core, ro_pages);
+
+	if(ro_pages>0)
+		set_memory_ro( (unsigned long)mod->module_core, ro_pages);
+
+	/* Set all data as NX */
+	DEBUGP(	"  NX: 0x%lx %lu\n",
+		(unsigned long)mod->module_core +
+		(text_pages << PAGE_SHIFT) ,
+		total_pages - text_pages );
+
+	if(total_pages > text_pages)
+		set_memory_nx( (unsigned long)mod->module_core +
+			  	(text_pages << PAGE_SHIFT) ,
+				 total_pages - text_pages );
+
+	/* calculate RO and NX regions for init */
+	total_pages = NUMBER_OF_PAGES( mod->module_init, mod->init_size);
+	text_pages = NUMBER_OF_PAGES( mod->module_init, mod->init_text_size);
+	ro_pages = NUMBER_OF_PAGES( mod->module_init, mod->init_ro_size);
+	
+	DEBUGP(	"INIT:\n  text_pages: %lu\n"
+		"  ro_pages: %lu\n  total_pages: %lu\n",
+		text_pages, ro_pages, total_pages);
+
+	/* Set text and RO data as RO */
+	DEBUGP(	"  RO: 0x%lx %lu\n",
+		(unsigned long)mod->module_init, ro_pages);
+
+	if(ro_pages>0)
+		set_memory_ro( (unsigned long)mod->module_init, ro_pages);
+
+	/* Set all data as NX */
+	DEBUGP(	"  NX: 0x%lx %lu\n",
+		(unsigned long)mod->module_init + (text_pages << PAGE_SHIFT),
+		total_pages - text_pages );
+
+	if(total_pages > text_pages)
+		set_memory_nx( (unsigned long)mod->module_init +
+			  	(text_pages << PAGE_SHIFT) ,
+				 total_pages - text_pages );
+#endif
+
 	/* Done! */
 	return mod;

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

[James Morris]

On Thu, 18 Jun 2009, Siarhei Liakh wrote:

> This patch is a logical extension of the protection provided by
> CONFIG_DEBUG_RODATA to LKMs. The protection is provided by splitting
> module_core and module_init into three logical parts each and setting
> appropriate page access permissions for each individual section:
> 
>    1. Code: RO+X
>    2. RO data: RO+NX
>    3. RW data: RW+NX
> 
> In order to achieve proper protection, layout_sections() have been
> modified to align each of the three parts mentioned above onto page
> boundary. Next, the corresponding page access permissions are set
> right before successful exit from load_module(). Further,
> module_free() have been modified to set module_core or module_init as
> RW+NX right before calling vfree(). Functionality of this patch is
> enabled only when CONFIG_DEBUG_RODATA defined at compile time.
> 

This looks potentially useful to me, but I'm not an x86 expert (several 
now added to Cc:).


> The patch have been developed for Linux 2.6.30 i386.
> 
> If there are any issues with the patch formatting and/or kernel
> version, please let me know and I will gladly correct the issues.
> Thank you.
> 
> Signed-off-by: Siarhei Liakh <sliakh.lkml@gmail.com>
> ---
> 
> diff --git a/arch/x86/kernel/module_32.c b/arch/x86/kernel/module_32.c
> index 0edd819..8a72ef8 100644
> --- a/arch/x86/kernel/module_32.c
> +++ b/arch/x86/kernel/module_32.c
> @@ -22,6 +22,7 @@
>  #include <linux/string.h>
>  #include <linux/kernel.h>
>  #include <linux/bug.h>
> +#include <asm/cacheflush.h>
> 
>  #if 0
>  #define DEBUGP printk
> @@ -29,6 +30,14 @@
>  #define DEBUGP(fmt...)
>  #endif
> 
> +#ifdef CONFIG_DEBUG_RODATA
> +/* Given BASE and SIZE this macro calculates the number of pages this
> + * memory regions occupies */
> +#define NUMBER_OF_PAGES(BASE, SIZE) ( \
> +	( ( (unsigned long) BASE + (unsigned long) SIZE ) >> PAGE_SHIFT ) - \
> +	( ( (unsigned long) BASE ) >> PAGE_SHIFT) + ( (SIZE>0) ? 1:0 ) )
> +#endif
> +
>  void *module_alloc(unsigned long size)
>  {
>  	if (size == 0)
> @@ -40,6 +49,26 @@ void *module_alloc(unsigned long size)
>  /* Free memory returned from module_alloc */
>  void module_free(struct module *mod, void *module_region)
>  {
> +#ifdef CONFIG_DEBUG_RODATA
> +	unsigned long total_pages;
> +
> +	if(mod->module_core == module_region){
> +		/* Set core as NX+RW */
> +		total_pages = NUMBER_OF_PAGES( mod->module_core, mod->core_size );
> +		DEBUGP(KERN_WARNING "RELEASING MODULE CORE: 0x%lx %lu\n",
> +				(unsigned long)mod->module_core, total_pages);
> +		set_memory_nx( (unsigned long)mod->module_core, total_pages);
> +		set_memory_rw( (unsigned long)mod->module_core, total_pages);
> +
> +	}else if(mod->module_init == module_region){
> +		/* Set init as NX+RW */
> +		total_pages = NUMBER_OF_PAGES( mod->module_init, mod->init_size );
> +		DEBUGP(KERN_WARNING "RELEASING MODULE INIT: 0x%lx %lu\n",
> +				(unsigned long)mod->module_init, total_pages);
> +		set_memory_nx( (unsigned long)mod->module_init, total_pages);
> +		set_memory_rw( (unsigned long)mod->module_init, total_pages);
> +	}
> +#endif
>  	vfree(module_region);
>  	/* FIXME: If module_region == mod->init_region, trim exception
>  	   table entries. */
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 627ac08..75e7428 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -293,6 +293,11 @@ struct module
>  	/* The size of the executable code in each section.  */
>  	unsigned int init_text_size, core_text_size;
> 
> +#ifdef CONFIG_DEBUG_RODATA
> +	/* Size of RO sections of the module (text+rodata) */
> +	unsigned int init_ro_size, core_ro_size;
> +#endif
> +
>  	/* Arch-specific module values */
>  	struct mod_arch_specific arch;
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index e797812..4afcb06 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -63,6 +63,14 @@
>  #define ARCH_SHF_SMALL 0
>  #endif
> 
> +#ifdef CONFIG_DEBUG_RODATA
> +/* Given BASE and SIZE this macro calculates the number of pages this
> + * memory regions occupies */
> +#define NUMBER_OF_PAGES(BASE, SIZE) ( \
> +	( ( (unsigned long) BASE + (unsigned long) SIZE ) >> PAGE_SHIFT ) - \
> +	( ( (unsigned long) BASE ) >> PAGE_SHIFT) + ( (SIZE>0) ? 1:0 ) )
> +#endif
> +
>  /* If this is set, the section belongs in the init part of the module */
>  #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
> 
> @@ -1661,6 +1669,10 @@ static void layout_sections(struct module *mod,
>  		{ ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>  	};
>  	unsigned int m, i;
> +#ifdef CONFIG_DEBUG_RODATA
> +	/* always align first section on page boundary */
> +	int need_alignment = 1;
> +#endif
> 
>  	for (i = 0; i < hdr->e_shnum; i++)
>  		sechdrs[i].sh_entsize = ~0UL;
> @@ -1675,13 +1687,39 @@ static void layout_sections(struct module *mod,
>  			    || s->sh_entsize != ~0UL
>  			    || strstarts(secstrings + s->sh_name, ".init"))
>  				continue;
> +#ifdef CONFIG_DEBUG_RODATA
> +			if(need_alignment){
> +				mod->core_size = ALIGN(mod->core_size, PAGE_SIZE);
> +				need_alignment = 0;
> +			}
> +#endif
>  			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
>  			DEBUGP("\t%s\n", secstrings + s->sh_name);
>  		}
> -		if (m == 0)
> +
> +		if (m == 0) {
> +			/* module code size */
>  			mod->core_text_size = mod->core_size;
> +#ifdef CONFIG_DEBUG_RODATA
> +			/* next section should start on new page */
> +			need_alignment = 1;
> +#endif
> +		}
> +
> +#ifdef CONFIG_DEBUG_RODATA
> +		if (m == 1) {
> +			/* module RO size (text+rodata) */
> +			mod->core_ro_size = mod->core_size;
> +			/* next section should start on new page */
> +			need_alignment = 1;
> +		}
> +#endif
>  	}
> 
> +#ifdef CONFIG_DEBUG_RODATA
> +	/* always align first section on page boundary */
> +	need_alignment = 1;
> +#endif
>  	DEBUGP("Init section allocation order:\n");
>  	for (m = 0; m < ARRAY_SIZE(masks); ++m) {
>  		for (i = 0; i < hdr->e_shnum; ++i) {
> @@ -1692,13 +1730,50 @@ static void layout_sections(struct module *mod,
>  			    || s->sh_entsize != ~0UL
>  			    || !strstarts(secstrings + s->sh_name, ".init"))
>  				continue;
> +#ifdef CONFIG_DEBUG_RODATA
> +			if(need_alignment){
> +				mod->init_size = ALIGN(mod->init_size, PAGE_SIZE);
> +				need_alignment = 0;
> +			}
> +#endif			
> +
>  			s->sh_entsize = (get_offset(mod, &mod->init_size, s, i)
>  					 | INIT_OFFSET_MASK);
>  			DEBUGP("\t%s\n", secstrings + s->sh_name);
>  		}
> -		if (m == 0)
> +
> +		if (m == 0) {
> +			/* module code size */
>  			mod->init_text_size = mod->init_size;
> +#ifdef CONFIG_DEBUG_RODATA
> +			/* next section should start on new page */
> +			need_alignment = 1;
> +#endif
> +		}
> +
> +#ifdef CONFIG_DEBUG_RODATA
> +		if (m == 1) {
> +			// module RO size (text+rodata)
> +			mod->init_ro_size = mod->init_size;
> +			// next section should start on new page
> +			need_alignment = 1;
> +		}
> +#endif
>  	}
> +#ifdef CONFIG_DEBUG_RODATA
> +	DEBUGP( "core_text_size: %i\n"
> +		"core_ro_size: %i\n"
> +		"core_size: %i\n"
> +		"init_text_size: %i\n"
> +		"init_ro_size: %i\n"
> +		"init_size: %i\n",
> +		mod->core_text_size,
> +		mod->core_ro_size,
> +		mod->core_size,
> +		mod->init_text_size,
> +		mod->init_ro_size,
> +		mod->init_size);
> +#endif
>  }
> 
>  static void set_license(struct module *mod, const char *license)
> @@ -1898,6 +1973,12 @@ static noinline struct module *load_module(void
> __user *umod,
>  	void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
>  	unsigned long *mseg;
>  	mm_segment_t old_fs;
> +#ifdef CONFIG_DEBUG_RODATA
> +	/* these variables are used to set RO and NX regions */
> +	unsigned long total_pages;
> +	unsigned long text_pages;
> +	unsigned long ro_pages;
> +#endif
> 
>  	DEBUGP("load_module: umod=%p, len=%lu, uargs=%p\n",
>  	       umod, len, uargs);
> @@ -2291,6 +2372,73 @@ static noinline struct module *load_module(void
> __user *umod,
>  	/* Get rid of temporary copy */
>  	vfree(hdr);
> 
> +#ifdef CONFIG_DEBUG_RODATA
> +	/* In this section we calculate the number of pages for
> +	 * each part of core and init: code, ro data, rw data.
> +	 * Then we set code as RO+X, ro data as RO+NX, and rw data as RW+NX
> +	 * Siarhei Liakh
> +	 * Xuxian Jiang  */
> +	DEBUGP(	"module_core: 0x%lx %lu\n"
> +		"module_init: 0x%lx %lu\n",
> +		(unsigned long)mod->module_core,
> +		(unsigned long)mod->core_size,
> +		(unsigned long)mod->module_init,
> +		(unsigned long)mod->init_size);
> +
> +	/* calculate RO and NX regions for core */
> +	total_pages = NUMBER_OF_PAGES( mod->module_core, mod->core_size);
> +	text_pages = NUMBER_OF_PAGES( mod->module_core, mod->core_text_size);
> +	ro_pages = NUMBER_OF_PAGES( mod->module_core, mod->core_ro_size);
> +
> +	DEBUGP(	"CORE:\n  text_pages: %lu\n"
> +		"  ro_pages: %lu\n  total_pages: %lu\n",
> +		text_pages, ro_pages,total_pages);
> +
> +	/* Set text and RO data as RO */
> +	DEBUGP(	"  RO: 0x%lx %lu\n",
> +		(unsigned long)mod->module_core, ro_pages);
> +
> +	if(ro_pages>0)
> +		set_memory_ro( (unsigned long)mod->module_core, ro_pages);
> +
> +	/* Set all data as NX */
> +	DEBUGP(	"  NX: 0x%lx %lu\n",
> +		(unsigned long)mod->module_core +
> +		(text_pages << PAGE_SHIFT) ,
> +		total_pages - text_pages );
> +
> +	if(total_pages > text_pages)
> +		set_memory_nx( (unsigned long)mod->module_core +
> +			  	(text_pages << PAGE_SHIFT) ,
> +				 total_pages - text_pages );
> +
> +	/* calculate RO and NX regions for init */
> +	total_pages = NUMBER_OF_PAGES( mod->module_init, mod->init_size);
> +	text_pages = NUMBER_OF_PAGES( mod->module_init, mod->init_text_size);
> +	ro_pages = NUMBER_OF_PAGES( mod->module_init, mod->init_ro_size);
> +	
> +	DEBUGP(	"INIT:\n  text_pages: %lu\n"
> +		"  ro_pages: %lu\n  total_pages: %lu\n",
> +		text_pages, ro_pages, total_pages);
> +
> +	/* Set text and RO data as RO */
> +	DEBUGP(	"  RO: 0x%lx %lu\n",
> +		(unsigned long)mod->module_init, ro_pages);
> +
> +	if(ro_pages>0)
> +		set_memory_ro( (unsigned long)mod->module_init, ro_pages);
> +
> +	/* Set all data as NX */
> +	DEBUGP(	"  NX: 0x%lx %lu\n",
> +		(unsigned long)mod->module_init + (text_pages << PAGE_SHIFT),
> +		total_pages - text_pages );
> +
> +	if(total_pages > text_pages)
> +		set_memory_nx( (unsigned long)mod->module_init +
> +			  	(text_pages << PAGE_SHIFT) ,
> +				 total_pages - text_pages );
> +#endif
> +
>  	/* Done! */
>  	return mod;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
James Morris
<jmorris@namei.org>

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

[Ingo Molnar]

* James Morris <jmorris@namei.org> wrote:

> On Thu, 18 Jun 2009, Siarhei Liakh wrote:
> 
> > This patch is a logical extension of the protection provided by
> > CONFIG_DEBUG_RODATA to LKMs. The protection is provided by splitting
> > module_core and module_init into three logical parts each and setting
> > appropriate page access permissions for each individual section:
> > 
> >    1. Code: RO+X
> >    2. RO data: RO+NX
> >    3. RW data: RW+NX
> > 
> > In order to achieve proper protection, layout_sections() have been
> > modified to align each of the three parts mentioned above onto page
> > boundary. Next, the corresponding page access permissions are set
> > right before successful exit from load_module(). Further,
> > module_free() have been modified to set module_core or module_init as
> > RW+NX right before calling vfree(). Functionality of this patch is
> > enabled only when CONFIG_DEBUG_RODATA defined at compile time.
> > 
> 
> This looks potentially useful to me, but I'm not an x86 expert 
> (several now added to Cc:).

Pinged a few folks about this already. It looks useful, with the 
main worry being:

 1) the increase in effective module size (probably worth the price)
 2) some uglies in the patch (fixable)

The main ugliness is the excessive use of #ifdefs - those should be 
eliminated. Also, most scripts/checkpatch.pl warnings about this 
patch should be taken seriously.

	Ingo

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

[Siarhei Liakh]

On Tue, Jun 23, 2009 at 6:00 AM, Ingo Molnar<mingo@elte.hu> wrote:
>
> * James Morris <jmorris@namei.org> wrote:
>
>> On Thu, 18 Jun 2009, Siarhei Liakh wrote:
>>
>> > This patch is a logical extension of the protection provided by
>> > CONFIG_DEBUG_RODATA to LKMs. The protection is provided by splitting
>> > module_core and module_init into three logical parts each and setting
>> > appropriate page access permissions for each individual section:
>> >
>> >    1. Code: RO+X
>> >    2. RO data: RO+NX
>> >    3. RW data: RW+NX
>> >
>> > In order to achieve proper protection, layout_sections() have been
>> > modified to align each of the three parts mentioned above onto page
>> > boundary. Next, the corresponding page access permissions are set
>> > right before successful exit from load_module(). Further,
>> > module_free() have been modified to set module_core or module_init as
>> > RW+NX right before calling vfree(). Functionality of this patch is
>> > enabled only when CONFIG_DEBUG_RODATA defined at compile time.
>> >
>>
>> This looks potentially useful to me, but I'm not an x86 expert
>> (several now added to Cc:).
>
> Pinged a few folks about this already. It looks useful, with the
> main worry being:
>
>  1) the increase in effective module size (probably worth the price)
>  2) some uglies in the patch (fixable)
>
> The main ugliness is the excessive use of #ifdefs - those should be
> eliminated. Also, most scripts/checkpatch.pl warnings about this
> patch should be taken seriously.
>
>        Ingo

1: You are correct. This patch effectively sets the lower limit of 3
pages for a module (well, there are some modules that do not have
.text and/or .data, but we are not talking about these extremes). So,
for small (sub-one-page) modules this will create 300% overhead.
However, for large modules the overhead is not that significant.

2: Agree. Will fix.

The main reason the #ifdefs are there is to make this patch dependent
on CONFIG_DEBUG_RODATA, since not everyone may be willing to pay 300%
overhead for the modules they use. I guess I could do some code
re-factoring, but #ifdefs will still be there. Or did you really mean
to eliminate all of them, making this patch a permanent feature of the
kernel? Can you please point me into right direction on how to
eliminate the #ifdefs while allowing to exclude the patch at compile
time if necessary?

Thanks.