Re: cross memory attach && security check

[Christopher Yeoh]

On Thu, 5 Jan 2012 16:10:12 +0100
Oleg Nesterov <oleg@xxxxxxxxxx> 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;
> 
> 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?

Signed-off-by: Chris Yeoh <yeohc@xxxxxxxxxxx>
 process_vm_access.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index e920aa3..207d7cc 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -298,9 +298,14 @@ static ssize_t process_vm_rw_core(pid_t pid, const struct iovec *lvec,
 		goto free_proc_pages;
 	}
 
+	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);
 		rc = -EPERM;
 		goto put_task_struct;
 	}
@@ -308,12 +313,14 @@ static ssize_t process_vm_rw_core(pid_t pid, const struct iovec *lvec,
 
 	if (!mm || (task->flags & PF_KTHREAD)) {
 		task_unlock(task);
+		mutex_unlock(&task->signal->cred_guard_mutex);
 		rc = -EINVAL;
 		goto put_task_struct;
 	}
 
 	atomic_inc(&mm->mm_users);
 	task_unlock(task);
+	mutex_unlock(&task->signal->cred_guard_mutex);
 
 	for (i = 0; i < riovcnt && iov_l_curr_idx < liovcnt; i++) {
 		rc = process_vm_rw_single_vec(

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

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?

> 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 which 
describes where to read/write from/to. So we need to do part of what
rw_copy_check_uvector ordinarily does (validate and copy the iovec
data), but we don't want to check the memory it points to at this point
because it refers to memory in the other process (this check is done later).

So the change to rw_copy_check_uvector to optionally not do this second check
allows us to reuse the code rather than have another function which is almost
identical.

Regards,

Chris
-- 
cyeoh@xxxxxxxxxx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/