[PATCH 0/2] powerpc: Remove system call emulation

This is an update to the series posted by Nick here:
http://lkml.kernel.org/r/20220124055741.3686496-1-npiggin@gmail.com

The first patch disables probes and breakpoints on instructions that
can't be single stepped, including sc and scv. The second patch removes
system call emulation for powerpc64.

- Naveen



Naveen N. Rao (1):
  powerpc: Reject probes on instructions that can't be single stepped

Nicholas Piggin (1):
  powerpc/64: remove system call instruction emulation

 arch/powerpc/include/asm/probes.h  | 55 ++++++++++++++++++++++++++++++
 arch/powerpc/kernel/interrupt_64.S | 10 ------
 arch/powerpc/kernel/kprobes.c      |  4 +--
 arch/powerpc/kernel/uprobes.c      |  5 +++
 arch/powerpc/lib/sstep.c           | 46 ++++++-------------------
 arch/powerpc/xmon/xmon.c           | 11 +++---
 6 files changed, 77 insertions(+), 54 deletions(-)


base-commit: e8833c5edc5903f8c8c4fa3dd4f34d6b813c87c8
-- 
2.35.1

Per the ISA, a Trace interrupt is not generated for:
- [h|u]rfi[d]
- rfscv
- sc, scv, and Trap instructions that trap
- Power-Saving Mode instructions
- other instructions that cause interrupts (other than Trace interrupts)
- the first instructions of any interrupt handler (applies to Branch and Single Step tracing;
CIABR matches may still occur)
- instructions that are emulated by software

Add a helper to check for instructions belonging to the first four
categories above and to reject kprobes, uprobes and xmon breakpoints on
such instructions. We reject probing on instructions belonging to these
categories across all ISA versions and across both BookS and BookE.

For trap instructions, we can't know in advance if they can cause a
trap, and there is no good reason to allow probing on those. Also,
uprobes already refuses to probe trap instructions and kprobes does not
allow probes on trap instructions used for kernel warnings and bugs. As
such, stop allowing any type of probes/breakpoints on trap instruction
across uprobes, kprobes and xmon.

For some of the fp/altivec instructions that can generate an interrupt
and which we emulate in the kernel (altivec assist, for example), we
check and turn off single stepping in emulate_single_step().

Instructions generating a DSI are restarted and single stepping normally
completes once the instruction is completed.

In uprobes, if a single stepped instruction results in a non-fatal
signal to be delivered to the task, such signals are "delayed" until
after the instruction completes. For fatal signals, single stepping is
cancelled and the instruction restarted in-place so that core dump
captures proper addresses.

In kprobes, we do not allow probes on instructions having an extable
entry and we also do not allow probing interrupt vectors.

Signed-off-by: Naveen N. Rao <redacted>
---
 arch/powerpc/include/asm/probes.h | 55 +++++++++++++++++++++++++++++++
 arch/powerpc/kernel/kprobes.c     |  4 +--
 arch/powerpc/kernel/uprobes.c     |  5 +++
 arch/powerpc/xmon/xmon.c          | 11 +++----
 4 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/probes.h b/arch/powerpc/include/asm/probes.h
index c5d984700d241a..21ab5b6256b590 100644
--- a/arch/powerpc/include/asm/probes.h
+++ b/arch/powerpc/include/asm/probes.h

@@ -31,6 +31,61 @@ typedef u32 ppc_opcode_t;
 #define MSR_SINGLESTEP	(MSR_SE)
 #endif
 
+static inline bool can_single_step(u32 inst)
+{
+	switch (inst >> 26) {
+	case 2:		/* tdi */
+		return false;
+	case 3:		/* twi */
+		return false;
+	case 17:	/* sc and scv */
+		return false;
+	case 19:
+		switch ((inst >> 1) & 0x3ff) {
+		case 18:	/* rfid */
+			return false;
+		case 38:	/* rfmci */
+			return false;
+		case 39:	/* rfdi */
+			return false;
+		case 50:	/* rfi */
+			return false;
+		case 51:	/* rfci */
+			return false;
+		case 82:	/* rfscv */
+			return false;
+		case 274:	/* hrfid */
+			return false;
+		case 306:	/* urfid */
+			return false;
+		case 370:	/* stop */
+			return false;
+		case 402:	/* doze */
+			return false;
+		case 434:	/* nap */
+			return false;
+		case 466:	/* sleep */
+			return false;
+		case 498:	/* rvwinkle */
+			return false;
+		}
+		break;
+	case 31:
+		switch ((inst >> 1) & 0x3ff) {
+		case 4:		/* tw */
+			return false;
+		case 68:	/* td */
+			return false;
+		case 146:	/* mtmsr */
+			return false;
+		case 178:	/* mtmsrd */
+			return false;
+		}
+		break;
+	}
+	return true;
+}
+
 /* Enable single stepping for the current task */
 static inline void enable_single_step(struct pt_regs *regs)
 {

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 9a492fdec1dfbe..0936a6c8c256b9 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c

@@ -129,8 +129,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)) {
-		printk("Cannot register a kprobe on mtmsr[d]/rfi[d]\n");
+	} else if (!can_single_step(ppc_inst_val(insn))) {
+		printk("Cannot register a kprobe on instructions that can't be single stepped\n");
 		ret = -EINVAL;
 	} else if ((unsigned long)p->addr & ~PAGE_MASK &&
 		   ppc_inst_prefixed(ppc_inst_read(p->addr - 1))) {

diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index c6975467d9ffdc..95a41ae9dfa755 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c

@@ -48,6 +48,11 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
 		return -EINVAL;
 	}
 
+	if (!can_single_step(ppc_inst_val(ppc_inst_read(auprobe->insn)))) {
+		pr_info_ratelimited("Cannot register a uprobe on instructions that can't be single stepped\n");
+		return -ENOTSUPP;
+	}
+
 	return 0;
 }
 

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index fd72753e8ad502..a92c5739d954e2 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c

@@ -921,9 +921,9 @@ static void insert_bpts(void)
 			bp->enabled = 0;
 			continue;
 		}
-		if (IS_MTMSRD(instr) || IS_RFID(instr)) {
-			printf("Breakpoint at %lx is on an mtmsrd or rfid "
-			       "instruction, disabling it\n", bp->address);
+		if (!can_single_step(ppc_inst_val(instr))) {
+			printf("Breakpoint at %lx is on an instruction that can't be single stepped, disabling it\n",
+					bp->address);
 			bp->enabled = 0;
 			continue;
 		}

@@ -1470,9 +1470,8 @@ static long check_bp_loc(unsigned long addr)
 		printf("Can't read instruction at address %lx\n", addr);
 		return 0;
 	}
-	if (IS_MTMSRD(instr) || IS_RFID(instr)) {
-		printf("Breakpoints may not be placed on mtmsrd or rfid "
-		       "instructions\n");
+	if (!can_single_step(ppc_inst_val(instr))) {
+		printf("Breakpoints may not be placed on instructions that can't be single stepped\n");
 		return 0;
 	}
 	return 1;

-- 
2.35.1

From: Nicholas Piggin <npiggin@gmail.com>

emulate_step instruction emulation including sc instruction emulation
initially appeared in xmon. It then emulation code was then moved into
sstep.c where kprobes could use it too, and later hw_breakpoint and
uprobes started to use it.

Until uprobes, the only instruction emulation users were for kernel
mode instructions.

- xmon only steps / breaks on kernel addresses.
- kprobes is kernel only.
- hw_breakpoint only emulates kernel instructions, single steps user.

At one point there was support for the kernel to execute sc
instructions, although that is long removed and it's not clear whether
there was any in-tree code. So system call emulation is not required by
the above users.

uprobes uses emulate_step and it appears possible to emulate sc
instruction in userspace. Userspace system call emulation is broken and
it's not clear it ever worked well.

The big complication is that userspace takes an interrupt to the kernel
to emulate the instruction. The user->kernel interrupt sets up registers
and interrupt stack frame expecting to return to userspace, then system
call instruction emulation re-directs that stack frame to the kernel,
early in the system call interrupt handler. This means the the interrupt
return code takes the kernel->kernel restore path, which does not restore
everything as the system call interrupt handler would expect coming from
userspace. regs->iamr appears to get lost for example, because the
kernel->kernel return does not restore the user iamr. Accounting such as
irqflags tracing and CPU accounting does not get flipped back to user
mode as the system call handler expects, so those appear to enter the
kernel twice without returning to userspace.

These things may be individually fixable with various complication, but
it is a big complexity for unclear real benefit.

This patch removes system call emulation and disables stepping system
calls (because they don't work with trace interrupts, as commented).

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
[also get rid of '#ifdef CONFIG_PPC64']
Signed-off-by: Naveen N. Rao <redacted>
---
 arch/powerpc/kernel/interrupt_64.S | 10 -------
 arch/powerpc/lib/sstep.c           | 46 +++++++-----------------------
 2 files changed, 10 insertions(+), 46 deletions(-)

diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
index 7bab2d7de372e0..6471034c790973 100644
--- a/arch/powerpc/kernel/interrupt_64.S
+++ b/arch/powerpc/kernel/interrupt_64.S

@@ -219,16 +219,6 @@ system_call_vectored common 0x3000
  */
 system_call_vectored sigill 0x7ff0
 
-
-/*
- * Entered via kernel return set up by kernel/sstep.c, must match entry regs
- */
-	.globl system_call_vectored_emulate
-system_call_vectored_emulate:
-_ASM_NOKPROBE_SYMBOL(system_call_vectored_emulate)
-	li	r10,IRQS_ALL_DISABLED
-	stb	r10,PACAIRQSOFTMASK(r13)
-	b	system_call_vectored_common
 #endif /* CONFIG_PPC_BOOK3S */
 
 	.balign IFETCH_ALIGN_BYTES

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 3fda8d0a05b43f..01c8fd39f34981 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c

@@ -15,9 +15,6 @@
 #include <asm/cputable.h>
 #include <asm/disassemble.h>
 
-extern char system_call_common[];
-extern char system_call_vectored_emulate[];
-
 #ifdef CONFIG_PPC64
 /* Bits in SRR1 that are copied from MSR */
 #define MSR_MASK	0xffffffff87c0ffffUL

@@ -1376,7 +1373,6 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
 		if (branch_taken(word, regs, op))
 			op->type |= BRTAKEN;
 		return 1;
-#ifdef CONFIG_PPC64
 	case 17:	/* sc */
 		if ((word & 0xfe2) == 2)
 			op->type = SYSCALL;

@@ -1388,7 +1384,6 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
 		} else
 			op->type = UNKNOWN;
 		return 0;
-#endif
 	case 18:	/* b */
 		op->type = BRANCH | BRTAKEN;
 		imm = word & 0x03fffffc;

@@ -3643,43 +3638,22 @@ int emulate_step(struct pt_regs *regs, ppc_inst_t instr)
 		regs_set_return_msr(regs, (regs->msr & ~op.val) | (val & op.val));
 		goto instr_done;
 
-#ifdef CONFIG_PPC64
 	case SYSCALL:	/* sc */
 		/*
-		 * N.B. this uses knowledge about how the syscall
-		 * entry code works.  If that is changed, this will
-		 * need to be changed also.
+		 * Per ISA v3.1, section 7.5.15 'Trace Interrupt', we can't
+		 * single step a system call instruction:
+		 *
+		 *   Successful completion for an instruction means that the
+		 *   instruction caused no other interrupt. Thus a Trace
+		 *   interrupt never occurs for a System Call or System Call
+		 *   Vectored instruction, or for a Trap instruction that
+		 *   traps.
 		 */
-		if (IS_ENABLED(CONFIG_PPC_FAST_ENDIAN_SWITCH) &&
-				cpu_has_feature(CPU_FTR_REAL_LE) &&
-				regs->gpr[0] == 0x1ebe) {
-			regs_set_return_msr(regs, regs->msr ^ MSR_LE);
-			goto instr_done;
-		}
-		regs->gpr[9] = regs->gpr[13];
-		regs->gpr[10] = MSR_KERNEL;
-		regs->gpr[11] = regs->nip + 4;
-		regs->gpr[12] = regs->msr & MSR_MASK;
-		regs->gpr[13] = (unsigned long) get_paca();
-		regs_set_return_ip(regs, (unsigned long) &system_call_common);
-		regs_set_return_msr(regs, MSR_KERNEL);
-		return 1;
-
-#ifdef CONFIG_PPC_BOOK3S_64
+		return -1;
 	case SYSCALL_VECTORED_0:	/* scv 0 */
-		regs->gpr[9] = regs->gpr[13];
-		regs->gpr[10] = MSR_KERNEL;
-		regs->gpr[11] = regs->nip + 4;
-		regs->gpr[12] = regs->msr & MSR_MASK;
-		regs->gpr[13] = (unsigned long) get_paca();
-		regs_set_return_ip(regs, (unsigned long) &system_call_vectored_emulate);
-		regs_set_return_msr(regs, MSR_KERNEL);
-		return 1;
-#endif
-
+		return -1;
 	case RFI:
 		return -1;
-#endif
 	}
 	return 0;
 

-- 
2.35.1

Hi, Naveen.

Some comments below.

On 3/23/22 08:51, Naveen N. Rao wrote:

Can't ppc_inst_primary_opcode() be used instead?

+	case 2:		/* tdi */
+		return false;
+	case 3:		/* twi */
+		return false;
+	case 17:	/* sc and scv */
+		return false;
+	case 19:
+		switch ((inst >> 1) & 0x3ff) {
+		case 18:	/* rfid */
+			return false;
+		case 38:	/* rfmci */
+			return false;
+		case 39:	/* rfdi */
+			return false;
+		case 50:	/* rfi */
+			return false;
+		case 51:	/* rfci */
+			return false;
+		case 82:	/* rfscv */
+			return false;
+		case 274:	/* hrfid */
+			return false;
+		case 306:	/* urfid */
+			return false;
+		case 370:	/* stop */
+			return false;
+		case 402:	/* doze */
+			return false;
+		case 434:	/* nap */
+			return false;
+		case 466:	/* sleep */
+			return false;
+		case 498:	/* rvwinkle */
+			return false;
+		}
+		break;
+	case 31:
+		switch ((inst >> 1) & 0x3ff) {
+		case 4:		/* tw */
+			return false;
+		case 68:	/* td */
+			return false;
+		case 146:	/* mtmsr */
+			return false;
+		case 178:	/* mtmsrd */
+			return false;
+		}
+		break;
+	}
+	return true;
+}
+

Can't OP_* definitions from ppc-opcode.h be used for all of these switch-case statements?

Cheers!

-- 
Murilo

Murilo Opsfelder Araújo [off-list ref] writes:

On 3/23/22 08:51, Naveen N. Rao wrote:

...

quoted

+	case 31:
+		switch ((inst >> 1) & 0x3ff) {
+		case 4:		/* tw */
+			return false;
+		case 68:	/* td */
+			return false;
+		case 146:	/* mtmsr */
+			return false;
+		case 178:	/* mtmsrd */
+			return false;
+		}
+		break;
+	}
+	return true;
+}
+

Can't OP_* definitions from ppc-opcode.h be used for all of these switch-case statements?

Yes please. And add any that are missing.

cheers

Michael Ellerman wrote:

Murilo Opsfelder Araújo [off-list ref] writes:

quoted

On 3/23/22 08:51, Naveen N. Rao wrote:

quoted

+static inline bool can_single_step(u32 inst)
+{
+	switch (inst >> 26) {

Can't ppc_inst_primary_opcode() be used instead?

I didn't want to add a dependency on inst.h. But I guess I can very well 
move this out of the header into some .c file. I will see if I can make 
that work.

quoted

quoted

+	case 31:
+		switch ((inst >> 1) & 0x3ff) {
+		case 4:		/* tw */
+			return false;
+		case 68:	/* td */
+			return false;
+		case 146:	/* mtmsr */
+			return false;
+		case 178:	/* mtmsrd */
+			return false;
+		}
+		break;
+	}
+	return true;
+}
+

Can't OP_* definitions from ppc-opcode.h be used for all of these switch-case statements?

Yes please. And add any that are missing.

We only have OP_31 from the above list now. I'll add the rest.


Thanks,
Naveen

Le 28/03/2022 à 19:20, Naveen N. Rao a écrit :

Michael Ellerman wrote:

quoted

Murilo Opsfelder Araújo [off-list ref] writes:

quoted

On 3/23/22 08:51, Naveen N. Rao wrote:

quoted

+static inline bool can_single_step(u32 inst)
+{
+    switch (inst >> 26) {

Can't ppc_inst_primary_opcode() be used instead?

I didn't want to add a dependency on inst.h. But I guess I can very well 
move this out of the header into some .c file. I will see if I can make 
that work.

Maybe use get_op() from asm/disassemble.h ?

quoted

quoted

quoted

+    case 31:
+        switch ((inst >> 1) & 0x3ff) {

For that one you have get_xop() in asm/disassemble.h

quoted

quoted

quoted

+        case 4:        /* tw */

OP_31_XOP_TRAP

quoted

quoted

quoted

+            return false;
+        case 68:    /* td */

OP_31_XOP_TRAP_64

quoted

quoted

quoted

+            return false;
+        case 146:    /* mtmsr */
+            return false;
+        case 178:    /* mtmsrd */
+            return false;
+        }
+        break;
+    }
+    return true;
+}
+

Can't OP_* definitions from ppc-opcode.h be used for all of these 
switch-case statements?

Yes please. And add any that are missing.

We only have OP_31 from the above list now. I'll add the rest.

Isn't there also OP_TRAP and OP_TRAP_64 ?

Christophe

Christophe Leroy wrote:

Le 28/03/2022 à 19:20, Naveen N. Rao a écrit :

quoted

Michael Ellerman wrote:

quoted

Murilo Opsfelder Araújo [off-list ref] writes:

quoted

On 3/23/22 08:51, Naveen N. Rao wrote:

quoted

+static inline bool can_single_step(u32 inst)
+{
+    switch (inst >> 26) {

Can't ppc_inst_primary_opcode() be used instead?

I didn't want to add a dependency on inst.h. But I guess I can very well 
move this out of the header into some .c file. I will see if I can make 
that work.

Maybe use get_op() from asm/disassemble.h ?

quoted

quoted

quoted

quoted

+    case 31:
+        switch ((inst >> 1) & 0x3ff) {

For that one you have get_xop() in asm/disassemble.h

Nice! I will use those.

quoted

quoted

quoted

quoted

+        case 4:        /* tw */

OP_31_XOP_TRAP

quoted

quoted

quoted

quoted

+            return false;
+        case 68:    /* td */

OP_31_XOP_TRAP_64

quoted

quoted

quoted

quoted

+            return false;
+        case 146:    /* mtmsr */
+            return false;
+        case 178:    /* mtmsrd */
+            return false;
+        }
+        break;
+    }
+    return true;
+}
+

Can't OP_* definitions from ppc-opcode.h be used for all of these 
switch-case statements?

Yes please. And add any that are missing.

We only have OP_31 from the above list now. I'll add the rest.

Isn't there also OP_TRAP and OP_TRAP_64 ?

Ah, the list clearly isn't sorted, and there are some duplicates 
there :)


Thanks,
Naveen