From: Michael Ellerman <mpe@ellerman.id.au> To: christophe leroy <christophe.leroy@c-s.fr>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Scott Wood <oss@buserror.net> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/3] powerpc/mm: Implement CONFIG_DEBUG_RODATA on PPC32 Date: Thu, 20 Apr 2017 16:04:14 +1000 [thread overview] Message-ID: <87shl34ntt.fsf@concordia.ellerman.id.au> (raw) In-Reply-To: <694d02a4-d2bd-a276-928f-0dd5189a936a@c-s.fr> christophe leroy <christophe.leroy@c-s.fr> writes: > Le 19/04/2017 à 16:22, Christophe LEROY a écrit : >> >> >> Le 19/04/2017 à 16:01, Michael Ellerman a écrit : >>> Christophe Leroy <christophe.leroy@c-s.fr> 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
WARNING: multiple messages have this Message-ID (diff)
From: Michael Ellerman <mpe@ellerman.id.au> To: christophe leroy <christophe.leroy@c-s.fr>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Scott Wood <oss@buserror.net> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/3] powerpc/mm: Implement CONFIG_DEBUG_RODATA on PPC32 Date: Thu, 20 Apr 2017 16:04:14 +1000 [thread overview] Message-ID: <87shl34ntt.fsf@concordia.ellerman.id.au> (raw) In-Reply-To: <694d02a4-d2bd-a276-928f-0dd5189a936a@c-s.fr> christophe leroy <christophe.leroy@c-s.fr> writes: > Le 19/04/2017 =C3=A0 16:22, Christophe LEROY a =C3=A9crit : >> >> >> Le 19/04/2017 =C3=A0 16:01, Michael Ellerman a =C3=A9crit : >>> Christophe Leroy <christophe.leroy@c-s.fr> 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= =20 > 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 =3D aarch64_insn_write(tp, insn); if (ret =3D=3D 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 =3D patch_map(addr, FIX_TEXT_POKE0); ret =3D 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
next prev parent reply other threads:[~2017-04-20 6:04 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-04-19 10:59 [PATCH 0/3] powerpc/mm: Fix kernel protection and implement CONFIG_DEBUG_RODATA on PPC32 Christophe Leroy 2017-04-19 10:59 ` [PATCH 1/3] powerpc/mm: Ensure change_page_attr() doesn't invalidate pinned TLBs Christophe Leroy 2017-04-19 11:00 ` [PATCH 2/3] powerpc/mm: Fix kernel RAM protection after freeing unused memory on PPC32 Christophe Leroy 2017-04-19 11:00 ` [PATCH 3/3] powerpc/mm: Implement CONFIG_DEBUG_RODATA " Christophe Leroy 2017-04-19 14:01 ` Michael Ellerman 2017-04-19 14:22 ` Christophe LEROY 2017-04-19 18:36 ` christophe leroy 2017-04-20 6:04 ` Michael Ellerman [this message] 2017-04-20 6:04 ` Michael Ellerman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87shl34ntt.fsf@concordia.ellerman.id.au \ --to=mpe@ellerman.id.au \ --cc=benh@kernel.crashing.org \ --cc=christophe.leroy@c-s.fr \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=oss@buserror.net \ --cc=paulus@samba.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.