From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751406AbdFAS0L (ORCPT ); Thu, 1 Jun 2017 14:26:11 -0400 Received: from mx2.suse.de ([195.135.220.15]:33180 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751145AbdFAS0J (ORCPT ); Thu, 1 Jun 2017 14:26:09 -0400 Date: Thu, 1 Jun 2017 20:26:01 +0200 From: "Luis R. Rodriguez" To: Jessica Yu Cc: Masami Hiramatsu , "Luis R. Rodriguez" , Ingo Molnar , Thomas Gleixner , Steven Rostedt , Kees Cook , LKML , x86@kernel.org, Peter Zijlstra , Rusty Russell , Alexei Starovoitov , Daniel Borkmann Subject: Re: [BUGFIX PATCH] kprobes/x86: Fix to set RWX bits correctly before releasing trampoline Message-ID: <20170601182601.GM8951@wotan.suse.de> References: <149570868652.3518.14120169373590420503.stgit@devbox> <20170525172426.GW8951@wotan.suse.de> <20170526092443.7090278fef461d3085bf9e58@kernel.org> <20170528014610.2rt2xmfv3fshqlov@jeyu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170528014610.2rt2xmfv3fshqlov@jeyu> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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" 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 > > > > 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 > > > > #include > > > > #include > > > > +#include > > > > > > > > #include > > > > #include > > > > @@ -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