lkml.org 
[lkml]   [2010]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCHv11 2.6.36-rc2-tip 2/15] 2: uprobes: Breakpoint insertion/removal in user space applications.
From
Date
On Wed, 2010-08-25 at 19:11 +0530, Srikar Dronamraju wrote:

> +struct user_bkpt_arch_info *arch = &user_bkpt_arch_info;

That really wants to be static, 'arch' is a way too generic a name to
inject in the global namespace.

> +unsigned long uprobes_read_vm(struct task_struct *tsk, void __user *vaddr,
> + void *kbuf, unsigned long nbytes)
> +{
> + if (tsk == current) {
> + unsigned long nleft = copy_from_user(kbuf, vaddr, nbytes);
> + return nbytes - nleft;
> + } else
> + return access_process_vm(tsk, (unsigned long) vaddr, kbuf,
> + nbytes, 0);
> +}
> +

> +unsigned long uprobes_write_data(struct task_struct *tsk,
> + void __user *vaddr, const void *kbuf,
> + unsigned long nbytes)
> +{
> + unsigned long nleft;
> +
> + if (tsk == current) {
> + nleft = copy_to_user(vaddr, kbuf, nbytes);
> + return nbytes - nleft;
> + } else
> + return access_process_vm(tsk, (unsigned long) vaddr,
> + (void *) kbuf, nbytes, 1);
> +}

either: s/uprobes_read_vm/uprobes_read_data/ or
s/uproves_write_data/uproves_write_vm/

> +static int write_opcode(struct task_struct *tsk, unsigned long vaddr,
> + user_bkpt_opcode_t opcode)
> +{
> + struct mm_struct *mm;
> + struct vm_area_struct *vma;
> + struct page *old_page, *new_page;
> + void *vaddr_old, *vaddr_new;
> + pte_t orig_pte;
> + int ret = -EINVAL;
> +
> + if (!tsk)
> + return ret;
> +
> + mm = get_task_mm(tsk);
> + if (!mm)
> + return ret;
> +
> + down_read(&mm->mmap_sem);
> +
> + /* Read the page with vaddr into memory */
> + ret = get_user_pages(tsk, mm, vaddr, 1, 1, 1, &old_page, &vma);
> + if (ret <= 0)
> + goto mmput_out;
> +
> + /*
> + * check if the page we are interested is read-only mapped
> + * Since we are interested in text pages, Our pages of interest
> + * should be mapped read-only.
> + */
> + if ((vma->vm_flags && (VM_READ|VM_WRITE)) != VM_READ) {

s/&&/&/

> + ret = -EINVAL;
> + goto put_out;
> + }
> +
> + /* If its VM_SHARED vma, lets not write to such vma's. */
> + if (vma->vm_flags & VM_SHARED) {
> + ret = -EINVAL;
> + goto put_out;
> + }

Something like:

/* private, read-only, executable maps only */
if ((vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)) != (VM_READ|VM_EXEC))

maybe?

> + /* Allocate a page */
> + new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, vaddr);
> + if (!new_page) {
> + ret = -ENOMEM;
> + goto put_out;
> + }

> +int __weak set_orig_insn(struct task_struct *tsk,
> + struct user_bkpt *user_bkpt, bool verify)
> +{
> + if (verify) {
> + user_bkpt_opcode_t opcode;
> + int result = read_opcode(tsk, user_bkpt->vaddr, &opcode);
> + if (result)
> + return result;
> + if (opcode != arch->bkpt_insn)

This assumes user_bkpt_opcode_t is a scalar value, but there's no
assertion of that, if someone were to define it like char[5] or somesuch
the comparison would still compile but not do what you'd expect.

> + return -EINVAL;
> + }
> + return write_opcode(tsk, user_bkpt->vaddr, user_bkpt->opcode);
> +}

> +/**
> + * check_vma - verify if the address is in a executable vma.
> + * @tsk: the probed task
> + * @vaddr: virtual address of the instruction to be verified.
> + *
> + * Return 0 if vaddr is in an executable VM area,
> + * or -EINVAL otherwise.
> + */
> +static int check_vma(struct task_struct *tsk, unsigned long vaddr)
> +{
> + struct vm_area_struct *vma;
> + struct mm_struct *mm;
> + int ret = -EINVAL;
> +
> + mm = get_task_mm(tsk);
> + if (!mm)
> + return -EINVAL;
> + down_read(&mm->mmap_sem);
> + vma = find_vma(mm, vaddr);
> + if (vma && vaddr >= vma->vm_start && (vma->vm_flags & VM_EXEC))

you fail to check vma->vm_end

Also, do we want to do the full private,ro,exec check here again?

> + ret = 0;
> + up_read(&mm->mmap_sem);
> + mmput(mm);
> + return ret;
> +}

> +int __weak validate_address(struct task_struct *tsk, unsigned long vaddr)
> +{
> + return check_vma(tsk, vaddr);
> +}

So here check_vma() is the default implementation of validate_address(),
so why not name them accordingly?

> +/*
> + * __insert_bkpt - insert breakpoint
> + * Insert a breakpoint into the process that includes @tsk, at the
> + * virtual address @user_bkpt->vaddr.
> + *
> + * All threads of the probed process must be stopped while
> + * @__insert_bkpt() runs.

I hope not,.. the pte swizzle we do above does not require any such
thing, stale comment?

> + * Possible errors:
> + * -%ENOSYS: user_bkpt not supported for this architecture
> + * -%EINVAL: invalid instruction address
> + * -%EEXIST: breakpoint instruction already exists at that address
> + * -%EPERM: cannot probe this instruction
> + * -%EFAULT: failed to insert breakpoint instruction
> + */



> +static int pre_sstep(struct task_struct *tsk, struct user_bkpt *user_bkpt,
> + struct user_bkpt_task_arch_info *tskinfo, struct pt_regs *regs)
> +{
> + return pre_xol(tsk, user_bkpt, tskinfo, regs);
> +}
> +

> +static int post_sstep(struct task_struct *tsk, struct user_bkpt *user_bkpt,
> + struct user_bkpt_task_arch_info *tskinfo, struct pt_regs *regs)
> +{
> + return post_xol(tsk, user_bkpt, tskinfo, regs);
> +}

What's the point of these functions?


> +static int __remove_bkpt(struct task_struct *tsk,
> + struct user_bkpt *user_bkpt)
> +{
> + if (validate_address(tsk, user_bkpt->vaddr) != 0)
> + return -EINVAL;
> + return set_orig_insn(tsk, user_bkpt, true);
> +}

Why would we even consider calling this function on something that would
fail the validate_address() test? If that fails we would not have
installed the breakpoint to begin with, hence there would be no reason
to remove it.

> +bool __weak is_bkpt_insn(struct user_bkpt *user_bkpt)
> +{
> + return (user_bkpt->opcode == arch->bkpt_insn);
> +}

Again, assumes the instruction thing is a scalar.



The big thing I'm missing in this patch is generic code handling the
actual breakpoint.. but maybe that's somewhere in the next patches.. /me
goes look.




\
 
 \ /
  Last update: 2010-09-01 21:41    [W:0.282 / U:67.208 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site