Re: [PATCH RESEND] Fix race in process_vm_rw_core

From: Christopher Yeoh
Date: Wed Feb 01 2012 - 00:53:43 EST


On Mon, 30 Jan 2012 16:09:22 +0100
Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> On 01/30, Christopher Yeoh wrote:
> >
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -198,7 +198,7 @@ static int proc_root_link(struct dentry
> > *dentry, struct path *path) return result;
> > }
> >
> > -static struct mm_struct *mm_access(struct task_struct *task,
> > unsigned int mode) +struct mm_struct *mm_access(struct task_struct
> > *task, unsigned int mode)
>
> This is not enough, we should move it outside of fs/proc/, otherwise
> the kernel can't be compiled without CONFIG_PROC.
>

Updated patch below. Have moved mm_access to kernel/fork.c

This patch fixes the race in process_vm_rw_core found by Oleg
(http://article.gmane.org/gmane.linux.kernel/1235667/) which can occur
between __ptrace_may_access on the target task and if that task
is executing through execve at the same time.

Chris
--
cyeoh@xxxxxxxxxx
fs/proc/base.c | 20 --------------------
include/linux/sched.h | 6 ++++++
kernel/fork.c | 20 ++++++++++++++++++++
mm/process_vm_access.c | 20 ++++++--------------
4 files changed, 32 insertions(+), 34 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9cde9edf..2773412 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -198,26 +198,6 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
return result;
}

-static struct mm_struct *mm_access(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;
-}
-
struct mm_struct *mm_for_maps(struct task_struct *task)
{
return mm_access(task, PTRACE_MODE_READ);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2234985..7d379a6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2259,6 +2259,12 @@ static inline void mmdrop(struct mm_struct * mm)
extern void mmput(struct mm_struct *);
/* Grab a reference to a task's mm, if it is not already going away */
extern struct mm_struct *get_task_mm(struct task_struct *task);
+/*
+ * Grab a reference to a task's mm, if it is not already going away
+ * and ptrace_may_access with the mode parameter passed to it
+ * succeeds.
+ */
+extern struct mm_struct *mm_access(struct task_struct *task, unsigned int mode);
/* Remove the current tasks stale references to the old mm_struct */
extern void mm_release(struct task_struct *, struct mm_struct *);
/* Allocate a new mm structure and copy contents from tsk->mm */
diff --git a/kernel/fork.c b/kernel/fork.c
index 051f090..1b2ef3c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -647,6 +647,26 @@ struct mm_struct *get_task_mm(struct task_struct *task)
}
EXPORT_SYMBOL_GPL(get_task_mm);

+struct mm_struct *mm_access(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.
diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c
index e920aa3..82b6824 100644
--- a/mm/process_vm_access.c
+++ b/mm/process_vm_access.c
@@ -298,23 +298,15 @@ static ssize_t process_vm_rw_core(pid_t pid, const struct iovec *lvec,
goto free_proc_pages;
}

- task_lock(task);
- if (__ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
- task_unlock(task);
- rc = -EPERM;
- goto put_task_struct;
- }
- mm = task->mm;
-
- if (!mm || (task->flags & PF_KTHREAD)) {
- task_unlock(task);
- rc = -EINVAL;
+ mm = mm_access(task, PTRACE_MODE_ATTACH);
+ if (!mm || IS_ERR(mm)) {
+ if (!mm)
+ rc = -EINVAL;
+ else
+ rc = -EPERM;
goto put_task_struct;
}

- atomic_inc(&mm->mm_users);
- task_unlock(task);
-
for (i = 0; i < riovcnt && iov_l_curr_idx < liovcnt; i++) {
rc = process_vm_rw_single_vec(
(unsigned long)rvec[i].iov_base, rvec[i].iov_len,

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