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

[Masami Hiramatsu <mhiramat@kernel.org>]

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>