[PATCH v2 0/3] CONFIG_DEBUG_SET_MODULE_RONX bug fix and cleanups

[PATCH v2 0/3] CONFIG_DEBUG_SET_MODULE_RONX bug fix and cleanups

[Josh Poimboeuf]

Patch 1/3 is a livepatch bug fix for a crash which occurs when loading a
patch module on a kernel without CONFIG_DEBUG_SET_MODULE_RONX.

Patch 2/3 is a module code cleanup to make setting and clearing RO and
NX more symmetrical.  It also adds some new functions which are used by
patch 3.

Patch 3/3 is a livepatch cleanup for simplification of the livepatch
relocation code.

v2:
- removed set_page_attributes() duplication in livepatch cleanup patch
  in favor of using new {un}set_module_core_ro_nx() functions
- changed 'size' from int to size_t

Josh Poimboeuf (3):
  livepatch: Fix crash with !CONFIG_DEBUG_SET_MODULE_RONX
  module: Use the same logic for setting and unsetting RO/NX
  livepatch: Cleanup module page permission changes

 arch/x86/kernel/livepatch.c | 24 ++----------------
 include/linux/module.h      |  4 +++
 kernel/livepatch/core.c     | 15 ++++++++----
 kernel/module.c             | 59 +++++++++++++++++++++++----------------------
 4 files changed, 46 insertions(+), 56 deletions(-)

-- 
2.4.3

[PATCH v2 1/3] livepatch: Fix crash with !CONFIG_DEBUG_SET_MODULE_RONX

[Josh Poimboeuf]

When loading a patch module on a kernel with
!CONFIG_DEBUG_SET_MODULE_RONX, the following crash occurs:

  [  205.988776] livepatch: enabling patch 'kpatch_meminfo_string'
  [  205.989829] BUG: unable to handle kernel paging request at ffffffffa08d2fc0
  [  205.989863] IP: [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
  [  205.989888] PGD 1a10067 PUD 1a11063 PMD 7bcde067 PTE 3740e161
  [  205.989915] Oops: 0003 [#1] SMP
  [  205.990187] CPU: 2 PID: 14570 Comm: insmod Tainted: G           O  K 4.1.12
  [  205.990214] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
  [  205.990249] task: ffff8800374aaa90 ti: ffff8800794b8000 task.ti: ffff8800794b8000
  [  205.990276] RIP: 0010:[<ffffffff8154fecb>]  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
  [  205.990307] RSP: 0018:ffff8800794bbd58  EFLAGS: 00010246
  [  205.990327] RAX: 0000000000000000 RBX: ffffffffa08d2fc0 RCX: 0000000000000000
  [  205.990356] RDX: 01ffff8000000080 RSI: 0000000000000000 RDI: ffffffff81a54b40
  [  205.990382] RBP: ffff88007b4c4d80 R08: 0000000000000007 R09: 0000000000000000
  [  205.990408] R10: 0000000000000008 R11: ffffea0001f18840 R12: 0000000000000000
  [  205.990433] R13: 0000000000000001 R14: ffffffffa08d2fc0 R15: ffff88007bd0bc40
  [  205.990459] FS:  00007f1128fbc700(0000) GS:ffff88007fc80000(0000) knlGS:0000000000000000
  [  205.990488] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [  205.990509] CR2: ffffffffa08d2fc0 CR3: 000000002606e000 CR4: 00000000001406e0
  [  205.990536] Stack:
  [  205.990545]  ffff8800794bbec8 0000000000000001 ffffffffa08d3010 ffffffff810ecea9
  [  205.990576]  ffffffff810e8e40 000000000005f360 ffff88007bd0bc50 ffffffffa08d3240
  [  205.990608]  ffffffffa08d52c0 ffffffffa08d3210 ffff8800794bbed8 ffff8800794bbf1c
  [  205.990639] Call Trace:
  [  205.990651]  [<ffffffff810ecea9>] ? load_module+0x1e59/0x23a0
  [  205.990672]  [<ffffffff810e8e40>] ? store_uevent+0x40/0x40
  [  205.990693]  [<ffffffff810e99b5>] ? copy_module_from_fd.isra.49+0xb5/0x140
  [  205.990718]  [<ffffffff810ed5bd>] ? SyS_finit_module+0x7d/0xa0
  [  205.990741]  [<ffffffff81556832>] ? system_call_fastpath+0x16/0x75
  [  205.990763] Code: f9 00 00 00 74 23 49 c7 c0 92 e1 60 81 48 8d 53 18 89 c1 4c 89 c6 48 c7 c7 f0 85 7d 81 31 c0 e8 71 fa ff ff e8 58 0e 00 00 31 f6 <c7> 03 00 00 00 00 48 89 da 48 c7 c7 20 c7 a5 81 e8 d0 ec b3 ff
  [  205.990916] RIP  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
  [  205.990940]  RSP <ffff8800794bbd58>
  [  205.990953] CR2: ffffffffa08d2fc0

With !CONFIG_DEBUG_SET_MODULE_RONX, module text and rodata pages are
writable, and the debug_align() macro allows the module struct to share
a page with executable text.  When klp_write_module_reloc() calls
set_memory_ro() on the page, it effectively turns the module struct into
a read-only structure, resulting in a page fault when load_module() does
"mod->state = MODULE_STATE_LIVE".

Reported-by: Cyril B. <cbay@alwaysdata.com>
Tested-by: Cyril B. <cbay@alwaysdata.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/livepatch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
index ff3c3101d..d1d35cc 100644
--- a/arch/x86/kernel/livepatch.c
+++ b/arch/x86/kernel/livepatch.c
@@ -42,7 +42,6 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
 	bool readonly;
 	unsigned long val;
 	unsigned long core = (unsigned long)mod->module_core;
-	unsigned long core_ro_size = mod->core_ro_size;
 	unsigned long core_size = mod->core_size;
 
 	switch (type) {
@@ -70,10 +69,12 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
 		/* loc does not point to any symbol inside the module */
 		return -EINVAL;
 
-	if (loc < core + core_ro_size)
+	readonly = false;
+
+#ifdef CONFIG_DEBUG_SET_MODULE_RONX
+	if (loc < core + mod->core_ro_size)
 		readonly = true;
-	else
-		readonly = false;
+#endif
 
 	/* determine if the relocation spans a page boundary */
 	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
-- 
2.4.3

Re: [PATCH v2 1/3] livepatch: Fix crash with !CONFIG_DEBUG_SET_MODULE_RONX

[Jiri Kosina]

On Thu, 5 Nov 2015, Josh Poimboeuf wrote:

> When loading a patch module on a kernel with
> !CONFIG_DEBUG_SET_MODULE_RONX, the following crash occurs:
> 
>   [  205.988776] livepatch: enabling patch 'kpatch_meminfo_string'
>   [  205.989829] BUG: unable to handle kernel paging request at ffffffffa08d2fc0
>   [  205.989863] IP: [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
>   [  205.989888] PGD 1a10067 PUD 1a11063 PMD 7bcde067 PTE 3740e161
>   [  205.989915] Oops: 0003 [#1] SMP
>   [  205.990187] CPU: 2 PID: 14570 Comm: insmod Tainted: G           O  K 4.1.12
>   [  205.990214] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
>   [  205.990249] task: ffff8800374aaa90 ti: ffff8800794b8000 task.ti: ffff8800794b8000
>   [  205.990276] RIP: 0010:[<ffffffff8154fecb>]  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
>   [  205.990307] RSP: 0018:ffff8800794bbd58  EFLAGS: 00010246
>   [  205.990327] RAX: 0000000000000000 RBX: ffffffffa08d2fc0 RCX: 0000000000000000
>   [  205.990356] RDX: 01ffff8000000080 RSI: 0000000000000000 RDI: ffffffff81a54b40
>   [  205.990382] RBP: ffff88007b4c4d80 R08: 0000000000000007 R09: 0000000000000000
>   [  205.990408] R10: 0000000000000008 R11: ffffea0001f18840 R12: 0000000000000000
>   [  205.990433] R13: 0000000000000001 R14: ffffffffa08d2fc0 R15: ffff88007bd0bc40
>   [  205.990459] FS:  00007f1128fbc700(0000) GS:ffff88007fc80000(0000) knlGS:0000000000000000
>   [  205.990488] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   [  205.990509] CR2: ffffffffa08d2fc0 CR3: 000000002606e000 CR4: 00000000001406e0
>   [  205.990536] Stack:
>   [  205.990545]  ffff8800794bbec8 0000000000000001 ffffffffa08d3010 ffffffff810ecea9
>   [  205.990576]  ffffffff810e8e40 000000000005f360 ffff88007bd0bc50 ffffffffa08d3240
>   [  205.990608]  ffffffffa08d52c0 ffffffffa08d3210 ffff8800794bbed8 ffff8800794bbf1c
>   [  205.990639] Call Trace:
>   [  205.990651]  [<ffffffff810ecea9>] ? load_module+0x1e59/0x23a0
>   [  205.990672]  [<ffffffff810e8e40>] ? store_uevent+0x40/0x40
>   [  205.990693]  [<ffffffff810e99b5>] ? copy_module_from_fd.isra.49+0xb5/0x140
>   [  205.990718]  [<ffffffff810ed5bd>] ? SyS_finit_module+0x7d/0xa0
>   [  205.990741]  [<ffffffff81556832>] ? system_call_fastpath+0x16/0x75
>   [  205.990763] Code: f9 00 00 00 74 23 49 c7 c0 92 e1 60 81 48 8d 53 18 89 c1 4c 89 c6 48 c7 c7 f0 85 7d 81 31 c0 e8 71 fa ff ff e8 58 0e 00 00 31 f6 <c7> 03 00 00 00 00 48 89 da 48 c7 c7 20 c7 a5 81 e8 d0 ec b3 ff
>   [  205.990916] RIP  [<ffffffff8154fecb>] do_init_module+0x8c/0x1ba
>   [  205.990940]  RSP <ffff8800794bbd58>
>   [  205.990953] CR2: ffffffffa08d2fc0
> 
> With !CONFIG_DEBUG_SET_MODULE_RONX, module text and rodata pages are
> writable, and the debug_align() macro allows the module struct to share
> a page with executable text.  When klp_write_module_reloc() calls
> set_memory_ro() on the page, it effectively turns the module struct into
> a read-only structure, resulting in a page fault when load_module() does
> "mod->state = MODULE_STATE_LIVE".
> 
> Reported-by: Cyril B. <cbay@alwaysdata.com>
> Tested-by: Cyril B. <cbay@alwaysdata.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

I've cherry-picked this one into livepatching.git#for-4.4/upstream, 
leaving 2/3 and 3/3 for further discussion with Rusty first.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH v2 2/3] module: Use the same logic for setting and unsetting RO/NX

[Josh Poimboeuf]

When setting a module's RO and NX permissions, set_section_ro_nx() is
used, but when clearing them, unset_module_{init,core}_ro_nx() are used.
The unset functions don't have the same checks the set function has for
partial page protections.  It's probably harmless, but it's still
confusingly asymmetrical.

Instead, use the same logic to do both.  Also add some new
set_module_{init,core}_ro_nx() helper functions for more symmetry with
the unset functions.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 kernel/module.c | 57 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 8f051a1..14b2249 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1886,7 +1886,9 @@ void set_page_attributes(void *start, void *end, int (*set)(unsigned long start,
 static void set_section_ro_nx(void *base,
 			unsigned long text_size,
 			unsigned long ro_size,
-			unsigned long total_size)
+			unsigned long total_size,
+			int (*set_ro)(unsigned long start, int num_pages),
+			int (*set_nx)(unsigned long start, int num_pages))
 {
 	/* begin and end PFNs of the current subsection */
 	unsigned long begin_pfn;
@@ -1898,7 +1900,7 @@ static void set_section_ro_nx(void *base,
 	 * - Do not protect last partial page.
 	 */
 	if (ro_size > 0)
-		set_page_attributes(base, base + ro_size, set_memory_ro);
+		set_page_attributes(base, base + ro_size, set_ro);
 
 	/*
 	 * Set NX permissions for module data:
@@ -1909,28 +1911,36 @@ static void set_section_ro_nx(void *base,
 		begin_pfn = PFN_UP((unsigned long)base + text_size);
 		end_pfn = PFN_UP((unsigned long)base + total_size);
 		if (end_pfn > begin_pfn)
-			set_memory_nx(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
+			set_nx(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
 	}
 }
 
+static void set_module_core_ro_nx(struct module *mod)
+{
+	set_section_ro_nx(mod->module_core, mod->core_text_size,
+			  mod->core_ro_size, mod->core_size,
+			  set_memory_ro, set_memory_nx);
+}
+
 static void unset_module_core_ro_nx(struct module *mod)
 {
-	set_page_attributes(mod->module_core + mod->core_text_size,
-		mod->module_core + mod->core_size,
-		set_memory_x);
-	set_page_attributes(mod->module_core,
-		mod->module_core + mod->core_ro_size,
-		set_memory_rw);
+	set_section_ro_nx(mod->module_core, mod->core_text_size,
+			  mod->core_ro_size, mod->core_size,
+			  set_memory_rw, set_memory_x);
+}
+
+static void set_module_init_ro_nx(struct module *mod)
+{
+	set_section_ro_nx(mod->module_init, mod->init_text_size,
+			  mod->init_ro_size, mod->init_size,
+			  set_memory_ro, set_memory_nx);
 }
 
 static void unset_module_init_ro_nx(struct module *mod)
 {
-	set_page_attributes(mod->module_init + mod->init_text_size,
-		mod->module_init + mod->init_size,
-		set_memory_x);
-	set_page_attributes(mod->module_init,
-		mod->module_init + mod->init_ro_size,
-		set_memory_rw);
+	set_section_ro_nx(mod->module_init, mod->init_text_size,
+			  mod->init_ro_size, mod->init_size,
+			  set_memory_rw, set_memory_x);
 }
 
 /* Iterate through all modules and set each module's text as RW */
@@ -1979,7 +1989,8 @@ void set_all_modules_text_ro(void)
 	mutex_unlock(&module_mutex);
 }
 #else
-static inline void set_section_ro_nx(void *base, unsigned long text_size, unsigned long ro_size, unsigned long total_size) { }
+static void set_module_core_ro_nx(struct module *mod) { }
+static void set_module_init_ro_nx(struct module *mod) { }
 static void unset_module_core_ro_nx(struct module *mod) { }
 static void unset_module_init_ro_nx(struct module *mod) { }
 #endif
@@ -3373,17 +3384,9 @@ 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 for core */
-	set_section_ro_nx(mod->module_core,
-				mod->core_text_size,
-				mod->core_ro_size,
-				mod->core_size);
-
-	/* Set RO and NX regions for init */
-	set_section_ro_nx(mod->module_init,
-				mod->init_text_size,
-				mod->init_ro_size,
-				mod->init_size);
+	/* Set RO and NX regions */
+	set_module_init_ro_nx(mod);
+	set_module_core_ro_nx(mod);
 
 	/* Mark state as coming so strong_try_module_get() ignores us,
 	 * but kallsyms etc. can see us. */
-- 
2.4.3

Re: [PATCH v2 2/3] module: Use the same logic for setting and unsetting RO/NX

[Rusty Russell]

Josh Poimboeuf <jpoimboe@redhat.com> writes:
> When setting a module's RO and NX permissions, set_section_ro_nx() is
> used, but when clearing them, unset_module_{init,core}_ro_nx() are used.
> The unset functions don't have the same checks the set function has for
> partial page protections.  It's probably harmless, but it's still
> confusingly asymmetrical.
>
> Instead, use the same logic to do both.  Also add some new
> set_module_{init,core}_ro_nx() helper functions for more symmetry with
> the unset functions.

Yes, this seems sensible.

One nit to pick:

> +			unsigned long total_size,
> +			int (*set_ro)(unsigned long start, int num_pages),
> +			int (*set_nx)(unsigned long start, int num_pages))

...
> +	set_section_ro_nx(mod->module_core, mod->core_text_size,
> +			  mod->core_ro_size, mod->core_size,
> +			  set_memory_rw, set_memory_x);

set_ro == set_memory_rw here.  That's just confusing.

I think we have to avoid the word "set" in the function parameters
since it may unset instead.

Suggest "alter_ro" or "frob_ro" instead?

Thanks,
Rusty.

[PATCH v2 3/3] livepatch: Cleanup module page permission changes

[Josh Poimboeuf]

Calling set_memory_rw() and set_memory_ro() for every iteration of the
loop in klp_write_object_relocations() is messy, inefficient, and
error-prone.

Change all the read-only pages to read-write before the loop and convert
them back to read-only again afterwards.

The {un}set_module_core_ro_nx() functions are used to change the
page permissions.  Toggling NX isn't necessary in this case, but it's
not highly performance sensitive code so it should be fine.

Suggested-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/livepatch.c | 25 ++-----------------------
 include/linux/module.h      |  4 ++++
 kernel/livepatch/core.c     | 15 ++++++++++-----
 kernel/module.c             |  6 ++----
 4 files changed, 18 insertions(+), 32 deletions(-)

diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c
index d1d35cc..13eaf68 100644
--- a/arch/x86/kernel/livepatch.c
+++ b/arch/x86/kernel/livepatch.c
@@ -20,8 +20,6 @@
 
 #include <linux/module.h>
 #include <linux/uaccess.h>
-#include <asm/cacheflush.h>
-#include <asm/page_types.h>
 #include <asm/elf.h>
 #include <asm/livepatch.h>
 
@@ -38,8 +36,7 @@
 int klp_write_module_reloc(struct module *mod, unsigned long type,
 			   unsigned long loc, unsigned long value)
 {
-	int ret, numpages, size = 4;
-	bool readonly;
+	size_t size = 4;
 	unsigned long val;
 	unsigned long core = (unsigned long)mod->module_core;
 	unsigned long core_size = mod->core_size;
@@ -69,23 +66,5 @@ int klp_write_module_reloc(struct module *mod, unsigned long type,
 		/* loc does not point to any symbol inside the module */
 		return -EINVAL;
 
-	readonly = false;
-
-#ifdef CONFIG_DEBUG_SET_MODULE_RONX
-	if (loc < core + mod->core_ro_size)
-		readonly = true;
-#endif
-
-	/* determine if the relocation spans a page boundary */
-	numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
-
-	if (readonly)
-		set_memory_rw(loc & PAGE_MASK, numpages);
-
-	ret = probe_kernel_write((void *)loc, &val, size);
-
-	if (readonly)
-		set_memory_ro(loc & PAGE_MASK, numpages);
-
-	return ret;
+	return probe_kernel_write((void *)loc, &val, size);
 }
diff --git a/include/linux/module.h b/include/linux/module.h
index 3a19c79..557be36 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -766,9 +766,13 @@ extern int module_sysfs_initialized;
 #define __MODULE_STRING(x) __stringify(x)
 
 #ifdef CONFIG_DEBUG_SET_MODULE_RONX
+extern void set_module_core_ro_nx(struct module *mod);
+extern void unset_module_core_ro_nx(struct module *mod);
 extern void set_all_modules_text_rw(void);
 extern void set_all_modules_text_ro(void);
 #else
+static inline void set_module_core_ro_nx(struct module *mod) { }
+static inline void unset_module_core_ro_nx(struct module *mod) { }
 static inline void set_all_modules_text_rw(void) { }
 static inline void set_all_modules_text_ro(void) { }
 #endif
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6e53441..1c94c4b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -28,6 +28,7 @@
 #include <linux/list.h>
 #include <linux/kallsyms.h>
 #include <linux/livepatch.h>
+#include <asm/cacheflush.h>
 
 /**
  * struct klp_ops - structure for tracking registered ftrace ops structs
@@ -283,7 +284,7 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
 static int klp_write_object_relocations(struct module *pmod,
 					struct klp_object *obj)
 {
-	int ret;
+	int ret = 0;
 	struct klp_reloc *reloc;
 
 	if (WARN_ON(!klp_is_object_loaded(obj)))
@@ -292,12 +293,14 @@ static int klp_write_object_relocations(struct module *pmod,
 	if (WARN_ON(!obj->relocs))
 		return -EINVAL;
 
+	unset_module_core_ro_nx(pmod);
+
 	for (reloc = obj->relocs; reloc->name; reloc++) {
 		if (!klp_is_module(obj)) {
 			ret = klp_verify_vmlinux_symbol(reloc->name,
 							reloc->val);
 			if (ret)
-				return ret;
+				goto out;
 		} else {
 			/* module, reloc->val needs to be discovered */
 			if (reloc->external)
@@ -309,18 +312,20 @@ static int klp_write_object_relocations(struct module *pmod,
 							     reloc->name,
 							     &reloc->val);
 			if (ret)
-				return ret;
+				goto out;
 		}
 		ret = klp_write_module_reloc(pmod, reloc->type, reloc->loc,
 					     reloc->val + reloc->addend);
 		if (ret) {
 			pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
 			       reloc->name, reloc->val, ret);
-			return ret;
+			goto out;
 		}
 	}
 
-	return 0;
+out:
+	set_module_core_ro_nx(pmod);
+	return ret;
 }
 
 static void notrace klp_ftrace_handler(unsigned long ip,
diff --git a/kernel/module.c b/kernel/module.c
index 14b2249..eb6a924 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1915,14 +1915,14 @@ static void set_section_ro_nx(void *base,
 	}
 }
 
-static void set_module_core_ro_nx(struct module *mod)
+void set_module_core_ro_nx(struct module *mod)
 {
 	set_section_ro_nx(mod->module_core, mod->core_text_size,
 			  mod->core_ro_size, mod->core_size,
 			  set_memory_ro, set_memory_nx);
 }
 
-static void unset_module_core_ro_nx(struct module *mod)
+void unset_module_core_ro_nx(struct module *mod)
 {
 	set_section_ro_nx(mod->module_core, mod->core_text_size,
 			  mod->core_ro_size, mod->core_size,
@@ -1989,9 +1989,7 @@ void set_all_modules_text_ro(void)
 	mutex_unlock(&module_mutex);
 }
 #else
-static void set_module_core_ro_nx(struct module *mod) { }
 static void set_module_init_ro_nx(struct module *mod) { }
-static void unset_module_core_ro_nx(struct module *mod) { }
 static void unset_module_init_ro_nx(struct module *mod) { }
 #endif
 
-- 
2.4.3

Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes

[Petr Mladek]

On Thu 2015-11-05 15:18:05, Josh Poimboeuf wrote:
> Calling set_memory_rw() and set_memory_ro() for every iteration of the
> loop in klp_write_object_relocations() is messy, inefficient, and
> error-prone.
> 
> Change all the read-only pages to read-write before the loop and convert
> them back to read-only again afterwards.
> 
> The {un}set_module_core_ro_nx() functions are used to change the
> page permissions.  Toggling NX isn't necessary in this case, but it's
> not highly performance sensitive code so it should be fine.

Hmm, the name (un)set_module_core_ro_nx() still sounds a bit strange,
especially the "ro_nx" suffix. Alternative solution would be to create

   set_module_text_rw()
   set_module_text_ro()

There already exists

   set_all_modules_text_rw()
   set_all_modules_text_ro()

They modify only the ro/rw flags. IMHO, the name is more descriptive
They are used by ftrace for very similar purpose.

They modify also the init section. But we might want to touch it
as well. klp_module_notify() is called too late now. But once we
have a more complex consistency model, we will need to reject
the module when the patching fails. We will need to call the
livepatch init earlier, close to ftrace_module_init(mod).
Then the init section might be interesting as well.


Best Regards,
Petr

Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes

[Josh Poimboeuf]

On Fri, Nov 06, 2015 at 11:40:55AM +0100, Petr Mladek wrote:
> On Thu 2015-11-05 15:18:05, Josh Poimboeuf wrote:
> > Calling set_memory_rw() and set_memory_ro() for every iteration of the
> > loop in klp_write_object_relocations() is messy, inefficient, and
> > error-prone.
> > 
> > Change all the read-only pages to read-write before the loop and convert
> > them back to read-only again afterwards.
> > 
> > The {un}set_module_core_ro_nx() functions are used to change the
> > page permissions.  Toggling NX isn't necessary in this case, but it's
> > not highly performance sensitive code so it should be fine.
> 
> Hmm, the name (un)set_module_core_ro_nx() still sounds a bit strange,
> especially the "ro_nx" suffix.

> Alternative solution would be to create
> 
>    set_module_text_rw()
>    set_module_text_ro()
> 
> There already exists
> 
>    set_all_modules_text_rw()
>    set_all_modules_text_ro()
> 
> They modify only the ro/rw flags. IMHO, the name is more descriptive
> They are used by ftrace for very similar purpose.

That wouldn't be enough.  Relocations can occur not only in text, but
also in data.  That includes read-only data.

The (un)set_module_core_ro_nx() naming was taken from the names of
existing module functions (unset_module_{core,init}_ro_nx()).  They
enable/disable the CONFIG_DEBUG_SET_MODULE_RONX feature on the core part
of the module.  The name makes sense to me, though I'm certainly open to
other ideas.

> They modify also the init section. But we might want to touch it
> as well. klp_module_notify() is called too late now. But once we
> have a more complex consistency model, we will need to reject
> the module when the patching fails. We will need to call the
> livepatch init earlier, close to ftrace_module_init(mod).
> Then the init section might be interesting as well.

Init section functions don't have the __fentry() call, so they can't be
patched.  If that were to change in the future, we could use the
(un)set_module_init_ro_nx() functions, which already exist.

-- 
Josh

Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes

[Petr Mladek]

On Fri 2015-11-06 06:12:47, Josh Poimboeuf wrote:
> On Fri, Nov 06, 2015 at 11:40:55AM +0100, Petr Mladek wrote:
> > On Thu 2015-11-05 15:18:05, Josh Poimboeuf wrote:
> > > Calling set_memory_rw() and set_memory_ro() for every iteration of the
> > > loop in klp_write_object_relocations() is messy, inefficient, and
> > > error-prone.
> > > 
> > > Change all the read-only pages to read-write before the loop and convert
> > > them back to read-only again afterwards.
> > > 
> > > The {un}set_module_core_ro_nx() functions are used to change the
> > > page permissions.  Toggling NX isn't necessary in this case, but it's
> > > not highly performance sensitive code so it should be fine.
> > 
> > Hmm, the name (un)set_module_core_ro_nx() still sounds a bit strange,
> > especially the "ro_nx" suffix.
> 
> > Alternative solution would be to create
> > 
> >    set_module_text_rw()
> >    set_module_text_ro()
> > 
> > There already exists
> > 
> >    set_all_modules_text_rw()
> >    set_all_modules_text_ro()
> > 
> > They modify only the ro/rw flags. IMHO, the name is more descriptive
> > They are used by ftrace for very similar purpose.
> 
> That wouldn't be enough.  Relocations can occur not only in text, but
> also in data.  That includes read-only data.

I see. This just shows how this all is confusing. Or maybe I am just
dumb :-)

> The (un)set_module_core_ro_nx() naming was taken from the names of
> existing module functions (unset_module_{core,init}_ro_nx()).  They
> enable/disable the CONFIG_DEBUG_SET_MODULE_RONX feature on the core part
> of the module.  The name makes sense to me, though I'm certainly open to
> other ideas.

I think that we should not mix

   set_*_ro()
   set_*_rw()

with

   set_*_ro*()
   unset_*_ro*()

naming schemes. What about adding into the public API?

  set_module_ro()
  set_module_rw()

It should modify everything: init, core, text, and data but only
the ro/rw flags.

Sigh, we went quite far from the few lines patch :-/

> > They modify also the init section. But we might want to touch it
> > as well. klp_module_notify() is called too late now. But once we
> > have a more complex consistency model, we will need to reject
> > the module when the patching fails. We will need to call the
> > livepatch init earlier, close to ftrace_module_init(mod).
> > Then the init section might be interesting as well.
> 
> Init section functions don't have the __fentry() call, so they can't be
> patched.  If that were to change in the future, we could use the
> (un)set_module_init_ro_nx() functions, which already exist.

I see.

Best Regards,
Petr

Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes

[Josh Poimboeuf]

On Fri, Nov 06, 2015 at 02:42:46PM +0100, Petr Mladek wrote:
> On Fri 2015-11-06 06:12:47, Josh Poimboeuf wrote:
> > On Fri, Nov 06, 2015 at 11:40:55AM +0100, Petr Mladek wrote:
> > > On Thu 2015-11-05 15:18:05, Josh Poimboeuf wrote:
> > > > Calling set_memory_rw() and set_memory_ro() for every iteration of the
> > > > loop in klp_write_object_relocations() is messy, inefficient, and
> > > > error-prone.
> > > > 
> > > > Change all the read-only pages to read-write before the loop and convert
> > > > them back to read-only again afterwards.
> > > > 
> > > > The {un}set_module_core_ro_nx() functions are used to change the
> > > > page permissions.  Toggling NX isn't necessary in this case, but it's
> > > > not highly performance sensitive code so it should be fine.
> > > 
> > > Hmm, the name (un)set_module_core_ro_nx() still sounds a bit strange,
> > > especially the "ro_nx" suffix.
> > 
> > > Alternative solution would be to create
> > > 
> > >    set_module_text_rw()
> > >    set_module_text_ro()
> > > 
> > > There already exists
> > > 
> > >    set_all_modules_text_rw()
> > >    set_all_modules_text_ro()
> > > 
> > > They modify only the ro/rw flags. IMHO, the name is more descriptive
> > > They are used by ftrace for very similar purpose.
> > 
> > That wouldn't be enough.  Relocations can occur not only in text, but
> > also in data.  That includes read-only data.
> 
> I see. This just shows how this all is confusing. Or maybe I am just
> dumb :-)
> 
> > The (un)set_module_core_ro_nx() naming was taken from the names of
> > existing module functions (unset_module_{core,init}_ro_nx()).  They
> > enable/disable the CONFIG_DEBUG_SET_MODULE_RONX feature on the core part
> > of the module.  The name makes sense to me, though I'm certainly open to
> > other ideas.
> 
> I think that we should not mix
> 
>    set_*_ro()
>    set_*_rw()
> 
> with
> 
>    set_*_ro*()
>    unset_*_ro*()
> 
> naming schemes. What about adding into the public API?
> 
>   set_module_ro()
>   set_module_rw()
> 
> It should modify everything: init, core, text, and data but only
> the ro/rw flags.

Even that naming is not without its problems.  For example,
set_module_ro() is false advertising -- it wouldn't change *all* module
memory to be read-only.  (It wouldn't touch the r/w data areas.)

But I don't really care what the interfaces are called.  It's really
Rusty's call.  I just stuck to the existing naming convention in the
module code with the set/unset ro_nx stuff.

-- 
Josh

Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes

[Rusty Russell]

Josh Poimboeuf <jpoimboe@redhat.com> writes:
> On Fri, Nov 06, 2015 at 02:42:46PM +0100, Petr Mladek wrote:
>> naming schemes. What about adding into the public API?
>> 
>>   set_module_ro()
>>   set_module_rw()
>> 
>> It should modify everything: init, core, text, and data but only
>> the ro/rw flags.
>
> Even that naming is not without its problems.  For example,
> set_module_ro() is false advertising -- it wouldn't change *all* module
> memory to be read-only.  (It wouldn't touch the r/w data areas.)
>
> But I don't really care what the interfaces are called.  It's really
> Rusty's call.  I just stuck to the existing naming convention in the
> module code with the set/unset ro_nx stuff.

I'm looking at the ro/nx stuff now, and it seems like a mess.  For
example, set_all_modules_text_rw() and set_all_modules_text_ro() use
mod->core_text_size instead of mod->core_ro_size.  Which is probably
what they want (ftrace doesn't care about rodata) but pretty damn
confusing.

So I'll extend your cleanup.  Expect a patch for testing RSN...

Thanks,
Rusty.