On 2022-01-27 13:14, Nicholas Piggin wrote:

Excerpts from Michael Ellerman's message of January 25, 2022 9:45 pm:

quoted

Nicholas Piggin [off-list ref] writes:

quoted

Per the ISA, a Trace interrupt is not generated for a system call
[vectored] instruction. Reject uprobes on such instructions as we are
not emulating a system call [vectored] instruction anymore.

This should really be patch 1, otherwise there's a single commit 
window
where we allow uprobes on sc but don't honour them.

Yep true. I also messed up Naveen's attribution! Will re-send (or maybe
Naveen would take over the series).

Yes, let me come up with a better, more complete patch for this.

quoted

quoted

Signed-off-by: Naveen N. Rao <redacted>
[np: Switch to pr_info_ratelimited]
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/ppc-opcode.h | 1 +
 arch/powerpc/kernel/uprobes.c         | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/arch/powerpc/include/asm/ppc-opcode.h 

b/arch/powerpc/include/asm/ppc-opcode.h
index 9675303b724e..8bbe16ce5173 100644

--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h

@@ -411,6 +411,7 @@
 #define PPC_RAW_DCBFPS(a, b)		(0x7c0000ac | ___PPC_RA(a) | 

___PPC_RB(b) | (4 << 21))
 #define PPC_RAW_DCBSTPS(a, b)		(0x7c0000ac | ___PPC_RA(a) | 
___PPC_RB(b) | (6 << 21))
 #define PPC_RAW_SC()			(0x44000002)
+#define PPC_RAW_SCV()			(0x44000001)
 #define PPC_RAW_SYNC()			(0x7c0004ac)
 #define PPC_RAW_ISYNC()			(0x4c00012c)

diff --git a/arch/powerpc/kernel/uprobes.c 

b/arch/powerpc/kernel/uprobes.c
index c6975467d9ff..3779fde804bd 100644

--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c

@@ -41,6 +41,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe 

*auprobe,
 	if (addr & 0x03)
 		return -EINVAL;

+	if (ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SC() ||
+	    ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SCV()) {

We should probably reject hypercall too?

There's also a lot of reserved fields in `sc`, so doing an exact match
like this risks missing instructions that are badly formed but the CPU
will happily execute as `sc`.

Yeah, scv as well has lev != 0 unsupported so should be excluded.

quoted

We'd obviously never expect to see those in compiler generated code, 
but
it'd still be safer to mask. We could probably just reject opcode 17
entirely.

Indeed, thanks.

quoted

And I guess for a subsequent patch, but we should be rejecting some
others here as well shouldn't we? Like rfid etc.

Traps under discussion I guess. For uprobe, rfid will be just another
privilege fault. Is that dealt with somehow or do all privileged and
illegal instructions also need to be excluded from stepping? (I assume
we must handle that in a general way somehow)

Yes, this is all handled in our interrupt code if we emulate any of 
those
privileged instructions. Otherwise, if a signal is generated, that would
be caught by uprobe_deny_signal().


Thanks,
Naveen