Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1263559 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] CONFIG_DEBUG_SET_MODULE_RONX bug fix and cleanups Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-05 22:20 +0100
    [PATCH v2 3/3] livepatch: Cleanup module page permission changes Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-05 22:20 +0100
      Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes Petr Mladek <pmladek@suse.com> - 2015-11-06 11:50 +0100
        Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-06 13:20 +0100
          Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes Petr Mladek <pmladek@suse.com> - 2015-11-06 14:50 +0100
            Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-06 18:20 +0100
              Re: [PATCH v2 3/3] livepatch: Cleanup module page permission changes Rusty Russell <rusty@rustcorp.com.au> - 2015-11-09 00:10 +0100
    [PATCH v2 1/3] livepatch: Fix crash with !CONFIG_DEBUG_SET_MODULE_RONX Josh Poimboeuf <jpoimboe@redhat.com> - 2015-11-05 22:30 +0100
      Re: [PATCH v2 1/3] livepatch: Fix crash with  !CONFIG_DEBUG_SET_MODULE_RONX Jiri Kosina <jikos@kernel.org> - 2015-11-06 11:20 +0100

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web