Saturday, November 27, 2010

posix cpu timers: RCU read-side critical section

POSIX cpu timers were calling find_task_by_vpid in insecure way
(since 4221a9918e38b7494cee341dda7b7b4bb8c04bde which requires
RCU read-side critical section).

Thomas Gleixner wrote:
| We can remove the tasklist_lock while at it. rcu_read_lock is enough.

Patch also replaces thread_group_leader with has_group_leader_pid
in accordance to comment by Oleg Nesterov:

| ... thread_group_leader() check is not relaible without
| tasklist. If we race with de_thread() find_task_by_vpid() can find
| the new leader before it updates its ->group_leader.
|
| perhaps it makes sense to change posix_cpu_timer_create() to use
| has_group_leader_pid() instead, just to make this code not look racy
| and avoid adding new problems.



Thanks to:
    Reviewed-by: Oleg Nesterov
    Cc: Peter Zijlstra
    Cc: Stanislaw Gruszka
    Signed-off-by: Thomas Gleixner


BTW, Oleg, in turn, fixed security issue
commit e0a70217107e6f9844628120412cb27bb4cea194
Author: Oleg Nesterov <>
posix-cpu-timers: workaround to suppress the problems with mt exec


---

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index 6842eeb..05bb717 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -37,13 +37,13 @@ static int check_clock(const clockid_t which_clock)
     if (pid == 0)
         return 0;

-    read_lock(&tasklist_lock);
+    rcu_read_lock();
     p = find_task_by_vpid(pid);
     if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
-           same_thread_group(p, current) : thread_group_leader(p))) {
+           same_thread_group(p, current) : has_group_leader_pid(p))) {
         error = -EINVAL;
     }
-    read_unlock(&tasklist_lock);
+    rcu_read_unlock();

     return error;
 }
@@ -390,7 +390,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)

     INIT_LIST_HEAD(&new_timer->it.cpu.entry);

-    read_lock(&tasklist_lock);
+    rcu_read_lock();
     if (CPUCLOCK_PERTHREAD(new_timer->it_clock)) {
         if (pid == 0) {
             p = current;
@@ -404,7 +404,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
             p = current->group_leader;
         } else {
             p = find_task_by_vpid(pid);
-            if (p && !thread_group_leader(p))
+            if (p && !has_group_leader_pid(p))
                 p = NULL;
         }
     }
@@ -414,7 +414,7 @@ int posix_cpu_timer_create(struct k_itimer *new_timer)
     } else {
         ret = -EINVAL;
     }
-    read_unlock(&tasklist_lock);
+    rcu_read_unlock();

     return ret;
 }

No comments:

Post a Comment