[RFC 0/2] Consolidate patch_instruction

[RFC 0/2] Consolidate patch_instruction

[Balbir Singh]

patch_instruction is enhanced in this RFC to support
patching via a different virtual address (text_poke_area).
The mapping of text_poke_area->addr is RW and not RWX.
This way the mapping allows write for patching and then we tear
down the mapping. The downside is that we introduce a spinlock
which serializes our patching to one patch at a time.

In this patchset we also consolidate instruction changes
in kprobes to use patch_instruction().

Balbir Singh (2):
  powerpc/lib/code-patching: Enhance code patching
  powerpc/kprobes: Move kprobes over to patch_instruction

 arch/powerpc/kernel/kprobes.c    |  4 +-
 arch/powerpc/lib/code-patching.c | 88 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 86 insertions(+), 6 deletions(-)

-- 
2.9.3

[RFC 1/2] powerpc/lib/code-patching: Enhance code patching

[Balbir Singh]

Today our patching happens via direct copy and
patch_instruction. The patching code is well
contained in the sense that copying bits are limited.

While considering implementation of CONFIG_STRICT_RWX,
the first requirement is to a create another mapping
that will allow for patching. We create the window using
text_poke_area, allocated via get_vm_area(), which might
be an overkill. We can do per-cpu stuff as well. The
downside of these patches that patch_instruction is
now synchornized using a lock. Other arches do similar
things, but use fixmaps. The reason for not using
fixmaps is to make use of any randomization in the
future. The code also relies on set_pte_at and pte_clear
to do the appropriate tlb flushing.

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/lib/code-patching.c | 88 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 500b0f6..4aae016 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -16,19 +16,98 @@
 #include <asm/code-patching.h>
 #include <linux/uaccess.h>
 #include <linux/kprobes.h>
+#include <asm/pgtable.h>
+#include <asm/tlbflush.h>
 
+struct vm_struct *text_poke_area;
+static DEFINE_RAW_SPINLOCK(text_poke_lock);
 
-int patch_instruction(unsigned int *addr, unsigned int instr)
+/*
+ * This is an early_initcall and early_initcalls happen at the right time
+ * for us, after slab is enabled and before we mark ro pages R/O. In the
+ * future if get_vm_area is randomized, this will be more flexible than
+ * fixmap
+ */
+static int __init setup_text_poke_area(void)
 {
+	text_poke_area = get_vm_area(PAGE_SIZE, VM_ALLOC);
+	if (!text_poke_area) {
+		WARN_ONCE(1, "could not create area for mapping kernel addrs"
+				" which allow for patching kernel code\n");
+		return 0;
+	}
+	pr_info("text_poke area ready...\n");
+	raw_spin_lock_init(&text_poke_lock);
+	return 0;
+}
+
+/*
+ * This can be called for kernel text or a module.
+ */
+static int kernel_map_addr(void *addr)
+{
+	unsigned long pfn;
 	int err;
 
-	__put_user_size(instr, addr, 4, err);
+	if (is_vmalloc_addr(addr))
+		pfn = vmalloc_to_pfn(addr);
+	else
+		pfn = __pa_symbol(addr) >> PAGE_SHIFT;
+
+	err = map_kernel_page((unsigned long)text_poke_area->addr,
+				(pfn << PAGE_SHIFT), _PAGE_KERNEL_RW | _PAGE_PRESENT);
+	pr_devel("Mapped addr %p with pfn %lx\n", text_poke_area->addr, pfn);
 	if (err)
-		return err;
-	asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" : : "r" (addr));
+		return -1;
 	return 0;
 }
 
+static inline void kernel_unmap_addr(void *addr)
+{
+	pte_t *pte;
+	unsigned long kaddr = (unsigned long)addr;
+
+	pte = pte_offset_kernel(pmd_offset(pud_offset(pgd_offset_k(kaddr),
+				kaddr), kaddr), kaddr);
+	pr_devel("clearing mm %p, pte %p, kaddr %lx\n", &init_mm, pte, kaddr);
+	pte_clear(&init_mm, kaddr, pte);
+}
+
+int patch_instruction(unsigned int *addr, unsigned int instr)
+{
+	int err;
+	unsigned int *dest = NULL;
+	unsigned long flags;
+	unsigned long kaddr = (unsigned long)addr;
+
+	/*
+	 * During early early boot patch_instruction is called
+	 * when text_poke_area is not ready, but we still need
+	 * to allow patching. We just do the plain old patching
+	 */
+	if (!text_poke_area) {
+		__put_user_size(instr, addr, 4, err);
+		asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" : : "r" (addr));
+		return 0;
+	}
+
+	raw_spin_lock_irqsave(&text_poke_lock, flags);
+	if (kernel_map_addr(addr)) {
+		err = -1;
+		goto out;
+	}
+
+	dest = (unsigned int *)(text_poke_area->addr) +
+		((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
+	__put_user_size(instr, dest, 4, err);
+	asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" : : "r" (dest));
+	kernel_unmap_addr(text_poke_area->addr);
+out:
+	raw_spin_unlock_irqrestore(&text_poke_lock, flags);
+	return err;
+}
+NOKPROBE_SYMBOL(patch_instruction);
+
 int patch_branch(unsigned int *addr, unsigned long target, int flags)
 {
 	return patch_instruction(addr, create_branch(addr, target, flags));
@@ -514,3 +593,4 @@ static int __init test_code_patching(void)
 late_initcall(test_code_patching);
 
 #endif /* CONFIG_CODE_PATCHING_SELFTEST */
+early_initcall(setup_text_poke_area);
-- 
2.9.3

[RFC 2/2] powerpc/kprobes: Move kprobes over to patch_instruction

[Balbir Singh]

arch_arm/disarm_probe use direct assignment for copying
instructions, replace them with patch_instruction

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/kernel/kprobes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 160ae0f..5e1fa86 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -158,7 +158,7 @@ NOKPROBE_SYMBOL(arch_prepare_kprobe);
 
 void arch_arm_kprobe(struct kprobe *p)
 {
-	*p->addr = BREAKPOINT_INSTRUCTION;
+	patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
 	flush_icache_range((unsigned long) p->addr,
 			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
@@ -166,7 +166,7 @@ NOKPROBE_SYMBOL(arch_arm_kprobe);
 
 void arch_disarm_kprobe(struct kprobe *p)
 {
-	*p->addr = p->opcode;
+	patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
 	flush_icache_range((unsigned long) p->addr,
 			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
-- 
2.9.3

Re: [RFC 0/2] Consolidate patch_instruction

[Anshuman Khandual]

On 05/16/2017 09:19 AM, Balbir Singh wrote:
> patch_instruction is enhanced in this RFC to support
> patching via a different virtual address (text_poke_area).

Why writing instruction directly into the address is not
sufficient and need to go through this virtual address ?

> The mapping of text_poke_area->addr is RW and not RWX.
> This way the mapping allows write for patching and then we tear
> down the mapping. The downside is that we introduce a spinlock
> which serializes our patching to one patch at a time.

So whats the benifits we get otherwise in this approach when
we are adding a new lock into the equation.

Re: [RFC 2/2] powerpc/kprobes: Move kprobes over to patch_instruction

[Naveen N. Rao]

On 2017/05/16 01:49PM, Balbir Singh wrote:
> arch_arm/disarm_probe use direct assignment for copying
> instructions, replace them with patch_instruction

Thanks for doing this!

We will also have to convert optprobes and ftrace to use 
patch_instruction, but that can be done once the basic infrastructure is 
in.

Regards,
Naveen

Re: [RFC 0/2] Consolidate patch_instruction

[Naveen N. Rao]

On 2017/05/16 10:56AM, Anshuman Khandual wrote:
> On 05/16/2017 09:19 AM, Balbir Singh wrote:
> > patch_instruction is enhanced in this RFC to support
> > patching via a different virtual address (text_poke_area).
> 
> Why writing instruction directly into the address is not
> sufficient and need to go through this virtual address ?

To enable KERNEL_STRICT_RWX and map all of kernel text to be read-only?

> 
> > The mapping of text_poke_area->addr is RW and not RWX.
> > This way the mapping allows write for patching and then we tear
> > down the mapping. The downside is that we introduce a spinlock
> > which serializes our patching to one patch at a time.
> 
> So whats the benifits we get otherwise in this approach when
> we are adding a new lock into the equation.

Instruction patching isn't performance critical, so the slow down is 
likely not noticeable. Marking kernel text read-only helps harden the 
kernel by catching unintended code modifications whether through 
exploits or through bugs.

- Naveen

Re: [RFC 0/2] Consolidate patch_instruction

[LEROY Christophe]

Balbir Singh <bsingharora@gmail.com> a =C3=A9crit=C2=A0:

> patch_instruction is enhanced in this RFC to support
> patching via a different virtual address (text_poke_area).
> The mapping of text_poke_area->addr is RW and not RWX.
> This way the mapping allows write for patching and then we tear
> down the mapping. The downside is that we introduce a spinlock
> which serializes our patching to one patch at a time.

Very nice patch, would fit great with my patch for impmementing=20=20
CONFIG_DEBUG_RODATA=20(https://patchwork.ozlabs.org/patch/754289 ).
Would avoid having to set the text area back to RW for patching

Christophe

>
> In this patchset we also consolidate instruction changes
> in kprobes to use patch_instruction().
>
> Balbir Singh (2):
>   powerpc/lib/code-patching: Enhance code patching
>   powerpc/kprobes: Move kprobes over to patch_instruction
>
>  arch/powerpc/kernel/kprobes.c    |  4 +-
>  arch/powerpc/lib/code-patching.c | 88=20=20
>=20++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 86 insertions(+), 6 deletions(-)
>
> --
> 2.9.3

Re: [RFC 0/2] Consolidate patch_instruction

[Balbir Singh]

On Tue, 2017-05-16 at 19:11 +0530, Naveen N. Rao wrote:
> On 2017/05/16 10:56AM, Anshuman Khandual wrote:
> > On 05/16/2017 09:19 AM, Balbir Singh wrote:
> > > patch_instruction is enhanced in this RFC to support
> > > patching via a different virtual address (text_poke_area).
> > 
> > Why writing instruction directly into the address is not
> > sufficient and need to go through this virtual address ?
> 
> To enable KERNEL_STRICT_RWX and map all of kernel text to be read-only?
>

Precisely, the rest of the bits are still being developed.
 
> > 
> > > The mapping of text_poke_area->addr is RW and not RWX.
> > > This way the mapping allows write for patching and then we tear
> > > down the mapping. The downside is that we introduce a spinlock
> > > which serializes our patching to one patch at a time.
> > 
> > So whats the benifits we get otherwise in this approach when
> > we are adding a new lock into the equation.
> 
> Instruction patching isn't performance critical, so the slow down is 
> likely not noticeable. Marking kernel text read-only helps harden the 
> kernel by catching unintended code modifications whether through 
> exploits or through bugs.
>

Precisely!

Balbir Singh. 

Re: [RFC 2/2] powerpc/kprobes: Move kprobes over to patch_instruction

[Balbir Singh]

On Tue, 2017-05-16 at 19:05 +0530, Naveen N. Rao wrote:
> On 2017/05/16 01:49PM, Balbir Singh wrote:
> > arch_arm/disarm_probe use direct assignment for copying
> > instructions, replace them with patch_instruction
> 
> Thanks for doing this!
> 
> We will also have to convert optprobes and ftrace to use 
> patch_instruction, but that can be done once the basic infrastructure is 
> in.
>

I think these patches can go in without even patch 1. I looked quickly at
optprobes and ftrace and thought they were already using patch_instruction
(ftrace_modify_code() and arch_optimize_kprobes()), are there other paths
I missed?

Balbir Singh
 

Re: [RFC 0/2] Consolidate patch_instruction

[Balbir Singh]

On Tue, 2017-05-16 at 22:20 +0200, LEROY Christophe wrote:
> Balbir Singh <bsingharora@gmail.com> a écrit :
> 
> > patch_instruction is enhanced in this RFC to support
> > patching via a different virtual address (text_poke_area).
> > The mapping of text_poke_area->addr is RW and not RWX.
> > This way the mapping allows write for patching and then we tear
> > down the mapping. The downside is that we introduce a spinlock
> > which serializes our patching to one patch at a time.
> 
> Very nice patch, would fit great with my patch for impmementing  
> CONFIG_DEBUG_RODATA (https://patchwork.ozlabs.org/patch/754289 ).
> Would avoid having to set the text area back to RW for patching
> 

Awesome! It seems like you have some of the work for CONFIG_STRICT_KERNEL_RWX
any reason why this is under CONFIG_DEBUG_RODATA? But I think there is
reuse capability across the future patches and the current set.

Cheers,
Balbir  Singh.

Re: [RFC 0/2] Consolidate patch_instruction

[LEROY Christophe]

Balbir Singh <bsingharora@gmail.com> a =C3=A9crit=C2=A0:

> On Tue, 2017-05-16 at 22:20 +0200, LEROY Christophe wrote:
>> Balbir Singh <bsingharora@gmail.com> a =C3=A9crit=C2=A0:
>>
>> > patch_instruction is enhanced in this RFC to support
>> > patching via a different virtual address (text_poke_area).
>> > The mapping of text_poke_area->addr is RW and not RWX.
>> > This way the mapping allows write for patching and then we tear
>> > down the mapping. The downside is that we introduce a spinlock
>> > which serializes our patching to one patch at a time.
>>
>> Very nice patch, would fit great with my patch for impmementing
>> CONFIG_DEBUG_RODATA (https://patchwork.ozlabs.org/patch/754289 ).
>> Would avoid having to set the text area back to RW for patching
>>
>
> Awesome! It seems like you have some of the work for CONFIG_STRICT_KERNEL=
_RWX
> any reason why this is under CONFIG_DEBUG_RODATA? But I think there is
> reuse capability across the future patches and the current set.
>

Indeed it looks the same, see https://patchwork.kernel.org/patch/9554881

Christophe


> Cheers,
> Balbir  Singh.

Re: [RFC 2/2] powerpc/kprobes: Move kprobes over to patch_instruction

[Naveen N. Rao]

On 2017/05/17 11:40AM, Balbir Singh wrote:
> On Tue, 2017-05-16 at 19:05 +0530, Naveen N. Rao wrote:
> > On 2017/05/16 01:49PM, Balbir Singh wrote:
> > > arch_arm/disarm_probe use direct assignment for copying
> > > instructions, replace them with patch_instruction
> > 
> > Thanks for doing this!
> > 
> > We will also have to convert optprobes and ftrace to use 
> > patch_instruction, but that can be done once the basic infrastructure is 
> > in.
> >
> 
> I think these patches can go in without even patch 1. I looked quickly at
> optprobes and ftrace and thought they were already using patch_instruction
> (ftrace_modify_code() and arch_optimize_kprobes()), are there other paths
> I missed?

[Sorry for the delay...]

Yes, all the patch_*_insns() functions need to be converted since the 
area they patch comes from .text. There is also a memcpy() where we copy 
the instruction template in, so perhaps a patch_instructions() helper 
may be useful too.

- Naveen