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.bango...@linux.ibm.com> > > --- > > v1: > > http://lore.kernel.org/r/20210119091234.76317-1-ravi.bango...@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 > > <ana...@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)
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