Re: [PATCH 1/2] powerpc: Reject probes on instructions that can't be single stepped

["Murilo Opsfelder Araújo" <mopsfelder@gmail.com>]

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