[RFC 1/7] arch: add __ro_mostly_after_init section marker

[RFC 1/7] arch: add __ro_mostly_after_init section marker

[Hoeun Ryu]

 After `__ro_after_init` marker is included in kernel, many kernel data
objects can be read-only-after-init. But there are many other places that
would be good to read-only-after-init but `__ro_after_init` can not be simply
applicable to them because they should be writable at some points, which are
during module_init/exit or dynamic de/registration for a specific subsystem.
 `__ro_mostly_after_init` is basically the same to `__ro_after_init`. The
section is mapped as read-only after kernel init. The different thing is
this section is temporarily mapped as read-write during module_init/exit and
de/registration of a subsystem using set_ro_mostly_after_init_rw/ro pair.
 Use `__ro_mostly_after_init` as a way to mark such memory instead when
`__ro_after_init` is not applicable because the memory should be writable
at the described points of time. They are read-only right after kernel init
and writable temporarily only during module_init/exit and dynamic
de/registration for a subsystem.

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
 include/asm-generic/sections.h    |  1 +
 include/asm-generic/vmlinux.lds.h | 10 ++++++++++
 include/linux/cache.h             | 11 +++++++++++
 3 files changed, 22 insertions(+)

diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 4df64a1..16a6f21 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -34,6 +34,7 @@ extern char __bss_start[], __bss_stop[];
 extern char __init_begin[], __init_end[];
 extern char _sinittext[], _einittext[];
 extern char __start_data_ro_after_init[], __end_data_ro_after_init[];
+extern char __start_data_ro_mostly_after_init[], __end_data_ro_mostly_after_init[];
 extern char _end[];
 extern char __per_cpu_load[], __per_cpu_start[], __per_cpu_end[];
 extern char __kprobes_text_start[], __kprobes_text_end[];
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 4e09b28..cc5f44e 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -265,6 +265,15 @@
 	__end_data_ro_after_init = .;
 #endif
 
+#ifndef RO_MOSTLY_AFTER_INIT_DATA
+#define RO_MOSTLY_AFTER_INIT_DATA(align)				\
+	. = ALIGN(align);						\
+	VMLINUX_SYMBOL(__start_data_ro_mostly_after_init) = .;		\
+	*(.data..ro_mostly_after_init)					\
+	. = ALIGN(align);						\
+	VMLINUX_SYMBOL(__end_data_ro_mostly_after_init) = .;
+#endif
+
 /*
  * Read only Data
  */
@@ -275,6 +284,7 @@
 		*(.rodata) *(.rodata.*)					\
 		RO_AFTER_INIT_DATA	/* Read only after init */	\
 		KEEP(*(__vermagic))	/* Kernel version magic */	\
+		RO_MOSTLY_AFTER_INIT_DATA(align)			\
 		. = ALIGN(8);						\
 		VMLINUX_SYMBOL(__start___tracepoints_ptrs) = .;		\
 		KEEP(*(__tracepoints_ptrs)) /* Tracepoints: pointer array */ \
diff --git a/include/linux/cache.h b/include/linux/cache.h
index 1be04f8..fd1cb9b 100644
--- a/include/linux/cache.h
+++ b/include/linux/cache.h
@@ -30,6 +30,17 @@
 #define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
 #endif
 
+/*
+ * __ro_mostly_after_init is almost like __ro_after_init.
+ * but __ro_mostly_after_init section is temporarily writable only during
+ * module_init/exit or dynamic de/registeration of a subsystem using
+ * set_ro_mostly_after_init_rw/ro pair.
+ */
+#ifndef __ro_mostly_after_init
+#define __ro_mostly_after_init \
+	__attribute__((__section__(".data..ro_mostly_after_init")))
+#endif
+
 #ifndef ____cacheline_aligned
 #define ____cacheline_aligned __attribute__((__aligned__(SMP_CACHE_BYTES)))
 #endif
-- 
2.7.4

[RFC 2/7] init: add set_ro_mostly_after_init_rw/ro function

[Hoeun Ryu]

 Add set_ro_mostly_after_init_rw/ro pair to modify memory attributes for
memory marked as `ro_mostly_after_init`.

 I am doubtful that this is the right place where these functions reside and
these functions are suitable for all architectures for memory attributes
modification. Please comment.

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
 include/linux/init.h |  6 ++++++
 init/main.c          | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/linux/init.h b/include/linux/init.h
index 79af096..d68e4f7 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -131,6 +131,12 @@ extern bool rodata_enabled;
 #endif
 #ifdef CONFIG_STRICT_KERNEL_RWX
 void mark_rodata_ro(void);
+
+void set_ro_mostly_after_init_rw(void);
+void set_ro_mostly_after_init_ro(void);
+#else
+static inline void set_ro_mostly_after_init_rw(void) { }
+static inline void set_ro_mostly_after_init_ro(void) { }
 #endif
 
 extern void (*late_time_init)(void);
diff --git a/init/main.c b/init/main.c
index 4719abf..a5d4873 100644
--- a/init/main.c
+++ b/init/main.c
@@ -941,6 +941,30 @@ static void mark_readonly(void)
 	} else
 		pr_info("Kernel memory protection disabled.\n");
 }
+
+void set_ro_mostly_after_init_rw(void)
+{
+	unsigned long start = PFN_ALIGN(__start_data_ro_mostly_after_init);
+	unsigned long end = PFN_ALIGN(&__end_data_ro_mostly_after_init);
+	unsigned long nr_pages = (end - start) >> PAGE_SHIFT;
+
+	if (!rodata_enabled)
+		return;
+
+	set_memory_rw(start, nr_pages);
+}
+
+void set_ro_mostly_after_init_ro(void)
+{
+	unsigned long start = PFN_ALIGN(__start_data_ro_mostly_after_init);
+	unsigned long end = PFN_ALIGN(&__end_data_ro_mostly_after_init);
+	unsigned long nr_pages = (end - start) >> PAGE_SHIFT;
+
+	if (!rodata_enabled)
+		return;
+
+	set_memory_ro(start, nr_pages);
+}
 #else
 static inline void mark_readonly(void)
 {
-- 
2.7.4

[RFC 3/7] module: modify memory attrs for __ro_mostly_after_init during module_init/exit

[Hoeun Ryu]

 `__ro_mostly_after_init` is almost like `__ro_after_init`. The section is
read-only as same as `__ro_after_init` after kernel init. This patch makes
`__ro_mostly_after_init` section read-write temporarily only during
module_init/module_exit.

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
 kernel/module.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 7eba6de..3b25e0e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -987,8 +987,11 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 
 	mutex_unlock(&module_mutex);
 	/* Final destruction now no one is using it. */
-	if (mod->exit != NULL)
+	if (mod->exit != NULL) {
+		set_ro_mostly_after_init_rw();
 		mod->exit();
+		set_ro_mostly_after_init_ro();
+	}
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
 	klp_module_going(mod);
@@ -3396,8 +3399,11 @@ static noinline int do_init_module(struct module *mod)
 
 	do_mod_ctors(mod);
 	/* Start the module */
-	if (mod->init != NULL)
+	if (mod->init != NULL) {
+		set_ro_mostly_after_init_rw();
 		ret = do_one_initcall(mod->init);
+		set_ro_mostly_after_init_ro();
+	}
 	if (ret < 0) {
 		goto fail_free_freeinit;
 	}
-- 
2.7.4

[RFC 4/7] selinux: mark __ro_mostly_after_init for selinux_hooks/selinux_nf_ops

[Hoeun Ryu]

 It would be good that selinux hooks objects are marked as
`__ro_mostly_after_init`. They can not be simply marked as `__ro_after_init'
because they should be writable during selinux_disable procedure.
`__ro_mostly_after_init` section is temporarily read-write during
selinux_disable procedure via set_ro_mostly_after_init_rw/ro pair. Now that
they can be read-only except during the procedure.

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
 security/selinux/hooks.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9a8f12f..64fd799 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6106,7 +6106,7 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 
 #endif
 
-static struct security_hook_list selinux_hooks[] = {
+static struct security_hook_list selinux_hooks[] __ro_mostly_after_init = {
 	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
 	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
 	LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
@@ -6381,7 +6381,7 @@ security_initcall(selinux_init);
 
 #if defined(CONFIG_NETFILTER)
 
-static struct nf_hook_ops selinux_nf_ops[] = {
+static struct nf_hook_ops selinux_nf_ops[] __ro_mostly_after_init = {
 	{
 		.hook =		selinux_ipv4_postroute,
 		.pf =		NFPROTO_IPV4,
@@ -6477,13 +6477,17 @@ int selinux_disable(void)
 	selinux_disabled = 1;
 	selinux_enabled = 0;
 
+	set_ro_mostly_after_init_rw();
 	security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
+	set_ro_mostly_after_init_ro();
 
 	/* Try to destroy the avc node cache */
 	avc_disable();
 
 	/* Unregister netfilter hooks. */
+	set_ro_mostly_after_init_ro();
 	selinux_nf_ip_exit();
+	set_ro_mostly_after_init_rw();
 
 	/* Unregister selinuxfs. */
 	exit_sel_fs();
-- 
2.7.4

[RFC 5/7] cpu: mark ro_mostly_after_init for cpuhp_ap/bp_states

[Hoeun Ryu]

It would be good that `__ro_mostly_after_init` is marked to cpuhp state
objects. They can not be simply marked as `__ro_after_init` because they
should be writable during module_init/exit. Now that they can be read-only
except during module_init/exit

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
 kernel/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 0a5f630..12ad4c2 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1126,7 +1126,7 @@ core_initcall(cpu_hotplug_pm_sync_init);
 #endif /* CONFIG_SMP */
 
 /* Boot processor state steps */
-static struct cpuhp_step cpuhp_bp_states[] = {
+static struct cpuhp_step cpuhp_bp_states[] __ro_mostly_after_init = {
 	[CPUHP_OFFLINE] = {
 		.name			= "offline",
 		.startup.single		= NULL,
@@ -1212,7 +1212,7 @@ static struct cpuhp_step cpuhp_bp_states[] = {
 };
 
 /* Application processor state steps */
-static struct cpuhp_step cpuhp_ap_states[] = {
+static struct cpuhp_step cpuhp_ap_states[] __ro_mostly_after_init = {
 #ifdef CONFIG_SMP
 	/* Final state before CPU kills itself */
 	[CPUHP_AP_IDLE_DEAD] = {
-- 
2.7.4

[RFC 6/7] arm64: add __map_kernel_segment to accept additional vm flags

[Hoeun Ryu]

 Memory attribute for `__ro_mostly_after_init` section should be changed
via set_memory_rw/ro that doesn't work against vm areas which don't have
VM_ALLOC. Add this function to map `__ro_mostly_after_init` section with
VM_ALLOC flag set in map_kernel.

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
 arch/arm64/mm/mmu.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index b805c01..91271b1 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -444,8 +444,10 @@ void mark_rodata_ro(void)
 	debug_checkwx();
 }
 
-static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
-				      pgprot_t prot, struct vm_struct *vma)
+static void __init __map_kernel_segment(pgd_t *pgd,
+					void *va_start, void *va_end,
+					pgprot_t prot, struct vm_struct *vma,
+					unsigned long flags)
 {
 	phys_addr_t pa_start = __pa_symbol(va_start);
 	unsigned long size = va_end - va_start;
@@ -459,12 +461,18 @@ static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
 	vma->addr	= va_start;
 	vma->phys_addr	= pa_start;
 	vma->size	= size;
-	vma->flags	= VM_MAP;
+	vma->flags	= flags;
 	vma->caller	= __builtin_return_address(0);
 
 	vm_area_add_early(vma);
 }
 
+static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
+				      pgprot_t prot, struct vm_struct *vma)
+{
+	return __map_kernel_segment(pgd, va_start, va_end, prot, vma, VM_MAP);
+}
+
 /*
  * Create fine-grained mappings for the kernel.
  */
-- 
2.7.4

[RFC 7/7] arm64: map seperately rodata sections for __ro_mostly_after_init section

[Hoeun Ryu]

Map rodata sections seperately for the new __ro_mostly_after_init section.
Attribute of memory for __ro_mostly_after_init section can be changed later
so we need a dedicated vmalloced region for set_memory_rw/ro api.

Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
---
 arch/arm64/mm/mmu.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 91271b1..4a89a2e 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -434,8 +434,22 @@ void mark_rodata_ro(void)
 	 * mark .rodata as read only. Use __init_begin rather than __end_rodata
 	 * to cover NOTES and EXCEPTION_TABLE.
 	 */
-	section_size = (unsigned long)__init_begin - (unsigned long)__start_rodata;
-	create_mapping_late(__pa_symbol(__start_rodata), (unsigned long)__start_rodata,
+	section_size = (unsigned long)__start_data_ro_mostly_after_init -
+		(unsigned long)__start_rodata;
+	create_mapping_late(__pa_symbol(__start_rodata),
+			    (unsigned long)__start_rodata,
+			    section_size, PAGE_KERNEL_RO);
+
+	section_size = (unsigned long)__end_data_ro_mostly_after_init -
+		(unsigned long)__start_data_ro_mostly_after_init;
+	create_mapping_late(__pa_symbol(__start_data_ro_mostly_after_init),
+			    (unsigned long)__start_data_ro_mostly_after_init,
+			    section_size, PAGE_KERNEL_RO);
+
+	section_size = (unsigned long)__init_begin -
+		(unsigned long)__end_data_ro_mostly_after_init;
+	create_mapping_late(__pa_symbol(__end_data_ro_mostly_after_init),
+			    (unsigned long)__end_data_ro_mostly_after_init,
 			    section_size, PAGE_KERNEL_RO);
 
 	/* flush the TLBs after updating live kernel mappings */
@@ -478,10 +492,18 @@ static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
  */
 static void __init map_kernel(pgd_t *pgd)
 {
-	static struct vm_struct vmlinux_text, vmlinux_rodata, vmlinux_init, vmlinux_data;
+	static struct vm_struct vmlinux_text, vmlinux_rodata1, vmlinux_rodata2, vmlinux_ro_mostly_after_init, vmlinux_init, vmlinux_data;
 
 	map_kernel_segment(pgd, _text, _etext, PAGE_KERNEL_EXEC, &vmlinux_text);
-	map_kernel_segment(pgd, __start_rodata, __init_begin, PAGE_KERNEL, &vmlinux_rodata);
+	map_kernel_segment(pgd, __start_rodata, __start_data_ro_mostly_after_init, PAGE_KERNEL, &vmlinux_rodata1);
+	__map_kernel_segment(pgd,
+			     __start_data_ro_mostly_after_init,
+			     __end_data_ro_mostly_after_init,
+			     PAGE_KERNEL,
+			     &vmlinux_ro_mostly_after_init,
+			     VM_MAP | VM_ALLOC);
+	map_kernel_segment(pgd, __end_data_ro_mostly_after_init, __init_begin, PAGE_KERNEL, &vmlinux_rodata2);
+
 	map_kernel_segment(pgd, __init_begin, __init_end, PAGE_KERNEL_EXEC,
 			   &vmlinux_init);
 	map_kernel_segment(pgd, _data, _end, PAGE_KERNEL, &vmlinux_data);
-- 
2.7.4

Re: [RFC 6/7] arm64: add __map_kernel_segment to accept additional vm flags

[Ard Biesheuvel]

On 19 February 2017 at 10:04, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>  Memory attribute for `__ro_mostly_after_init` section should be changed
> via set_memory_rw/ro that doesn't work against vm areas which don't have
> VM_ALLOC.

This is for a good reason: VMALLOC regions are guaranteed to be mapped
down to pages, which is the only thing set_memory_rX() supports. On
arm64, kernel segments may be mapped using contiguous page mappings or
block mappings, which cannot be modified like this.

> Add this function to map `__ro_mostly_after_init` section with
> VM_ALLOC flag set in map_kernel.
>

If we add this functionality, I'd rather we have a special handler for
the __ro_mostly_after_init section and not use set_memory_rX().

> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> ---
>  arch/arm64/mm/mmu.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index b805c01..91271b1 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -444,8 +444,10 @@ void mark_rodata_ro(void)
>         debug_checkwx();
>  }
>
> -static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
> -                                     pgprot_t prot, struct vm_struct *vma)
> +static void __init __map_kernel_segment(pgd_t *pgd,
> +                                       void *va_start, void *va_end,
> +                                       pgprot_t prot, struct vm_struct *vma,
> +                                       unsigned long flags)
>  {
>         phys_addr_t pa_start = __pa_symbol(va_start);
>         unsigned long size = va_end - va_start;
> @@ -459,12 +461,18 @@ static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
>         vma->addr       = va_start;
>         vma->phys_addr  = pa_start;
>         vma->size       = size;
> -       vma->flags      = VM_MAP;
> +       vma->flags      = flags;
>         vma->caller     = __builtin_return_address(0);
>
>         vm_area_add_early(vma);
>  }
>
> +static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
> +                                     pgprot_t prot, struct vm_struct *vma)
> +{
> +       return __map_kernel_segment(pgd, va_start, va_end, prot, vma, VM_MAP);
> +}
> +
>  /*
>   * Create fine-grained mappings for the kernel.
>   */
> --
> 2.7.4
>

Re: [kernel-hardening] [RFC 1/7] arch: add __ro_mostly_after_init section marker

[Ard Biesheuvel]

On 19 February 2017 at 10:04, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>  After `__ro_after_init` marker is included in kernel, many kernel data
> objects can be read-only-after-init. But there are many other places that
> would be good to read-only-after-init but `__ro_after_init` can not be simply
> applicable to them because they should be writable at some points, which are
> during module_init/exit or dynamic de/registration for a specific subsystem.
>  `__ro_mostly_after_init` is basically the same to `__ro_after_init`. The
> section is mapped as read-only after kernel init. The different thing is
> this section is temporarily mapped as read-write during module_init/exit and
> de/registration of a subsystem using set_ro_mostly_after_init_rw/ro pair.
>  Use `__ro_mostly_after_init` as a way to mark such memory instead when
> `__ro_after_init` is not applicable because the memory should be writable
> at the described points of time. They are read-only right after kernel init
> and writable temporarily only during module_init/exit and dynamic
> de/registration for a subsystem.
>
> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> ---
>  include/asm-generic/sections.h    |  1 +
>  include/asm-generic/vmlinux.lds.h | 10 ++++++++++
>  include/linux/cache.h             | 11 +++++++++++
>  3 files changed, 22 insertions(+)
>
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 4df64a1..16a6f21 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -34,6 +34,7 @@ extern char __bss_start[], __bss_stop[];
>  extern char __init_begin[], __init_end[];
>  extern char _sinittext[], _einittext[];
>  extern char __start_data_ro_after_init[], __end_data_ro_after_init[];
> +extern char __start_data_ro_mostly_after_init[], __end_data_ro_mostly_after_init[];
>  extern char _end[];
>  extern char __per_cpu_load[], __per_cpu_start[], __per_cpu_end[];
>  extern char __kprobes_text_start[], __kprobes_text_end[];
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 4e09b28..cc5f44e 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -265,6 +265,15 @@
>         __end_data_ro_after_init = .;
>  #endif
>
> +#ifndef RO_MOSTLY_AFTER_INIT_DATA
> +#define RO_MOSTLY_AFTER_INIT_DATA(align)                               \
> +       . = ALIGN(align);                                               \
> +       VMLINUX_SYMBOL(__start_data_ro_mostly_after_init) = .;          \
> +       *(.data..ro_mostly_after_init)                                  \
> +       . = ALIGN(align);                                               \
> +       VMLINUX_SYMBOL(__end_data_ro_mostly_after_init) = .;
> +#endif
> +
>  /*
>   * Read only Data
>   */
> @@ -275,6 +284,7 @@
>                 *(.rodata) *(.rodata.*)                                 \
>                 RO_AFTER_INIT_DATA      /* Read only after init */      \
>                 KEEP(*(__vermagic))     /* Kernel version magic */      \
> +               RO_MOSTLY_AFTER_INIT_DATA(align)                        \

You can't really drop this in the middle of a section like this. On
arm64, we try very hard to use a minimal segment alignment of 64 KB
(of 2 MB if DEBUG_ALIGN_RODATA=y), to ensure that the TLB footprint of
the kernel image is minimized.

So this should be a separate section in the arm64 linker script.

>                 . = ALIGN(8);                                           \
>                 VMLINUX_SYMBOL(__start___tracepoints_ptrs) = .;         \
>                 KEEP(*(__tracepoints_ptrs)) /* Tracepoints: pointer array */ \
> diff --git a/include/linux/cache.h b/include/linux/cache.h
> index 1be04f8..fd1cb9b 100644
> --- a/include/linux/cache.h
> +++ b/include/linux/cache.h
> @@ -30,6 +30,17 @@
>  #define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
>  #endif
>
> +/*
> + * __ro_mostly_after_init is almost like __ro_after_init.
> + * but __ro_mostly_after_init section is temporarily writable only during
> + * module_init/exit or dynamic de/registeration of a subsystem using
> + * set_ro_mostly_after_init_rw/ro pair.
> + */
> +#ifndef __ro_mostly_after_init
> +#define __ro_mostly_after_init \
> +       __attribute__((__section__(".data..ro_mostly_after_init")))
> +#endif
> +
>  #ifndef ____cacheline_aligned
>  #define ____cacheline_aligned __attribute__((__aligned__(SMP_CACHE_BYTES)))
>  #endif
> --
> 2.7.4
>

Re: [RFC 7/7] arm64: map seperately rodata sections for __ro_mostly_after_init section

[Ard Biesheuvel]

On 19 February 2017 at 10:04, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
> Map rodata sections seperately for the new __ro_mostly_after_init section.
> Attribute of memory for __ro_mostly_after_init section can be changed later
> so we need a dedicated vmalloced region for set_memory_rw/ro api.
>
> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> ---
>  arch/arm64/mm/mmu.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 91271b1..4a89a2e 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -434,8 +434,22 @@ void mark_rodata_ro(void)
>          * mark .rodata as read only. Use __init_begin rather than __end_rodata
>          * to cover NOTES and EXCEPTION_TABLE.
>          */
> -       section_size = (unsigned long)__init_begin - (unsigned long)__start_rodata;
> -       create_mapping_late(__pa_symbol(__start_rodata), (unsigned long)__start_rodata,
> +       section_size = (unsigned long)__start_data_ro_mostly_after_init -
> +               (unsigned long)__start_rodata;
> +       create_mapping_late(__pa_symbol(__start_rodata),
> +                           (unsigned long)__start_rodata,
> +                           section_size, PAGE_KERNEL_RO);
> +
> +       section_size = (unsigned long)__end_data_ro_mostly_after_init -
> +               (unsigned long)__start_data_ro_mostly_after_init;
> +       create_mapping_late(__pa_symbol(__start_data_ro_mostly_after_init),
> +                           (unsigned long)__start_data_ro_mostly_after_init,
> +                           section_size, PAGE_KERNEL_RO);
> +
> +       section_size = (unsigned long)__init_begin -
> +               (unsigned long)__end_data_ro_mostly_after_init;
> +       create_mapping_late(__pa_symbol(__end_data_ro_mostly_after_init),
> +                           (unsigned long)__end_data_ro_mostly_after_init,
>                             section_size, PAGE_KERNEL_RO);
>
>         /* flush the TLBs after updating live kernel mappings */
> @@ -478,10 +492,18 @@ static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
>   */
>  static void __init map_kernel(pgd_t *pgd)
>  {
> -       static struct vm_struct vmlinux_text, vmlinux_rodata, vmlinux_init, vmlinux_data;
> +       static struct vm_struct vmlinux_text, vmlinux_rodata1, vmlinux_rodata2, vmlinux_ro_mostly_after_init, vmlinux_init, vmlinux_data;
>
>         map_kernel_segment(pgd, _text, _etext, PAGE_KERNEL_EXEC, &vmlinux_text);
> -       map_kernel_segment(pgd, __start_rodata, __init_begin, PAGE_KERNEL, &vmlinux_rodata);
> +       map_kernel_segment(pgd, __start_rodata, __start_data_ro_mostly_after_init, PAGE_KERNEL, &vmlinux_rodata1);
> +       __map_kernel_segment(pgd,
> +                            __start_data_ro_mostly_after_init,
> +                            __end_data_ro_mostly_after_init,
> +                            PAGE_KERNEL,
> +                            &vmlinux_ro_mostly_after_init,
> +                            VM_MAP | VM_ALLOC);
> +       map_kernel_segment(pgd, __end_data_ro_mostly_after_init, __init_begin, PAGE_KERNEL, &vmlinux_rodata2);
> +
>         map_kernel_segment(pgd, __init_begin, __init_end, PAGE_KERNEL_EXEC,
>                            &vmlinux_init);
>         map_kernel_segment(pgd, _data, _end, PAGE_KERNEL, &vmlinux_data);
> --
> 2.7.4
>

While it is correct that you are splitting this into three separate
segments (otherwise we would not be able to change the permissions
later without risking splitting to occur), I think this leads to
unnecessary fragmentation.

If there is demand for this feature (but you still need to make the
argument for that), I wonder if it wouldn't be sufficient, and much
more straightforward, to redefine the __ro_after_init semantics to
include the kind of subsystem registration and module init context you
are targeting, and implement some hooks to temporarily lift the
__ro_after_init r/o permission restrictions in a controlled manner.

Kees: any thoughts?

Re: [RFC 5/7] cpu: mark ro_mostly_after_init for cpuhp_ap/bp_states

[Sebastian Andrzej Siewior]

On 2017-02-19 19:04:08 [+0900], Hoeun Ryu wrote:
> It would be good that `__ro_mostly_after_init` is marked to cpuhp state
> objects. 
why?

Sebastian

Re: [kernel-hardening] [RFC 2/7] init: add set_ro_mostly_after_init_rw/ro function

[Mark Rutland]

On Sun, Feb 19, 2017 at 07:04:05PM +0900, Hoeun Ryu wrote:
>  Add set_ro_mostly_after_init_rw/ro pair to modify memory attributes for
> memory marked as `ro_mostly_after_init`.
> 
>  I am doubtful that this is the right place where these functions reside and
> these functions are suitable for all architectures for memory attributes
> modification. Please comment.

These won't work for arm64, since set_memory_* only work on
page-granular mappings in the vmalloc area.

The "real" kernel mappings can use larger block mappings, and would need
to be split (which cannot be done at runtime) before permissions could
be changed at page granularity.

Thanks,
Mark.

> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> ---
>  include/linux/init.h |  6 ++++++
>  init/main.c          | 24 ++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 79af096..d68e4f7 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -131,6 +131,12 @@ extern bool rodata_enabled;
>  #endif
>  #ifdef CONFIG_STRICT_KERNEL_RWX
>  void mark_rodata_ro(void);
> +
> +void set_ro_mostly_after_init_rw(void);
> +void set_ro_mostly_after_init_ro(void);
> +#else
> +static inline void set_ro_mostly_after_init_rw(void) { }
> +static inline void set_ro_mostly_after_init_ro(void) { }
>  #endif
>  
>  extern void (*late_time_init)(void);
> diff --git a/init/main.c b/init/main.c
> index 4719abf..a5d4873 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -941,6 +941,30 @@ static void mark_readonly(void)
>  	} else
>  		pr_info("Kernel memory protection disabled.\n");
>  }
> +
> +void set_ro_mostly_after_init_rw(void)
> +{
> +	unsigned long start = PFN_ALIGN(__start_data_ro_mostly_after_init);
> +	unsigned long end = PFN_ALIGN(&__end_data_ro_mostly_after_init);
> +	unsigned long nr_pages = (end - start) >> PAGE_SHIFT;
> +
> +	if (!rodata_enabled)
> +		return;
> +
> +	set_memory_rw(start, nr_pages);
> +}
> +
> +void set_ro_mostly_after_init_ro(void)
> +{
> +	unsigned long start = PFN_ALIGN(__start_data_ro_mostly_after_init);
> +	unsigned long end = PFN_ALIGN(&__end_data_ro_mostly_after_init);
> +	unsigned long nr_pages = (end - start) >> PAGE_SHIFT;
> +
> +	if (!rodata_enabled)
> +		return;
> +
> +	set_memory_ro(start, nr_pages);
> +}
>  #else
>  static inline void mark_readonly(void)
>  {
> -- 
> 2.7.4
> 

Re: [kernel-hardening] [RFC 3/7] module: modify memory attrs for __ro_mostly_after_init during module_init/exit

[Mark Rutland]

On Sun, Feb 19, 2017 at 07:04:06PM +0900, Hoeun Ryu wrote:
>  `__ro_mostly_after_init` is almost like `__ro_after_init`. The section is
> read-only as same as `__ro_after_init` after kernel init. This patch makes
> `__ro_mostly_after_init` section read-write temporarily only during
> module_init/module_exit.
> 
> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
> ---
>  kernel/module.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 7eba6de..3b25e0e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -987,8 +987,11 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  
>  	mutex_unlock(&module_mutex);
>  	/* Final destruction now no one is using it. */
> -	if (mod->exit != NULL)
> +	if (mod->exit != NULL) {
> +		set_ro_mostly_after_init_rw();
>  		mod->exit();
> +		set_ro_mostly_after_init_ro();
> +	}
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
>  	klp_module_going(mod);
> @@ -3396,8 +3399,11 @@ static noinline int do_init_module(struct module *mod)
>  
>  	do_mod_ctors(mod);
>  	/* Start the module */
> -	if (mod->init != NULL)
> +	if (mod->init != NULL) {
> +		set_ro_mostly_after_init_rw();
>  		ret = do_one_initcall(mod->init);
> +		set_ro_mostly_after_init_ro();
> +	}

This looks very much like the pax_{open,close}_kernel() approach for
write-rarely data.

I think it would be better to implement a first class write-rarely
mechanism rather than trying to extend __ro_after_init to cover this
case.

As mentioned previously, I *think* we can have a generic implementation
that uses an mm to temporarily map a (thread/cpu-local) RW alias of the
data in question in what would otherwise be the user half of the address
space. Regardless, we can have a generic interface [1] that can cater
for that style of approach and/or something like ARM's domains or x86's
pkeys.

Thanks,
Mark.

[1] http://www.openwall.com/lists/kernel-hardening/2016/11/18/3

Re: [RFC 7/7] arm64: map seperately rodata sections for __ro_mostly_after_init section

[Mark Rutland]

On Sun, Feb 19, 2017 at 11:35:51AM +0000, Ard Biesheuvel wrote:
> On 19 February 2017 at 10:04, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
> > Map rodata sections seperately for the new __ro_mostly_after_init section.
> > Attribute of memory for __ro_mostly_after_init section can be changed later
> > so we need a dedicated vmalloced region for set_memory_rw/ro api.

> While it is correct that you are splitting this into three separate
> segments (otherwise we would not be able to change the permissions
> later without risking splitting to occur), I think this leads to
> unnecessary fragmentation.
> 
> If there is demand for this feature (but you still need to make the
> argument for that), I wonder if it wouldn't be sufficient, and much
> more straightforward, to redefine the __ro_after_init semantics to
> include the kind of subsystem registration and module init context you
> are targeting, and implement some hooks to temporarily lift the
> __ro_after_init r/o permission restrictions in a controlled manner.

>From a look over the series, I think this is just __write_rarely in
disguise. I personally think that we should keep __write_rarely and
__ro_after_init separate, the later being a strictly one-shot affair.

I had some ideas [1] as to how we could implement __write_rarely without
carving up the kernel mapping further (and keeping the RW permissions
local to the thread needing it), but I have not had the time to look
into that further.

Thanks,
Mark.

[1] http://www.openwall.com/lists/kernel-hardening/2016/11/18/3

Re: [RFC 5/7] cpu: mark ro_mostly_after_init for cpuhp_ap/bp_states

[Ho-Eun Ryu]

> On 20 Feb 2017, at 5:20 PM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> On 2017-02-19 19:04:08 [+0900], Hoeun Ryu wrote:
>> It would be good that `__ro_mostly_after_init` is marked to cpuhp state
>> objects. 
> why?
> 

I’m requesting for comments of a new feature called __ro_mostly_after_init section marker.
It’s similar to __ro_after_init, but the section can be writable for some point of time.
Please see the cover letter [1] and the first patch [2].

[1] : https://lkml.org/lkml/2017/2/19/29
[2] : https://lkml.org/lkml/2017/2/19/32

> Sebastian

Re: [kernel-hardening] [RFC 1/7] arch: add __ro_mostly_after_init section marker

[Ho-Eun Ryu]

> On 19 Feb 2017, at 8:24 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> On 19 February 2017 at 10:04, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>> After `__ro_after_init` marker is included in kernel, many kernel data
>> objects can be read-only-after-init. But there are many other places that
>> would be good to read-only-after-init but `__ro_after_init` can not be simply
>> applicable to them because they should be writable at some points, which are
>> during module_init/exit or dynamic de/registration for a specific subsystem.
>> `__ro_mostly_after_init` is basically the same to `__ro_after_init`. The
>> section is mapped as read-only after kernel init. The different thing is
>> this section is temporarily mapped as read-write during module_init/exit and
>> de/registration of a subsystem using set_ro_mostly_after_init_rw/ro pair.
>> Use `__ro_mostly_after_init` as a way to mark such memory instead when
>> `__ro_after_init` is not applicable because the memory should be writable
>> at the described points of time. They are read-only right after kernel init
>> and writable temporarily only during module_init/exit and dynamic
>> de/registration for a subsystem.
>> 
>> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
>> ---
>> include/asm-generic/sections.h    |  1 +
>> include/asm-generic/vmlinux.lds.h | 10 ++++++++++
>> include/linux/cache.h             | 11 +++++++++++
>> 3 files changed, 22 insertions(+)
>> 
>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>> index 4df64a1..16a6f21 100644
>> --- a/include/asm-generic/sections.h
>> +++ b/include/asm-generic/sections.h
>> @@ -34,6 +34,7 @@ extern char __bss_start[], __bss_stop[];
>> extern char __init_begin[], __init_end[];
>> extern char _sinittext[], _einittext[];
>> extern char __start_data_ro_after_init[], __end_data_ro_after_init[];
>> +extern char __start_data_ro_mostly_after_init[], __end_data_ro_mostly_after_init[];
>> extern char _end[];
>> extern char __per_cpu_load[], __per_cpu_start[], __per_cpu_end[];
>> extern char __kprobes_text_start[], __kprobes_text_end[];
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index 4e09b28..cc5f44e 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -265,6 +265,15 @@
>>        __end_data_ro_after_init = .;
>> #endif
>> 
>> +#ifndef RO_MOSTLY_AFTER_INIT_DATA
>> +#define RO_MOSTLY_AFTER_INIT_DATA(align)                               \
>> +       . = ALIGN(align);                                               \
>> +       VMLINUX_SYMBOL(__start_data_ro_mostly_after_init) = .;          \
>> +       *(.data..ro_mostly_after_init)                                  \
>> +       . = ALIGN(align);                                               \
>> +       VMLINUX_SYMBOL(__end_data_ro_mostly_after_init) = .;
>> +#endif
>> +
>> /*
>>  * Read only Data
>>  */
>> @@ -275,6 +284,7 @@
>>                *(.rodata) *(.rodata.*)                                 \
>>                RO_AFTER_INIT_DATA      /* Read only after init */      \
>>                KEEP(*(__vermagic))     /* Kernel version magic */      \
>> +               RO_MOSTLY_AFTER_INIT_DATA(align)                        \
> 
> You can't really drop this in the middle of a section like this. On
> arm64, we try very hard to use a minimal segment alignment of 64 KB
> (of 2 MB if DEBUG_ALIGN_RODATA=y), to ensure that the TLB footprint of
> the kernel image is minimized.
> 
> So this should be a separate section in the arm64 linker script.
> 

Could we achieve that using ALIGN(SECTION_SIZE) not ALIGN(PAGE_SIZE) for RO_DATA ?

>>                . = ALIGN(8);                                           \
>>                VMLINUX_SYMBOL(__start___tracepoints_ptrs) = .;         \
>>                KEEP(*(__tracepoints_ptrs)) /* Tracepoints: pointer array */ \
>> diff --git a/include/linux/cache.h b/include/linux/cache.h
>> index 1be04f8..fd1cb9b 100644
>> --- a/include/linux/cache.h
>> +++ b/include/linux/cache.h
>> @@ -30,6 +30,17 @@
>> #define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
>> #endif
>> 
>> +/*
>> + * __ro_mostly_after_init is almost like __ro_after_init.
>> + * but __ro_mostly_after_init section is temporarily writable only during
>> + * module_init/exit or dynamic de/registeration of a subsystem using
>> + * set_ro_mostly_after_init_rw/ro pair.
>> + */
>> +#ifndef __ro_mostly_after_init
>> +#define __ro_mostly_after_init \
>> +       __attribute__((__section__(".data..ro_mostly_after_init")))
>> +#endif
>> +
>> #ifndef ____cacheline_aligned
>> #define ____cacheline_aligned __attribute__((__aligned__(SMP_CACHE_BYTES)))
>> #endif
>> --
>> 2.7.4

Re: [kernel-hardening] [RFC 2/7] init: add set_ro_mostly_after_init_rw/ro function

[Ho-Eun Ryu]

> On 20 Feb 2017, at 7:22 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Sun, Feb 19, 2017 at 07:04:05PM +0900, Hoeun Ryu wrote:
>> Add set_ro_mostly_after_init_rw/ro pair to modify memory attributes for
>> memory marked as `ro_mostly_after_init`.
>> 
>> I am doubtful that this is the right place where these functions reside and
>> these functions are suitable for all architectures for memory attributes
>> modification. Please comment.
> 
> These won't work for arm64, since set_memory_* only work on
> page-granular mappings in the vmalloc area.
> 
> The "real" kernel mappings can use larger block mappings, and would need
> to be split (which cannot be done at runtime) before permissions could
> be changed at page granularity.

So I sent RFC 6/7 [1] and 7/7 [2] that splits the block mapping to the page granular.
I think you and Ard Biesheuvel don’t like it anyway.

[1] : https://lkml.org/lkml/2017/2/19/38
[2] : https://lkml.org/lkml/2017/2/19/39

> 
> Thanks,
> Mark.
> 
>> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
>> ---
>> include/linux/init.h |  6 ++++++
>> init/main.c          | 24 ++++++++++++++++++++++++
>> 2 files changed, 30 insertions(+)
>> 
>> diff --git a/include/linux/init.h b/include/linux/init.h
>> index 79af096..d68e4f7 100644
>> --- a/include/linux/init.h
>> +++ b/include/linux/init.h
>> @@ -131,6 +131,12 @@ extern bool rodata_enabled;
>> #endif
>> #ifdef CONFIG_STRICT_KERNEL_RWX
>> void mark_rodata_ro(void);
>> +
>> +void set_ro_mostly_after_init_rw(void);
>> +void set_ro_mostly_after_init_ro(void);
>> +#else
>> +static inline void set_ro_mostly_after_init_rw(void) { }
>> +static inline void set_ro_mostly_after_init_ro(void) { }
>> #endif
>> 
>> extern void (*late_time_init)(void);
>> diff --git a/init/main.c b/init/main.c
>> index 4719abf..a5d4873 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -941,6 +941,30 @@ static void mark_readonly(void)
>> 	} else
>> 		pr_info("Kernel memory protection disabled.\n");
>> }
>> +
>> +void set_ro_mostly_after_init_rw(void)
>> +{
>> +	unsigned long start = PFN_ALIGN(__start_data_ro_mostly_after_init);
>> +	unsigned long end = PFN_ALIGN(&__end_data_ro_mostly_after_init);
>> +	unsigned long nr_pages = (end - start) >> PAGE_SHIFT;
>> +
>> +	if (!rodata_enabled)
>> +		return;
>> +
>> +	set_memory_rw(start, nr_pages);
>> +}
>> +
>> +void set_ro_mostly_after_init_ro(void)
>> +{
>> +	unsigned long start = PFN_ALIGN(__start_data_ro_mostly_after_init);
>> +	unsigned long end = PFN_ALIGN(&__end_data_ro_mostly_after_init);
>> +	unsigned long nr_pages = (end - start) >> PAGE_SHIFT;
>> +
>> +	if (!rodata_enabled)
>> +		return;
>> +
>> +	set_memory_ro(start, nr_pages);
>> +}
>> #else
>> static inline void mark_readonly(void)
>> {
>> -- 
>> 2.7.4
>> 

Re: [RFC 4/7] selinux: mark __ro_mostly_after_init for selinux_hooks/selinux_nf_ops

[Tetsuo Handa]

On 2017/02/19 19:04, Hoeun Ryu wrote:
>  It would be good that selinux hooks objects are marked as
> `__ro_mostly_after_init`. They can not be simply marked as `__ro_after_init'
> because they should be writable during selinux_disable procedure.
> `__ro_mostly_after_init` section is temporarily read-write during
> selinux_disable procedure via set_ro_mostly_after_init_rw/ro pair. Now that
> they can be read-only except during the procedure.
> 
> -static struct security_hook_list selinux_hooks[] = {
> +static struct security_hook_list selinux_hooks[] __ro_mostly_after_init = {

This won't work. This variable is array of "struct list_head".
You need to set same attribute to variables pointed by
"struct list_head"->next and "struct list_head"->prev .

>  	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
>  	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
>  	LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),

Re: [kernel-hardening] [RFC 3/7] module: modify memory attrs for __ro_mostly_after_init during module_init/exit

[Ho-Eun Ryu]

> On 20 Feb 2017, at 7:30 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Sun, Feb 19, 2017 at 07:04:06PM +0900, Hoeun Ryu wrote:
>> `__ro_mostly_after_init` is almost like `__ro_after_init`. The section is
>> read-only as same as `__ro_after_init` after kernel init. This patch makes
>> `__ro_mostly_after_init` section read-write temporarily only during
>> module_init/module_exit.
>> 
>> Signed-off-by: Hoeun Ryu <hoeun.ryu@gmail.com>
>> ---
>> kernel/module.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 7eba6de..3b25e0e 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -987,8 +987,11 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>> 
>> 	mutex_unlock(&module_mutex);
>> 	/* Final destruction now no one is using it. */
>> -	if (mod->exit != NULL)
>> +	if (mod->exit != NULL) {
>> +		set_ro_mostly_after_init_rw();
>> 		mod->exit();
>> +		set_ro_mostly_after_init_ro();
>> +	}
>> 	blocking_notifier_call_chain(&module_notify_list,
>> 				     MODULE_STATE_GOING, mod);
>> 	klp_module_going(mod);
>> @@ -3396,8 +3399,11 @@ static noinline int do_init_module(struct module *mod)
>> 
>> 	do_mod_ctors(mod);
>> 	/* Start the module */
>> -	if (mod->init != NULL)
>> +	if (mod->init != NULL) {
>> +		set_ro_mostly_after_init_rw();
>> 		ret = do_one_initcall(mod->init);
>> +		set_ro_mostly_after_init_ro();
>> +	}
> 
> This looks very much like the pax_{open,close}_kernel() approach for
> write-rarely data.

I read the discussion [1] and I agree that __ro_mostly_after_init marker
looks very similar to __write_rarely. 

> 
> I think it would be better to implement a first class write-rarely
> mechanism rather than trying to extend __ro_after_init to cover this
> case.

I’m not extending __ro_after_init. __ro_mostly_after_init resides in the same section of rodata though.

> 
> As mentioned previously, I *think* we can have a generic implementation
> that uses an mm to temporarily map a (thread/cpu-local) RW alias of the
> data in question in what would otherwise be the user half of the address
> space. Regardless, we can have a generic interface [1] that can cater
> for that style of approach and/or something like ARM's domains or x86's
> pkeys.
> 

I’m still learning cpu/kernel architectures, It would be very thankful if you tell me more about the detail of the implementation itself.

The mm that maps temporary RW alias is like
    * special mm like idmap/init_mm which have its own page tables?
    * the page tables have the same content of page tables of init_mm’s swapper_pg_dir except for RW permissions for a specific section (let’s say __write_rarely)
    * then use switch_mm(special_rw_mm) to change the address space before the access happens to the section
    * then use switch_mm(current->mm) to change the address space to original after the access is done

And the interface itself. rare_write(__val, __val), is it a single value access interface.
I’m intending to make data in __ro_mostly_after_init section RW during multiple accesses like during module_init/exit.
and __rare_rw_map()/unmap() used in rare_write() seems to work like open/close api.

How could __rare_rw_ptr() be implemented and what happens when `__rw_var = __rare_rw_ptr(&(__var))` is done ?

However the interface will look like, Do we still need a special data section that is mapped RO in general but RW in some cases ?
if then, doesn’t __ro_mostly_after_init marker itself make sense and we still need it ?

> Thanks,
> Mark.
> 
> [1] http://www.openwall.com/lists/kernel-hardening/2016/11/18/3

Re: [kernel-hardening] [RFC 3/7] module: modify memory attrs for __ro_mostly_after_init during module_init/exit

[Mark Rutland]

On Tue, Feb 21, 2017 at 10:36:05PM +0900, Ho-Eun Ryu wrote:
> > On 20 Feb 2017, at 7:30 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Sun, Feb 19, 2017 at 07:04:06PM +0900, Hoeun Ryu wrote:

> >> @@ -3396,8 +3399,11 @@ static noinline int do_init_module(struct module *mod)
> >> 
> >> 	do_mod_ctors(mod);
> >> 	/* Start the module */
> >> -	if (mod->init != NULL)
> >> +	if (mod->init != NULL) {
> >> +		set_ro_mostly_after_init_rw();
> >> 		ret = do_one_initcall(mod->init);
> >> +		set_ro_mostly_after_init_ro();
> >> +	}
> > 
> > This looks very much like the pax_{open,close}_kernel() approach for
> > write-rarely data.
> 
> I read the discussion [1] and I agree that __ro_mostly_after_init marker
> looks very similar to __write_rarely. 
> 
> > I think it would be better to implement a first class write-rarely
> > mechanism rather than trying to extend __ro_after_init to cover this
> > case.
> 
> I’m not extending __ro_after_init. __ro_mostly_after_init resides in
> the same section of rodata though.

Sorry; I was confused when I wrote that email. I now understand that
you're adding a separate annotation.

> > As mentioned previously, I *think* we can have a generic implementation
> > that uses an mm to temporarily map a (thread/cpu-local) RW alias of the
> > data in question in what would otherwise be the user half of the address
> > space. Regardless, we can have a generic interface [1] that can cater
> > for that style of approach and/or something like ARM's domains or x86's
> > pkeys.
> > 
> 
> I’m still learning cpu/kernel architectures, It would be very thankful if you tell me more about the detail of the implementation itself.
> 
> The mm that maps temporary RW alias is like
>     * special mm like idmap/init_mm which have its own page tables?
>     * the page tables have the same content of page tables of
>       init_mm’s swapper_pg_dir except for RW permissions for a
>       specific section (let’s say __write_rarely)

This would be a special mm, like a user mm, that only mapped the
relevant VA(s).

That might map the relevant variable on-demand, or the mapping could
cover the whole write_rarely area.

>     * then use switch_mm(special_rw_mm) to change the address space
>       before the access happens to the section
>     * then use switch_mm(current->mm) to change the address space to
>       original after the access is done

Yes.

> And the interface itself. rare_write(__val, __val), is it a single
> value access interface.
> I’m intending to make data in __ro_mostly_after_init section RW during
> multiple accesses like during module_init/exit.
> and __rare_rw_map()/unmap() used in rare_write() seems to work like
> open/close api.

The __rare_rw_{map,unmap}() functions would map in the RW alias, but do
not necessarily change the RO alias to RW. This is why __rare_rw_ptr()
would be necessary, and is the major difference to the open/close API.

We could certainly allow several writes between a map/unmap. The key
requirement is that each write is instrumented so that it goes via the
RW alias.

> How could __rare_rw_ptr() be implemented and what happens when
> `__rw_var = __rare_rw_ptr(&(__var))` is done ?

__rare_rw_ptr() would take a pointer to the usual RO alias, and derive
its RW alias. What exactly this should do depends on how the RW alias is
implemented.

On a system using an RW mm, let's assume we place all __write_rarely
variables in a region bounded by __rare_write_begin/__rare_write_end,
and when the mm is installed place, we have an RW alias of this region
beginning at __rw_alias_start. In this case, it'd look something like:

#define __rare_rw_ptr(ptr) ({				\
	unsigned long __ptr = (unsigned long)(ptr);	\
	__ptr -= __rare_write_start;			\
	__ptr += __rw_alias_start;			\
	(typeof(ptr))__ptr;				\
})

... does that make sense?

For systems where you can freely/easily alter (local) permissions (e.g.
using ARM's domains), that can be done within __rare_rw_{map,unmap}(),
and __rare_rw_ptr can just return the original pointer.

> However the interface will look like, Do we still need a special data
> section that is mapped RO in general but RW in some cases ?

With the above, I think the usual mapping can always be RO.

> if then, doesn’t __ro_mostly_after_init marker itself make sense and
> we still need it ?

We may need a marker to bound the set of variables we wish to map in
this way.

Thanks,
Mark.

Re: [RFC 7/7] arm64: map seperately rodata sections for __ro_mostly_after_init section

[Kees Cook]

On Mon, Feb 20, 2017 at 4:45 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Sun, Feb 19, 2017 at 11:35:51AM +0000, Ard Biesheuvel wrote:
>> On 19 February 2017 at 10:04, Hoeun Ryu <hoeun.ryu@gmail.com> wrote:
>> > Map rodata sections seperately for the new __ro_mostly_after_init section.
>> > Attribute of memory for __ro_mostly_after_init section can be changed later
>> > so we need a dedicated vmalloced region for set_memory_rw/ro api.
>
>> While it is correct that you are splitting this into three separate
>> segments (otherwise we would not be able to change the permissions
>> later without risking splitting to occur), I think this leads to
>> unnecessary fragmentation.
>>
>> If there is demand for this feature (but you still need to make the
>> argument for that), I wonder if it wouldn't be sufficient, and much
>> more straightforward, to redefine the __ro_after_init semantics to
>> include the kind of subsystem registration and module init context you
>> are targeting, and implement some hooks to temporarily lift the
>> __ro_after_init r/o permission restrictions in a controlled manner.
>
> From a look over the series, I think this is just __write_rarely in
> disguise. I personally think that we should keep __write_rarely and
> __ro_after_init separate, the later being a strictly one-shot affair.

That's my thinking too.

> I had some ideas [1] as to how we could implement __write_rarely without
> carving up the kernel mapping further (and keeping the RW permissions
> local to the thread needing it), but I have not had the time to look
> into that further.

I'm working on a series to do this for x86, but I keep getting
distracted. I hope to get an RFC posted this week.

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] [RFC 3/7] module: modify memory attrs for __ro_mostly_after_init during module_init/exit

[Hoeun Ryu]

Thank you for your detailed explanation. It helped a lot for understandings.

> On Feb 21, 2017, at 10:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Tue, Feb 21, 2017 at 10:36:05PM +0900, Ho-Eun Ryu wrote:
>>> On 20 Feb 2017, at 7:30 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Sun, Feb 19, 2017 at 07:04:06PM +0900, Hoeun Ryu wrote:
> 
>>>> @@ -3396,8 +3399,11 @@ static noinline int do_init_module(struct module *mod)
>>>> 
>>>>   do_mod_ctors(mod);
>>>>   /* Start the module */
>>>> -    if (mod->init != NULL)
>>>> +    if (mod->init != NULL) {
>>>> +        set_ro_mostly_after_init_rw();
>>>>       ret = do_one_initcall(mod->init);
>>>> +        set_ro_mostly_after_init_ro();
>>>> +    }
>>> 
>>> This looks very much like the pax_{open,close}_kernel() approach for
>>> write-rarely data.
>> 
>> I read the discussion [1] and I agree that __ro_mostly_after_init marker
>> looks very similar to __write_rarely. 
>> 
>>> I think it would be better to implement a first class write-rarely
>>> mechanism rather than trying to extend __ro_after_init to cover this
>>> case.
>> 
>> I’m not extending __ro_after_init. __ro_mostly_after_init resides in
>> the same section of rodata though.
> 
> Sorry; I was confused when I wrote that email. I now understand that
> you're adding a separate annotation.
> 
>>> As mentioned previously, I *think* we can have a generic implementation
>>> that uses an mm to temporarily map a (thread/cpu-local) RW alias of the
>>> data in question in what would otherwise be the user half of the address
>>> space. Regardless, we can have a generic interface [1] that can cater
>>> for that style of approach and/or something like ARM's domains or x86's
>>> pkeys.
>> 
>> I’m still learning cpu/kernel architectures, It would be very thankful if you tell me more about the detail of the implementation itself.
>> 
>> The mm that maps temporary RW alias is like
>>   * special mm like idmap/init_mm which have its own page tables?
>>   * the page tables have the same content of page tables of
>>     init_mm’s swapper_pg_dir except for RW permissions for a
>>     specific section (let’s say __write_rarely)
> 
> This would be a special mm, like a user mm, that only mapped the
> relevant VA(s).

we need a separate mm/pgd for ttbr0_el1 in kernel image section as idmap and swapper_pg_dir currently do and we make VA alias mapping for RO section with RW permission under TASK_SIZE during kernel init. And then we can switch to the mm by setting the pgd to ttbr0_el1. Right ?

It came to my mind that how about the relationship with SW_TTBR0_PAN .
What if copy_from_user tries to do something against RW alias ?

val_rw = __rw_ptr(&val);
__rw_map();
copy_from_user(&val_rw, user_ptr);
__re_unmap();

__rw_map() will install rw_mm->gpd to ttbr0_el1 but uaccess_enable() will immediately reinstall thread_info->pgd to ttbr0_el1 and we loose RW alias.
Am I something wrong or confused ?

> 
> That might map the relevant variable on-demand, or the mapping could
> cover the whole write_rarely area.
> 
>>   * then use switch_mm(special_rw_mm) to change the address space
>>     before the access happens to the section
>>   * then use switch_mm(current->mm) to change the address space to
>>     original after the access is done
> 
> Yes.
> 
>> And the interface itself. rare_write(__val, __val), is it a single
>> value access interface.
>> I’m intending to make data in __ro_mostly_after_init section RW during
>> multiple accesses like during module_init/exit.
>> and __rare_rw_map()/unmap() used in rare_write() seems to work like
>> open/close api.
> 
> The __rare_rw_{map,unmap}() functions would map in the RW alias, but do
> not necessarily change the RO alias to RW. This is why __rare_rw_ptr()
> would be necessary, and is the major difference to the open/close API.
> 
> We could certainly allow several writes between a map/unmap. The key
> requirement is that each write is instrumented so that it goes via the
> RW alias.
> 
>> How could __rare_rw_ptr() be implemented and what happens when
>> `__rw_var = __rare_rw_ptr(&(__var))` is done ?
> 
> __rare_rw_ptr() would take a pointer to the usual RO alias, and derive
> its RW alias. What exactly this should do depends on how the RW alias is
> implemented.
> 
> On a system using an RW mm, let's assume we place all __write_rarely
> variables in a region bounded by __rare_write_begin/__rare_write_end,
> and when the mm is installed place, we have an RW alias of this region
> beginning at __rw_alias_start. In this case, it'd look something like:
> 
> #define __rare_rw_ptr(ptr) ({                \
>   unsigned long __ptr = (unsigned long)(ptr);    \
>   __ptr -= __rare_write_start;            \
>   __ptr += __rw_alias_start;            \
>   (typeof(ptr))__ptr;                \
> })
> 
> ... does that make sense?

Yes. Cool.

> 
> For systems where you can freely/easily alter (local) permissions (e.g.
> using ARM's domains), that can be done within __rare_rw_{map,unmap}(),
> and __rare_rw_ptr can just return the original pointer.
> 
>> However the interface will look like, Do we still need a special data
>> section that is mapped RO in general but RW in some cases ?
> 
> With the above, I think the usual mapping can always be RO.
> 
>> if then, doesn’t __ro_mostly_after_init marker itself make sense and
>> we still need it ?
> 
> We may need a marker to bound the set of variables we wish to map in
> this way.
> 
> Thanks,
> Mark.