From: Nicholas Piggin <npiggin@gmail.com> Date: 2022-01-24 05:59:14
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
From: Nicholas Piggin <npiggin@gmail.com> Date: 2022-01-24 05:58:37
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 <redacted>
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(-)
@@ -15,9 +15,6 @@#include<asm/cputable.h>#include<asm/disassemble.h>-externcharsystem_call_common[];-externcharsystem_call_vectored_emulate[];-#ifdef CONFIG_PPC64/* Bits in SRR1 that are copied from MSR */#define MSR_MASK 0xffffffff87c0ffffUL
From: Nicholas Piggin <npiggin@gmail.com> Date: 2022-01-24 05:59:51
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 <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(+)
@@ -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){
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
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
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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2022-01-25 11:46:31
Nicholas Piggin [off-list ref] 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.
quoted hunk
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(+)
@@ -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
From: Nicholas Piggin <npiggin@gmail.com> Date: 2022-01-27 07:45:18
Excerpts from Michael Ellerman's message of January 25, 2022 9:45 pm:
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).
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(+)
@@ -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
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.
quoted
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
From: Naveen N. Rao <hidden> Date: 2022-01-28 11:31:26
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(+)
@@ -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