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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20170526092443.7090278fef461d3085bf9e58@kernel.org>
Date:   Fri, 26 May 2017 09:24:43 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Steven Rostedt <rostedt@...dmis.org>,
        Kees Cook <keescook@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Jessica Yu <jeyu@...hat.com>,
        Rusty Russell <rusty@...tcorp.com.au>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly
 before releasing trampoline

On Thu, 25 May 2017 19:24:26 +0200
"Luis R. Rodriguez" <mcgrof@...nel.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@...nel.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@...nel.org>