From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750793AbdE1BqQ (ORCPT ); Sat, 27 May 2017 21:46:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57988 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750762AbdE1BqO (ORCPT ); Sat, 27 May 2017 21:46:14 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 81F6C37EE1 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jeyu@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 81F6C37EE1 Date: Sat, 27 May 2017 18:46:12 -0700 From: Jessica Yu To: Masami Hiramatsu Cc: "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: <20170528014610.2rt2xmfv3fshqlov@jeyu> References: <149570868652.3518.14120169373590420503.stgit@devbox> <20170525172426.GW8951@wotan.suse.de> <20170526092443.7090278fef461d3085bf9e58@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20170526092443.7090278fef461d3085bf9e58@kernel.org> X-OS: Linux jeyu 4.11.0-rc2+ x86_64 User-Agent: NeoMutt/20161126 (1.7.1) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Sun, 28 May 2017 01:46:14 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ 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). Jessica