[RFC 0/2] 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(-)

-- 
2.9.3

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

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

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.

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

On 2017/05/16 10:56AM, Anshuman Khandual wrote:

On 05/16/2017 09:19 AM, Balbir Singh wrote:

quoted

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

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 [off-list ref] 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

On Tue, 2017-05-16 at 19:11 +0530, Naveen N. Rao wrote:

On 2017/05/16 10:56AM, Anshuman Khandual wrote:

quoted

On 05/16/2017 09:19 AM, Balbir Singh wrote:

quoted

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

quoted

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. 

On Tue, 2017-05-16 at 19:05 +0530, Naveen N. Rao wrote:

On 2017/05/16 01:49PM, Balbir Singh wrote:

quoted

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
 

On Tue, 2017-05-16 at 22:20 +0200, LEROY Christophe wrote:

Balbir Singh [off-list ref] a écrit :

quoted

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.

Balbir Singh [off-list ref] a =C3=A9crit=C2=A0:

On Tue, 2017-05-16 at 22:20 +0200, LEROY Christophe wrote:

quoted

Balbir Singh [off-list ref] a =C3=A9crit=C2=A0:

quoted

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.

On 2017/05/17 11:40AM, Balbir Singh wrote:

On Tue, 2017-05-16 at 19:05 +0530, Naveen N. Rao wrote:

quoted

On 2017/05/16 01:49PM, Balbir Singh wrote:

quoted

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