[PATCH 1/2] powerpc: Reject probes on instructions that can't be single stepped
Murilo Opsfelder Araújo
mopsfelder at gmail.com
Fri Mar 25 01:15:32 AEDT 2022
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 <naveen.n.rao at linux.vnet.ibm.com>
> ---
> 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 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 (!can_single_step(ppc_inst_val(insn))) {
> + printk("Cannot register a kprobe on instructions that can't be single stepped\n");
> ret = -EINVAL;
> } else if ((unsigned long)p->addr & ~PAGE_MASK &&
> ppc_inst_prefixed(ppc_inst_read(p->addr - 1))) {
> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> index c6975467d9ffdc..95a41ae9dfa755 100644
> --- a/arch/powerpc/kernel/uprobes.c
> +++ b/arch/powerpc/kernel/uprobes.c
> @@ -48,6 +48,11 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
> return -EINVAL;
> }
>
> + if (!can_single_step(ppc_inst_val(ppc_inst_read(auprobe->insn)))) {
> + pr_info_ratelimited("Cannot register a uprobe on instructions that can't be single stepped\n");
> + return -ENOTSUPP;
> + }
> +
> return 0;
> }
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index fd72753e8ad502..a92c5739d954e2 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -921,9 +921,9 @@ static void insert_bpts(void)
> bp->enabled = 0;
> continue;
> }
> - if (IS_MTMSRD(instr) || IS_RFID(instr)) {
> - printf("Breakpoint at %lx is on an mtmsrd or rfid "
> - "instruction, disabling it\n", bp->address);
> + if (!can_single_step(ppc_inst_val(instr))) {
> + printf("Breakpoint at %lx is on an instruction that can't be single stepped, disabling it\n",
> + bp->address);
> bp->enabled = 0;
> continue;
> }
> @@ -1470,9 +1470,8 @@ static long check_bp_loc(unsigned long addr)
> printf("Can't read instruction at address %lx\n", addr);
> return 0;
> }
> - if (IS_MTMSRD(instr) || IS_RFID(instr)) {
> - printf("Breakpoints may not be placed on mtmsrd or rfid "
> - "instructions\n");
> + if (!can_single_step(ppc_inst_val(instr))) {
> + printf("Breakpoints may not be placed on instructions that can't be single stepped\n");
> return 0;
> }
> return 1;
Cheers!
--
Murilo
More information about the Linuxppc-dev
mailing list