[PATCH 0/2] powerpc: Disable syscall emulation and stepping

[PATCH 0/2] powerpc: Disable syscall emulation and stepping

[Nicholas Piggin]

As discussed previously

https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html

I'm wondering whether PPC32 should be returning -1 for syscall
instructions too here? That could be done in another patch anyway.

Thanks,
Nick

Nicholas Piggin (2):
  powerpc/64: remove system call instruction emulation
  powerpc/uprobes: Reject uprobe on a system call instruction

 arch/powerpc/include/asm/ppc-opcode.h |  1 +
 arch/powerpc/kernel/interrupt_64.S    | 10 -------
 arch/powerpc/kernel/uprobes.c         |  6 ++++
 arch/powerpc/lib/sstep.c              | 42 +++++++--------------------
 4 files changed, 18 insertions(+), 41 deletions(-)

-- 
2.23.0

[PATCH 1/2] powerpc/64: remove system call instruction emulation

[Nicholas Piggin]

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).

Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/interrupt_64.S | 10 -------
 arch/powerpc/lib/sstep.c           | 42 ++++++++----------------------
 2 files changed, 11 insertions(+), 41 deletions(-)

diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
index 7bab2d7de372..6471034c7909 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 a94b0cd0bdc5..5f317b12b2db 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
@@ -3650,39 +3647,22 @@ int emulate_step(struct pt_regs *regs, ppc_inst_t instr)
 		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;
-
+	case SYSCALL:	/* sc */
+		return -1;
 #ifdef CONFIG_PPC_BOOK3S_64
 	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;
+		return -1;
 #endif
-
 	case RFI:
 		return -1;
 #endif
-- 
2.23.0

[PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction

[Nicholas Piggin]

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.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
[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()) {
+		pr_info_ratelimited("Rejecting uprobe on system call instruction\n");
+		return -EINVAL;
+	}
+
 	if (cpu_has_feature(CPU_FTR_ARCH_31) &&
 	    ppc_inst_prefixed(ppc_inst_read(auprobe->insn)) &&
 	    (addr & 0x3f) == 60) {
-- 
2.23.0

Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping

[Christophe Leroy]

Le 24/01/2022 à 06:57, Nicholas Piggin a écrit :
> As discussed previously
> 
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html
> 
> I'm wondering whether PPC32 should be returning -1 for syscall
> instructions too here? That could be done in another patch anyway.
> 

The 'Programming Environments Manual for 32-Bit Implementations of the 
PowerPC™ Architecture' says:

The following are not traced:
• rfi instruction
• sc and trap instructions that trap
• Other instructions that cause interrupts (other than trace interrupts)
• The first instruction of any interrupt handler
• Instructions that are emulated by software


So I think PPC32 should return -1 as well.

Christophe

Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping

[Nicholas Piggin]

+Naveen (sorry missed cc'ing you at first)

Excerpts from Christophe Leroy's message of January 24, 2022 4:39 pm:
> 
> 
> Le 24/01/2022 à 06:57, Nicholas Piggin a écrit :
>> As discussed previously
>> 
>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html
>> 
>> I'm wondering whether PPC32 should be returning -1 for syscall
>> instructions too here? That could be done in another patch anyway.
>> 
> 
> The 'Programming Environments Manual for 32-Bit Implementations of the 
> PowerPC™ Architecture' says:
> 
> The following are not traced:
> • rfi instruction
> • sc and trap instructions that trap
> • Other instructions that cause interrupts (other than trace interrupts)
> • The first instruction of any interrupt handler
> • Instructions that are emulated by software
> 
> 
> So I think PPC32 should return -1 as well.

I agree.

What about the trap instructions? analyse_instr returns 0 for them
which falls through to return 0 for emulate_step, should they
return -1 as well or am I missing something?

Thanks,
Nick

Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping

[Christophe Leroy]

Le 25/01/2022 à 04:04, Nicholas Piggin a écrit :
> +Naveen (sorry missed cc'ing you at first)
> 
> Excerpts from Christophe Leroy's message of January 24, 2022 4:39 pm:
>>
>>
>> Le 24/01/2022 à 06:57, Nicholas Piggin a écrit :
>>> As discussed previously
>>>
>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html
>>>
>>> I'm wondering whether PPC32 should be returning -1 for syscall
>>> instructions too here? That could be done in another patch anyway.
>>>
>>
>> The 'Programming Environments Manual for 32-Bit Implementations of the
>> PowerPC™ Architecture' says:
>>
>> The following are not traced:
>> • rfi instruction
>> • sc and trap instructions that trap
>> • Other instructions that cause interrupts (other than trace interrupts)
>> • The first instruction of any interrupt handler
>> • Instructions that are emulated by software
>>
>>
>> So I think PPC32 should return -1 as well.
> 
> I agree.
> 
> What about the trap instructions? analyse_instr returns 0 for them
> which falls through to return 0 for emulate_step, should they
> return -1 as well or am I missing something?
> 

For the traps I don't know. The manual says "trap instructions that 
trap" are not traced. It means that "trap instructions that _don't_ 
trap" are traced. Taking into account that trap instructions don't trap 
at least 99.9% of the time, not sure if returning -1 is needed.

Allthought that'd probably be the safest.

But then what happens with other instruction that will sparsely generate 
an exception like a DSI or so ? If we do it for the traps then we should 
do it for this as well, and then it becomes a non ending story.

So at the end it's probably ok with return 0, both for them and for traps.

Christophe

Re: [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> 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.

> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> [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`.

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.

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

cheers


> +		pr_info_ratelimited("Rejecting uprobe on system call instruction\n");
> +		return -EINVAL;
> +	}
> +
>  	if (cpu_has_feature(CPU_FTR_ARCH_31) &&
>  	    ppc_inst_prefixed(ppc_inst_read(auprobe->insn)) &&
>  	    (addr & 0x3f) == 60) {
> -- 
> 2.23.0

Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping

[Nicholas Piggin]

Excerpts from naverao1's message of January 25, 2022 8:48 pm:
> On 2022-01-25 11:23, Christophe Leroy wrote:
>> Le 25/01/2022 à 04:04, Nicholas Piggin a écrit :
>>> +Naveen (sorry missed cc'ing you at first)
>>> 
>>> Excerpts from Christophe Leroy's message of January 24, 2022 4:39 pm:
>>>> 
>>>> 
>>>> Le 24/01/2022 à 06:57, Nicholas Piggin a écrit :
>>>>> As discussed previously
>>>>> 
>>>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html
>>>>> 
>>>>> I'm wondering whether PPC32 should be returning -1 for syscall
>>>>> instructions too here? That could be done in another patch anyway.
>>>>> 
>>>> 
>>>> The 'Programming Environments Manual for 32-Bit Implementations of 
>>>> the
>>>> PowerPC™ Architecture' says:
>>>> 
>>>> The following are not traced:
>>>> • rfi instruction
>>>> • sc and trap instructions that trap
>>>> • Other instructions that cause interrupts (other than trace 
>>>> interrupts)
>>>> • The first instruction of any interrupt handler
>>>> • Instructions that are emulated by software
>>>> 
>>>> 
>>>> So I think PPC32 should return -1 as well.
>>> 
>>> I agree.
>>> 
>>> What about the trap instructions? analyse_instr returns 0 for them
>>> which falls through to return 0 for emulate_step, should they
>>> return -1 as well or am I missing something?
> 
> Yeah, good point about the trap instructions.
> 
>>> 
>> 
>> For the traps I don't know. The manual says "trap instructions that
>> trap" are not traced. It means that "trap instructions that _don't_
>> trap" are traced. Taking into account that trap instructions don't trap
>> at least 99.9% of the time, not sure if returning -1 is needed.
>> 
>> Allthought that'd probably be the safest.
> 
> 'trap' is a special case since it is predominantly used by debuggers
> and/or tracing infrastructure. Kprobes and Uprobes do not allow probes
> on a trap instruction. But, xmon can be asked to step on a trap
> instruction and that can interfere with kprobes in weird ways.
> 
> So, I think it is best if we also exclude trap instructions from being
> single stepped.
> 
>> 
>> But then what happens with other instruction that will sparsely 
>> generate
>> an exception like a DSI or so ? If we do it for the traps then we 
>> should
>> do it for this as well, and then it becomes a non ending story.
> 
> For a DSI, we restart the same instruction after handling the page 
> fault.
> The single step exception is raised on the subsequent successful
> completion of the instruction.

Although it can cause a signal, and the signal handler can decide
to resume somewhere else. Or kernel mode equivalent it can go to a
fixup handler and resume somewhere else.

How are those handled?

Thanks,
Nick

> For most other interrupts (alignment, vsx
> unavailable, ...), we end up emulating the single step exception itself
> (see emulate_single_step()). So, those are ok if caused by an 
> instruction
> being stepped.
> 
> 
> - Naveen
> 

Re: [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction

[Nicholas Piggin]

Excerpts from Michael Ellerman's message of January 25, 2022 9:45 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> 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).

> 
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>> [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.
> 
> 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.
> 
> 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)


Thanks,
Nick

Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping

[Naveen N. Rao]

[Sorry if you receive this in duplicate. Resending since this message 
didn't hit the list]


On 2022-01-25 11:23, Christophe Leroy wrote:
> Le 25/01/2022 à 04:04, Nicholas Piggin a écrit :
>> +Naveen (sorry missed cc'ing you at first)
>> 
>> Excerpts from Christophe Leroy's message of January 24, 2022 4:39 pm:
>>> 
>>> 
>>> Le 24/01/2022 à 06:57, Nicholas Piggin a écrit :
>>>> As discussed previously
>>>> 
>>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html
>>>> 
>>>> I'm wondering whether PPC32 should be returning -1 for syscall
>>>> instructions too here? That could be done in another patch anyway.
>>>> 
>>> 
>>> The 'Programming Environments Manual for 32-Bit Implementations of 
>>> the
>>> PowerPC™ Architecture' says:
>>> 
>>> The following are not traced:
>>> • rfi instruction
>>> • sc and trap instructions that trap
>>> • Other instructions that cause interrupts (other than trace 
>>> interrupts)
>>> • The first instruction of any interrupt handler
>>> • Instructions that are emulated by software
>>> 
>>> 
>>> So I think PPC32 should return -1 as well.
>> 
>> I agree.
>> 
>> What about the trap instructions? analyse_instr returns 0 for them
>> which falls through to return 0 for emulate_step, should they
>> return -1 as well or am I missing something?

Yeah, good point about the trap instructions.

>> 
> 
> For the traps I don't know. The manual says "trap instructions that
> trap" are not traced. It means that "trap instructions that _don't_
> trap" are traced. Taking into account that trap instructions don't trap
> at least 99.9% of the time, not sure if returning -1 is needed.
> 
> Allthought that'd probably be the safest.

'trap' is a special case since it is predominantly used by debuggers
and/or tracing infrastructure. Kprobes and Uprobes do not allow probes
on a trap instruction. But, xmon can be asked to step on a trap
instruction and that can interfere with kprobes in weird ways.

So, I think it is best if we also exclude trap instructions from being
single stepped.

> 
> But then what happens with other instruction that will sparsely 
> generate
> an exception like a DSI or so ? If we do it for the traps then we 
> should
> do it for this as well, and then it becomes a non ending story.

For a DSI, we restart the same instruction after handling the page 
fault.
The single step exception is raised on the subsequent successful
completion of the instruction. For most other interrupts (alignment, vsx
unavailable, ...), we end up emulating the single step exception itself
(see emulate_single_step()). So, those are ok if caused by an 
instruction
being stepped.


- Naveen

Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping

[Naveen N. Rao]

On 2022-01-27 13:09, Nicholas Piggin wrote:
> Excerpts from naverao1's message of January 25, 2022 8:48 pm:
>> On 2022-01-25 11:23, Christophe Leroy wrote:
>>> Le 25/01/2022 à 04:04, Nicholas Piggin a écrit :
>>>> +Naveen (sorry missed cc'ing you at first)
>>>> 
>>>> Excerpts from Christophe Leroy's message of January 24, 2022 4:39 
>>>> pm:
>>>>> 
>>>>> 
>>>>> Le 24/01/2022 à 06:57, Nicholas Piggin a écrit :
>>>>>> As discussed previously
>>>>>> 
>>>>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html
>>>>>> 
>>>>>> I'm wondering whether PPC32 should be returning -1 for syscall
>>>>>> instructions too here? That could be done in another patch anyway.
>>>>>> 
>>>>> 
>>>>> The 'Programming Environments Manual for 32-Bit Implementations of
>>>>> the
>>>>> PowerPC™ Architecture' says:
>>>>> 
>>>>> The following are not traced:
>>>>> • rfi instruction
>>>>> • sc and trap instructions that trap
>>>>> • Other instructions that cause interrupts (other than trace
>>>>> interrupts)
>>>>> • The first instruction of any interrupt handler
>>>>> • Instructions that are emulated by software
>>>>> 
>>>>> 
>>>>> So I think PPC32 should return -1 as well.
>>>> 
>>>> I agree.
>>>> 
>>>> What about the trap instructions? analyse_instr returns 0 for them
>>>> which falls through to return 0 for emulate_step, should they
>>>> return -1 as well or am I missing something?
>> 
>> Yeah, good point about the trap instructions.
>> 
>>>> 
>>> 
>>> For the traps I don't know. The manual says "trap instructions that
>>> trap" are not traced. It means that "trap instructions that _don't_
>>> trap" are traced. Taking into account that trap instructions don't 
>>> trap
>>> at least 99.9% of the time, not sure if returning -1 is needed.
>>> 
>>> Allthought that'd probably be the safest.
>> 
>> 'trap' is a special case since it is predominantly used by debuggers
>> and/or tracing infrastructure. Kprobes and Uprobes do not allow probes
>> on a trap instruction. But, xmon can be asked to step on a trap
>> instruction and that can interfere with kprobes in weird ways.
>> 
>> So, I think it is best if we also exclude trap instructions from being
>> single stepped.
>> 
>>> 
>>> But then what happens with other instruction that will sparsely
>>> generate
>>> an exception like a DSI or so ? If we do it for the traps then we
>>> should
>>> do it for this as well, and then it becomes a non ending story.
>> 
>> For a DSI, we restart the same instruction after handling the page
>> fault.
>> The single step exception is raised on the subsequent successful
>> completion of the instruction.
> 
> Although it can cause a signal, and the signal handler can decide
> to resume somewhere else.

If a signal is generated while we are single-stepping, we delay signal
delivery (see uprobe_deny_signal()) until after the single stepping.
For fatal signals, single stepping is disabled before we allow the
signal to be delivered.

> Or kernel mode equivalent it can go to a
> fixup handler and resume somewhere else.

For kprobes, we do not allow probing instructions that have an extable
entry.

- Naveen

Re: [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction

[Naveen N. Rao]

On 2022-01-27 13:14, Nicholas Piggin wrote:
> Excerpts from Michael Ellerman's message of January 25, 2022 9:45 pm:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> 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.

> 
>> 
>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>> [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.
>> 
>> 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.

>> 
>> 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