* [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline @ 2017-05-25 10:38 Masami Hiramatsu 2017-05-25 17:24 ` Luis R. Rodriguez 2017-05-26 4:44 ` [PATCH] selftests/ftrace: Add a testcase for many kprobe events Masami Hiramatsu 0 siblings, 2 replies; 7+ messages in thread From: Masami Hiramatsu @ 2017-05-25 10:38 UTC (permalink / raw) To: Ingo Molnar Cc: Thomas Gleixner, Steven Rostedt, Kees Cook, LKML, x86, Masami Hiramatsu, Luis R . Rodriguez, Peter Zijlstra 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 <mhiramat@kernel.org> 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(-) diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index 5b2bbfb..6b87780 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -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; diff --git a/kernel/kprobes.c b/kernel/kprobes.c index 9f60567..6756d75 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -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); } ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline 2017-05-25 10:38 [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline Masami Hiramatsu @ 2017-05-25 17:24 ` Luis R. Rodriguez 2017-05-26 0:24 ` Masami Hiramatsu 2017-05-26 4:44 ` [PATCH] selftests/ftrace: Add a testcase for many kprobe events Masami Hiramatsu 1 sibling, 1 reply; 7+ messages in thread From: Luis R. Rodriguez @ 2017-05-25 17:24 UTC (permalink / raw) To: Masami Hiramatsu Cc: Ingo Molnar, Thomas Gleixner, Steven Rostedt, Kees Cook, LKML, x86, Luis R . Rodriguez, Peter Zijlstra On Thu, May 25, 2017 at 07:38:17PM +0900, Masami Hiramatsu wrote: > 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 <mhiramat@kernel.org> > 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(-) > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c > index 5b2bbfb..6b87780 100644 > --- a/arch/x86/kernel/kprobes/core.c > +++ b/arch/x86/kernel/kprobes/core.c > @@ -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); > +} Is this needed for all module_memfree() ? If so should / could it just do it for alloc users ? Luis ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline 2017-05-25 17:24 ` Luis R. Rodriguez @ 2017-05-26 0:24 ` Masami Hiramatsu 2017-05-28 1:46 ` Jessica Yu 0 siblings, 1 reply; 7+ messages in thread From: Masami Hiramatsu @ 2017-05-26 0:24 UTC (permalink / raw) To: Luis R. Rodriguez Cc: Ingo Molnar, Thomas Gleixner, Steven Rostedt, Kees Cook, LKML, x86, Peter Zijlstra, Jessica Yu, Rusty Russell, Alexei Starovoitov, Daniel Borkmann On Thu, 25 May 2017 19:24:26 +0200 "Luis R. Rodriguez" <mcgrof@kernel.org> wrote: > On Thu, May 25, 2017 at 07:38:17PM +0900, Masami Hiramatsu wrote: > > 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 <mhiramat@kernel.org> > > 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(-) > > > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c > > index 5b2bbfb..6b87780 100644 > > --- a/arch/x86/kernel/kprobes/core.c > > +++ b/arch/x86/kernel/kprobes/core.c > > @@ -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); > > +} > > 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 -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline 2017-05-26 0:24 ` Masami Hiramatsu @ 2017-05-28 1:46 ` Jessica Yu 2017-06-01 18:26 ` Luis R. Rodriguez 0 siblings, 1 reply; 7+ messages in thread From: Jessica Yu @ 2017-05-28 1:46 UTC (permalink / raw) To: Masami Hiramatsu Cc: Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner, Steven Rostedt, Kees Cook, LKML, x86, Peter Zijlstra, Rusty Russell, Alexei Starovoitov, Daniel Borkmann +++ Masami Hiramatsu [26/05/17 09:24 +0900]: >On Thu, 25 May 2017 19:24:26 +0200 >"Luis R. Rodriguez" <mcgrof@kernel.org> wrote: > >> On Thu, May 25, 2017 at 07:38:17PM +0900, Masami Hiramatsu wrote: >> > 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 <mhiramat@kernel.org> >> > 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(-) >> > >> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c >> > index 5b2bbfb..6b87780 100644 >> > --- a/arch/x86/kernel/kprobes/core.c >> > +++ b/arch/x86/kernel/kprobes/core.c >> > @@ -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); >> > +} >> >> 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. 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline 2017-05-28 1:46 ` Jessica Yu @ 2017-06-01 18:26 ` Luis R. Rodriguez 0 siblings, 0 replies; 7+ messages in thread From: Luis R. Rodriguez @ 2017-06-01 18:26 UTC (permalink / raw) To: Jessica Yu Cc: Masami Hiramatsu, Luis R. Rodriguez, Ingo Molnar, Thomas Gleixner, Steven Rostedt, Kees Cook, LKML, x86, Peter Zijlstra, Rusty Russell, Alexei Starovoitov, Daniel Borkmann On Sat, May 27, 2017 at 06:46:12PM -0700, Jessica Yu wrote: > +++ Masami Hiramatsu [26/05/17 09:24 +0900]: > > On Thu, 25 May 2017 19:24:26 +0200 > > "Luis R. Rodriguez" <mcgrof@kernel.org> wrote: > > > > > On Thu, May 25, 2017 at 07:38:17PM +0900, Masami Hiramatsu wrote: > > > > 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 <mhiramat@kernel.org> > > > > 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(-) > > > > > > > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c > > > > index 5b2bbfb..6b87780 100644 > > > > --- a/arch/x86/kernel/kprobes/core.c > > > > +++ b/arch/x86/kernel/kprobes/core.c > > > > @@ -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); > > > > +} > > > > > > 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. > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] selftests/ftrace: Add a testcase for many kprobe events 2017-05-25 10:38 [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline Masami Hiramatsu 2017-05-25 17:24 ` Luis R. Rodriguez @ 2017-05-26 4:44 ` Masami Hiramatsu 2017-06-02 21:37 ` Shuah Khan 1 sibling, 1 reply; 7+ messages in thread From: Masami Hiramatsu @ 2017-05-26 4:44 UTC (permalink / raw) To: Ingo Molnar, Shuah Khan Cc: Thomas Gleixner, Steven Rostedt, Kees Cook, LKML, x86, Masami Hiramatsu, Luis R . Rodriguez, Peter Zijlstra, linux-kselftest 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 <mhiramat@kernel.org> Suggested-by: Steven Rostedt <rostedt@goodmis.org> --- 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 diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc new file mode 100644 index 0000000..f4d1ff7 --- /dev/null +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc @@ -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" ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] selftests/ftrace: Add a testcase for many kprobe events 2017-05-26 4:44 ` [PATCH] selftests/ftrace: Add a testcase for many kprobe events Masami Hiramatsu @ 2017-06-02 21:37 ` Shuah Khan 0 siblings, 0 replies; 7+ messages in thread From: Shuah Khan @ 2017-06-02 21:37 UTC (permalink / raw) To: Masami Hiramatsu, Ingo Molnar Cc: Thomas Gleixner, Steven Rostedt, Kees Cook, LKML, x86, Luis R . Rodriguez, Peter Zijlstra, linux-kselftest, Shuah Khan On 05/25/2017 10:44 PM, Masami Hiramatsu wrote: > 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 <mhiramat@kernel.org> > Suggested-by: Steven Rostedt <rostedt@goodmis.org> Thanks. Applied to linux-kselftest next for 4.13-rc1 -- Shuah > --- > 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 > > diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > new file mode 100644 > index 0000000..f4d1ff7 > --- /dev/null > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/multiple_kprobes.tc > @@ -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" > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-06-02 21:37 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-25 10:38 [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline Masami Hiramatsu 2017-05-25 17:24 ` Luis R. Rodriguez 2017-05-26 0:24 ` Masami Hiramatsu 2017-05-28 1:46 ` Jessica Yu 2017-06-01 18:26 ` Luis R. Rodriguez 2017-05-26 4:44 ` [PATCH] selftests/ftrace: Add a testcase for many kprobe events Masami Hiramatsu 2017-06-02 21:37 ` Shuah Khan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).