Re: [PATCHv11 2.6.36-rc2-tip 2/15] 2: uprobes: Breakpoint insertion/removal in user space applications.

[Peter Zijlstra <peterz@infradead.org>]

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.