powerpc/kprobes: fixes and cleanups

Started by Naveen N. Rao on May 19, 2021 · 9 messages from 2 authors · Last reply Jun 26, 2021

Series

5 patches, 2 files

+56 −89

Subsystems (2)

Maintainers (2)

STALE1961d PENDING · 3 of 5 landed

Status: Pending, 3 of 5 landed.

open the first message on its own page

  1. [PATCH 1/5] powerpc/kprobes: Fix validation of prefixed instructions across page boundary
    Related patches
  2. [PATCH 2/5] powerpc/kprobes: Roll IS_RFI() macro into IS_RFID()
    Related patches
  3. [PATCH 3/5] powerpc/kprobes: Check instruction validity during kprobe registration
    Related patches
  4. [PATCH 4/5] powerpc/kprobes: Refactor arch_prepare_kprobe()
    Related patches
  5. [PATCH 5/5] powerpc/kprobes: Warn if instruction patching failed
    Related patches
Message details and related content
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

Message details and related content
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: <redacted> # v5.8+
Fixes: b4657f7650babc ("powerpc/kprobes: Don't allow breakpoints on suffixes")
Reported-by: Christophe Leroy <redacted>
Signed-off-by: Naveen N. Rao <redacted>
---
 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()

Message details and related content
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 <redacted>
---
 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

Message details and related content
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 <redacted>
---
 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()

Message details and related content
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 <redacted>
---
 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

Message details and related content
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 <redacted>
---
 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
Message details and related content
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

Message details and related content
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

Message details and related content
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
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help