Re: [3/3 PATCH] Kprobes: User space probes support- single stepping out-of-line

Andrew Morton
2006-03-20T11:12:57Z

Prasanna S Panchamukhi <prasanna@in.ibm.com> wrote:
>
> This patch provides a mechanism for probe handling and
> executing the user-specified handlers.
>
> Each userspace probe is uniquely identified by the combination of
> inode and offset, hence during registeration the inode and offset
> combination is added to uprobes hash table. Initially when
> breakpoint instruction is hit, the uprobes hash table is looked up
> for matching inode and offset. The pre_handlers are called in
> sequence if multiple probes are registered. Similar to kprobes,
> uprobes also adopts to single step out-of-line, so that probe miss in
> SMP environment can be avoided. But for userspace probes, instruction
> copied into kernel address space cannot be single stepped, hence the
> instruction must be copied to user address space. The solution is to
> find free space in the current process address space and then copy the
> original instruction and single step that instruction.
>
> User processes use stack space to store local variables, agruments and
> return values. Normally the stack space either below or above the
> stack pointer indicates the free stack space.
>
> The instruction to be single stepped can modify the stack space,
> hence before using the free stack space, sufficient stack space should
> be left. The instruction is copied to the bottom of the page and check
> is made such that the copied instruction does not cross the page
> boundry. The copied instruction is then single stepped. Several
> architectures does not allow the instruction to be executed from the
> stack location, since no-exec bit is set for the stack pages. In those
> architectures, the page table entry corresponding to the stack page is
> identified and the no-exec bit is unset making the instruction on that
> stack page to be executed.
>
> There are situations where even the free stack space is not enough for
> the user instruction to be copied and single stepped. In such
> situations, the virtual memory area(vma) can be expanded beyond the
> current stack vma. This expaneded stack can be used to copy the
> original instruction and single step out-of-line.
>
> Even if the vma cannot be extended then the instruction much be
> executed inline, by replacing the breakpoint instruction with original
> instruction.
>
> ...
>
> +
> +/**
> + * This routines get the pte of the page containing the specified address.
> + */
> +static pte_t __kprobes *get_uprobe_pte(unsigned long address)
> +{
> + pgd_t *pgd;
> + pud_t *pud;
> + pmd_t *pmd;
> + pte_t *pte = NULL;
> +
> + pgd = pgd_offset(current->mm, address);
> + if (!pgd)
> + goto out;
> +
> + pud = pud_offset(pgd, address);
> + if (!pud)
> + goto out;
> +
> + pmd = pmd_offset(pud, address);
> + if (!pmd)
> + goto out;
> +
> + pte = pte_alloc_map(current->mm, pmd, address);
> +
> +out:
> + return pte;
> +}

That's familiar looking code..

I guess this should be given a more generic name then placed in
mm/memory.c, which is where we do pagetable walking.

> +/**
> + * This routine check for space in the current process's stack
> + * address space. If enough address space is found, copy the original
> + * instruction on that page for single stepping out-of-line.
> + */
> +static int __kprobes copy_insn_on_new_page(struct uprobe *uprobe ,
> + struct pt_regs *regs, struct vm_area_struct *vma)
> +{
> + unsigned long addr, stack_addr = regs->esp;
> + int size = MAX_INSN_SIZE * sizeof(kprobe_opcode_t);
> +
> + if (vma->vm_flags & VM_GROWSDOWN) {
> + if (((stack_addr - sizeof(long long))) <
> + (vma->vm_start + size))
> + return -ENOMEM;
> + addr = vma->vm_start;
> + } else if (vma->vm_flags & VM_GROWSUP) {
> + if ((vma->vm_end - size) < (stack_addr + sizeof(long long)))
> + return -ENOMEM;
> + addr = vma->vm_end - size;
> + } else
> + return -EFAULT;
> +
> + vma->vm_flags |= VM_LOCKED;
> +
> + if (__copy_to_user_inatomic((unsigned long *)addr,
> + (unsigned long *)uprobe->kp.ainsn.insn, size))
> + return -EFAULT;
> +
> + regs->eip = addr;
> +
> + return 0;
> +}

If we're going to use __copy_to_user_inatomic() then we'll need some nice
comments explaining why this is happening.

And we'll need to actually *be* in-atomic. That means we need an
open-coded inc_preempt_count() and dec_preempt_count() in there and I don't
see them.

> +/**
> + * This routine expands the stack beyond the present process address
> + * space and copies the instruction to that location, so that
> + * processor can single step out-of-line.
> + */
> +static int __kprobes copy_insn_onexpstack(struct uprobe *uprobe,
> + struct pt_regs *regs, struct vm_area_struct *vma)
> +{
> + unsigned long addr, vm_addr;
> + int size = MAX_INSN_SIZE * sizeof(kprobe_opcode_t);
> + struct vm_area_struct *new_vma;
> + struct mm_struct *mm = current->mm;
> +
> +
> + if (!down_read_trylock(&current->mm->mmap_sem))
> + return -ENOMEM;
> +
> + if (vma->vm_flags & VM_GROWSDOWN)
> + vm_addr = vma->vm_start - size;
> + else if (vma->vm_flags & VM_GROWSUP)
> + vm_addr = vma->vm_end + size;
> + else {
> + up_read(&current->mm->mmap_sem);
> + return -EFAULT;
> + }
> +
> + new_vma = find_extend_vma(mm, vm_addr);
> + if (!new_vma) {
> + up_read(&current->mm->mmap_sem);
> + return -ENOMEM;
> + }
> +
> + if (new_vma->vm_flags & VM_GROWSDOWN)
> + addr = new_vma->vm_start;
> + else
> + addr = new_vma->vm_end - size;
> +
> + new_vma->vm_flags |= VM_LOCKED;
> + up_read(&current->mm->mmap_sem);
> +
> + if (__copy_to_user_inatomic((unsigned long *)addr,
> + (unsigned long *)uprobe->kp.ainsn.insn, size))
> + return -EFAULT;
> +
> + regs->eip = addr;
> +
> + return 0;
> +}

Why is VM_LOCKED being set? (It needs a comment).

Where does it get unset?

> +
> + if (__copy_to_user_inatomic((unsigned long *)page_addr,
> + source, size))
> + if (__copy_to_user_inatomic(
> + (unsigned long *)(page_addr - size), source, size))

See above.

> +
> +/**
> + * This routines get the page containing the probe, maps it and
> + * replaced the instruction at the probed address with specified
> + * opcode.
> + */
> +void __kprobes replace_original_insn(struct uprobe *uprobe,
> + struct pt_regs *regs, kprobe_opcode_t opcode)
> +{
> + kprobe_opcode_t *addr;
> + struct page *page;
> +
> + page = find_get_page(uprobe->inode->i_mapping,
> + uprobe->offset >> PAGE_CACHE_SHIFT);
> + BUG_ON(!page);
> +
> + __lock_page(page);

Whoa. Why is __lock_page() being used here? It looks like a bug is being
covered up.

> + addr = (kprobe_opcode_t *)kmap_atomic(page, KM_USER1);
> + addr = (kprobe_opcode_t *)((unsigned long)addr +
> + (unsigned long)(uprobe->offset & ~PAGE_MASK));
> + *addr = opcode;
> + /*TODO: flush vma ? */

flush_dcache_page() would be needed.

But then, what happens if the page is shared by other processes? Do they
all start taking debug traps?

> + kunmap_atomic(addr, KM_USER1);
> +
> + unlock_page(page);
> +
> + if (page)
> + page_cache_release(page);
> + regs->eip = (unsigned long)uprobe->kp.addr;
> +}
> +
> +/**
> + * This routine provides the functionality of single stepping
> + * out-of-line. If single stepping out-of-line cannot be achieved,
> + * it replaces with the original instruction allowing it to single
> + * step inline.
> + */
> +static inline int prepare_singlestep_uprobe(struct uprobe *uprobe,
> + struct uprobe_ctlblk *ucb, struct pt_regs *regs)
> +{
> + unsigned long stack_addr = regs->esp, flags;
> + struct vm_area_struct *vma = NULL;
> + int err = 0;
> +
> + vma = find_vma(current->mm, (stack_addr & PAGE_MASK));

I don't think mmap_sem is held here?

> +static inline int uprobe_fault_handler(struct pt_regs *regs, int trapnr)

If, for some reason, the compiler decides to not inline this function then
you have a hunk of code which isn't marked __kprobes, but it should be.

I'd suggest that you remove all inlining from this code and add the
appropriate section markers.

Or I guess you could use __always_inline, but I'm not sure that it's really
worth the fuss and obscurity of doing that.

All kprobes-related code should be audited for this problem.