* [PATCH RESEND] Fix race in process_vm_rw_core @ 2012-01-30 2:14 Christopher Yeoh 2012-01-30 15:09 ` Oleg Nesterov 0 siblings, 1 reply; 7+ messages in thread From: Christopher Yeoh @ 2012-01-30 2:14 UTC (permalink / raw) To: Linus Torvalds, Oleg Nesterov; +Cc: Andrew Morton, linux-kernel Hi Linus, Below is a patch which fixes the race in process_vm_core found by Oleg (http://article.gmane.org/gmane.linux.kernel/1235667/). This has been updated since I last sent it since the creation of mm_access did almost exactly the same thing as parts of the previous version of this patch did. Regards, Chris -- cyeoh@au.ibm.com Signed-off-by: Chris Yeoh <yeohc@au1.ibm.com> Cc: stable@vger.kernel.org fs/proc/base.c | 2 +- include/linux/sched.h | 3 +++ mm/process_vm_access.c | 20 ++++++-------------- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 9cde9edf..0635dc0 100644 --- 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) { struct mm_struct *mm; int err; diff --git a/include/linux/sched.h b/include/linux/sched.h index 4032ec1..e6268d1 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -2259,6 +2259,9 @@ 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 */ +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/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, ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Fix race in process_vm_rw_core 2012-01-30 2:14 [PATCH RESEND] Fix race in process_vm_rw_core Christopher Yeoh @ 2012-01-30 15:09 ` Oleg Nesterov 2012-02-01 5:53 ` Christopher Yeoh 0 siblings, 1 reply; 7+ messages in thread From: Oleg Nesterov @ 2012-01-30 15:09 UTC (permalink / raw) To: Christopher Yeoh; +Cc: Linus Torvalds, Andrew Morton, linux-kernel 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. > --- 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); > - Looks obviously correct. Oleg. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Fix race in process_vm_rw_core 2012-01-30 15:09 ` Oleg Nesterov @ 2012-02-01 5:53 ` Christopher Yeoh 2012-02-01 6:09 ` Cong Wang 2012-02-01 6:10 ` Linus Torvalds 0 siblings, 2 replies; 7+ messages in thread From: Christopher Yeoh @ 2012-02-01 5:53 UTC (permalink / raw) To: Oleg Nesterov, Linus Torvalds, Andrew Morton; +Cc: linux-kernel On Mon, 30 Jan 2012 16:09:22 +0100 Oleg Nesterov <oleg@redhat.com> 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@au.ibm.com 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, ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Fix race in process_vm_rw_core 2012-02-01 5:53 ` Christopher Yeoh @ 2012-02-01 6:09 ` Cong Wang 2012-02-01 6:10 ` Linus Torvalds 1 sibling, 0 replies; 7+ messages in thread From: Cong Wang @ 2012-02-01 6:09 UTC (permalink / raw) To: Christopher Yeoh Cc: Oleg Nesterov, Linus Torvalds, Andrew Morton, linux-kernel On 02/01/2012 01:53 PM, Christopher Yeoh wrote: > + mm = mm_access(task, PTRACE_MODE_ATTACH); > + if (!mm || IS_ERR(mm)) { > + if (!mm) > + rc = -EINVAL; > + else > + rc = -EPERM; > goto put_task_struct; If IS_ERR(mm), you need to return PTR_ERR(mm), rather than -EPERM. And, is -EINVAL proper for !mm case? For me, -ENOENT is better. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Fix race in process_vm_rw_core 2012-02-01 5:53 ` Christopher Yeoh 2012-02-01 6:09 ` Cong Wang @ 2012-02-01 6:10 ` Linus Torvalds 2012-02-01 8:08 ` Christopher Yeoh 1 sibling, 1 reply; 7+ messages in thread From: Linus Torvalds @ 2012-02-01 6:10 UTC (permalink / raw) To: Christopher Yeoh; +Cc: Oleg Nesterov, Andrew Morton, linux-kernel On Tue, Jan 31, 2012 at 9:53 PM, Christopher Yeoh <cyeoh@au1.ibm.com> wrote: > + mm = mm_access(task, PTRACE_MODE_ATTACH); > + if (!mm || IS_ERR(mm)) { > + if (!mm) > + rc = -EINVAL; > + else > + rc = -EPERM; > goto put_task_struct; Btw, do you really want to throw away the error code? IOW, maybe it should be rc = IS_ERR(mm) ? PTR_ERR(mm) : -EINVAL; or something? Instead of forcing the EPERM? And the -EINVAL might be better off as an ESRCH? I dunno. Right now mm_access() returns EACCES for a permission problem, not EPERM. EACCES is the normal filesystem access "Permission denied", while EPERM is "Operation not permitted". I do agree that EPERM tends to go with non-filesystem system calls (like ptrace() or sending signals to a process that you aren't allowed to), so I do agree that EPERM makes perfect sense within the context of process_vm_rw(). HOWEVER. mm_access() can actually also return EINTR. Now, admittedly it only returns that for killable signals, so user applications should never *see* that, but it's a special-case example of other errors at least being possible. What if we ever have some situation where we end up needing a temporary memory allocation and could return ENOMEM? Right now you turn all errors into EPERM, whether they were really about permission problems or not. And that just makes be a bit nervous. I wonder if we wouldn't be better off just returning EACCES (and any possible future problem) than try so hard to always return EPERM? I dunno. I don't have any really *strong* opinion and I see why you do it, but my gut feel is still that the error number change really does seem a bit arbitrary. Linus ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Fix race in process_vm_rw_core 2012-02-01 6:10 ` Linus Torvalds @ 2012-02-01 8:08 ` Christopher Yeoh 2012-02-02 1:04 ` Christopher Yeoh 0 siblings, 1 reply; 7+ messages in thread From: Christopher Yeoh @ 2012-02-01 8:08 UTC (permalink / raw) To: Linus Torvalds; +Cc: Oleg Nesterov, Andrew Morton, linux-kernel On Tue, 31 Jan 2012 22:10:13 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Jan 31, 2012 at 9:53 PM, Christopher Yeoh <cyeoh@au1.ibm.com> > wrote: > > + mm = mm_access(task, PTRACE_MODE_ATTACH); > > + if (!mm || IS_ERR(mm)) { > > + if (!mm) > > + rc = -EINVAL; > > + else > > + rc = -EPERM; > > goto put_task_struct; > > Btw, do you really want to throw away the error code? > > IOW, maybe it should be > > rc = IS_ERR(mm) ? PTR_ERR(mm) : -EINVAL; > > or something? Instead of forcing the EPERM? And the -EINVAL might be > better off as an ESRCH? I dunno. Yea, that probably makes more sense. > Right now you turn all errors into EPERM, whether they were really > about permission problems or not. And that just makes be a bit > nervous. I wonder if we wouldn't be better off just returning EACCES > (and any possible future problem) than try so hard to always return > EPERM? > > I dunno. I don't have any really *strong* opinion and I see why you do > it, but my gut feel is still that the error number change really does > seem a bit arbitrary. I'm not super attached to returning EPERM instead of EACCES though I do think it would look at bit odd from a user of the syscall view. Is it too ugly to do: rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH; if (rc == -EACCES) rc = -EPERM; That way we avoid the problem of overwriting EINTR and if there are changes in the future which return different error codes we won't override those. If you think it is too ugly then I'll give in and just return EACESS. Just should to get it settled before too many people start using the syscalls. Chris -- cyeoh@au.ibm.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RESEND] Fix race in process_vm_rw_core 2012-02-01 8:08 ` Christopher Yeoh @ 2012-02-02 1:04 ` Christopher Yeoh 0 siblings, 0 replies; 7+ messages in thread From: Christopher Yeoh @ 2012-02-02 1:04 UTC (permalink / raw) To: Linus Torvalds; +Cc: Oleg Nesterov, Andrew Morton, linux-kernel On Wed, 1 Feb 2012 18:38:36 +1030 cyeoh@ozlabs.au.ibm.com wrote: > On Tue, 31 Jan 2012 22:10:13 -0800 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > On Tue, Jan 31, 2012 at 9:53 PM, Christopher Yeoh > > <cyeoh@au1.ibm.com> wrote: > > > + mm = mm_access(task, PTRACE_MODE_ATTACH); > > > + if (!mm || IS_ERR(mm)) { > > > + if (!mm) > > > + rc = -EINVAL; > > > + else > > > + rc = -EPERM; > > > goto put_task_struct; > > > > Btw, do you really want to throw away the error code? > > > > IOW, maybe it should be > > > > rc = IS_ERR(mm) ? PTR_ERR(mm) : -EINVAL; > > > > or something? Instead of forcing the EPERM? And the -EINVAL might be > > better off as an ESRCH? I dunno. > > Yea, that probably makes more sense. > > > Right now you turn all errors into EPERM, whether they were really > > about permission problems or not. And that just makes be a bit > > nervous. I wonder if we wouldn't be better off just returning EACCES > > (and any possible future problem) than try so hard to always return > > EPERM? > > > > I dunno. I don't have any really *strong* opinion and I see why you > > do it, but my gut feel is still that the error number change really > > does seem a bit arbitrary. > > I'm not super attached to returning EPERM instead of EACCES though I > do think it would look at bit odd from a user of the syscall view. Is > it too ugly to do: > > rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH; > if (rc == -EACCES) > rc = -EPERM; > > That way we avoid the problem of overwriting EINTR and if there are > changes in the future which return different error codes we won't > override those. > Here's an updated patch doing the above of mapping EACCES but only EACCES to EPERM. -- cyeoh@au.ibm.com Signed-off-by: Chris Yeoh <yeohc@au1.ibm.com> fs/proc/base.c | 20 -------------------- include/linux/sched.h | 6 ++++++ kernel/fork.c | 20 ++++++++++++++++++++ mm/process_vm_access.c | 23 +++++++++-------------- 4 files changed, 35 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..c20ff48 100644 --- a/mm/process_vm_access.c +++ b/mm/process_vm_access.c @@ -298,23 +298,18 @@ 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)) { + rc = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH; + /* + * Explicitly map EACCES to EPERM as EPERM is a more a + * appropriate error code for process_vw_readv/writev + */ + if (rc == -EACCES) + 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, ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-02-02 1:04 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-01-30 2:14 [PATCH RESEND] Fix race in process_vm_rw_core Christopher Yeoh 2012-01-30 15:09 ` Oleg Nesterov 2012-02-01 5:53 ` Christopher Yeoh 2012-02-01 6:09 ` Cong Wang 2012-02-01 6:10 ` Linus Torvalds 2012-02-01 8:08 ` Christopher Yeoh 2012-02-02 1:04 ` Christopher Yeoh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).