cross memory attach && security check

Just noticed the new file in mm/ ;) A couple of questions.

process_vm_rw_core() does

	task_lock(task);
	if (__ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
		task_unlock(task);
		rc = -EPERM;
		goto put_task_struct;
	}
	mm = task->mm;

this is racy, task_lock() can't help. And I don't think you should use it directly.

execve() does exec_mmap() first, this switches to the new ->mm. After that install_exec_creds() changes task->cred. The window is not that small.

I guess you need ->cred_guard_mutex, please look at mm_for_maps().

Another question, process_vm_rw_pages() does get_user_pages() without FOLL_FORCE. Is this on purpose? This limits the usage of the new syscalls.

Hmm. And could you please explain the change in rw_copy_check_uvector()? Why process_vm_rw() does rw_copy_check_uvector(READ, rvec, check_access => 0) ?

Quoted 12 lines · On Thu, 5 Jan 2012 16:10:12 +0100 Oleg Nesterov <[email protected]> wrote:

Just noticed the new file in mm/ ;) A couple of questions.

process_vm_rw_core() does

	task_lock(task);
	if (__ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
		task_unlock(task);
		rc = -EPERM;
		goto put_task_struct;
	}
	mm = task->mm;

Quoted 9 lines

this is racy, task_lock() can't help. And I don't think you should use it directly.

execve() does exec_mmap() first, this switches to the new ->mm. After that install_exec_creds() changes task->cred. The window is not that small.

I guess you need ->cred_guard_mutex, please look at mm_for_maps().

Thanks, agreed this looks like it's a problem. Need to do a bit more testing, but I think the following patch fixes the race?

mm/process_vm_access.c +0 -0

@@ -298,9 +298,14 @@ static ssize_t process_vm_rw_core(pid_t pid, const struct iovec *lvec,
 		goto free_proc_pages;

Other than reading the comment for get_user_pages saying that I don't want to set the force flag, I didn't really consider it. The use cases where I'm interested (intranode communication) has the cooperation of the target process anyway so its not needed. Any downsides to having FOLL_FORCE enabled?

Quoted 3 lines

Hmm. And could you please explain the change in rw_copy_check_uvector()? Why process_vm_rw() does rw_copy_check_uvector(READ, rvec, check_access => 0) ?

process_vm_readv/writev get passed an iovec for another process

Partly I was confused because (I guess) there is another minor bug in process_vm_rw(), I think we need

	--- x/mm/process_vm_access.c
	+++ x/mm/process_vm_access.c
	@@ -375,10 +375,10 @@ static ssize_t process_vm_rw(pid_t pid,
	 
		/* Check iovecs */
		if (vm_write)
	-		rc = rw_copy_check_uvector(WRITE, lvec, liovcnt, UIO_FASTIOV,
	+		rc = rw_copy_check_uvector(READ, lvec, liovcnt, UIO_FASTIOV,
						   iovstack_l, &iov_l, 1);
		else
	-		rc = rw_copy_check_uvector(READ, lvec, liovcnt, UIO_FASTIOV,
	+		rc = rw_copy_check_uvector(WRITE, lvec, liovcnt, UIO_FASTIOV,
						   iovstack_l, &iov_l, 1);

Quoted 15 lines · Christopher Yeoh

Quoted 3 lines · On Thu, 5 Jan 2012 16:10:12 +0100 Oleg Nesterov <[email protected]> wrote:

I guess you need ->cred_guard_mutex, please look at mm_for_maps().

Thanks, agreed this looks like it's a problem. Need to do a bit more testing, but I think the following patch fixes the race?

(quoted diff) +0 -0

@@ -298,9 +298,14 @@ static ssize_t process_vm_rw_core(pid_t pid,
 const struct iovec *lvec, goto free_proc_pages;
 	}
 

Review static ssize_t process_vm_rw_core(pid_t pid, · line 309

Quoted 9 lines

(quoted diff) +5 -0

+	rc =
 mutex_lock_interruptible(&task->signal->cred_guard_mutex);
+	if (rc)
+		goto put_task_struct;
+
 	task_lock(task);
 	if (__ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
 		task_unlock(task);
+		mutex_unlock(&task->signal->cred_guard_mutex);

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

Quoted 5 lines

Other than reading the comment for get_user_pages saying that I don't want to set the force flag, I didn't really consider it. The use cases where I'm interested (intranode communication) has the cooperation of the target process anyway so its not needed. Any downsides to having FOLL_FORCE enabled?

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.

However. Yes, this is subjective, but imho the new argument looks a bit ugly. Please look at this code again,

	rw_copy_check_uvector(READ, rvec, check_access => 0);

what does this READ means without check_access? Plus we need another argument. Can't we do

	--- 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?

Quoted 5 lines · On Mon, 9 Jan 2012 15:53:42 +0100 Oleg Nesterov <[email protected]> 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.

kernel/fork.c +15 -0

+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;
+	}
+

Quoted 4 lines

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.

Quoted 33 lines

	--- 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.

Quoted 1 line

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?).

So perhaps maybe IOVEC_ONLY or something like that?