From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941117AbdDTGEU convert rfc822-to-8bit (ORCPT ); Thu, 20 Apr 2017 02:04:20 -0400 Received: from ozlabs.org ([103.22.144.67]:37181 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S939714AbdDTGES (ORCPT ); Thu, 20 Apr 2017 02:04:18 -0400 From: Michael Ellerman To: christophe leroy , Benjamin Herrenschmidt , Paul Mackerras , Scott Wood Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/3] powerpc/mm: Implement CONFIG_DEBUG_RODATA on PPC32 In-Reply-To: <694d02a4-d2bd-a276-928f-0dd5189a936a@c-s.fr> References: <87inm05we4.fsf@concordia.ellerman.id.au> <694d02a4-d2bd-a276-928f-0dd5189a936a@c-s.fr> User-Agent: Notmuch/0.21 (https://notmuchmail.org) Date: Thu, 20 Apr 2017 16:04:14 +1000 Message-ID: <87shl34ntt.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org christophe leroy writes: > Le 19/04/2017 à 16:22, Christophe LEROY a écrit : >> >> >> Le 19/04/2017 à 16:01, Michael Ellerman a écrit : >>> Christophe Leroy writes: >>> >>>> diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c >>>> index 32509de6ce4c..4af81fb23653 100644 >>>> --- a/arch/powerpc/kernel/ftrace.c >>>> +++ b/arch/powerpc/kernel/ftrace.c >>>> @@ -526,7 +526,9 @@ void ftrace_replace_code(int enable) >>>> */ >>>> void arch_ftrace_update_code(int command) >>>> { >>>> + set_kernel_text_rw(); >>>> ftrace_modify_all_code(command); >>>> + set_kernel_text_ro(); >>>> } >>> >>> I'm not sure that's the right place for that. >> >> I took arch/arm/ as model. It does the following. Is that wrong ? It's not wrong, it's just not optimal. You're setting all text RW to modify one instruction, which is more work than necessary and also creates a larger attack window. > Could you provide more details on what you have seen on other arches ? I > didn't notice anything related in any other arches' ftrace_modify_code() I was looking at arm64 specifically: static int ftrace_modify_code(unsigned long pc, u32 old, u32 new, bool validate) { ... if (aarch64_insn_patch_text_nosync((void *)pc, new)) return -EPERM; return 0; } int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn) { ... ret = aarch64_insn_write(tp, insn); if (ret == 0) flush_icache_range((uintptr_t)tp, (uintptr_t)tp + AARCH64_INSN_SIZE); return ret; } static int __kprobes __aarch64_insn_write(void *addr, u32 insn) { ... raw_spin_lock_irqsave(&patch_lock, flags); waddr = patch_map(addr, FIX_TEXT_POKE0); ret = probe_kernel_write(waddr, &insn, AARCH64_INSN_SIZE); patch_unmap(FIX_TEXT_POKE0); raw_spin_unlock_irqrestore(&patch_lock, flags); return ret; } So if I'm reading it right they actually create a new RW mapping, patch that, and then unmap, before flushing the icache. That's definitely the nicest approach, but is maybe more work than you signed up for :) But even if you just shift your logic into ftrace_modify_code(), you then have the ip, so you can call change_page_attr() on the single page you're patching rather than all of text. cheers