Re: cross memory attach && security check

[Oleg Nesterov <oleg@redhat.com>]

On 01/11, Christopher Yeoh wrote:
>
> On Mon, 9 Jan 2012 15:53:42 +0100
> Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Yes, I think this works, but I don't think you should play with
> > task_lock() or ->mm_users, just use get_task_mm(). Better yet, can't
> > we do
> >
>
> I agree with the consolidation with mm_for_maps (though we might need
> to argue over EPERM vs EACCES. However, I originally broke out
> get_task_mm and ptrace_may_access using __ptrace_may_access instead
> because both these functions grab the task lock at the start and
> release it at the end. Seemed better just to take it once.
>
> +struct mm_struct *get_check_task_mm(struct task_struct *task, unsigned int mode)
> +{
> +	struct mm_struct *mm;
> +	int err;
> +
> +	err =  mutex_lock_killable(&task->signal->cred_guard_mutex);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	task_lock(task);
> +	if (__ptrace_may_access(task, mode)) {
> +		mm = ERR_PTR(-EPERM);
> +		goto out;
> +	}
> +
> +	mm = task->mm;
> +	if (mm) {
> +		if (task->flags & PF_KTHREAD)
> +			mm = NULL;
> +		else
> +			atomic_inc(&mm->mm_users);
> +	}
> +
> +out:
> +	task_unlock(task);

Well, this saves unlock+lock, but adds more copy-and-paste code.
Personally I'd prefer to consolidate.

> > Without FOLL_FORCE, say, gdb can't use the new syscall to set the
> > breakpoint or to read the !VM_READ mappings. OK, process_vm_rw() has
> > flags, we can add PROCESS_VM_FORCE if needed.
> >
>
> It wasn't really intended for gdb, but perhaps it could be
> used/consolidated with other ptrace stuff (I don't know). I'm not
> really sure what the best thing to do here is. As I mentioned there is
> a level of cooperation where I am using it and honouring mprotect may
> help pickup inadvertent application errors. Perhaps a PROCESS_VM_FORCE
> flag would be the more conservative option.

OK, agreed.

> > 	--- x/fs/read_write.c
> > 	+++ x/fs/read_write.c
> > 	@@ -633,8 +633,7 @@ ssize_t do_loop_readv_writev(struct file
> > 	 ssize_t rw_copy_check_uvector(int type, const struct iovec
> > __user * uvector, unsigned long nr_segs, unsigned long fast_segs,
> > 				      struct iovec *fast_pointer,
> > 	-			      struct iovec **ret_pointer,
> > 	-			      int check_access)
> > 	+			      struct iovec **ret_pointer)
> > 	 {
> > 		unsigned long seg;
> > 		ssize_t ret;
> > 	@@ -690,8 +689,8 @@ ssize_t rw_copy_check_uvector(int type,
> > 				ret = -EINVAL;
> > 				goto out;
> > 			}
> > 	-		if (check_access
> > 	-		    && unlikely(!access_ok(vrfy_dir(type),
> > buf, len))) {
> > 	+		if (type >= 0 &&
> > 	+		    unlikely(!access_ok(vrfy_dir(type), buf,
> > len))) { ret = -EFAULT;
> > 				goto out;
> > 			}
> >
> > and update the callers? In this case all callers just lose the
> > unneeded argument and the code above does
> >
> > 	rw_copy_check_uvector(-1, rvec);
> >
> > Perhaps we can add another NOCHECK (or whatever) define near
> > READ/WRITE.
> >
> > What do you think?
>
> Yes, this is much better. I think for readability we do need a define.
>
> rw_copy_check_uvector(NOCHECK, rvec)
>
> looks a bit odd (why am I passing NOCHECK to a function with the
> word check in it?).

But it also has "copy" in the name. However, I agree that NOCHECK doesn't
look very good, and

> So perhaps maybe IOVEC_ONLY or something like that?

I agree with any naming ;)

Oleg.