From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753827AbdHKXJ7 (ORCPT ); Fri, 11 Aug 2017 19:09:59 -0400 Received: from mail.kernel.org ([198.145.29.99]:37538 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753127AbdHKXJ5 (ORCPT ); Fri, 11 Aug 2017 19:09:57 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 13D2E22BCD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mhiramat@kernel.org Date: Sat, 12 Aug 2017 08:09:52 +0900 From: Masami Hiramatsu To: Ingo Molnar Cc: Ingo Molnar , "H . Peter Anvin" , x86@kernel.org, Ananth N Mavinakayanahalli , Anil S Keshavamurthy , "David S . Miller" , linux-kernel@vger.kernel.org Subject: Re: [PATCH -tip 1/2] kprobes/x86: Don't forget to set memory back to RO on failure Message-Id: <20170812080952.beb2d4d0b6574a2bc503784e@kernel.org> In-Reply-To: <20170810152956.n3dhwjtv2nyxmrpr@gmail.com> References: <150150825899.31981.16898805685027748636.stgit@devbox> <150150832476.31981.15380750058308167313.stgit@devbox> <20170810152956.n3dhwjtv2nyxmrpr@gmail.com> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.30; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 10 Aug 2017 17:29:56 +0200 Ingo Molnar wrote: > > * Masami Hiramatsu wrote: > > > Do not forget to set kprobes insn buffer memory back > > to RO on failure path. Without this fix, if there is > > an unexpected error on copying instructions, kprobes > > insn buffer kept RW, which can allow unexpected modifying > > instruction buffer. > > > > Fixes: d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only") > > Signed-off-by: Masami Hiramatsu > > --- > > arch/x86/kernel/kprobes/core.c | 4 +++- > > arch/x86/kernel/kprobes/opt.c | 1 + > > 2 files changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c > > index f0153714ddac..b16b10114e20 100644 > > --- a/arch/x86/kernel/kprobes/core.c > > +++ b/arch/x86/kernel/kprobes/core.c > > @@ -435,8 +435,10 @@ static int arch_copy_kprobe(struct kprobe *p) > > > > /* Copy an instruction with recovering if other optprobe modifies it.*/ > > len = __copy_instruction(p->ainsn.insn, p->addr, &insn); > > - if (!len) > > + if (!len) { > > + set_memory_ro((unsigned long)p->ainsn.insn & PAGE_MASK, 1); > > return -EINVAL; > > + } > > So variable usage in the arch_copy_kprobe() is really awful: 'p->ainsn.insn' is > repeated 6 times! > > Please consolidate all that via a helper variable. OK, I'll cleanup it soon. > > Also, regarding the merits of the patch: do we know that the page in question was > RO before? If it was RW we'll unexpectedly mark it RO here in the failure path ... No need to take care previous state, this page has to be RO after setup (even if it was failed), since the page is shared by other kprobes. If we missed it, insn buffers for other kprobes will be left in RW state. > > index 69ea0bc1cfa3..853614560a4f 100644 > > --- a/arch/x86/kernel/kprobes/opt.c > > +++ b/arch/x86/kernel/kprobes/opt.c > > @@ -368,6 +368,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, > > ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr); > > if (ret < 0) { > > __arch_remove_optimized_kprobe(op, 0); > > + set_memory_ro((unsigned long)buf & PAGE_MASK, 1); > > return ret; > > } > > op->optinsn.size = ret; > > Ditto. As same as above, this page is shared by other optprobes. Thank you, > > Thanks, > > Ingo -- Masami Hiramatsu