Consolidate patch_instruction

RFC In review Balbir Singh 4 participants 12 messages May 16, 2017 – May 30, 2017 PowerPC

Share & tools

Balbir Singh

author

2017-05-16 03:49 UTC #0 lore raw

cover Consolidate patch_instruction

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(-)

Signature · 1 line

2.9.3

Balbir Singh

author

2017-05-16 03:49 UTC #1 lore raw

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

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 <[email protected]>

---
 arch/powerpc/lib/code-patching.c | 88 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 4 deletions(-)

arch/powerpc/lib/code-patching.c +84 -4

index 500b0f6..4aae016 100644
@@ -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);

Signature · 1 line

2.9.3

Balbir Singh

author

2017-05-16 03:49 UTC #2 lore raw

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

In reply to Balbir Singh

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

Signed-off-by Balbir Singh <[email protected]>

---
 arch/powerpc/kernel/kprobes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/powerpc/kernel/kprobes.c +2 -2

index 160ae0f..5e1fa86 100644
@@ -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));
 }

Signature · 1 line

2.9.3

Naveen N. Rao

2017-05-16 13:36 UTC #3 lore raw

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

Quoted 2 lines · Balbir Singh

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

Balbir Singh

author

2017-05-17 01:40 UTC #4 lore raw

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

Quoted 10 lines · Naveen N. Rao

Quoted 2 lines · Balbir Singh
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

Naveen N. Rao

2017-05-30 14:29 UTC #5 lore raw

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

Quoted 12 lines · Balbir Singh

Quoted 10 lines · Naveen N. Rao
Quoted 2 lines · Balbir Singh
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.

Quoted 4 lines

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

Anshuman Khandual

2017-05-16 05:27 UTC #6 lore raw

In reply to Balbir Singh

Quoted 2 lines · Balbir Singh

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 ?

Quoted 4 lines

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.

Naveen N. Rao

2017-05-16 13:42 UTC #7 lore raw

Quoted 6 lines · Anshuman Khandual

Quoted 2 lines · Balbir Singh
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?

Quoted 8 lines

Quoted 5 lines
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

Balbir Singh

author

2017-05-17 01:23 UTC #8 lore raw

Quoted 10 lines · Naveen N. Rao

Quoted 6 lines · Anshuman Khandual
Quoted 2 lines · Balbir Singh
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.

Quoted 6 lines

Quoted 6 lines
Quoted 5 lines
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.

Quoted 8 lines

Quoted 2 lines
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.

LEROY Christophe

2017-05-16 20:21 UTC #9 lore raw

In reply to Balbir Singh

Balbir Singh <[email protected]> a =C3=A9crit=C2=A0:

Quoted 6 lines

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

Quoted 15 lines

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

Balbir Singh

author

2017-05-17 02:10 UTC #10 lore raw

Quoted 9 lines · LEROY Christophe

Balbir Singh <[email protected]> a écrit :
Quoted 6 lines
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.

Quoted 4 lines

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.

LEROY Christophe

2017-05-17 07:05 UTC #11 lore raw

Balbir Singh <[email protected]> a =C3=A9crit=C2=A0:

Quoted 10 lines · LEROY Christophe

Quoted 9 lines
Balbir Singh <[email protected]> a =C3=A9crit=C2=A0:
Quoted 6 lines
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.

Quoted 6 lines

Quoted 4 lines
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

Quoted 3 lines

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

Quoted 2 lines

Cheers,
Balbir Singh.