Messages in this thread |  | | Subject | Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction | From | Ravi Bangoria <> | Date | Mon, 8 Feb 2021 16:40:58 +0530 |
| |
On 2/4/21 9:42 PM, Naveen N. Rao wrote: > 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@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) > > 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.
Yes, this patch will fail to detect such scenario. I think I should read the instruction directly from file, like what copy_insn() does. With that, I'll get original instruction rather that 'trap'.
I'll think more along this line.
Ravi
|  |