[PATCH 0/5] powerpc/kprobes: fixes and cleanups

[PATCH 0/5] powerpc/kprobes: fixes and cleanups

[Naveen N. Rao]

Various fixes and some code refactoring for kprobes on powerpc. The 
first patch fixes an invalid access if probing the first instruction in 
a kernel module. The rest are small cleanups. More details in the 
individual patches.

- Naveen

Naveen N. Rao (5):
  powerpc/kprobes: Fix validation of prefixed instructions across page
    boundary
  powerpc/kprobes: Roll IS_RFI() macro into IS_RFID()
  powerpc/kprobes: Check instruction validity during kprobe registration
  powerpc/kprobes: Refactor arch_prepare_kprobe()
  powerpc/kprobes: Warn if instruction patching failed

 arch/powerpc/include/asm/sstep.h |   7 +-
 arch/powerpc/kernel/kprobes.c    | 112 +++++++++++--------------------
 2 files changed, 43 insertions(+), 76 deletions(-)


base-commit: 3a81c0495fdb91fd9a9b4f617098c283131eeae1
-- 
2.30.2

[PATCH 1/5] powerpc/kprobes: Fix validation of prefixed instructions across page boundary

[Naveen N. Rao]

When checking if the probed instruction is the suffix of a prefixed
instruction, we access the instruction at the previous word. If the
probed instruction is the very first word of a module, we can end up
trying to access an invalid page. Fix this by skipping the check for all
instructions at the beginning of a page. Prefixed instructions cannot
cross a 64-byte boundary and as such, preventing probing on such
instructions is not worthwhile.

Cc: stable@vger.kernel.org # v5.8+
Fixes: b4657f7650babc ("powerpc/kprobes: Don't allow breakpoints on suffixes")
Reported-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.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 01ab2163659e4b..f611d9eb3562d7 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -108,7 +108,6 @@ int arch_prepare_kprobe(struct kprobe *p)
 	int ret = 0;
 	struct kprobe *prev;
 	struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
-	struct ppc_inst prefix = ppc_inst_read((struct ppc_inst *)(p->addr - 1));
 
 	if ((unsigned long)p->addr & 0x03) {
 		printk("Attempt to register kprobe at an unaligned address\n");
@@ -116,7 +115,8 @@ int arch_prepare_kprobe(struct kprobe *p)
 	} else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
 		printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
 		ret = -EINVAL;
-	} else if (ppc_inst_prefixed(prefix)) {
+	} else if ((unsigned long)p->addr & ~PAGE_MASK &&
+			ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)(p->addr - 1)))) {
 		printk("Cannot register a kprobe on the second word of prefixed instruction\n");
 		ret = -EINVAL;
 	}
-- 
2.30.2

[PATCH 2/5] powerpc/kprobes: Roll IS_RFI() macro into IS_RFID()

[Naveen N. Rao]

In kprobes and xmon, we should exclude both 32-bit and 64-bit variants
of mtmsr and rfi instructions from being stepped. Have IS_RFID() also
detect a rfi instruction similar to IS_MTMSRD().

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/sstep.h | 7 +++----
 arch/powerpc/kernel/kprobes.c    | 4 ++--
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 972ed0df154d60..1df867c2e054e5 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -13,12 +13,11 @@ struct pt_regs;
  * we don't allow putting a breakpoint on an mtmsrd instruction.
  * Similarly we don't allow breakpoints on rfid instructions.
  * These macros tell us if an instruction is a mtmsrd or rfid.
- * Note that IS_MTMSRD returns true for both an mtmsr (32-bit)
- * and an mtmsrd (64-bit).
+ * Note that these return true for both mtmsr/rfi (32-bit)
+ * and mtmsrd/rfid (64-bit).
  */
 #define IS_MTMSRD(instr)	((ppc_inst_val(instr) & 0xfc0007be) == 0x7c000124)
-#define IS_RFID(instr)		((ppc_inst_val(instr) & 0xfc0007fe) == 0x4c000024)
-#define IS_RFI(instr)		((ppc_inst_val(instr) & 0xfc0007fe) == 0x4c000064)
+#define IS_RFID(instr)		((ppc_inst_val(instr) & 0xfc0007be) == 0x4c000024)
 
 enum instruction_type {
 	COMPUTE,		/* arith/logical/CR op, etc. */
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index f611d9eb3562d7..b7b20875d34d91 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -112,8 +112,8 @@ int arch_prepare_kprobe(struct kprobe *p)
 	if ((unsigned long)p->addr & 0x03) {
 		printk("Attempt to register kprobe at an unaligned address\n");
 		ret = -EINVAL;
-	} else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
-		printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
+	} else if (IS_MTMSRD(insn) || IS_RFID(insn)) {
+		printk("Cannot register a kprobe on mtmsr[d]/rfi[d]\n");
 		ret = -EINVAL;
 	} else if ((unsigned long)p->addr & ~PAGE_MASK &&
 			ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)(p->addr - 1)))) {
-- 
2.30.2

[PATCH 3/5] powerpc/kprobes: Check instruction validity during kprobe registration

[Naveen N. Rao]

In trap-based (classic) kprobes, we try to emulate the probed
instruction so as to avoid having to single step it. We use a flag to
determine if the probed instruction was successfully emulated, so that we
can speed up subsequent probe hits.

However, emulate_step() doesn't differentiate between unknown
instructions and an emulation attempt that failed. As such, the current
heuristic is not of much use. Instead, use analyse_instr() during kprobe
registration to determine if the probed instruction can be decoded by
our instruction emulation infrastructure. For unknown instructions, we
can then directly single-step while for other instructions, we can
attempt to emulate and fall back to single stepping if that fails.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 62 +++++++++--------------------------
 1 file changed, 16 insertions(+), 46 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index b7b20875d34d91..bbef9e918ecb39 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -107,6 +107,8 @@ int arch_prepare_kprobe(struct kprobe *p)
 {
 	int ret = 0;
 	struct kprobe *prev;
+	struct pt_regs regs;
+	struct instruction_op op;
 	struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
 
 	if ((unsigned long)p->addr & 0x03) {
@@ -140,9 +142,18 @@ int arch_prepare_kprobe(struct kprobe *p)
 	if (!ret) {
 		patch_instruction((struct ppc_inst *)p->ainsn.insn, insn);
 		p->opcode = ppc_inst_val(insn);
+
+		/* Check if this is an instruction we recognise */
+		p->ainsn.boostable = 0;
+		memset(&regs, 0, sizeof(struct pt_regs));
+		regs.nip = (unsigned long)p->addr;
+		regs.msr = MSR_KERNEL;
+		ret = analyse_instr(&op, &regs, insn);
+		if (ret == 1 || (ret == 0 && GETTYPE(op.type) != UNKNOWN))
+			p->ainsn.boostable = 1;
+		ret = 0;
 	}
 
-	p->ainsn.boostable = 0;
 	return ret;
 }
 NOKPROBE_SYMBOL(arch_prepare_kprobe);
@@ -225,47 +236,6 @@ void arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(arch_prepare_kretprobe);
 
-static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
-{
-	int ret;
-	struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
-
-	/* regs->nip is also adjusted if emulate_step returns 1 */
-	ret = emulate_step(regs, insn);
-	if (ret > 0) {
-		/*
-		 * Once this instruction has been boosted
-		 * successfully, set the boostable flag
-		 */
-		if (unlikely(p->ainsn.boostable == 0))
-			p->ainsn.boostable = 1;
-	} else if (ret < 0) {
-		/*
-		 * We don't allow kprobes on mtmsr(d)/rfi(d), etc.
-		 * So, we should never get here... but, its still
-		 * good to catch them, just in case...
-		 */
-		printk("Can't step on instruction %s\n", ppc_inst_as_str(insn));
-		BUG();
-	} else {
-		/*
-		 * If we haven't previously emulated this instruction, then it
-		 * can't be boosted. Note it down so we don't try to do so again.
-		 *
-		 * If, however, we had emulated this instruction in the past,
-		 * then this is just an error with the current run (for
-		 * instance, exceptions due to a load/store). We return 0 so
-		 * that this is now single-stepped, but continue to try
-		 * emulating it in subsequent probe hits.
-		 */
-		if (unlikely(p->ainsn.boostable != 1))
-			p->ainsn.boostable = -1;
-	}
-
-	return ret;
-}
-NOKPROBE_SYMBOL(try_to_emulate);
-
 int kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *p;
@@ -334,8 +304,8 @@ int kprobe_handler(struct pt_regs *regs)
 		set_current_kprobe(p, regs, kcb);
 		kprobes_inc_nmissed_count(p);
 		kcb->kprobe_status = KPROBE_REENTER;
-		if (p->ainsn.boostable >= 0) {
-			ret = try_to_emulate(p, regs);
+		if (p->ainsn.boostable) {
+			ret = emulate_step(regs, ppc_inst_read((struct ppc_inst *)p->ainsn.insn));
 
 			if (ret > 0) {
 				restore_previous_kprobe(kcb);
@@ -356,8 +326,8 @@ int kprobe_handler(struct pt_regs *regs)
 		return 1;
 	}
 
-	if (p->ainsn.boostable >= 0) {
-		ret = try_to_emulate(p, regs);
+	if (p->ainsn.boostable) {
+		ret = emulate_step(regs, ppc_inst_read((struct ppc_inst *)p->ainsn.insn));
 
 		if (ret > 0) {
 			if (p->post_handler)
-- 
2.30.2

[PATCH 4/5] powerpc/kprobes: Refactor arch_prepare_kprobe()

[Naveen N. Rao]

Clean up the function to look sane:
- return immediately on error, rather than pointlessly setting the
  return value
- pr_info() instead of printk()
- check return value of patch_instruction()
- and to top it all of: a reverse christmas tree!

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/kprobes.c | 64 +++++++++++++++++------------------
 1 file changed, 31 insertions(+), 33 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index bbef9e918ecb39..7195162362941f 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -105,56 +105,54 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
 
 int arch_prepare_kprobe(struct kprobe *p)
 {
-	int ret = 0;
+	struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
+	struct instruction_op op;
 	struct kprobe *prev;
 	struct pt_regs regs;
-	struct instruction_op op;
-	struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
+	int ret = 0;
 
 	if ((unsigned long)p->addr & 0x03) {
-		printk("Attempt to register kprobe at an unaligned address\n");
-		ret = -EINVAL;
+		pr_info("Attempt to register kprobe at an unaligned address\n");
+		return -EINVAL;
 	} else if (IS_MTMSRD(insn) || IS_RFID(insn)) {
-		printk("Cannot register a kprobe on mtmsr[d]/rfi[d]\n");
-		ret = -EINVAL;
+		pr_info("Cannot register a kprobe on mtmsr[d]/rfi[d]\n");
+		return -EINVAL;
 	} else if ((unsigned long)p->addr & ~PAGE_MASK &&
 			ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)(p->addr - 1)))) {
-		printk("Cannot register a kprobe on the second word of prefixed instruction\n");
-		ret = -EINVAL;
+		pr_info("Cannot register a kprobe on the second word of prefixed instruction\n");
+		return -EINVAL;
 	}
+
+	/* Check if the previous instruction is a prefix instruction with an active kprobe */
 	preempt_disable();
 	prev = get_kprobe(p->addr - 1);
 	preempt_enable_no_resched();
-	if (prev &&
-	    ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)prev->ainsn.insn))) {
-		printk("Cannot register a kprobe on the second word of prefixed instruction\n");
-		ret = -EINVAL;
+	if (prev && ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)prev->ainsn.insn))) {
+		pr_info("Cannot register a kprobe on the second word of prefixed instruction\n");
+		return -EINVAL;
 	}
 
 	/* insn must be on a special executable page on ppc64.  This is
 	 * not explicitly required on ppc32 (right now), but it doesn't hurt */
-	if (!ret) {
-		p->ainsn.insn = get_insn_slot();
-		if (!p->ainsn.insn)
-			ret = -ENOMEM;
-	}
+	p->ainsn.insn = get_insn_slot();
+	if (!p->ainsn.insn)
+		return -ENOMEM;
 
-	if (!ret) {
-		patch_instruction((struct ppc_inst *)p->ainsn.insn, insn);
-		p->opcode = ppc_inst_val(insn);
-
-		/* Check if this is an instruction we recognise */
-		p->ainsn.boostable = 0;
-		memset(&regs, 0, sizeof(struct pt_regs));
-		regs.nip = (unsigned long)p->addr;
-		regs.msr = MSR_KERNEL;
-		ret = analyse_instr(&op, &regs, insn);
-		if (ret == 1 || (ret == 0 && GETTYPE(op.type) != UNKNOWN))
-			p->ainsn.boostable = 1;
-		ret = 0;
-	}
+	if (patch_instruction((struct ppc_inst *)p->ainsn.insn, insn))
+		return -EFAULT;
 
-	return ret;
+	p->opcode = ppc_inst_val(insn);
+
+	/* Check if this is an instruction we recognise */
+	p->ainsn.boostable = 0;
+	memset(&regs, 0, sizeof(struct pt_regs));
+	regs.nip = (unsigned long)p->addr;
+	regs.msr = MSR_KERNEL;
+	ret = analyse_instr(&op, &regs, insn);
+	if (ret == 1 || (ret == 0 && GETTYPE(op.type) != UNKNOWN))
+		p->ainsn.boostable = 1;
+
+	return 0;
 }
 NOKPROBE_SYMBOL(arch_prepare_kprobe);
 
-- 
2.30.2

[PATCH 5/5] powerpc/kprobes: Warn if instruction patching failed

[Naveen N. Rao]

When arming and disarming probes, we currently assume that instruction
patching can never fail, and don't have a mechanism to surface errors.
Add a warning in case instruction patching ever fails.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.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 7195162362941f..e0190e75a221eb 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -158,13 +158,13 @@ NOKPROBE_SYMBOL(arch_prepare_kprobe);
 
 void arch_arm_kprobe(struct kprobe *p)
 {
-	patch_instruction((struct ppc_inst *)p->addr, ppc_inst(BREAKPOINT_INSTRUCTION));
+	WARN_ON_ONCE(patch_instruction((struct ppc_inst *)p->addr, ppc_inst(BREAKPOINT_INSTRUCTION)));
 }
 NOKPROBE_SYMBOL(arch_arm_kprobe);
 
 void arch_disarm_kprobe(struct kprobe *p)
 {
-	patch_instruction((struct ppc_inst *)p->addr, ppc_inst(p->opcode));
+	WARN_ON_ONCE(patch_instruction((struct ppc_inst *)p->addr, ppc_inst(p->opcode)));
 }
 NOKPROBE_SYMBOL(arch_disarm_kprobe);
 
-- 
2.30.2

Re: [PATCH 0/5] powerpc/kprobes: fixes and cleanups

[Michael Ellerman]

On Wed, 19 May 2021 16:17:16 +0530, Naveen N. Rao wrote:
> Various fixes and some code refactoring for kprobes on powerpc. The
> first patch fixes an invalid access if probing the first instruction in
> a kernel module. The rest are small cleanups. More details in the
> individual patches.
> 
> - Naveen
> 
> [...]

Patch 1 applied to powerpc/fixes.

[1/5] powerpc/kprobes: Fix validation of prefixed instructions across page boundary
      https://git.kernel.org/powerpc/c/82123a3d1d5a306fdf50c968a474cc60fe43a80f

cheers

Re: (subset) [PATCH 0/5] powerpc/kprobes: fixes and cleanups

[Michael Ellerman]

On Wed, 19 May 2021 16:17:16 +0530, Naveen N. Rao wrote:
> Various fixes and some code refactoring for kprobes on powerpc. The
> first patch fixes an invalid access if probing the first instruction in
> a kernel module. The rest are small cleanups. More details in the
> individual patches.
> 
> - Naveen
> 
> [...]

Applied to powerpc/next.

[5/5] powerpc/kprobes: Warn if instruction patching failed
      https://git.kernel.org/powerpc/c/12b58492e60bf5a31d7f41e8a6f8ceb6f87e710e

cheers

Re: (subset) [PATCH 0/5] powerpc/kprobes: fixes and cleanups

[Michael Ellerman]

On Wed, 19 May 2021 16:17:16 +0530, Naveen N. Rao wrote:
> Various fixes and some code refactoring for kprobes on powerpc. The
> first patch fixes an invalid access if probing the first instruction in
> a kernel module. The rest are small cleanups. More details in the
> individual patches.
> 
> - Naveen
> 
> [...]

Patch 2 applied to powerpc/next.

[2/5] powerpc/kprobes: Roll IS_RFI() macro into IS_RFID()
      https://git.kernel.org/powerpc/c/0566fa760d235c119cef92119efc3ab11486a08a

cheers