From: Oleg Nesterov <oleg@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>,
"Eric W. Biederman" <ebiederm@xmission.com>,
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
Michal Hocko <mhocko@suse.cz>, Sergey Dyasly <dserrg@gmail.com>,
Sha Zhengju <handai.szj@taobao.com>,
linux-kernel@vger.kernel.org
Subject: [PATCH 1/3] proc: first_tid: fix the potential use-after-free
Date: Mon, 27 May 2013 22:28:16 +0200 [thread overview]
Message-ID: <20130527202816.GA19277@redhat.com> (raw)
In-Reply-To: <20130527202751.GA19250@redhat.com>
proc_task_readdir() verifies that the result of get_proc_task()
is pid_alive() and thus its ->group_leader is fine too. However
this is not necessarily true after rcu_read_unlock(), we need
to recheck this after first_tid() does rcu_read_lock() again.
The race is subtle and unlikely, but still it is possible afaics.
To simplify lets ignore the "likely" case when tid != 0, f_version
can be cleared by proc_task_operations->llseek().
Suppose we have a main thread M and its subthread T. Suppose that
f_pos == 3, iow first_tid() should return T. Now suppose that the
following happens between rcu_read_unlock() and rcu_read_lock():
1. T execs and becomes the new leader. This removes M from
->thread_group but next_thread(M) is still T.
2. T creates another thread X which does exec as well, T
goes away.
3. X creates another subthread, this increments nr_threads.
4. first_tid() does next_thread(M) and returns the already
dead T.
Note that we need 2. and 3. only because of get_nr_threads() check,
and this check was supposed to be optimization only.
Note: I think that proc_task_readdir/first_tid interaction can be
simplified, but this needs another patch. proc_task_readdir() should
not play with ->group_leader at all. See the next patches.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
fs/proc/base.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index dd51e50..c939c9f 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3186,10 +3186,13 @@ static struct task_struct *first_tid(struct task_struct *leader,
goto found;
}
- /* If nr exceeds the number of threads there is nothing todo */
pos = NULL;
+ /* If nr exceeds the number of threads there is nothing todo */
if (nr && nr >= get_nr_threads(leader))
goto out;
+ /* It could be unhashed before we take rcu lock */
+ if (!pid_alive(leader))
+ goto out;
/* If we haven't found our starting place yet start
* with the leader and walk nr threads forward.
--
1.5.5.1
next prev parent reply other threads:[~2013-05-27 20:32 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-27 20:27 [PATCH 0/3] proc: first_tid() fix/cleanup Oleg Nesterov
2013-05-27 20:28 ` Oleg Nesterov [this message]
2013-05-29 4:08 ` [PATCH 1/3] proc: first_tid: fix the potential use-after-free Eric W. Biederman
2013-05-29 12:30 ` Oleg Nesterov
2013-05-27 20:28 ` [PATCH 2/3] proc: change first_tid() to use while_each_thread() Oleg Nesterov
2013-05-27 20:28 ` [PATCH 3/3] proc: simplify proc_task_readdir/first_tid paths Oleg Nesterov
2013-05-29 4:42 ` Eric W. Biederman
2013-05-29 13:39 ` Oleg Nesterov
2013-05-29 20:38 ` Eric W. Biederman
2013-05-31 16:38 ` Oleg Nesterov
2013-05-31 18:12 ` Eric W. Biederman
2013-05-31 18:34 ` Oleg Nesterov
2013-05-29 5:22 ` [PATCH 0/3] proc: first_tid() fix/cleanup Eric W. Biederman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130527202816.GA19277@redhat.com \
--to=oleg@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=dserrg@gmail.com \
--cc=ebiederm@xmission.com \
--cc=handai.szj@taobao.com \
--cc=kamezawa.hiroyu@jp.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mhocko@suse.cz \
--cc=rientjes@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.