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

From: Naveen N. Rao
Date: Thu Feb 04 2021 - 11:19:04 EST


On 2021/02/04 06:38PM, Naveen N. Rao wrote:
> 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@xxxxxxxxxxxxx>
> > ---
> > v1: http://lore.kernel.org/r/20210119091234.76317-1-ravi.bangoria@xxxxxxxxxxxxx
> > 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@xxxxxxxxxx>
> > */
> > #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)

Seeing the kprobes code, I realized that we have to check for another
scenario (Thanks, Jordan!). If this is the suffix of a prefix
instruction for which a uprobe has already been installed, then the
previous word will be a 'trap' instruction. You need to check if there
is a uprobe at the previous word, and if the original instruction there
was a prefix instruction.

- Naveen