[PATCH 0/1] Add ro_after_init support for modules

[PATCH 0/1] Add ro_after_init support for modules

[Jessica Yu]

Hi,

This patch adds ro_after_init support for modules by adding an additional
page-aligned section in the module layout. This new ro_after_init section
sits between rodata and writable data.

So, the new module layout looks like:
   [text] [rodata] [ro_after_init] [writable data]

RO after init data remains RW during init and RO protection is enabled
separately after module init runs.

Did some light testing with lkdtm compiled as a module, verified that
ro_after_init data is writable during init, and that it oopsed after attempted
writes after init. Also tested livepatch (which uses module_{enable,disable}_ro
for its own purposes) to make sure nothing broke. More testing is appreciated :-)

Some remarks on the implementation:
 * A new SHF_RO_AFTER_INIT flag is introduced in elf.h to make
   identification of .data..ro_after_init sections and the work of
   layout_sections() easier. Its chosen value is within the SHF_MASKOS
   range. If people don't like adding a new SHF flag to elf.h, I could
   just make the flag internal to module.c.

 * frob_ro_after_init() could have been separated from
   module_enable_ro() (i.e., put it in its own function, something
   like module_enable_ro_after_init()), but given that livepatch also
   uses module_enable_ro(), I did not want to make livepatch worry
   about calling yet another function just to re-enable all RO protections
   for a module.

 * If a module doesn't have a ro_after_init section, then
   core_layout.ro_after_init_size just takes the value of
   core_layout.ro_size, and frob_ro_after_init() should do nothing.

Based on linux-next.

Previous discussion here:
http://comments.gmane.org/gmane.linux.kernel/2234606

Jessica Yu (1):
  modules: add ro_after_init support

 include/linux/module.h   |  2 ++
 include/uapi/linux/elf.h |  1 +
 kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 66 insertions(+), 10 deletions(-)

-- 
2.4.3

[PATCH 0/1] Add ro_after_init support for modules

[Jessica Yu]

Hi,

This patch adds ro_after_init support for modules by adding an additional
page-aligned section in the module layout. This new ro_after_init section
sits between rodata and writable data.

So, the new module layout looks like:
   [text] [rodata] [ro_after_init] [writable data]

RO after init data remains RW during init and RO protection is enabled
separately after module init runs.

Did some light testing with lkdtm compiled as a module, verified that
ro_after_init data is writable during init, and that it oopsed after attempted
writes after init. Also tested livepatch (which uses module_{enable,disable}_ro
for its own purposes) to make sure nothing broke. More testing is appreciated :-)

Some remarks on the implementation:
 * A new SHF_RO_AFTER_INIT flag is introduced in elf.h to make
   identification of .data..ro_after_init sections and the work of
   layout_sections() easier. Its chosen value is within the SHF_MASKOS
   range. If people don't like adding a new SHF flag to elf.h, I could
   just make the flag internal to module.c.

 * frob_ro_after_init() could have been separated from
   module_enable_ro() (i.e., put it in its own function, something
   like module_enable_ro_after_init()), but given that livepatch also
   uses module_enable_ro(), I did not want to make livepatch worry
   about calling yet another function just to re-enable all RO protections
   for a module.

 * If a module doesn't have a ro_after_init section, then
   core_layout.ro_after_init_size just takes the value of
   core_layout.ro_size, and frob_ro_after_init() should do nothing.

Based on linux-next.

Previous discussion here:
http://comments.gmane.org/gmane.linux.kernel/2234606

Jessica Yu (1):
  modules: add ro_after_init support

 include/linux/module.h   |  2 ++
 include/uapi/linux/elf.h |  1 +
 kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 66 insertions(+), 10 deletions(-)

-- 
2.4.3

[PATCH 1/1] modules: add ro_after_init support

[Jessica Yu]

Add ro_after_init support for modules by adding a new page-aligned section
in the module layout (after rodata) for ro_after_init data and enabling RO
protection for that section after module init runs.

Signed-off-by: Jessica Yu <jeyu@redhat.com>
---
 include/linux/module.h   |  2 ++
 include/uapi/linux/elf.h |  1 +
 kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 66 insertions(+), 10 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index f777164..a698d23 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -311,6 +311,8 @@ struct module_layout {
 	unsigned int text_size;
 	/* Size of RO section of the module (text+rodata) */
 	unsigned int ro_size;
+	/* Size of RO after init section */
+	unsigned int ro_after_init_size;
 
 #ifdef CONFIG_MODULES_TREE_LOOKUP
 	struct mod_tree_node mtn;
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index cb4a72f..70b172ba 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -286,6 +286,7 @@ typedef struct elf64_phdr {
 #define SHF_ALLOC		0x2
 #define SHF_EXECINSTR		0x4
 #define SHF_RELA_LIVEPATCH	0x00100000
+#define SHF_RO_AFTER_INIT	0x00200000
 #define SHF_MASKPROC		0xf0000000
 
 /* special section indexes */
diff --git a/kernel/module.c b/kernel/module.c
index 7f21ab2..055bf6f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1857,10 +1857,11 @@ static void mod_sysfs_teardown(struct module *mod)
  * from modification and any data from execution.
  *
  * General layout of module is:
- *          [text] [read-only-data] [writable data]
- * text_size -----^                ^               ^
- * ro_size ------------------------|               |
- * size -------------------------------------------|
+ *          [text] [read-only-data] [ro-after-init] [writable data]
+ * text_size -----^                ^               ^               ^
+ * ro_size ------------------------|               |               |
+ * ro_after_init_size -----------------------------|               |
+ * size -----------------------------------------------------------|
  *
  * These values are always page-aligned (as is base)
  */
@@ -1883,14 +1884,24 @@ static void frob_rodata(const struct module_layout *layout,
 		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
 }
 
+static void frob_ro_after_init(const struct module_layout *layout,
+				int (*set_memory)(unsigned long start, int num_pages))
+{
+	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
+	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
+	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
+	set_memory((unsigned long)layout->base + layout->ro_size,
+		   (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
+}
+
 static void frob_writable_data(const struct module_layout *layout,
 			       int (*set_memory)(unsigned long start, int num_pages))
 {
 	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
-	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
+	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
 	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
-	set_memory((unsigned long)layout->base + layout->ro_size,
-		   (layout->size - layout->ro_size) >> PAGE_SHIFT);
+	set_memory((unsigned long)layout->base + layout->ro_after_init_size,
+		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
 }
 
 /* livepatching wants to disable read-only so it can frob module. */
@@ -1898,6 +1909,7 @@ void module_disable_ro(const struct module *mod)
 {
 	frob_text(&mod->core_layout, set_memory_rw);
 	frob_rodata(&mod->core_layout, set_memory_rw);
+	frob_ro_after_init(&mod->core_layout, set_memory_rw);
 	frob_text(&mod->init_layout, set_memory_rw);
 	frob_rodata(&mod->init_layout, set_memory_rw);
 }
@@ -1908,11 +1920,16 @@ void module_enable_ro(const struct module *mod)
 	frob_rodata(&mod->core_layout, set_memory_ro);
 	frob_text(&mod->init_layout, set_memory_ro);
 	frob_rodata(&mod->init_layout, set_memory_ro);
+
+	/* after init */
+	if (mod->state == MODULE_STATE_LIVE)
+		frob_ro_after_init(&mod->core_layout, set_memory_ro);
 }
 
 static void module_enable_nx(const struct module *mod)
 {
 	frob_rodata(&mod->core_layout, set_memory_nx);
+	frob_ro_after_init(&mod->core_layout, set_memory_nx);
 	frob_writable_data(&mod->core_layout, set_memory_nx);
 	frob_rodata(&mod->init_layout, set_memory_nx);
 	frob_writable_data(&mod->init_layout, set_memory_nx);
@@ -1921,6 +1938,7 @@ static void module_enable_nx(const struct module *mod)
 static void module_disable_nx(const struct module *mod)
 {
 	frob_rodata(&mod->core_layout, set_memory_x);
+	frob_ro_after_init(&mod->core_layout, set_memory_x);
 	frob_writable_data(&mod->core_layout, set_memory_x);
 	frob_rodata(&mod->init_layout, set_memory_x);
 	frob_writable_data(&mod->init_layout, set_memory_x);
@@ -1963,6 +1981,8 @@ static void disable_ro_nx(const struct module_layout *layout)
 	frob_text(layout, set_memory_rw);
 	frob_rodata(layout, set_memory_rw);
 	frob_rodata(layout, set_memory_x);
+	frob_ro_after_init(layout, set_memory_rw);
+	frob_ro_after_init(layout, set_memory_x);
 	frob_writable_data(layout, set_memory_x);
 }
 
@@ -2305,6 +2325,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
 		 * finder in the two loops below */
 		{ SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
 		{ SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
+		{ SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL },
 		{ SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
 		{ ARCH_SHF_SMALL | SHF_ALLOC, 0 }
 	};
@@ -2336,7 +2357,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
 			mod->core_layout.size = debug_align(mod->core_layout.size);
 			mod->core_layout.ro_size = mod->core_layout.size;
 			break;
-		case 3: /* whole core */
+		case 2: /* RO after init */
+			mod->core_layout.size = debug_align(mod->core_layout.size);
+			mod->core_layout.ro_after_init_size = mod->core_layout.size;
+			break;
+		case 4: /* whole core */
 			mod->core_layout.size = debug_align(mod->core_layout.size);
 			break;
 		}
@@ -2366,7 +2391,14 @@ static void layout_sections(struct module *mod, struct load_info *info)
 			mod->init_layout.size = debug_align(mod->init_layout.size);
 			mod->init_layout.ro_size = mod->init_layout.size;
 			break;
-		case 3: /* whole init */
+		case 2:
+			/*
+			 * RO after init doesn't apply to init_layout (only
+			 * core_layout), so it just takes the value of ro_size.
+			 */
+			mod->init_layout.ro_after_init_size = mod->init_layout.ro_size;
+			break;
+		case 4: /* whole init */
 			mod->init_layout.size = debug_align(mod->init_layout.size);
 			break;
 		}
@@ -3172,6 +3204,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
 	/* Module within temporary copy. */
 	struct module *mod;
+	unsigned int ndx;
 	int err;
 
 	mod = setup_load_info(info, flags);
@@ -3191,6 +3224,15 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	/* We will do a special allocation for per-cpu sections later. */
 	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
 
+	/*
+	 * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
+	 * layout_sections() can put it in the right place.
+	 * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
+	 */
+	ndx = find_sec(info, ".data..ro_after_init");
+	if (ndx)
+		info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
+
 	/* Determine total sizes, and put offsets in sh_entsize.  For now
 	   this is done generically; there doesn't appear to be any
 	   special cases for the architectures. */
@@ -3357,12 +3399,19 @@ static noinline int do_init_module(struct module *mod)
 	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
 	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
 #endif
+	/*
+	 * RO and NX regions should already be protected at this point,
+	 * so the below module_enable_ro() call enables additional RO
+	 * protection for the ro_after_init section only.
+	 */
+	module_enable_ro(mod);
 	mod_tree_remove_init(mod);
 	disable_ro_nx(&mod->init_layout);
 	module_arch_freeing_init(mod);
 	mod->init_layout.base = NULL;
 	mod->init_layout.size = 0;
 	mod->init_layout.ro_size = 0;
+	mod->init_layout.ro_after_init_size = 0;
 	mod->init_layout.text_size = 0;
 	/*
 	 * We want to free module_init, but be aware that kallsyms may be
@@ -3454,7 +3503,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	/* This relies on module_mutex for list integrity. */
 	module_bug_finalize(info->hdr, info->sechdrs, mod);
 
-	/* Set RO and NX regions */
+	/*
+	 * Set RO and NX regions. Since module is not LIVE yet,
+	 * the ro_after_init section will remain RW until after
+	 * do_one_initcall().
+	 */
 	module_enable_ro(mod);
 	module_enable_nx(mod);
 
-- 
2.4.3

Re: [PATCH 1/1] modules: add ro_after_init support

[Kees Cook]

On Mon, Jun 13, 2016 at 5:13 PM, Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Add ro_after_init support for modules by adding a new page-aligned section
> in the module layout (after rodata) for ro_after_init data and enabling RO
> protection for that section after module init runs.
>
> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

This looks awesome! Thanks for working on it.

> ---
>  include/linux/module.h   |  2 ++
>  include/uapi/linux/elf.h |  1 +
>  kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 66 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index f777164..a698d23 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -311,6 +311,8 @@ struct module_layout {
>         unsigned int text_size;
>         /* Size of RO section of the module (text+rodata) */
>         unsigned int ro_size;
> +       /* Size of RO after init section */
> +       unsigned int ro_after_init_size;
>
>  #ifdef CONFIG_MODULES_TREE_LOOKUP
>         struct mod_tree_node mtn;
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index cb4a72f..70b172ba 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -286,6 +286,7 @@ typedef struct elf64_phdr {
>  #define SHF_ALLOC              0x2
>  #define SHF_EXECINSTR          0x4
>  #define SHF_RELA_LIVEPATCH     0x00100000
> +#define SHF_RO_AFTER_INIT      0x00200000

This gives us some flexibility in the ELF flags, but it seems like
overkill since nothing will actually be setting it externally. I defer
to you and Rusty on this one. :)

>  #define SHF_MASKPROC           0xf0000000
>
>  /* special section indexes */
> diff --git a/kernel/module.c b/kernel/module.c
> index 7f21ab2..055bf6f 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1857,10 +1857,11 @@ static void mod_sysfs_teardown(struct module *mod)
>   * from modification and any data from execution.
>   *
>   * General layout of module is:
> - *          [text] [read-only-data] [writable data]
> - * text_size -----^                ^               ^
> - * ro_size ------------------------|               |
> - * size -------------------------------------------|
> + *          [text] [read-only-data] [ro-after-init] [writable data]
> + * text_size -----^                ^               ^               ^
> + * ro_size ------------------------|               |               |
> + * ro_after_init_size -----------------------------|               |
> + * size -----------------------------------------------------------|
>   *
>   * These values are always page-aligned (as is base)
>   */
> @@ -1883,14 +1884,24 @@ static void frob_rodata(const struct module_layout *layout,
>                    (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
>  }
>
> +static void frob_ro_after_init(const struct module_layout *layout,
> +                               int (*set_memory)(unsigned long start, int num_pages))
> +{
> +       BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> +       BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +       BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
> +       set_memory((unsigned long)layout->base + layout->ro_size,
> +                  (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
> +}
> +
>  static void frob_writable_data(const struct module_layout *layout,
>                                int (*set_memory)(unsigned long start, int num_pages))
>  {
>         BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> -       BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +       BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>         BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
> -       set_memory((unsigned long)layout->base + layout->ro_size,
> -                  (layout->size - layout->ro_size) >> PAGE_SHIFT);
> +       set_memory((unsigned long)layout->base + layout->ro_after_init_size,
> +                  (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
>  }
>
>  /* livepatching wants to disable read-only so it can frob module. */
> @@ -1898,6 +1909,7 @@ void module_disable_ro(const struct module *mod)
>  {
>         frob_text(&mod->core_layout, set_memory_rw);
>         frob_rodata(&mod->core_layout, set_memory_rw);
> +       frob_ro_after_init(&mod->core_layout, set_memory_rw);
>         frob_text(&mod->init_layout, set_memory_rw);
>         frob_rodata(&mod->init_layout, set_memory_rw);
>  }
> @@ -1908,11 +1920,16 @@ void module_enable_ro(const struct module *mod)
>         frob_rodata(&mod->core_layout, set_memory_ro);
>         frob_text(&mod->init_layout, set_memory_ro);
>         frob_rodata(&mod->init_layout, set_memory_ro);
> +
> +       /* after init */
> +       if (mod->state == MODULE_STATE_LIVE)
> +               frob_ro_after_init(&mod->core_layout, set_memory_ro);
>  }
>
>  static void module_enable_nx(const struct module *mod)
>  {
>         frob_rodata(&mod->core_layout, set_memory_nx);
> +       frob_ro_after_init(&mod->core_layout, set_memory_nx);
>         frob_writable_data(&mod->core_layout, set_memory_nx);
>         frob_rodata(&mod->init_layout, set_memory_nx);
>         frob_writable_data(&mod->init_layout, set_memory_nx);
> @@ -1921,6 +1938,7 @@ static void module_enable_nx(const struct module *mod)
>  static void module_disable_nx(const struct module *mod)
>  {
>         frob_rodata(&mod->core_layout, set_memory_x);
> +       frob_ro_after_init(&mod->core_layout, set_memory_x);
>         frob_writable_data(&mod->core_layout, set_memory_x);
>         frob_rodata(&mod->init_layout, set_memory_x);
>         frob_writable_data(&mod->init_layout, set_memory_x);
> @@ -1963,6 +1981,8 @@ static void disable_ro_nx(const struct module_layout *layout)
>         frob_text(layout, set_memory_rw);
>         frob_rodata(layout, set_memory_rw);
>         frob_rodata(layout, set_memory_x);
> +       frob_ro_after_init(layout, set_memory_rw);
> +       frob_ro_after_init(layout, set_memory_x);
>         frob_writable_data(layout, set_memory_x);
>  }
>
> @@ -2305,6 +2325,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
>                  * finder in the two loops below */
>                 { SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
>                 { SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
> +               { SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL },
>                 { SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>                 { ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>         };
> @@ -2336,7 +2357,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>                         mod->core_layout.size = debug_align(mod->core_layout.size);
>                         mod->core_layout.ro_size = mod->core_layout.size;
>                         break;
> -               case 3: /* whole core */
> +               case 2: /* RO after init */
> +                       mod->core_layout.size = debug_align(mod->core_layout.size);
> +                       mod->core_layout.ro_after_init_size = mod->core_layout.size;
> +                       break;
> +               case 4: /* whole core */
>                         mod->core_layout.size = debug_align(mod->core_layout.size);
>                         break;
>                 }
> @@ -2366,7 +2391,14 @@ static void layout_sections(struct module *mod, struct load_info *info)
>                         mod->init_layout.size = debug_align(mod->init_layout.size);
>                         mod->init_layout.ro_size = mod->init_layout.size;
>                         break;
> -               case 3: /* whole init */
> +               case 2:
> +                       /*
> +                        * RO after init doesn't apply to init_layout (only
> +                        * core_layout), so it just takes the value of ro_size.
> +                        */
> +                       mod->init_layout.ro_after_init_size = mod->init_layout.ro_size;
> +                       break;
> +               case 4: /* whole init */
>                         mod->init_layout.size = debug_align(mod->init_layout.size);
>                         break;
>                 }
> @@ -3172,6 +3204,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>  {
>         /* Module within temporary copy. */
>         struct module *mod;
> +       unsigned int ndx;
>         int err;
>
>         mod = setup_load_info(info, flags);
> @@ -3191,6 +3224,15 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>         /* We will do a special allocation for per-cpu sections later. */
>         info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>
> +       /*
> +        * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> +        * layout_sections() can put it in the right place.
> +        * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
> +        */
> +       ndx = find_sec(info, ".data..ro_after_init");
> +       if (ndx)
> +               info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> +
>         /* Determine total sizes, and put offsets in sh_entsize.  For now
>            this is done generically; there doesn't appear to be any
>            special cases for the architectures. */
> @@ -3357,12 +3399,19 @@ static noinline int do_init_module(struct module *mod)
>         /* Switch to core kallsyms now init is done: kallsyms may be walking! */
>         rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>  #endif
> +       /*
> +        * RO and NX regions should already be protected at this point,
> +        * so the below module_enable_ro() call enables additional RO
> +        * protection for the ro_after_init section only.
> +        */
> +       module_enable_ro(mod);

My only thought here is that this seems like needless setting of all
the already-set regions. I'm fine with this personally, but I wonder
if maybe it would be better to just call the frob instead?

    frob_ro_after_init(&mod->core_layout, set_memory_ro);


>         mod_tree_remove_init(mod);
>         disable_ro_nx(&mod->init_layout);
>         module_arch_freeing_init(mod);
>         mod->init_layout.base = NULL;
>         mod->init_layout.size = 0;
>         mod->init_layout.ro_size = 0;
> +       mod->init_layout.ro_after_init_size = 0;
>         mod->init_layout.text_size = 0;
>         /*
>          * We want to free module_init, but be aware that kallsyms may be
> @@ -3454,7 +3503,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
>         /* This relies on module_mutex for list integrity. */
>         module_bug_finalize(info->hdr, info->sechdrs, mod);
>
> -       /* Set RO and NX regions */
> +       /*
> +        * Set RO and NX regions. Since module is not LIVE yet,
> +        * the ro_after_init section will remain RW until after
> +        * do_one_initcall().
> +        */
>         module_enable_ro(mod);
>         module_enable_nx(mod);
>
> --
> 2.4.3
>

Regardless, either way:

Reviewed-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH 1/1] modules: add ro_after_init support

[Kees Cook]

On Mon, Jun 13, 2016 at 5:13 PM, Jessica Yu <jeyu@redhat.com> wrote:
> Add ro_after_init support for modules by adding a new page-aligned section
> in the module layout (after rodata) for ro_after_init data and enabling RO
> protection for that section after module init runs.
>
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

This looks awesome! Thanks for working on it.

> ---
>  include/linux/module.h   |  2 ++
>  include/uapi/linux/elf.h |  1 +
>  kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 66 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index f777164..a698d23 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -311,6 +311,8 @@ struct module_layout {
>         unsigned int text_size;
>         /* Size of RO section of the module (text+rodata) */
>         unsigned int ro_size;
> +       /* Size of RO after init section */
> +       unsigned int ro_after_init_size;
>
>  #ifdef CONFIG_MODULES_TREE_LOOKUP
>         struct mod_tree_node mtn;
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index cb4a72f..70b172ba 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -286,6 +286,7 @@ typedef struct elf64_phdr {
>  #define SHF_ALLOC              0x2
>  #define SHF_EXECINSTR          0x4
>  #define SHF_RELA_LIVEPATCH     0x00100000
> +#define SHF_RO_AFTER_INIT      0x00200000

This gives us some flexibility in the ELF flags, but it seems like
overkill since nothing will actually be setting it externally. I defer
to you and Rusty on this one. :)

>  #define SHF_MASKPROC           0xf0000000
>
>  /* special section indexes */
> diff --git a/kernel/module.c b/kernel/module.c
> index 7f21ab2..055bf6f 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1857,10 +1857,11 @@ static void mod_sysfs_teardown(struct module *mod)
>   * from modification and any data from execution.
>   *
>   * General layout of module is:
> - *          [text] [read-only-data] [writable data]
> - * text_size -----^                ^               ^
> - * ro_size ------------------------|               |
> - * size -------------------------------------------|
> + *          [text] [read-only-data] [ro-after-init] [writable data]
> + * text_size -----^                ^               ^               ^
> + * ro_size ------------------------|               |               |
> + * ro_after_init_size -----------------------------|               |
> + * size -----------------------------------------------------------|
>   *
>   * These values are always page-aligned (as is base)
>   */
> @@ -1883,14 +1884,24 @@ static void frob_rodata(const struct module_layout *layout,
>                    (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
>  }
>
> +static void frob_ro_after_init(const struct module_layout *layout,
> +                               int (*set_memory)(unsigned long start, int num_pages))
> +{
> +       BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> +       BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +       BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
> +       set_memory((unsigned long)layout->base + layout->ro_size,
> +                  (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
> +}
> +
>  static void frob_writable_data(const struct module_layout *layout,
>                                int (*set_memory)(unsigned long start, int num_pages))
>  {
>         BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> -       BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +       BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>         BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
> -       set_memory((unsigned long)layout->base + layout->ro_size,
> -                  (layout->size - layout->ro_size) >> PAGE_SHIFT);
> +       set_memory((unsigned long)layout->base + layout->ro_after_init_size,
> +                  (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
>  }
>
>  /* livepatching wants to disable read-only so it can frob module. */
> @@ -1898,6 +1909,7 @@ void module_disable_ro(const struct module *mod)
>  {
>         frob_text(&mod->core_layout, set_memory_rw);
>         frob_rodata(&mod->core_layout, set_memory_rw);
> +       frob_ro_after_init(&mod->core_layout, set_memory_rw);
>         frob_text(&mod->init_layout, set_memory_rw);
>         frob_rodata(&mod->init_layout, set_memory_rw);
>  }
> @@ -1908,11 +1920,16 @@ void module_enable_ro(const struct module *mod)
>         frob_rodata(&mod->core_layout, set_memory_ro);
>         frob_text(&mod->init_layout, set_memory_ro);
>         frob_rodata(&mod->init_layout, set_memory_ro);
> +
> +       /* after init */
> +       if (mod->state == MODULE_STATE_LIVE)
> +               frob_ro_after_init(&mod->core_layout, set_memory_ro);
>  }
>
>  static void module_enable_nx(const struct module *mod)
>  {
>         frob_rodata(&mod->core_layout, set_memory_nx);
> +       frob_ro_after_init(&mod->core_layout, set_memory_nx);
>         frob_writable_data(&mod->core_layout, set_memory_nx);
>         frob_rodata(&mod->init_layout, set_memory_nx);
>         frob_writable_data(&mod->init_layout, set_memory_nx);
> @@ -1921,6 +1938,7 @@ static void module_enable_nx(const struct module *mod)
>  static void module_disable_nx(const struct module *mod)
>  {
>         frob_rodata(&mod->core_layout, set_memory_x);
> +       frob_ro_after_init(&mod->core_layout, set_memory_x);
>         frob_writable_data(&mod->core_layout, set_memory_x);
>         frob_rodata(&mod->init_layout, set_memory_x);
>         frob_writable_data(&mod->init_layout, set_memory_x);
> @@ -1963,6 +1981,8 @@ static void disable_ro_nx(const struct module_layout *layout)
>         frob_text(layout, set_memory_rw);
>         frob_rodata(layout, set_memory_rw);
>         frob_rodata(layout, set_memory_x);
> +       frob_ro_after_init(layout, set_memory_rw);
> +       frob_ro_after_init(layout, set_memory_x);
>         frob_writable_data(layout, set_memory_x);
>  }
>
> @@ -2305,6 +2325,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
>                  * finder in the two loops below */
>                 { SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
>                 { SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
> +               { SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL },
>                 { SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>                 { ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>         };
> @@ -2336,7 +2357,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>                         mod->core_layout.size = debug_align(mod->core_layout.size);
>                         mod->core_layout.ro_size = mod->core_layout.size;
>                         break;
> -               case 3: /* whole core */
> +               case 2: /* RO after init */
> +                       mod->core_layout.size = debug_align(mod->core_layout.size);
> +                       mod->core_layout.ro_after_init_size = mod->core_layout.size;
> +                       break;
> +               case 4: /* whole core */
>                         mod->core_layout.size = debug_align(mod->core_layout.size);
>                         break;
>                 }
> @@ -2366,7 +2391,14 @@ static void layout_sections(struct module *mod, struct load_info *info)
>                         mod->init_layout.size = debug_align(mod->init_layout.size);
>                         mod->init_layout.ro_size = mod->init_layout.size;
>                         break;
> -               case 3: /* whole init */
> +               case 2:
> +                       /*
> +                        * RO after init doesn't apply to init_layout (only
> +                        * core_layout), so it just takes the value of ro_size.
> +                        */
> +                       mod->init_layout.ro_after_init_size = mod->init_layout.ro_size;
> +                       break;
> +               case 4: /* whole init */
>                         mod->init_layout.size = debug_align(mod->init_layout.size);
>                         break;
>                 }
> @@ -3172,6 +3204,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>  {
>         /* Module within temporary copy. */
>         struct module *mod;
> +       unsigned int ndx;
>         int err;
>
>         mod = setup_load_info(info, flags);
> @@ -3191,6 +3224,15 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>         /* We will do a special allocation for per-cpu sections later. */
>         info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>
> +       /*
> +        * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> +        * layout_sections() can put it in the right place.
> +        * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
> +        */
> +       ndx = find_sec(info, ".data..ro_after_init");
> +       if (ndx)
> +               info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> +
>         /* Determine total sizes, and put offsets in sh_entsize.  For now
>            this is done generically; there doesn't appear to be any
>            special cases for the architectures. */
> @@ -3357,12 +3399,19 @@ static noinline int do_init_module(struct module *mod)
>         /* Switch to core kallsyms now init is done: kallsyms may be walking! */
>         rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>  #endif
> +       /*
> +        * RO and NX regions should already be protected at this point,
> +        * so the below module_enable_ro() call enables additional RO
> +        * protection for the ro_after_init section only.
> +        */
> +       module_enable_ro(mod);

My only thought here is that this seems like needless setting of all
the already-set regions. I'm fine with this personally, but I wonder
if maybe it would be better to just call the frob instead?

    frob_ro_after_init(&mod->core_layout, set_memory_ro);


>         mod_tree_remove_init(mod);
>         disable_ro_nx(&mod->init_layout);
>         module_arch_freeing_init(mod);
>         mod->init_layout.base = NULL;
>         mod->init_layout.size = 0;
>         mod->init_layout.ro_size = 0;
> +       mod->init_layout.ro_after_init_size = 0;
>         mod->init_layout.text_size = 0;
>         /*
>          * We want to free module_init, but be aware that kallsyms may be
> @@ -3454,7 +3503,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
>         /* This relies on module_mutex for list integrity. */
>         module_bug_finalize(info->hdr, info->sechdrs, mod);
>
> -       /* Set RO and NX regions */
> +       /*
> +        * Set RO and NX regions. Since module is not LIVE yet,
> +        * the ro_after_init section will remain RW until after
> +        * do_one_initcall().
> +        */
>         module_enable_ro(mod);
>         module_enable_nx(mod);
>
> --
> 2.4.3
>

Regardless, either way:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: modules: add ro_after_init support

[Jessica Yu]

+++ Kees Cook [14/06/16 14:33 -0700]:
>On Mon, Jun 13, 2016 at 5:13 PM, Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Add ro_after_init support for modules by adding a new page-aligned section
>> in the module layout (after rodata) for ro_after_init data and enabling RO
>> protection for that section after module init runs.
>>
>> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
>This looks awesome! Thanks for working on it.
>
>> ---
>>  include/linux/module.h   |  2 ++
>>  include/uapi/linux/elf.h |  1 +
>>  kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
>>  3 files changed, 66 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index f777164..a698d23 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -311,6 +311,8 @@ struct module_layout {
>>         unsigned int text_size;
>>         /* Size of RO section of the module (text+rodata) */
>>         unsigned int ro_size;
>> +       /* Size of RO after init section */
>> +       unsigned int ro_after_init_size;
>>
>>  #ifdef CONFIG_MODULES_TREE_LOOKUP
>>         struct mod_tree_node mtn;
>> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
>> index cb4a72f..70b172ba 100644
>> --- a/include/uapi/linux/elf.h
>> +++ b/include/uapi/linux/elf.h
>> @@ -286,6 +286,7 @@ typedef struct elf64_phdr {
>>  #define SHF_ALLOC              0x2
>>  #define SHF_EXECINSTR          0x4
>>  #define SHF_RELA_LIVEPATCH     0x00100000
>> +#define SHF_RO_AFTER_INIT      0x00200000
>
>This gives us some flexibility in the ELF flags, but it seems like
>overkill since nothing will actually be setting it externally. I defer
>to you and Rusty on this one. :)

Hm yeah, the flag would only be used in the module code, so maybe it
should be internal to module.c..

>>  #define SHF_MASKPROC           0xf0000000
>>
>>  /* special section indexes */
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 7f21ab2..055bf6f 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1857,10 +1857,11 @@ static void mod_sysfs_teardown(struct module *mod)
>>   * from modification and any data from execution.
>>   *
>>   * General layout of module is:
>> - *          [text] [read-only-data] [writable data]
>> - * text_size -----^                ^               ^
>> - * ro_size ------------------------|               |
>> - * size -------------------------------------------|
>> + *          [text] [read-only-data] [ro-after-init] [writable data]
>> + * text_size -----^                ^               ^               ^
>> + * ro_size ------------------------|               |               |
>> + * ro_after_init_size -----------------------------|               |
>> + * size -----------------------------------------------------------|
>>   *
>>   * These values are always page-aligned (as is base)
>>   */
>> @@ -1883,14 +1884,24 @@ static void frob_rodata(const struct module_layout *layout,
>>                    (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
>>  }
>>
>> +static void frob_ro_after_init(const struct module_layout *layout,
>> +                               int (*set_memory)(unsigned long start, int num_pages))
>> +{
>> +       BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>> +       BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
>> +       BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>> +       set_memory((unsigned long)layout->base + layout->ro_size,
>> +                  (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
>> +}
>> +
>>  static void frob_writable_data(const struct module_layout *layout,
>>                                int (*set_memory)(unsigned long start, int num_pages))
>>  {
>>         BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>> -       BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
>> +       BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>>         BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
>> -       set_memory((unsigned long)layout->base + layout->ro_size,
>> -                  (layout->size - layout->ro_size) >> PAGE_SHIFT);
>> +       set_memory((unsigned long)layout->base + layout->ro_after_init_size,
>> +                  (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
>>  }
>>
>>  /* livepatching wants to disable read-only so it can frob module. */
>> @@ -1898,6 +1909,7 @@ void module_disable_ro(const struct module *mod)
>>  {
>>         frob_text(&mod->core_layout, set_memory_rw);
>>         frob_rodata(&mod->core_layout, set_memory_rw);
>> +       frob_ro_after_init(&mod->core_layout, set_memory_rw);
>>         frob_text(&mod->init_layout, set_memory_rw);
>>         frob_rodata(&mod->init_layout, set_memory_rw);
>>  }
>> @@ -1908,11 +1920,16 @@ void module_enable_ro(const struct module *mod)
>>         frob_rodata(&mod->core_layout, set_memory_ro);
>>         frob_text(&mod->init_layout, set_memory_ro);
>>         frob_rodata(&mod->init_layout, set_memory_ro);
>> +
>> +       /* after init */
>> +       if (mod->state == MODULE_STATE_LIVE)
>> +               frob_ro_after_init(&mod->core_layout, set_memory_ro);
>>  }
>>
>>  static void module_enable_nx(const struct module *mod)
>>  {
>>         frob_rodata(&mod->core_layout, set_memory_nx);
>> +       frob_ro_after_init(&mod->core_layout, set_memory_nx);
>>         frob_writable_data(&mod->core_layout, set_memory_nx);
>>         frob_rodata(&mod->init_layout, set_memory_nx);
>>         frob_writable_data(&mod->init_layout, set_memory_nx);
>> @@ -1921,6 +1938,7 @@ static void module_enable_nx(const struct module *mod)
>>  static void module_disable_nx(const struct module *mod)
>>  {
>>         frob_rodata(&mod->core_layout, set_memory_x);
>> +       frob_ro_after_init(&mod->core_layout, set_memory_x);
>>         frob_writable_data(&mod->core_layout, set_memory_x);
>>         frob_rodata(&mod->init_layout, set_memory_x);
>>         frob_writable_data(&mod->init_layout, set_memory_x);
>> @@ -1963,6 +1981,8 @@ static void disable_ro_nx(const struct module_layout *layout)
>>         frob_text(layout, set_memory_rw);
>>         frob_rodata(layout, set_memory_rw);
>>         frob_rodata(layout, set_memory_x);
>> +       frob_ro_after_init(layout, set_memory_rw);
>> +       frob_ro_after_init(layout, set_memory_x);
>>         frob_writable_data(layout, set_memory_x);
>>  }
>>
>> @@ -2305,6 +2325,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>                  * finder in the two loops below */
>>                 { SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
>>                 { SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
>> +               { SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL },
>>                 { SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>>                 { ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>>         };
>> @@ -2336,7 +2357,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>                         mod->core_layout.size = debug_align(mod->core_layout.size);
>>                         mod->core_layout.ro_size = mod->core_layout.size;
>>                         break;
>> -               case 3: /* whole core */
>> +               case 2: /* RO after init */
>> +                       mod->core_layout.size = debug_align(mod->core_layout.size);
>> +                       mod->core_layout.ro_after_init_size = mod->core_layout.size;
>> +                       break;
>> +               case 4: /* whole core */
>>                         mod->core_layout.size = debug_align(mod->core_layout.size);
>>                         break;
>>                 }
>> @@ -2366,7 +2391,14 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>                         mod->init_layout.size = debug_align(mod->init_layout.size);
>>                         mod->init_layout.ro_size = mod->init_layout.size;
>>                         break;
>> -               case 3: /* whole init */
>> +               case 2:
>> +                       /*
>> +                        * RO after init doesn't apply to init_layout (only
>> +                        * core_layout), so it just takes the value of ro_size.
>> +                        */
>> +                       mod->init_layout.ro_after_init_size = mod->init_layout.ro_size;
>> +                       break;
>> +               case 4: /* whole init */
>>                         mod->init_layout.size = debug_align(mod->init_layout.size);
>>                         break;
>>                 }
>> @@ -3172,6 +3204,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>  {
>>         /* Module within temporary copy. */
>>         struct module *mod;
>> +       unsigned int ndx;
>>         int err;
>>
>>         mod = setup_load_info(info, flags);
>> @@ -3191,6 +3224,15 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>         /* We will do a special allocation for per-cpu sections later. */
>>         info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>>
>> +       /*
>> +        * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
>> +        * layout_sections() can put it in the right place.
>> +        * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
>> +        */
>> +       ndx = find_sec(info, ".data..ro_after_init");
>> +       if (ndx)
>> +               info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
>> +
>>         /* Determine total sizes, and put offsets in sh_entsize.  For now
>>            this is done generically; there doesn't appear to be any
>>            special cases for the architectures. */
>> @@ -3357,12 +3399,19 @@ static noinline int do_init_module(struct module *mod)
>>         /* Switch to core kallsyms now init is done: kallsyms may be walking! */
>>         rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>>  #endif
>> +       /*
>> +        * RO and NX regions should already be protected at this point,
>> +        * so the below module_enable_ro() call enables additional RO
>> +        * protection for the ro_after_init section only.
>> +        */
>> +       module_enable_ro(mod);
>
>My only thought here is that this seems like needless setting of all
>the already-set regions. I'm fine with this personally, but I wonder
>if maybe it would be better to just call the frob instead?
>
>    frob_ro_after_init(&mod->core_layout, set_memory_ro);

Yeah, that works too. :-) I figured since module_enable_ro() needs to
call frob_ro_after_init() anyway (because livepatch is also a user of
module_{enable,disable}_ro), it wouldn't hurt to call
module_enable_ro() again here after mod state is LIVE. But maybe a
direct call to frob_ro_after_init() is less confusing.

>>         mod_tree_remove_init(mod);
>>         disable_ro_nx(&mod->init_layout);
>>         module_arch_freeing_init(mod);
>>         mod->init_layout.base = NULL;
>>         mod->init_layout.size = 0;
>>         mod->init_layout.ro_size = 0;
>> +       mod->init_layout.ro_after_init_size = 0;
>>         mod->init_layout.text_size = 0;
>>         /*
>>          * We want to free module_init, but be aware that kallsyms may be
>> @@ -3454,7 +3503,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
>>         /* This relies on module_mutex for list integrity. */
>>         module_bug_finalize(info->hdr, info->sechdrs, mod);
>>
>> -       /* Set RO and NX regions */
>> +       /*
>> +        * Set RO and NX regions. Since module is not LIVE yet,
>> +        * the ro_after_init section will remain RW until after
>> +        * do_one_initcall().
>> +        */
>>         module_enable_ro(mod);
>>         module_enable_nx(mod);
>>
>> --
>> 2.4.3
>>
>
>Regardless, either way:
>
>Reviewed-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Thanks for the review!

Jessica

Re: modules: add ro_after_init support

[Jessica Yu]

+++ Kees Cook [14/06/16 14:33 -0700]:
>On Mon, Jun 13, 2016 at 5:13 PM, Jessica Yu <jeyu@redhat.com> wrote:
>> Add ro_after_init support for modules by adding a new page-aligned section
>> in the module layout (after rodata) for ro_after_init data and enabling RO
>> protection for that section after module init runs.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>
>This looks awesome! Thanks for working on it.
>
>> ---
>>  include/linux/module.h   |  2 ++
>>  include/uapi/linux/elf.h |  1 +
>>  kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
>>  3 files changed, 66 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index f777164..a698d23 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -311,6 +311,8 @@ struct module_layout {
>>         unsigned int text_size;
>>         /* Size of RO section of the module (text+rodata) */
>>         unsigned int ro_size;
>> +       /* Size of RO after init section */
>> +       unsigned int ro_after_init_size;
>>
>>  #ifdef CONFIG_MODULES_TREE_LOOKUP
>>         struct mod_tree_node mtn;
>> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
>> index cb4a72f..70b172ba 100644
>> --- a/include/uapi/linux/elf.h
>> +++ b/include/uapi/linux/elf.h
>> @@ -286,6 +286,7 @@ typedef struct elf64_phdr {
>>  #define SHF_ALLOC              0x2
>>  #define SHF_EXECINSTR          0x4
>>  #define SHF_RELA_LIVEPATCH     0x00100000
>> +#define SHF_RO_AFTER_INIT      0x00200000
>
>This gives us some flexibility in the ELF flags, but it seems like
>overkill since nothing will actually be setting it externally. I defer
>to you and Rusty on this one. :)

Hm yeah, the flag would only be used in the module code, so maybe it
should be internal to module.c..

>>  #define SHF_MASKPROC           0xf0000000
>>
>>  /* special section indexes */
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 7f21ab2..055bf6f 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1857,10 +1857,11 @@ static void mod_sysfs_teardown(struct module *mod)
>>   * from modification and any data from execution.
>>   *
>>   * General layout of module is:
>> - *          [text] [read-only-data] [writable data]
>> - * text_size -----^                ^               ^
>> - * ro_size ------------------------|               |
>> - * size -------------------------------------------|
>> + *          [text] [read-only-data] [ro-after-init] [writable data]
>> + * text_size -----^                ^               ^               ^
>> + * ro_size ------------------------|               |               |
>> + * ro_after_init_size -----------------------------|               |
>> + * size -----------------------------------------------------------|
>>   *
>>   * These values are always page-aligned (as is base)
>>   */
>> @@ -1883,14 +1884,24 @@ static void frob_rodata(const struct module_layout *layout,
>>                    (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
>>  }
>>
>> +static void frob_ro_after_init(const struct module_layout *layout,
>> +                               int (*set_memory)(unsigned long start, int num_pages))
>> +{
>> +       BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>> +       BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
>> +       BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>> +       set_memory((unsigned long)layout->base + layout->ro_size,
>> +                  (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
>> +}
>> +
>>  static void frob_writable_data(const struct module_layout *layout,
>>                                int (*set_memory)(unsigned long start, int num_pages))
>>  {
>>         BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>> -       BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
>> +       BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>>         BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
>> -       set_memory((unsigned long)layout->base + layout->ro_size,
>> -                  (layout->size - layout->ro_size) >> PAGE_SHIFT);
>> +       set_memory((unsigned long)layout->base + layout->ro_after_init_size,
>> +                  (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
>>  }
>>
>>  /* livepatching wants to disable read-only so it can frob module. */
>> @@ -1898,6 +1909,7 @@ void module_disable_ro(const struct module *mod)
>>  {
>>         frob_text(&mod->core_layout, set_memory_rw);
>>         frob_rodata(&mod->core_layout, set_memory_rw);
>> +       frob_ro_after_init(&mod->core_layout, set_memory_rw);
>>         frob_text(&mod->init_layout, set_memory_rw);
>>         frob_rodata(&mod->init_layout, set_memory_rw);
>>  }
>> @@ -1908,11 +1920,16 @@ void module_enable_ro(const struct module *mod)
>>         frob_rodata(&mod->core_layout, set_memory_ro);
>>         frob_text(&mod->init_layout, set_memory_ro);
>>         frob_rodata(&mod->init_layout, set_memory_ro);
>> +
>> +       /* after init */
>> +       if (mod->state == MODULE_STATE_LIVE)
>> +               frob_ro_after_init(&mod->core_layout, set_memory_ro);
>>  }
>>
>>  static void module_enable_nx(const struct module *mod)
>>  {
>>         frob_rodata(&mod->core_layout, set_memory_nx);
>> +       frob_ro_after_init(&mod->core_layout, set_memory_nx);
>>         frob_writable_data(&mod->core_layout, set_memory_nx);
>>         frob_rodata(&mod->init_layout, set_memory_nx);
>>         frob_writable_data(&mod->init_layout, set_memory_nx);
>> @@ -1921,6 +1938,7 @@ static void module_enable_nx(const struct module *mod)
>>  static void module_disable_nx(const struct module *mod)
>>  {
>>         frob_rodata(&mod->core_layout, set_memory_x);
>> +       frob_ro_after_init(&mod->core_layout, set_memory_x);
>>         frob_writable_data(&mod->core_layout, set_memory_x);
>>         frob_rodata(&mod->init_layout, set_memory_x);
>>         frob_writable_data(&mod->init_layout, set_memory_x);
>> @@ -1963,6 +1981,8 @@ static void disable_ro_nx(const struct module_layout *layout)
>>         frob_text(layout, set_memory_rw);
>>         frob_rodata(layout, set_memory_rw);
>>         frob_rodata(layout, set_memory_x);
>> +       frob_ro_after_init(layout, set_memory_rw);
>> +       frob_ro_after_init(layout, set_memory_x);
>>         frob_writable_data(layout, set_memory_x);
>>  }
>>
>> @@ -2305,6 +2325,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>                  * finder in the two loops below */
>>                 { SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
>>                 { SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
>> +               { SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL },
>>                 { SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>>                 { ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>>         };
>> @@ -2336,7 +2357,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>                         mod->core_layout.size = debug_align(mod->core_layout.size);
>>                         mod->core_layout.ro_size = mod->core_layout.size;
>>                         break;
>> -               case 3: /* whole core */
>> +               case 2: /* RO after init */
>> +                       mod->core_layout.size = debug_align(mod->core_layout.size);
>> +                       mod->core_layout.ro_after_init_size = mod->core_layout.size;
>> +                       break;
>> +               case 4: /* whole core */
>>                         mod->core_layout.size = debug_align(mod->core_layout.size);
>>                         break;
>>                 }
>> @@ -2366,7 +2391,14 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>                         mod->init_layout.size = debug_align(mod->init_layout.size);
>>                         mod->init_layout.ro_size = mod->init_layout.size;
>>                         break;
>> -               case 3: /* whole init */
>> +               case 2:
>> +                       /*
>> +                        * RO after init doesn't apply to init_layout (only
>> +                        * core_layout), so it just takes the value of ro_size.
>> +                        */
>> +                       mod->init_layout.ro_after_init_size = mod->init_layout.ro_size;
>> +                       break;
>> +               case 4: /* whole init */
>>                         mod->init_layout.size = debug_align(mod->init_layout.size);
>>                         break;
>>                 }
>> @@ -3172,6 +3204,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>  {
>>         /* Module within temporary copy. */
>>         struct module *mod;
>> +       unsigned int ndx;
>>         int err;
>>
>>         mod = setup_load_info(info, flags);
>> @@ -3191,6 +3224,15 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>         /* We will do a special allocation for per-cpu sections later. */
>>         info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>>
>> +       /*
>> +        * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
>> +        * layout_sections() can put it in the right place.
>> +        * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
>> +        */
>> +       ndx = find_sec(info, ".data..ro_after_init");
>> +       if (ndx)
>> +               info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
>> +
>>         /* Determine total sizes, and put offsets in sh_entsize.  For now
>>            this is done generically; there doesn't appear to be any
>>            special cases for the architectures. */
>> @@ -3357,12 +3399,19 @@ static noinline int do_init_module(struct module *mod)
>>         /* Switch to core kallsyms now init is done: kallsyms may be walking! */
>>         rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>>  #endif
>> +       /*
>> +        * RO and NX regions should already be protected at this point,
>> +        * so the below module_enable_ro() call enables additional RO
>> +        * protection for the ro_after_init section only.
>> +        */
>> +       module_enable_ro(mod);
>
>My only thought here is that this seems like needless setting of all
>the already-set regions. I'm fine with this personally, but I wonder
>if maybe it would be better to just call the frob instead?
>
>    frob_ro_after_init(&mod->core_layout, set_memory_ro);

Yeah, that works too. :-) I figured since module_enable_ro() needs to
call frob_ro_after_init() anyway (because livepatch is also a user of
module_{enable,disable}_ro), it wouldn't hurt to call
module_enable_ro() again here after mod state is LIVE. But maybe a
direct call to frob_ro_after_init() is less confusing.

>>         mod_tree_remove_init(mod);
>>         disable_ro_nx(&mod->init_layout);
>>         module_arch_freeing_init(mod);
>>         mod->init_layout.base = NULL;
>>         mod->init_layout.size = 0;
>>         mod->init_layout.ro_size = 0;
>> +       mod->init_layout.ro_after_init_size = 0;
>>         mod->init_layout.text_size = 0;
>>         /*
>>          * We want to free module_init, but be aware that kallsyms may be
>> @@ -3454,7 +3503,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
>>         /* This relies on module_mutex for list integrity. */
>>         module_bug_finalize(info->hdr, info->sechdrs, mod);
>>
>> -       /* Set RO and NX regions */
>> +       /*
>> +        * Set RO and NX regions. Since module is not LIVE yet,
>> +        * the ro_after_init section will remain RW until after
>> +        * do_one_initcall().
>> +        */
>>         module_enable_ro(mod);
>>         module_enable_nx(mod);
>>
>> --
>> 2.4.3
>>
>
>Regardless, either way:
>
>Reviewed-by: Kees Cook <keescook@chromium.org>

Thanks for the review!

Jessica

Re: [PATCH 1/1] modules: add ro_after_init support

[Rusty Russell]

Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> Add ro_after_init support for modules by adding a new page-aligned section
> in the module layout (after rodata) for ro_after_init data and enabling RO
> protection for that section after module init runs.
>
> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

I would prefer a "bool after_init" flag to module_enable_ro().  It's
more explicit.

Exposing the flags via uapi looks like a wart, but it's kind of a
feature, since we don't *unset* it in any section; userspace may want to
know about it.

If you could re-spin, that would be great.

Thanks!
Rusty.

> ---
>  include/linux/module.h   |  2 ++
>  include/uapi/linux/elf.h |  1 +
>  kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 66 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index f777164..a698d23 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -311,6 +311,8 @@ struct module_layout {
>  	unsigned int text_size;
>  	/* Size of RO section of the module (text+rodata) */
>  	unsigned int ro_size;
> +	/* Size of RO after init section */
> +	unsigned int ro_after_init_size;
>  
>  #ifdef CONFIG_MODULES_TREE_LOOKUP
>  	struct mod_tree_node mtn;
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index cb4a72f..70b172ba 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -286,6 +286,7 @@ typedef struct elf64_phdr {
>  #define SHF_ALLOC		0x2
>  #define SHF_EXECINSTR		0x4
>  #define SHF_RELA_LIVEPATCH	0x00100000
> +#define SHF_RO_AFTER_INIT	0x00200000
>  #define SHF_MASKPROC		0xf0000000
>  
>  /* special section indexes */
> diff --git a/kernel/module.c b/kernel/module.c
> index 7f21ab2..055bf6f 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1857,10 +1857,11 @@ static void mod_sysfs_teardown(struct module *mod)
>   * from modification and any data from execution.
>   *
>   * General layout of module is:
> - *          [text] [read-only-data] [writable data]
> - * text_size -----^                ^               ^
> - * ro_size ------------------------|               |
> - * size -------------------------------------------|
> + *          [text] [read-only-data] [ro-after-init] [writable data]
> + * text_size -----^                ^               ^               ^
> + * ro_size ------------------------|               |               |
> + * ro_after_init_size -----------------------------|               |
> + * size -----------------------------------------------------------|
>   *
>   * These values are always page-aligned (as is base)
>   */
> @@ -1883,14 +1884,24 @@ static void frob_rodata(const struct module_layout *layout,
>  		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
>  }
>  
> +static void frob_ro_after_init(const struct module_layout *layout,
> +				int (*set_memory)(unsigned long start, int num_pages))
> +{
> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
> +	set_memory((unsigned long)layout->base + layout->ro_size,
> +		   (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
> +}
> +
>  static void frob_writable_data(const struct module_layout *layout,
>  			       int (*set_memory)(unsigned long start, int num_pages))
>  {
>  	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> -	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>  	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
> -	set_memory((unsigned long)layout->base + layout->ro_size,
> -		   (layout->size - layout->ro_size) >> PAGE_SHIFT);
> +	set_memory((unsigned long)layout->base + layout->ro_after_init_size,
> +		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
>  }
>  
>  /* livepatching wants to disable read-only so it can frob module. */
> @@ -1898,6 +1909,7 @@ void module_disable_ro(const struct module *mod)
>  {
>  	frob_text(&mod->core_layout, set_memory_rw);
>  	frob_rodata(&mod->core_layout, set_memory_rw);
> +	frob_ro_after_init(&mod->core_layout, set_memory_rw);
>  	frob_text(&mod->init_layout, set_memory_rw);
>  	frob_rodata(&mod->init_layout, set_memory_rw);
>  }
> @@ -1908,11 +1920,16 @@ void module_enable_ro(const struct module *mod)
>  	frob_rodata(&mod->core_layout, set_memory_ro);
>  	frob_text(&mod->init_layout, set_memory_ro);
>  	frob_rodata(&mod->init_layout, set_memory_ro);
> +
> +	/* after init */
> +	if (mod->state == MODULE_STATE_LIVE)
> +		frob_ro_after_init(&mod->core_layout, set_memory_ro);
>  }
>  
>  static void module_enable_nx(const struct module *mod)
>  {
>  	frob_rodata(&mod->core_layout, set_memory_nx);
> +	frob_ro_after_init(&mod->core_layout, set_memory_nx);
>  	frob_writable_data(&mod->core_layout, set_memory_nx);
>  	frob_rodata(&mod->init_layout, set_memory_nx);
>  	frob_writable_data(&mod->init_layout, set_memory_nx);
> @@ -1921,6 +1938,7 @@ static void module_enable_nx(const struct module *mod)
>  static void module_disable_nx(const struct module *mod)
>  {
>  	frob_rodata(&mod->core_layout, set_memory_x);
> +	frob_ro_after_init(&mod->core_layout, set_memory_x);
>  	frob_writable_data(&mod->core_layout, set_memory_x);
>  	frob_rodata(&mod->init_layout, set_memory_x);
>  	frob_writable_data(&mod->init_layout, set_memory_x);
> @@ -1963,6 +1981,8 @@ static void disable_ro_nx(const struct module_layout *layout)
>  	frob_text(layout, set_memory_rw);
>  	frob_rodata(layout, set_memory_rw);
>  	frob_rodata(layout, set_memory_x);
> +	frob_ro_after_init(layout, set_memory_rw);
> +	frob_ro_after_init(layout, set_memory_x);
>  	frob_writable_data(layout, set_memory_x);
>  }
>  
> @@ -2305,6 +2325,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
>  		 * finder in the two loops below */
>  		{ SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
>  		{ SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
> +		{ SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL },
>  		{ SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>  		{ ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>  	};
> @@ -2336,7 +2357,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>  			mod->core_layout.size = debug_align(mod->core_layout.size);
>  			mod->core_layout.ro_size = mod->core_layout.size;
>  			break;
> -		case 3: /* whole core */
> +		case 2: /* RO after init */
> +			mod->core_layout.size = debug_align(mod->core_layout.size);
> +			mod->core_layout.ro_after_init_size = mod->core_layout.size;
> +			break;
> +		case 4: /* whole core */
>  			mod->core_layout.size = debug_align(mod->core_layout.size);
>  			break;
>  		}
> @@ -2366,7 +2391,14 @@ static void layout_sections(struct module *mod, struct load_info *info)
>  			mod->init_layout.size = debug_align(mod->init_layout.size);
>  			mod->init_layout.ro_size = mod->init_layout.size;
>  			break;
> -		case 3: /* whole init */
> +		case 2:
> +			/*
> +			 * RO after init doesn't apply to init_layout (only
> +			 * core_layout), so it just takes the value of ro_size.
> +			 */
> +			mod->init_layout.ro_after_init_size = mod->init_layout.ro_size;
> +			break;
> +		case 4: /* whole init */
>  			mod->init_layout.size = debug_align(mod->init_layout.size);
>  			break;
>  		}
> @@ -3172,6 +3204,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>  {
>  	/* Module within temporary copy. */
>  	struct module *mod;
> +	unsigned int ndx;
>  	int err;
>  
>  	mod = setup_load_info(info, flags);
> @@ -3191,6 +3224,15 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>  	/* We will do a special allocation for per-cpu sections later. */
>  	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>  
> +	/*
> +	 * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> +	 * layout_sections() can put it in the right place.
> +	 * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
> +	 */
> +	ndx = find_sec(info, ".data..ro_after_init");
> +	if (ndx)
> +		info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> +
>  	/* Determine total sizes, and put offsets in sh_entsize.  For now
>  	   this is done generically; there doesn't appear to be any
>  	   special cases for the architectures. */
> @@ -3357,12 +3399,19 @@ static noinline int do_init_module(struct module *mod)
>  	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
>  	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>  #endif
> +	/*
> +	 * RO and NX regions should already be protected at this point,
> +	 * so the below module_enable_ro() call enables additional RO
> +	 * protection for the ro_after_init section only.
> +	 */
> +	module_enable_ro(mod);
>  	mod_tree_remove_init(mod);
>  	disable_ro_nx(&mod->init_layout);
>  	module_arch_freeing_init(mod);
>  	mod->init_layout.base = NULL;
>  	mod->init_layout.size = 0;
>  	mod->init_layout.ro_size = 0;
> +	mod->init_layout.ro_after_init_size = 0;
>  	mod->init_layout.text_size = 0;
>  	/*
>  	 * We want to free module_init, but be aware that kallsyms may be
> @@ -3454,7 +3503,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	/* This relies on module_mutex for list integrity. */
>  	module_bug_finalize(info->hdr, info->sechdrs, mod);
>  
> -	/* Set RO and NX regions */
> +	/*
> +	 * Set RO and NX regions. Since module is not LIVE yet,
> +	 * the ro_after_init section will remain RW until after
> +	 * do_one_initcall().
> +	 */
>  	module_enable_ro(mod);
>  	module_enable_nx(mod);
>  
> -- 
> 2.4.3

Re: [PATCH 1/1] modules: add ro_after_init support

[Rusty Russell]

Jessica Yu <jeyu@redhat.com> writes:
> Add ro_after_init support for modules by adding a new page-aligned section
> in the module layout (after rodata) for ro_after_init data and enabling RO
> protection for that section after module init runs.
>
> Signed-off-by: Jessica Yu <jeyu@redhat.com>

I would prefer a "bool after_init" flag to module_enable_ro().  It's
more explicit.

Exposing the flags via uapi looks like a wart, but it's kind of a
feature, since we don't *unset* it in any section; userspace may want to
know about it.

If you could re-spin, that would be great.

Thanks!
Rusty.

> ---
>  include/linux/module.h   |  2 ++
>  include/uapi/linux/elf.h |  1 +
>  kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 66 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index f777164..a698d23 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -311,6 +311,8 @@ struct module_layout {
>  	unsigned int text_size;
>  	/* Size of RO section of the module (text+rodata) */
>  	unsigned int ro_size;
> +	/* Size of RO after init section */
> +	unsigned int ro_after_init_size;
>  
>  #ifdef CONFIG_MODULES_TREE_LOOKUP
>  	struct mod_tree_node mtn;
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index cb4a72f..70b172ba 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -286,6 +286,7 @@ typedef struct elf64_phdr {
>  #define SHF_ALLOC		0x2
>  #define SHF_EXECINSTR		0x4
>  #define SHF_RELA_LIVEPATCH	0x00100000
> +#define SHF_RO_AFTER_INIT	0x00200000
>  #define SHF_MASKPROC		0xf0000000
>  
>  /* special section indexes */
> diff --git a/kernel/module.c b/kernel/module.c
> index 7f21ab2..055bf6f 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1857,10 +1857,11 @@ static void mod_sysfs_teardown(struct module *mod)
>   * from modification and any data from execution.
>   *
>   * General layout of module is:
> - *          [text] [read-only-data] [writable data]
> - * text_size -----^                ^               ^
> - * ro_size ------------------------|               |
> - * size -------------------------------------------|
> + *          [text] [read-only-data] [ro-after-init] [writable data]
> + * text_size -----^                ^               ^               ^
> + * ro_size ------------------------|               |               |
> + * ro_after_init_size -----------------------------|               |
> + * size -----------------------------------------------------------|
>   *
>   * These values are always page-aligned (as is base)
>   */
> @@ -1883,14 +1884,24 @@ static void frob_rodata(const struct module_layout *layout,
>  		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
>  }
>  
> +static void frob_ro_after_init(const struct module_layout *layout,
> +				int (*set_memory)(unsigned long start, int num_pages))
> +{
> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
> +	set_memory((unsigned long)layout->base + layout->ro_size,
> +		   (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
> +}
> +
>  static void frob_writable_data(const struct module_layout *layout,
>  			       int (*set_memory)(unsigned long start, int num_pages))
>  {
>  	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
> -	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>  	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
> -	set_memory((unsigned long)layout->base + layout->ro_size,
> -		   (layout->size - layout->ro_size) >> PAGE_SHIFT);
> +	set_memory((unsigned long)layout->base + layout->ro_after_init_size,
> +		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
>  }
>  
>  /* livepatching wants to disable read-only so it can frob module. */
> @@ -1898,6 +1909,7 @@ void module_disable_ro(const struct module *mod)
>  {
>  	frob_text(&mod->core_layout, set_memory_rw);
>  	frob_rodata(&mod->core_layout, set_memory_rw);
> +	frob_ro_after_init(&mod->core_layout, set_memory_rw);
>  	frob_text(&mod->init_layout, set_memory_rw);
>  	frob_rodata(&mod->init_layout, set_memory_rw);
>  }
> @@ -1908,11 +1920,16 @@ void module_enable_ro(const struct module *mod)
>  	frob_rodata(&mod->core_layout, set_memory_ro);
>  	frob_text(&mod->init_layout, set_memory_ro);
>  	frob_rodata(&mod->init_layout, set_memory_ro);
> +
> +	/* after init */
> +	if (mod->state == MODULE_STATE_LIVE)
> +		frob_ro_after_init(&mod->core_layout, set_memory_ro);
>  }
>  
>  static void module_enable_nx(const struct module *mod)
>  {
>  	frob_rodata(&mod->core_layout, set_memory_nx);
> +	frob_ro_after_init(&mod->core_layout, set_memory_nx);
>  	frob_writable_data(&mod->core_layout, set_memory_nx);
>  	frob_rodata(&mod->init_layout, set_memory_nx);
>  	frob_writable_data(&mod->init_layout, set_memory_nx);
> @@ -1921,6 +1938,7 @@ static void module_enable_nx(const struct module *mod)
>  static void module_disable_nx(const struct module *mod)
>  {
>  	frob_rodata(&mod->core_layout, set_memory_x);
> +	frob_ro_after_init(&mod->core_layout, set_memory_x);
>  	frob_writable_data(&mod->core_layout, set_memory_x);
>  	frob_rodata(&mod->init_layout, set_memory_x);
>  	frob_writable_data(&mod->init_layout, set_memory_x);
> @@ -1963,6 +1981,8 @@ static void disable_ro_nx(const struct module_layout *layout)
>  	frob_text(layout, set_memory_rw);
>  	frob_rodata(layout, set_memory_rw);
>  	frob_rodata(layout, set_memory_x);
> +	frob_ro_after_init(layout, set_memory_rw);
> +	frob_ro_after_init(layout, set_memory_x);
>  	frob_writable_data(layout, set_memory_x);
>  }
>  
> @@ -2305,6 +2325,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
>  		 * finder in the two loops below */
>  		{ SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
>  		{ SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
> +		{ SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL },
>  		{ SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>  		{ ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>  	};
> @@ -2336,7 +2357,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>  			mod->core_layout.size = debug_align(mod->core_layout.size);
>  			mod->core_layout.ro_size = mod->core_layout.size;
>  			break;
> -		case 3: /* whole core */
> +		case 2: /* RO after init */
> +			mod->core_layout.size = debug_align(mod->core_layout.size);
> +			mod->core_layout.ro_after_init_size = mod->core_layout.size;
> +			break;
> +		case 4: /* whole core */
>  			mod->core_layout.size = debug_align(mod->core_layout.size);
>  			break;
>  		}
> @@ -2366,7 +2391,14 @@ static void layout_sections(struct module *mod, struct load_info *info)
>  			mod->init_layout.size = debug_align(mod->init_layout.size);
>  			mod->init_layout.ro_size = mod->init_layout.size;
>  			break;
> -		case 3: /* whole init */
> +		case 2:
> +			/*
> +			 * RO after init doesn't apply to init_layout (only
> +			 * core_layout), so it just takes the value of ro_size.
> +			 */
> +			mod->init_layout.ro_after_init_size = mod->init_layout.ro_size;
> +			break;
> +		case 4: /* whole init */
>  			mod->init_layout.size = debug_align(mod->init_layout.size);
>  			break;
>  		}
> @@ -3172,6 +3204,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>  {
>  	/* Module within temporary copy. */
>  	struct module *mod;
> +	unsigned int ndx;
>  	int err;
>  
>  	mod = setup_load_info(info, flags);
> @@ -3191,6 +3224,15 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>  	/* We will do a special allocation for per-cpu sections later. */
>  	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>  
> +	/*
> +	 * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
> +	 * layout_sections() can put it in the right place.
> +	 * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
> +	 */
> +	ndx = find_sec(info, ".data..ro_after_init");
> +	if (ndx)
> +		info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
> +
>  	/* Determine total sizes, and put offsets in sh_entsize.  For now
>  	   this is done generically; there doesn't appear to be any
>  	   special cases for the architectures. */
> @@ -3357,12 +3399,19 @@ static noinline int do_init_module(struct module *mod)
>  	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
>  	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>  #endif
> +	/*
> +	 * RO and NX regions should already be protected at this point,
> +	 * so the below module_enable_ro() call enables additional RO
> +	 * protection for the ro_after_init section only.
> +	 */
> +	module_enable_ro(mod);
>  	mod_tree_remove_init(mod);
>  	disable_ro_nx(&mod->init_layout);
>  	module_arch_freeing_init(mod);
>  	mod->init_layout.base = NULL;
>  	mod->init_layout.size = 0;
>  	mod->init_layout.ro_size = 0;
> +	mod->init_layout.ro_after_init_size = 0;
>  	mod->init_layout.text_size = 0;
>  	/*
>  	 * We want to free module_init, but be aware that kallsyms may be
> @@ -3454,7 +3503,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	/* This relies on module_mutex for list integrity. */
>  	module_bug_finalize(info->hdr, info->sechdrs, mod);
>  
> -	/* Set RO and NX regions */
> +	/*
> +	 * Set RO and NX regions. Since module is not LIVE yet,
> +	 * the ro_after_init section will remain RW until after
> +	 * do_one_initcall().
> +	 */
>  	module_enable_ro(mod);
>  	module_enable_nx(mod);
>  
> -- 
> 2.4.3

Re: modules: add ro_after_init support

[Jessica Yu]

+++ Rusty Russell [29/06/16 10:38 +0930]:
>Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
>> Add ro_after_init support for modules by adding a new page-aligned section
>> in the module layout (after rodata) for ro_after_init data and enabling RO
>> protection for that section after module init runs.
>>
>> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
>I would prefer a "bool after_init" flag to module_enable_ro().  It's
>more explicit.

Sure thing, I was just initially worried about the
module_{enable,disable}_ro() asymmetry. :)

>Exposing the flags via uapi looks like a wart, but it's kind of a
>feature, since we don't *unset* it in any section; userspace may want to
>know about it.

Hm, I'm still unsure about this. I'm starting to think it might be a
bit overkill to expose SHF_RO_AFTER_INIT through uapi (although that
is where all the other SHF_* flags are defined) SHF_RO_AFTER_INIT
would technically be used only internally in the kernel (i.e. module
loader), and it'd also be considered a non-standard flag, using a bit
from SHF_MASKOS (OS-specific range). What do you think?

>If you could re-spin, that would be great.

Once I figure out where to put SHF_RO_AFTER_INIT, I'll send a v2.

Thanks for the review!
Jessica

>
>> ---
>>  include/linux/module.h   |  2 ++
>>  include/uapi/linux/elf.h |  1 +
>>  kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
>>  3 files changed, 66 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index f777164..a698d23 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -311,6 +311,8 @@ struct module_layout {
>>  	unsigned int text_size;
>>  	/* Size of RO section of the module (text+rodata) */
>>  	unsigned int ro_size;
>> +	/* Size of RO after init section */
>> +	unsigned int ro_after_init_size;
>>
>>  #ifdef CONFIG_MODULES_TREE_LOOKUP
>>  	struct mod_tree_node mtn;
>> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
>> index cb4a72f..70b172ba 100644
>> --- a/include/uapi/linux/elf.h
>> +++ b/include/uapi/linux/elf.h
>> @@ -286,6 +286,7 @@ typedef struct elf64_phdr {
>>  #define SHF_ALLOC		0x2
>>  #define SHF_EXECINSTR		0x4
>>  #define SHF_RELA_LIVEPATCH	0x00100000
>> +#define SHF_RO_AFTER_INIT	0x00200000
>>  #define SHF_MASKPROC		0xf0000000
>>
>>  /* special section indexes */
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 7f21ab2..055bf6f 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1857,10 +1857,11 @@ static void mod_sysfs_teardown(struct module *mod)
>>   * from modification and any data from execution.
>>   *
>>   * General layout of module is:
>> - *          [text] [read-only-data] [writable data]
>> - * text_size -----^                ^               ^
>> - * ro_size ------------------------|               |
>> - * size -------------------------------------------|
>> + *          [text] [read-only-data] [ro-after-init] [writable data]
>> + * text_size -----^                ^               ^               ^
>> + * ro_size ------------------------|               |               |
>> + * ro_after_init_size -----------------------------|               |
>> + * size -----------------------------------------------------------|
>>   *
>>   * These values are always page-aligned (as is base)
>>   */
>> @@ -1883,14 +1884,24 @@ static void frob_rodata(const struct module_layout *layout,
>>  		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
>>  }
>>
>> +static void frob_ro_after_init(const struct module_layout *layout,
>> +				int (*set_memory)(unsigned long start, int num_pages))
>> +{
>> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>> +	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
>> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>> +	set_memory((unsigned long)layout->base + layout->ro_size,
>> +		   (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
>> +}
>> +
>>  static void frob_writable_data(const struct module_layout *layout,
>>  			       int (*set_memory)(unsigned long start, int num_pages))
>>  {
>>  	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>> -	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
>> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>>  	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
>> -	set_memory((unsigned long)layout->base + layout->ro_size,
>> -		   (layout->size - layout->ro_size) >> PAGE_SHIFT);
>> +	set_memory((unsigned long)layout->base + layout->ro_after_init_size,
>> +		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
>>  }
>>
>>  /* livepatching wants to disable read-only so it can frob module. */
>> @@ -1898,6 +1909,7 @@ void module_disable_ro(const struct module *mod)
>>  {
>>  	frob_text(&mod->core_layout, set_memory_rw);
>>  	frob_rodata(&mod->core_layout, set_memory_rw);
>> +	frob_ro_after_init(&mod->core_layout, set_memory_rw);
>>  	frob_text(&mod->init_layout, set_memory_rw);
>>  	frob_rodata(&mod->init_layout, set_memory_rw);
>>  }
>> @@ -1908,11 +1920,16 @@ void module_enable_ro(const struct module *mod)
>>  	frob_rodata(&mod->core_layout, set_memory_ro);
>>  	frob_text(&mod->init_layout, set_memory_ro);
>>  	frob_rodata(&mod->init_layout, set_memory_ro);
>> +
>> +	/* after init */
>> +	if (mod->state == MODULE_STATE_LIVE)
>> +		frob_ro_after_init(&mod->core_layout, set_memory_ro);
>>  }
>>
>>  static void module_enable_nx(const struct module *mod)
>>  {
>>  	frob_rodata(&mod->core_layout, set_memory_nx);
>> +	frob_ro_after_init(&mod->core_layout, set_memory_nx);
>>  	frob_writable_data(&mod->core_layout, set_memory_nx);
>>  	frob_rodata(&mod->init_layout, set_memory_nx);
>>  	frob_writable_data(&mod->init_layout, set_memory_nx);
>> @@ -1921,6 +1938,7 @@ static void module_enable_nx(const struct module *mod)
>>  static void module_disable_nx(const struct module *mod)
>>  {
>>  	frob_rodata(&mod->core_layout, set_memory_x);
>> +	frob_ro_after_init(&mod->core_layout, set_memory_x);
>>  	frob_writable_data(&mod->core_layout, set_memory_x);
>>  	frob_rodata(&mod->init_layout, set_memory_x);
>>  	frob_writable_data(&mod->init_layout, set_memory_x);
>> @@ -1963,6 +1981,8 @@ static void disable_ro_nx(const struct module_layout *layout)
>>  	frob_text(layout, set_memory_rw);
>>  	frob_rodata(layout, set_memory_rw);
>>  	frob_rodata(layout, set_memory_x);
>> +	frob_ro_after_init(layout, set_memory_rw);
>> +	frob_ro_after_init(layout, set_memory_x);
>>  	frob_writable_data(layout, set_memory_x);
>>  }
>>
>> @@ -2305,6 +2325,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>  		 * finder in the two loops below */
>>  		{ SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
>>  		{ SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
>> +		{ SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL },
>>  		{ SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>>  		{ ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>>  	};
>> @@ -2336,7 +2357,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>  			mod->core_layout.size = debug_align(mod->core_layout.size);
>>  			mod->core_layout.ro_size = mod->core_layout.size;
>>  			break;
>> -		case 3: /* whole core */
>> +		case 2: /* RO after init */
>> +			mod->core_layout.size = debug_align(mod->core_layout.size);
>> +			mod->core_layout.ro_after_init_size = mod->core_layout.size;
>> +			break;
>> +		case 4: /* whole core */
>>  			mod->core_layout.size = debug_align(mod->core_layout.size);
>>  			break;
>>  		}
>> @@ -2366,7 +2391,14 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>  			mod->init_layout.size = debug_align(mod->init_layout.size);
>>  			mod->init_layout.ro_size = mod->init_layout.size;
>>  			break;
>> -		case 3: /* whole init */
>> +		case 2:
>> +			/*
>> +			 * RO after init doesn't apply to init_layout (only
>> +			 * core_layout), so it just takes the value of ro_size.
>> +			 */
>> +			mod->init_layout.ro_after_init_size = mod->init_layout.ro_size;
>> +			break;
>> +		case 4: /* whole init */
>>  			mod->init_layout.size = debug_align(mod->init_layout.size);
>>  			break;
>>  		}
>> @@ -3172,6 +3204,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>  {
>>  	/* Module within temporary copy. */
>>  	struct module *mod;
>> +	unsigned int ndx;
>>  	int err;
>>
>>  	mod = setup_load_info(info, flags);
>> @@ -3191,6 +3224,15 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>  	/* We will do a special allocation for per-cpu sections later. */
>>  	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>>
>> +	/*
>> +	 * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
>> +	 * layout_sections() can put it in the right place.
>> +	 * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
>> +	 */
>> +	ndx = find_sec(info, ".data..ro_after_init");
>> +	if (ndx)
>> +		info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
>> +
>>  	/* Determine total sizes, and put offsets in sh_entsize.  For now
>>  	   this is done generically; there doesn't appear to be any
>>  	   special cases for the architectures. */
>> @@ -3357,12 +3399,19 @@ static noinline int do_init_module(struct module *mod)
>>  	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
>>  	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>>  #endif
>> +	/*
>> +	 * RO and NX regions should already be protected at this point,
>> +	 * so the below module_enable_ro() call enables additional RO
>> +	 * protection for the ro_after_init section only.
>> +	 */
>> +	module_enable_ro(mod);
>>  	mod_tree_remove_init(mod);
>>  	disable_ro_nx(&mod->init_layout);
>>  	module_arch_freeing_init(mod);
>>  	mod->init_layout.base = NULL;
>>  	mod->init_layout.size = 0;
>>  	mod->init_layout.ro_size = 0;
>> +	mod->init_layout.ro_after_init_size = 0;
>>  	mod->init_layout.text_size = 0;
>>  	/*
>>  	 * We want to free module_init, but be aware that kallsyms may be
>> @@ -3454,7 +3503,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
>>  	/* This relies on module_mutex for list integrity. */
>>  	module_bug_finalize(info->hdr, info->sechdrs, mod);
>>
>> -	/* Set RO and NX regions */
>> +	/*
>> +	 * Set RO and NX regions. Since module is not LIVE yet,
>> +	 * the ro_after_init section will remain RW until after
>> +	 * do_one_initcall().
>> +	 */
>>  	module_enable_ro(mod);
>>  	module_enable_nx(mod);
>>
>> --
>> 2.4.3

Re: modules: add ro_after_init support

[Jessica Yu]

+++ Rusty Russell [29/06/16 10:38 +0930]:
>Jessica Yu <jeyu@redhat.com> writes:
>> Add ro_after_init support for modules by adding a new page-aligned section
>> in the module layout (after rodata) for ro_after_init data and enabling RO
>> protection for that section after module init runs.
>>
>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>
>I would prefer a "bool after_init" flag to module_enable_ro().  It's
>more explicit.

Sure thing, I was just initially worried about the
module_{enable,disable}_ro() asymmetry. :)

>Exposing the flags via uapi looks like a wart, but it's kind of a
>feature, since we don't *unset* it in any section; userspace may want to
>know about it.

Hm, I'm still unsure about this. I'm starting to think it might be a
bit overkill to expose SHF_RO_AFTER_INIT through uapi (although that
is where all the other SHF_* flags are defined) SHF_RO_AFTER_INIT
would technically be used only internally in the kernel (i.e. module
loader), and it'd also be considered a non-standard flag, using a bit
from SHF_MASKOS (OS-specific range). What do you think?

>If you could re-spin, that would be great.

Once I figure out where to put SHF_RO_AFTER_INIT, I'll send a v2.

Thanks for the review!
Jessica

>
>> ---
>>  include/linux/module.h   |  2 ++
>>  include/uapi/linux/elf.h |  1 +
>>  kernel/module.c          | 73 +++++++++++++++++++++++++++++++++++++++++-------
>>  3 files changed, 66 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index f777164..a698d23 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -311,6 +311,8 @@ struct module_layout {
>>  	unsigned int text_size;
>>  	/* Size of RO section of the module (text+rodata) */
>>  	unsigned int ro_size;
>> +	/* Size of RO after init section */
>> +	unsigned int ro_after_init_size;
>>
>>  #ifdef CONFIG_MODULES_TREE_LOOKUP
>>  	struct mod_tree_node mtn;
>> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
>> index cb4a72f..70b172ba 100644
>> --- a/include/uapi/linux/elf.h
>> +++ b/include/uapi/linux/elf.h
>> @@ -286,6 +286,7 @@ typedef struct elf64_phdr {
>>  #define SHF_ALLOC		0x2
>>  #define SHF_EXECINSTR		0x4
>>  #define SHF_RELA_LIVEPATCH	0x00100000
>> +#define SHF_RO_AFTER_INIT	0x00200000
>>  #define SHF_MASKPROC		0xf0000000
>>
>>  /* special section indexes */
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 7f21ab2..055bf6f 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -1857,10 +1857,11 @@ static void mod_sysfs_teardown(struct module *mod)
>>   * from modification and any data from execution.
>>   *
>>   * General layout of module is:
>> - *          [text] [read-only-data] [writable data]
>> - * text_size -----^                ^               ^
>> - * ro_size ------------------------|               |
>> - * size -------------------------------------------|
>> + *          [text] [read-only-data] [ro-after-init] [writable data]
>> + * text_size -----^                ^               ^               ^
>> + * ro_size ------------------------|               |               |
>> + * ro_after_init_size -----------------------------|               |
>> + * size -----------------------------------------------------------|
>>   *
>>   * These values are always page-aligned (as is base)
>>   */
>> @@ -1883,14 +1884,24 @@ static void frob_rodata(const struct module_layout *layout,
>>  		   (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
>>  }
>>
>> +static void frob_ro_after_init(const struct module_layout *layout,
>> +				int (*set_memory)(unsigned long start, int num_pages))
>> +{
>> +	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>> +	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
>> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>> +	set_memory((unsigned long)layout->base + layout->ro_size,
>> +		   (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
>> +}
>> +
>>  static void frob_writable_data(const struct module_layout *layout,
>>  			       int (*set_memory)(unsigned long start, int num_pages))
>>  {
>>  	BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
>> -	BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
>> +	BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
>>  	BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
>> -	set_memory((unsigned long)layout->base + layout->ro_size,
>> -		   (layout->size - layout->ro_size) >> PAGE_SHIFT);
>> +	set_memory((unsigned long)layout->base + layout->ro_after_init_size,
>> +		   (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
>>  }
>>
>>  /* livepatching wants to disable read-only so it can frob module. */
>> @@ -1898,6 +1909,7 @@ void module_disable_ro(const struct module *mod)
>>  {
>>  	frob_text(&mod->core_layout, set_memory_rw);
>>  	frob_rodata(&mod->core_layout, set_memory_rw);
>> +	frob_ro_after_init(&mod->core_layout, set_memory_rw);
>>  	frob_text(&mod->init_layout, set_memory_rw);
>>  	frob_rodata(&mod->init_layout, set_memory_rw);
>>  }
>> @@ -1908,11 +1920,16 @@ void module_enable_ro(const struct module *mod)
>>  	frob_rodata(&mod->core_layout, set_memory_ro);
>>  	frob_text(&mod->init_layout, set_memory_ro);
>>  	frob_rodata(&mod->init_layout, set_memory_ro);
>> +
>> +	/* after init */
>> +	if (mod->state == MODULE_STATE_LIVE)
>> +		frob_ro_after_init(&mod->core_layout, set_memory_ro);
>>  }
>>
>>  static void module_enable_nx(const struct module *mod)
>>  {
>>  	frob_rodata(&mod->core_layout, set_memory_nx);
>> +	frob_ro_after_init(&mod->core_layout, set_memory_nx);
>>  	frob_writable_data(&mod->core_layout, set_memory_nx);
>>  	frob_rodata(&mod->init_layout, set_memory_nx);
>>  	frob_writable_data(&mod->init_layout, set_memory_nx);
>> @@ -1921,6 +1938,7 @@ static void module_enable_nx(const struct module *mod)
>>  static void module_disable_nx(const struct module *mod)
>>  {
>>  	frob_rodata(&mod->core_layout, set_memory_x);
>> +	frob_ro_after_init(&mod->core_layout, set_memory_x);
>>  	frob_writable_data(&mod->core_layout, set_memory_x);
>>  	frob_rodata(&mod->init_layout, set_memory_x);
>>  	frob_writable_data(&mod->init_layout, set_memory_x);
>> @@ -1963,6 +1981,8 @@ static void disable_ro_nx(const struct module_layout *layout)
>>  	frob_text(layout, set_memory_rw);
>>  	frob_rodata(layout, set_memory_rw);
>>  	frob_rodata(layout, set_memory_x);
>> +	frob_ro_after_init(layout, set_memory_rw);
>> +	frob_ro_after_init(layout, set_memory_x);
>>  	frob_writable_data(layout, set_memory_x);
>>  }
>>
>> @@ -2305,6 +2325,7 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>  		 * finder in the two loops below */
>>  		{ SHF_EXECINSTR | SHF_ALLOC, ARCH_SHF_SMALL },
>>  		{ SHF_ALLOC, SHF_WRITE | ARCH_SHF_SMALL },
>> +		{ SHF_RO_AFTER_INIT | SHF_ALLOC, ARCH_SHF_SMALL },
>>  		{ SHF_WRITE | SHF_ALLOC, ARCH_SHF_SMALL },
>>  		{ ARCH_SHF_SMALL | SHF_ALLOC, 0 }
>>  	};
>> @@ -2336,7 +2357,11 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>  			mod->core_layout.size = debug_align(mod->core_layout.size);
>>  			mod->core_layout.ro_size = mod->core_layout.size;
>>  			break;
>> -		case 3: /* whole core */
>> +		case 2: /* RO after init */
>> +			mod->core_layout.size = debug_align(mod->core_layout.size);
>> +			mod->core_layout.ro_after_init_size = mod->core_layout.size;
>> +			break;
>> +		case 4: /* whole core */
>>  			mod->core_layout.size = debug_align(mod->core_layout.size);
>>  			break;
>>  		}
>> @@ -2366,7 +2391,14 @@ static void layout_sections(struct module *mod, struct load_info *info)
>>  			mod->init_layout.size = debug_align(mod->init_layout.size);
>>  			mod->init_layout.ro_size = mod->init_layout.size;
>>  			break;
>> -		case 3: /* whole init */
>> +		case 2:
>> +			/*
>> +			 * RO after init doesn't apply to init_layout (only
>> +			 * core_layout), so it just takes the value of ro_size.
>> +			 */
>> +			mod->init_layout.ro_after_init_size = mod->init_layout.ro_size;
>> +			break;
>> +		case 4: /* whole init */
>>  			mod->init_layout.size = debug_align(mod->init_layout.size);
>>  			break;
>>  		}
>> @@ -3172,6 +3204,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>  {
>>  	/* Module within temporary copy. */
>>  	struct module *mod;
>> +	unsigned int ndx;
>>  	int err;
>>
>>  	mod = setup_load_info(info, flags);
>> @@ -3191,6 +3224,15 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>  	/* We will do a special allocation for per-cpu sections later. */
>>  	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>>
>> +	/*
>> +	 * Mark ro_after_init section with SHF_RO_AFTER_INIT so that
>> +	 * layout_sections() can put it in the right place.
>> +	 * Note: ro_after_init sections also have SHF_{WRITE,ALLOC} set.
>> +	 */
>> +	ndx = find_sec(info, ".data..ro_after_init");
>> +	if (ndx)
>> +		info->sechdrs[ndx].sh_flags |= SHF_RO_AFTER_INIT;
>> +
>>  	/* Determine total sizes, and put offsets in sh_entsize.  For now
>>  	   this is done generically; there doesn't appear to be any
>>  	   special cases for the architectures. */
>> @@ -3357,12 +3399,19 @@ static noinline int do_init_module(struct module *mod)
>>  	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
>>  	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>>  #endif
>> +	/*
>> +	 * RO and NX regions should already be protected at this point,
>> +	 * so the below module_enable_ro() call enables additional RO
>> +	 * protection for the ro_after_init section only.
>> +	 */
>> +	module_enable_ro(mod);
>>  	mod_tree_remove_init(mod);
>>  	disable_ro_nx(&mod->init_layout);
>>  	module_arch_freeing_init(mod);
>>  	mod->init_layout.base = NULL;
>>  	mod->init_layout.size = 0;
>>  	mod->init_layout.ro_size = 0;
>> +	mod->init_layout.ro_after_init_size = 0;
>>  	mod->init_layout.text_size = 0;
>>  	/*
>>  	 * We want to free module_init, but be aware that kallsyms may be
>> @@ -3454,7 +3503,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
>>  	/* This relies on module_mutex for list integrity. */
>>  	module_bug_finalize(info->hdr, info->sechdrs, mod);
>>
>> -	/* Set RO and NX regions */
>> +	/*
>> +	 * Set RO and NX regions. Since module is not LIVE yet,
>> +	 * the ro_after_init section will remain RW until after
>> +	 * do_one_initcall().
>> +	 */
>>  	module_enable_ro(mod);
>>  	module_enable_nx(mod);
>>
>> --
>> 2.4.3

Re: modules: add ro_after_init support

[Rusty Russell]

Jessica Yu <jeyu@redhat.com> writes:
> +++ Rusty Russell [29/06/16 10:38 +0930]:
>>Jessica Yu <jeyu@redhat.com> writes:
>>> Add ro_after_init support for modules by adding a new page-aligned section
>>> in the module layout (after rodata) for ro_after_init data and enabling RO
>>> protection for that section after module init runs.
>>>
>>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>>
>>I would prefer a "bool after_init" flag to module_enable_ro().  It's
>>more explicit.
>
> Sure thing, I was just initially worried about the
> module_{enable,disable}_ro() asymmetry. :)

Yes, but I think compile-time-analyzable behaviour beats
runtime-analyzable behaviour for clarity.

>>Exposing the flags via uapi looks like a wart, but it's kind of a
>>feature, since we don't *unset* it in any section; userspace may want to
>>know about it.
>
> Hm, I'm still unsure about this. I'm starting to think it might be a
> bit overkill to expose SHF_RO_AFTER_INIT through uapi (although that
> is where all the other SHF_* flags are defined) SHF_RO_AFTER_INIT
> would technically be used only internally in the kernel (i.e. module
> loader), and it'd also be considered a non-standard flag, using a bit
> from SHF_MASKOS (OS-specific range). What do you think?

Some arch *could* use it by setting the flag in a section in their
module I think; we don't stop them.  Since the other flags are there,
I'd leave it.

We don't expose the flags via sysfs, though, so that's the only
exposure.

Thanks!
Rusty.

Re: modules: add ro_after_init support

[Kees Cook]

On Wed, Jun 29, 2016 at 9:56 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Jessica Yu <jeyu@redhat.com> writes:
>> +++ Rusty Russell [29/06/16 10:38 +0930]:
>>>Jessica Yu <jeyu@redhat.com> writes:
>>>> Add ro_after_init support for modules by adding a new page-aligned section
>>>> in the module layout (after rodata) for ro_after_init data and enabling RO
>>>> protection for that section after module init runs.
>>>>
>>>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>>>
>>>I would prefer a "bool after_init" flag to module_enable_ro().  It's
>>>more explicit.
>>
>> Sure thing, I was just initially worried about the
>> module_{enable,disable}_ro() asymmetry. :)
>
> Yes, but I think compile-time-analyzable behaviour beats
> runtime-analyzable behaviour for clarity.
>
>>>Exposing the flags via uapi looks like a wart, but it's kind of a
>>>feature, since we don't *unset* it in any section; userspace may want to
>>>know about it.
>>
>> Hm, I'm still unsure about this. I'm starting to think it might be a
>> bit overkill to expose SHF_RO_AFTER_INIT through uapi (although that
>> is where all the other SHF_* flags are defined) SHF_RO_AFTER_INIT
>> would technically be used only internally in the kernel (i.e. module
>> loader), and it'd also be considered a non-standard flag, using a bit
>> from SHF_MASKOS (OS-specific range). What do you think?
>
> Some arch *could* use it by setting the flag in a section in their
> module I think; we don't stop them.  Since the other flags are there,
> I'd leave it.
>
> We don't expose the flags via sysfs, though, so that's the only
> exposure.

What's the state of this series? I'd love it if the functionality
could land for v4.8...

-Kees

-- 
Kees Cook
Brillo & Chrome OS Security

Re: modules: add ro_after_init support

[Jessica Yu]

+++ Kees Cook [21/07/16 16:03 -0700]:
>On Wed, Jun 29, 2016 at 9:56 PM, Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> wrote:
>> Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
>>> +++ Rusty Russell [29/06/16 10:38 +0930]:
>>>>Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
>>>>> Add ro_after_init support for modules by adding a new page-aligned section
>>>>> in the module layout (after rodata) for ro_after_init data and enabling RO
>>>>> protection for that section after module init runs.
>>>>>
>>>>> Signed-off-by: Jessica Yu <jeyu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>>
>>>>I would prefer a "bool after_init" flag to module_enable_ro().  It's
>>>>more explicit.
>>>
>>> Sure thing, I was just initially worried about the
>>> module_{enable,disable}_ro() asymmetry. :)
>>
>> Yes, but I think compile-time-analyzable behaviour beats
>> runtime-analyzable behaviour for clarity.
>>
>>>>Exposing the flags via uapi looks like a wart, but it's kind of a
>>>>feature, since we don't *unset* it in any section; userspace may want to
>>>>know about it.
>>>
>>> Hm, I'm still unsure about this. I'm starting to think it might be a
>>> bit overkill to expose SHF_RO_AFTER_INIT through uapi (although that
>>> is where all the other SHF_* flags are defined) SHF_RO_AFTER_INIT
>>> would technically be used only internally in the kernel (i.e. module
>>> loader), and it'd also be considered a non-standard flag, using a bit
>>> from SHF_MASKOS (OS-specific range). What do you think?
>>
>> Some arch *could* use it by setting the flag in a section in their
>> module I think; we don't stop them.  Since the other flags are there,
>> I'd leave it.
>>
>> We don't expose the flags via sysfs, though, so that's the only
>> exposure.
>
>What's the state of this series? I'd love it if the functionality
>could land for v4.8...
>

Hi Kees,

Sorry for the delay! Have been busier than usual lately. I'll be able
to get v2 out tomorrow.

Thanks!
Jessica

Re: modules: add ro_after_init support

[Jessica Yu]

+++ Kees Cook [21/07/16 16:03 -0700]:
>On Wed, Jun 29, 2016 at 9:56 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Jessica Yu <jeyu@redhat.com> writes:
>>> +++ Rusty Russell [29/06/16 10:38 +0930]:
>>>>Jessica Yu <jeyu@redhat.com> writes:
>>>>> Add ro_after_init support for modules by adding a new page-aligned section
>>>>> in the module layout (after rodata) for ro_after_init data and enabling RO
>>>>> protection for that section after module init runs.
>>>>>
>>>>> Signed-off-by: Jessica Yu <jeyu@redhat.com>
>>>>
>>>>I would prefer a "bool after_init" flag to module_enable_ro().  It's
>>>>more explicit.
>>>
>>> Sure thing, I was just initially worried about the
>>> module_{enable,disable}_ro() asymmetry. :)
>>
>> Yes, but I think compile-time-analyzable behaviour beats
>> runtime-analyzable behaviour for clarity.
>>
>>>>Exposing the flags via uapi looks like a wart, but it's kind of a
>>>>feature, since we don't *unset* it in any section; userspace may want to
>>>>know about it.
>>>
>>> Hm, I'm still unsure about this. I'm starting to think it might be a
>>> bit overkill to expose SHF_RO_AFTER_INIT through uapi (although that
>>> is where all the other SHF_* flags are defined) SHF_RO_AFTER_INIT
>>> would technically be used only internally in the kernel (i.e. module
>>> loader), and it'd also be considered a non-standard flag, using a bit
>>> from SHF_MASKOS (OS-specific range). What do you think?
>>
>> Some arch *could* use it by setting the flag in a section in their
>> module I think; we don't stop them.  Since the other flags are there,
>> I'd leave it.
>>
>> We don't expose the flags via sysfs, though, so that's the only
>> exposure.
>
>What's the state of this series? I'd love it if the functionality
>could land for v4.8...
>

Hi Kees,

Sorry for the delay! Have been busier than usual lately. I'll be able
to get v2 out tomorrow.

Thanks!
Jessica