From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752882AbdHJPaC (ORCPT ); Thu, 10 Aug 2017 11:30:02 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:37766 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752782AbdHJPaA (ORCPT ); Thu, 10 Aug 2017 11:30:00 -0400 Date: Thu, 10 Aug 2017 17:29:56 +0200 From: Ingo Molnar To: Masami Hiramatsu 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: <20170810152956.n3dhwjtv2nyxmrpr@gmail.com> References: <150150825899.31981.16898805685027748636.stgit@devbox> <150150832476.31981.15380750058308167313.stgit@devbox> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <150150832476.31981.15380750058308167313.stgit@devbox> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * 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. 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 ... > 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. Thanks, Ingo