Message ID | 20220124055741.3686496-3-npiggin@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | powerpc: Disable syscall emulation and stepping | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
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
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
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
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) {