Messages in this thread Patch in this message | | | Date | Mon, 27 May 2013 22:28:16 +0200 | From | Oleg Nesterov <> | Subject | [PATCH 1/3] proc: first_tid: fix the potential use-after-free |
| |
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
| |