Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction

From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
To: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: mpe@ellerman.id.au, oleg@redhat.com, rostedt@goodmis.org,
	paulus@samba.org, jniethe5@gmail.com, naveen.n.rao@linux.ibm.com,
	sandipan@linux.ibm.com, linuxppc-dev@lists.ozlabs.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction
Date: Thu, 4 Feb 2021 18:38:21 +0530	[thread overview]
Message-ID: <20210204130821.GK210@DESKTOP-TDPLP67.localdomain> (raw)
In-Reply-To: <20210204104703.273429-1-ravi.bangoria@linux.ibm.com>

On 2021/02/04 04:17PM, Ravi Bangoria wrote:
> Don't allow Uprobe on 2nd word of a prefixed instruction. As per
> ISA 3.1, prefixed instruction should not cross 64-byte boundary.
> So don't allow Uprobe on such prefixed instruction as well.
> 
> There are two ways probed instruction is changed in mapped pages.
> First, when Uprobe is activated, it searches for all the relevant
> pages and replace instruction in them. In this case, if we notice
> that probe is on the 2nd word of prefixed instruction, error out
> directly. Second, when Uprobe is already active and user maps a
> relevant page via mmap(), instruction is replaced via mmap() code
> path. But because Uprobe is invalid, entire mmap() operation can
> not be stopped. In this case just print an error and continue.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
> v1: http://lore.kernel.org/r/20210119091234.76317-1-ravi.bangoria@linux.ibm.com
> v1->v2:
>   - Instead of introducing new arch hook from verify_opcode(), use
>     existing hook arch_uprobe_analyze_insn().
>   - Add explicit check for prefixed instruction crossing 64-byte
>     boundary. If probe is on such instruction, throw an error.
> 
>  arch/powerpc/kernel/uprobes.c | 66 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> index e8a63713e655..485d19a2a31f 100644
> --- a/arch/powerpc/kernel/uprobes.c
> +++ b/arch/powerpc/kernel/uprobes.c
> @@ -7,6 +7,7 @@
>   * Adapted from the x86 port by Ananth N Mavinakayanahalli <ananth@in.ibm.com>
>   */
>  #include <linux/kernel.h>
> +#include <linux/highmem.h>
>  #include <linux/sched.h>
>  #include <linux/ptrace.h>
>  #include <linux/uprobes.h>
> @@ -28,6 +29,69 @@ bool is_trap_insn(uprobe_opcode_t *insn)
>  	return (is_trap(*insn));
>  }
>  
> +#ifdef CONFIG_PPC64
> +static int get_instr(struct mm_struct *mm, unsigned long addr, u32 *instr)
> +{
> +	struct page *page;
> +	struct vm_area_struct *vma;
> +	void *kaddr;
> +	unsigned int gup_flags = FOLL_FORCE | FOLL_SPLIT_PMD;
> +
> +	if (get_user_pages_remote(mm, addr, 1, gup_flags, &page, &vma, NULL) <= 0)
> +		return -EINVAL;
> +
> +	kaddr = kmap_atomic(page);
> +	*instr = *((u32 *)(kaddr + (addr & ~PAGE_MASK)));
> +	kunmap_atomic(kaddr);
> +	put_page(page);
> +	return 0;
> +}
> +
> +static int validate_prefixed_instr(struct mm_struct *mm, unsigned long addr)
> +{
> +	struct ppc_inst inst;
> +	u32 prefix, suffix;
> +
> +	/*
> +	 * No need to check if addr is pointing to beginning of the
> +	 * page. Even if probe is on a suffix of page-unaligned
> +	 * prefixed instruction, hw will raise exception and kernel
> +	 * will send SIGBUS.
> +	 */
> +	if (!(addr & ~PAGE_MASK))
> +		return 0;
> +
> +	if (get_instr(mm, addr, &prefix) < 0)
> +		return -EINVAL;
> +	if (get_instr(mm, addr + 4, &suffix) < 0)
> +		return -EINVAL;
> +
> +	inst = ppc_inst_prefix(prefix, suffix);
> +	if (ppc_inst_prefixed(inst) && (addr & 0x3F) == 0x3C) {
> +		printk_ratelimited("Cannot register a uprobe on 64 byte "
		^^^^^^^^^^^^^^^^^^ pr_info_ratelimited()

It should be sufficient to check the primary opcode to determine if it 
is a prefixed instruction. You don't have to read the suffix. I see that 
we don't have a helper to do this currently, so you could do:

	if (ppc_inst_primary_opcode(ppc_inst(prefix)) == 1)

In the future, it might be worthwhile to add IS_PREFIX() as a macro 
similar to IS_MTMSRD() if there are more such uses.

Along with this, if you also add the below to the start of this 
function, you can get rid of the #ifdef:

	if (!IS_ENABLED(CONFIG_PPC64))
		return 0;

- Naveen