Re: [PATCH 1/2] powerpc: Reject probes on instructions that can't be single stepped
Christophe Leroy wrote:
Le 28/03/2022 à 19:20, Naveen N. Rao a écrit :
Michael Ellerman wrote:
Murilo Opsfelder Araújo writes:
On 3/23/22 08:51, Naveen N. Rao wrote:
+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 ?
+ case 31:
+ switch ((inst >> 1) & 0x3ff) {
For that one you have get_xop() in asm/disassemble.h
Nice! I will use those.
+ case 4: /* tw */
OP_31_XOP_TRAP
+ return false;
+ case 68: /* td */
OP_31_XOP_TRAP_64
+ 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
Re: [PATCH 1/2] powerpc: Reject probes on instructions that can't be single stepped
Le 28/03/2022 à 19:20, Naveen N. Rao a écrit :
> Michael Ellerman wrote:
>> Murilo Opsfelder Araújo writes:
>>> On 3/23/22 08:51, Naveen N. Rao wrote:
+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 ?
>
+ case 31:
+ switch ((inst >> 1) & 0x3ff) {
For that one you have get_xop() in asm/disassemble.h
+ case 4: /* tw */
OP_31_XOP_TRAP
+ return false;
+ case 68: /* td */
OP_31_XOP_TRAP_64
+ 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
Re: [PATCH 1/2] powerpc: Reject probes on instructions that can't be single stepped
Michael Ellerman wrote:
Murilo Opsfelder Araújo writes:
On 3/23/22 08:51, Naveen N. Rao wrote:
+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.
+ 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
Re: [PATCH 1/2] powerpc: Reject probes on instructions that can't be single stepped
Murilo Opsfelder Araújo writes:
> On 3/23/22 08:51, Naveen N. Rao wrote:
...
>> +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
Re: [PATCH 1/2] powerpc: Reject probes on instructions that can't be single stepped
Hi, Naveen.
Some comments below.
On 3/23/22 08:51, Naveen N. Rao wrote:
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
---
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) {
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?
/* 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
[PATCH 1/2] powerpc: Reject probes on instructions that can't be single stepped
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
---
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
