Skip to contentSkip to conversation

kprobes/x86: Fix to set RWX bits correctly before releasing trampoline

PATCH In review Masami Hiramatsu 4 participants 7 messages May 25, 2017 – Jun 2, 2017

Subsystems (4)
x86 Kselftest Tracing kprobes
lore
Share & tools
lore

Masami Hiramatsu

author maintainer
#0 lore raw

PATCH 1/1 kprobes/x86: Fix to set RWX bits correctly before releasing trampoline

Fix kprobes to set(recover) RWX bits correctly on trampoline buffer before releasing it. Releasing readonly page to module_memfree() crash the kernel.

Without this fix, if kprobes user register a bunch of kprobes in function body (since kprobes on function entry usually use ftrace) and unregister it, kernel hits a BUG and crash.

Signed-off-by Masami Hiramatsu <[email protected]>
Fixes d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
---
 arch/x86/kernel/kprobes/core.c |    9 +++++++++
 kernel/kprobes.c               |    2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)
arch/x86/kernel/kprobes/core.c +9 -0
index 5b2bbfb..6b87780 100644
@@ -52,6 +52,7 @@
 #include <linux/ftrace.h>
 #include <linux/frame.h>
 #include <linux/kasan.h>
+#include <linux/moduleloader.h>
 
 #include <asm/text-patching.h>
 #include <asm/cacheflush.h>

@@ -417,6 +418,14 @@ static void prepare_boost(struct kprobe *p, struct insn *insn)
 	}
 }
 
+/* Recover page to RW mode before releasing it */
+void free_insn_page(void *page)
+{
+	set_memory_nx((unsigned long)page & PAGE_MASK, 1);
+	set_memory_rw((unsigned long)page & PAGE_MASK, 1);
+	module_memfree(page);
+}
+
 static int arch_copy_kprobe(struct kprobe *p)
 {
 	struct insn insn;
kernel/kprobes.c +1 -1
index 9f60567..6756d75 100644
@@ -122,7 +122,7 @@ static void *alloc_insn_page(void)
 	return module_alloc(PAGE_SIZE);
 }
 
-static void free_insn_page(void *page)
+void __weak free_insn_page(void *page)
 {
 	module_memfree(page);
 }
#1 lore raw
Quoted 28 lines · Masami Hiramatsu

Fix kprobes to set(recover) RWX bits correctly on trampoline buffer before releasing it. Releasing readonly page to module_memfree() crash the kernel.

Without this fix, if kprobes user register a bunch of kprobes in function body (since kprobes on function entry usually use ftrace) and unregister it, kernel hits a BUG and crash.

Signed-off-by Masami Hiramatsu <[email protected]>
Fixes d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
---
 arch/x86/kernel/kprobes/core.c |    9 +++++++++
 kernel/kprobes.c               |    2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)
arch/x86/kernel/kprobes/core.c +1 -0
index 5b2bbfb..6b87780 100644
@@ -52,6 +52,7 @@
 #include <linux/ftrace.h>
 #include <linux/frame.h>
 #include <linux/kasan.h>
+#include <linux/moduleloader.h>
 
 #include <asm/text-patching.h>
 #include <asm/cacheflush.h>

arch/x86/kernel/kprobes/core.c static void prepare_boost(struct kprobe *p, struct insn *insn) · line 427

Quoted 10 lines
arch/x86/kernel/kprobes/core.c +7 -0
@@ -417,6 +418,14 @@ static void prepare_boost(struct kprobe *p, struct insn *insn)
 	}
 }
 
+/* Recover page to RW mode before releasing it */
+void free_insn_page(void *page)
+{
+	set_memory_nx((unsigned long)page & PAGE_MASK, 1);
+	set_memory_rw((unsigned long)page & PAGE_MASK, 1);
+	module_memfree(page);
+}

Is this needed for all module_memfree() ? If so should / could it just do it for alloc users ?

Luis

author maintainer
#2 lore raw
Quoted 32 lines · On Thu, 25 May 2017 19:24:26 +0200 "Luis R. Rodriguez" <[email protected]>…
Quoted 31 lines · Masami Hiramatsu

Fix kprobes to set(recover) RWX bits correctly on trampoline buffer before releasing it. Releasing readonly page to module_memfree() crash the kernel.

Without this fix, if kprobes user register a bunch of kprobes in function body (since kprobes on function entry usually use ftrace) and unregister it, kernel hits a BUG and crash.

Signed-off-by Masami Hiramatsu <[email protected]>
Fixes d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
---
 arch/x86/kernel/kprobes/core.c |    9 +++++++++
 kernel/kprobes.c               |    2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)
arch/x86/kernel/kprobes/core.c +1 -0
index 5b2bbfb..6b87780 100644
@@ -52,6 +52,7 @@
 #include <linux/ftrace.h>
 #include <linux/frame.h>
 #include <linux/kasan.h>
+#include <linux/moduleloader.h>
 
 #include <asm/text-patching.h>
 #include <asm/cacheflush.h>

@@ -417,6 +418,14 @@ static void prepare_boost(struct kprobe *p, struct insn *insn)
 	}
 }
Quoted 10 lines

arch/x86/kernel/kprobes/core.c static void prepare_boost(struct kprobe *p, struct insn *insn) · line 427

Quoted 7 lines
arch/x86/kernel/kprobes/core.c +7 -0
+/* Recover page to RW mode before releasing it */
+void free_insn_page(void *page)
+{
+	set_memory_nx((unsigned long)page & PAGE_MASK, 1);
+	set_memory_rw((unsigned long)page & PAGE_MASK, 1);
+	module_memfree(page);
+}

Is this needed for all module_memfree() ? If so should / could it just do it for alloc users ?

Hmm, would you mean setting those bits in module_memfree()?
I think it should be discussed with other users, kmodule, bpf and ftrace. It could be, but I'm not so sure about that because setting nx timing would be critical for some users. As far as I can see, for ftrace and kprobes, that is OK.

Thank y

Signature · 1 line
Masami Hiramatsu <[email protected]>
#3 lore raw

+++ Masami Hiramatsu [26/05/17 09:24 +0900]:

Quoted 46 lines
Quoted 32 lines · On Thu, 25 May 2017 19:24:26 +0200 "Luis R. Rodriguez" <[email protected]>…
Quoted 31 lines · Masami Hiramatsu

Fix kprobes to set(recover) RWX bits correctly on trampoline buffer before releasing it. Releasing readonly page to module_memfree() crash the kernel.

Without this fix, if kprobes user register a bunch of kprobes in function body (since kprobes on function entry usually use ftrace) and unregister it, kernel hits a BUG and crash.

Signed-off-by Masami Hiramatsu <[email protected]>
Fixes d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
---
 arch/x86/kernel/kprobes/core.c |    9 +++++++++
 kernel/kprobes.c               |    2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)
arch/x86/kernel/kprobes/core.c +1 -0
index 5b2bbfb..6b87780 100644
@@ -52,6 +52,7 @@
 #include <linux/ftrace.h>
 #include <linux/frame.h>
 #include <linux/kasan.h>
+#include <linux/moduleloader.h>
 
 #include <asm/text-patching.h>
 #include <asm/cacheflush.h>

@@ -417,6 +418,14 @@ static void prepare_boost(struct kprobe *p, struct insn *insn)
 	}
 }
Quoted 10 lines

arch/x86/kernel/kprobes/core.c static void prepare_boost(struct kprobe *p, struct insn *insn) · line 427

Quoted 7 lines
arch/x86/kernel/kprobes/core.c +7 -0
+/* Recover page to RW mode before releasing it */
+void free_insn_page(void *page)
+{
+	set_memory_nx((unsigned long)page & PAGE_MASK, 1);
+	set_memory_rw((unsigned long)page & PAGE_MASK, 1);
+	module_memfree(page);
+}

Is this needed for all module_memfree() ? If so should / could it just do it for alloc users ?

Quoted 5 lines

Hmm, would you mean setting those bits in module_memfree()?
I think it should be discussed with other users, kmodule, bpf and ftrace. It could be, but I'm not so sure about that because setting nx timing would be critical for some users. As far as I can see, for ftrace and kprobes, that is OK.

Memory does need to be rw before calling module_memfree(), although I think it might be better leave that responsibility/flexibility to the callers, instead of blanket calls to set_memory_rw/x. At least in the case of the module loader, we have finer-grained control of page protections; not all pages within the module_alloc'd region need set_memory_rw/x to be called before freeing (see disable_ro_nx() in module.c).

Jessica

#4 lore raw
Quoted 53 lines · Jessica Yu

+++ Masami Hiramatsu [26/05/17 09:24 +0900]:

Quoted 46 lines
Quoted 32 lines · On Thu, 25 May 2017 19:24:26 +0200 "Luis R. Rodriguez" <[email protected]>…
Quoted 31 lines · Masami Hiramatsu

Fix kprobes to set(recover) RWX bits correctly on trampoline buffer before releasing it. Releasing readonly page to module_memfree() crash the kernel.

Without this fix, if kprobes user register a bunch of kprobes in function body (since kprobes on function entry usually use ftrace) and unregister it, kernel hits a BUG and crash.

Signed-off-by Masami Hiramatsu <[email protected]>
Fixes d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")
---
 arch/x86/kernel/kprobes/core.c |    9 +++++++++
 kernel/kprobes.c               |    2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)
arch/x86/kernel/kprobes/core.c +1 -0
index 5b2bbfb..6b87780 100644
@@ -52,6 +52,7 @@
 #include <linux/ftrace.h>
 #include <linux/frame.h>
 #include <linux/kasan.h>
+#include <linux/moduleloader.h>
 
 #include <asm/text-patching.h>
 #include <asm/cacheflush.h>

@@ -417,6 +418,14 @@ static void prepare_boost(struct kprobe *p, struct insn *insn)
 	}
 }
Quoted 10 lines

arch/x86/kernel/kprobes/core.c static void prepare_boost(struct kprobe *p, struct insn *insn) · line 427

Quoted 7 lines
arch/x86/kernel/kprobes/core.c +7 -0
+/* Recover page to RW mode before releasing it */
+void free_insn_page(void *page)
+{
+	set_memory_nx((unsigned long)page & PAGE_MASK, 1);
+	set_memory_rw((unsigned long)page & PAGE_MASK, 1);
+	module_memfree(page);
+}

Is this needed for all module_memfree() ? If so should / could it just do it for alloc users ?

Quoted 5 lines

Hmm, would you mean setting those bits in module_memfree()?
I think it should be discussed with other users, kmodule, bpf and ftrace. It could be, but I'm not so sure about that because setting nx timing would be critical for some users. As far as I can see, for ftrace and kprobes, that is OK.

Quoted 7 lines

Memory does need to be rw before calling module_memfree(), although I think it might be better leave that responsibility/flexibility to the callers, instead of blanket calls to set_memory_rw/x. At least in the case of the module loader, we have finer-grained control of page protections; not all pages within the module_alloc'd region need set_memory_rw/x to be called before freeing (see disable_ro_nx() in module.c).

Is the module loader just a special case? If so then a special free, say __module_memfree(), which *does* not the rw bit could be used by the module loader ?

Luis

author maintainer
#5 lore raw

PATCH 1/1 selftests/ftrace: Add a testcase for many kprobe events

In reply to Masami Hiramatsu

Add a testcase to test kprobes via ftrace interface with many concurrent kprobe events.

This tries to add many kprobe events (up to 256) on kernel functions. To avoid making ftrace-based kprobes (kprobes on fentry), it skips first N bytes (on x86 N=5, on ppc or arm N=4) of function entry. After that, it enables all those events, disable it, and remove it.

Since the unoptimization buffer reclaiming will
be delayed, after removing events, it will wait
enough time.

Signed-off-by Masami Hiramatsu <[email protected]>
Suggested-by Steven Rostedt <[email protected]>

---
This ensures following bug is fixed and no regression.

https://lkml.org/lkml/2017/5/25/218

---
 .../ftrace/test.d/kprobe/multiple_kprobes.tc       |   21 ++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc new file +21 -0
new file mode 100644
index 0000000..f4d1ff7
@@ -0,0 +1,21 @@
+#!/bin/sh
+# description: Register/unregister many kprobe events
+
+# ftrace fentry skip size depends on the machine architecture.
+# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc
+case `uname -m` in
+  x86_64|i[3456]86) OFFS=5;;
+  ppc*) OFFS=4;;
+  *) OFFS=0;;
+esac
+
+echo "Setup up to 256 kprobes"
+grep t /proc/kallsyms | cut -f3 -d" " | grep -v .*\\..* | \
+head -n 256 | while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
+
+echo 1 > events/kprobes/enable
+echo 0 > events/kprobes/enable
+echo > kprobe_events
+echo "Waiting for unoptimizing & freeing"
+sleep 5
+echo "Done"
maintainer
#6 lore raw

Re: [PATCH] selftests/ftrace: Add a testcase for many kprobe events

Quoted 10 lines · Masami Hiramatsu

Add a testcase to test kprobes via ftrace interface with many concurrent kprobe events.

This tries to add many kprobe events (up to 256) on kernel functions. To avoid making ftrace-based kprobes (kprobes on fentry), it skips first N bytes (on x86 N=5, on ppc or arm N=4) of function entry. After that, it enables all those events, disable it, and remove it.

Quoted 6 lines

Since the unoptimization buffer reclaiming will
be delayed, after removing events, it will wait
enough time.

Signed-off-by Masami Hiramatsu <[email protected]>
Suggested-by Steven Rostedt <[email protected]>

Thanks. Applied to linux-kselftest next for 4.13-rc1

-- Shuah

Quoted 39 lines

---
This ensures following bug is fixed and no regression.

https://lkml.org/lkml/2017/5/25/218

---
 .../ftrace/test.d/kprobe/multiple_kprobes.tc       |   21 ++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc
tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc new file +21 -0
new file mode 100644
index 0000000..f4d1ff7
@@ -0,0 +1,21 @@
+#!/bin/sh
+# description: Register/unregister many kprobe events
+
+# ftrace fentry skip size depends on the machine architecture.
+# Currently HAVE_KPROBES_ON_FTRACE defined on x86 and powerpc
+case `uname -m` in
+  x86_64|i[3456]86) OFFS=5;;
+  ppc*) OFFS=4;;
+  *) OFFS=0;;
+esac
+
+echo "Setup up to 256 kprobes"
+grep t /proc/kallsyms | cut -f3 -d" " | grep -v .*\\..* | \
+head -n 256 | while read i; do echo p ${i}+${OFFS} ; done > kprobe_events ||:
+
+echo 1 > events/kprobes/enable
+echo 0 > events/kprobes/enable
+echo > kprobe_events
+echo "Waiting for unoptimizing & freeing"
+sleep 5
+echo "Done"