From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752085AbdHQJzh (ORCPT ); Thu, 17 Aug 2017 05:55:37 -0400 Received: from mail-wr0-f195.google.com ([209.85.128.195]:37820 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751848AbdHQJzf (ORCPT ); Thu, 17 Aug 2017 05:55:35 -0400 Date: Thu, 17 Aug 2017 11:55:30 +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 v2 0/2] kprobes/x86: RO text code bugfix and cleanup Message-ID: <20170817095530.zew2tppikufosf2u@gmail.com> References: <150249861299.18166.13400123503538960282.stgit@devbox> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <150249861299.18166.13400123503538960282.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: > Hi, > > This series fixes a kprobe-x86 bug related to RO text and > cleans up addressof operators. > > The first one is an obvious bug that misses to set memory > RO when the function fails. And the second one is just a > cleanup patch to remove addressof operators ("&") since > it is meaningless anymore. > > V2 has just a following update: > - [1/2] Use a helper variable instead of using p->ainsn.insn > directly. > > Thanks, > > --- > > Masami Hiramatsu (2): > kprobes/x86: Don't forget to set memory back to RO on failure > kprobes/x86: Remove addressof operators > > > arch/x86/include/asm/kprobes.h | 4 ++-- > arch/x86/kernel/kprobes/core.c | 15 +++++++++------ > arch/x86/kernel/kprobes/opt.c | 9 +++++---- > 3 files changed, 16 insertions(+), 12 deletions(-) So I'm totally opposed to the whole approach of modifying the permissions of the kernel text virtual memory area. Firstly, it's racy against other kernel subsystems: what happens if some other code patching mechanism does a similar 'mark RWX and then back to RX'? Who provides the synchronization? set_memory_*() certainly does not. Secondly, it's racy against attackers: if an attacker can time the attack to the time when a kprobe is installed, then the kernel is still vulnerable. So how about avoiding the problem altogether by patching the kernel not in its virtual text address, but in the direct mappings? Then page permissions won't have to be modified, and the whole solution will be more robust and secure. Is there anything I'm missing? Thanks, Ingo