linux-kernel - Re: [PATCH 3/3] powerpc/mm: Implement CONFIG_DEBUG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87shl34ntt.fsf@concordia.ellerman.id.au>
Date:   Thu, 20 Apr 2017 16:04:14 +1000
From:   Michael Ellerman <mpe@...erman.id.au>
To:     christophe leroy <christophe.leroy@....fr>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Scott Wood <oss@...error.net>
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] powerpc/mm: Implement CONFIG_DEBUG_RODATA on PPC32

christophe leroy <christophe.leroy@....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@....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