Re: [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction

[Michael Ellerman <mpe@ellerman.id.au>]

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