[BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline

[BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline

[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 <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);
 }

Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline

[Luis R. Rodriguez]

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

Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline

[Masami Hiramatsu]

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>

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

[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 <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"

Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline

[Jessica Yu]

+++ 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   

Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline

[Luis R. Rodriguez]

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

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

[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"
> 
> 
>