Re: cross memory attach && security check

From: Oleg Nesterov
Date: Mon Jan 09 2012 - 09:59:33 EST

On 01/09, Christopher Yeoh wrote:
>
> 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?
>
> @@ -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);

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

--- x/fs/proc/base.c
+++ x/fs/proc/base.c
@@ -254,22 +254,7 @@ static struct mm_struct *check_mem_permission(struct task_struct *task)

struct mm_struct *mm_for_maps(struct task_struct *task)
{
- struct mm_struct *mm;
- int err;
-
- err = mutex_lock_killable(&task->signal->cred_guard_mutex);
- if (err)
- return ERR_PTR(err);
-
- mm = get_task_mm(task);
- if (mm && mm != current->mm &&
- !ptrace_may_access(task, PTRACE_MODE_READ)) {
- mmput(mm);
- mm = ERR_PTR(-EACCES);
- }
- mutex_unlock(&task->signal->cred_guard_mutex);
-
- return mm;
+ return get_check_task_mm(task, PTRACE_MODE_READ);
}

static int proc_pid_cmdline(struct task_struct *task, char * buffer)
--- x/kernel/fork.c
+++ x/kernel/fork.c
@@ -644,6 +644,25 @@ struct mm_struct *get_task_mm(struct task_struct *task)
}
EXPORT_SYMBOL_GPL(get_task_mm);

+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);
+
+ mm = get_task_mm(task);
+ if (mm && mm != current->mm && !ptrace_may_access(task, mode)) {
+ mmput(mm);
+ mm = ERR_PTR(-EACCES);
+ }
+ mutex_unlock(&task->signal->cred_guard_mutex);
+
+ return mm;
+}
+
/* Please note the differences between mmput and mm_release.
* mmput is called whenever we stop holding onto a mm_struct,
* error success whatever.

?

Then process_vm_rw_core() can use get_check_task_mm(PTRACE_MODE_ATTACH).

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

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

Ah. Thanks I see. And I didn't realize that rvec means "remote vec".
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);
if (rc <= 0)
goto free_iovecs;


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?

Oleg.

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