Changeset 07d4271 in mainline for kernel/generic/src/proc/task.c

Timestamp:

2024-01-25T16:22:55Z (12 months ago)

Author:

Jiří Zárevúcky <zarevucky.jiri@…>

Branches:

master

Children:

f8b69a1e

Parents:

1a1e124

git-author:

Jiří Zárevúcky <zarevucky.jiri@…> (2024-01-25 15:56:31)

git-committer:

Jiří Zárevúcky <zarevucky.jiri@…> (2024-01-25 16:22:55)

Message:

Fix some unsound task reference manipulation and locking

In some operations that take task ID as an argument,
there's a possibility of the task being destroyed mid-operation
and a subsequent use-after-free situation.
As a general solution, task_find_by_id() is reimplemented to
check for this situation and always return a valid strong reference.
The callers then only need to handle the reference itself, and
don't need to concern themselves with tasks_lock.

File:

• kernel/generic/src/proc/task.c (modified) (12 diffs)

kernel/generic/src/proc/task.c

@@ -158 +158 @@
                 return rc;
 
-        atomic_store(&task->refcount, 0);
         atomic_store(&task->lifecount, 0);
 
@@ -201 +200 @@
         if (!task)
                 return NULL;
+
+        refcount_init(&task->refcount);
 
         task_create_arch(task);
@@ -268 +269 @@
  *
  */
-void task_destroy(task_t *task)
+static void task_destroy(task_t *task)
 {
         /*
@@ -299 +300 @@
 void task_hold(task_t *task)
 {
-        atomic_inc(&task->refcount);
+        refcount_up(&task->refcount);
 }
 
@@ -311 +312 @@
 void task_release(task_t *task)
 {
-        if ((atomic_predec(&task->refcount)) == 0)
+        if (refcount_down(&task->refcount))
                 task_destroy(task);
 }
@@ -416 +417 @@
 /** Find task structure corresponding to task ID.
  *
- * The tasks_lock must be already held by the caller of this function and
- * interrupts must be disabled.
- *
  * @param id Task ID.
  *
- * @return Task structure address or NULL if there is no such task ID.
+ * @return Task reference or NULL if there is no such task ID.
  *
  */
 task_t *task_find_by_id(task_id_t id)
 {
-        assert(interrupts_disabled());
-        assert(irq_spinlock_locked(&tasks_lock));
+        task_t *task = NULL;
+
+        irq_spinlock_lock(&tasks_lock, true);
 
         odlink_t *odlink = odict_find_eq(&tasks, &id, NULL);
-        if (odlink != NULL)
-                return odict_get_instance(odlink, task_t, ltasks);
-
-        return NULL;
+        if (odlink != NULL) {
+                task = odict_get_instance(odlink, task_t, ltasks);
+
+                /*
+                 * The directory of tasks can't hold a reference, since that would
+                 * prevent task from ever being destroyed. That means we have to
+                 * check for the case where the task is already being destroyed, but
+                 * not yet removed from the directory.
+                 */
+                if (!refcount_try_up(&task->refcount))
+                        task = NULL;
+        }
+
+        irq_spinlock_unlock(&tasks_lock, true);
+
+        return task;
 }
 
@@ -524 +535 @@
 static void task_kill_internal(task_t *task)
 {
-        irq_spinlock_lock(&task->lock, false);
+        irq_spinlock_lock(&task->lock, true);
 
         /*
@@ -534 +545 @@
         }
 
-        irq_spinlock_unlock(&task->lock, false);
+        irq_spinlock_unlock(&task->lock, true);
 }
 
@@ -552 +563 @@
                 return EPERM;
 
-        irq_spinlock_lock(&tasks_lock, true);
-
         task_t *task = task_find_by_id(id);
-        if (!task) {
-                irq_spinlock_unlock(&tasks_lock, true);
+        if (!task)
                 return ENOENT;
-        }
 
         task_kill_internal(task);
-        irq_spinlock_unlock(&tasks_lock, true);
-
+        task_release(task);
         return EOK;
 }
@@ -592 +598 @@
         }
 
-        irq_spinlock_lock(&tasks_lock, true);
         task_kill_internal(TASK);
-        irq_spinlock_unlock(&tasks_lock, true);
-
         thread_exit();
 }
@@ -624 +627 @@
         if (additional)
                 printf("%-8" PRIu64 " %9zu", task->taskid,
-                    atomic_load(&task->refcount));
+                    atomic_load(&task->lifecount));
         else
                 printf("%-8" PRIu64 " %-14s %-5" PRIu32 " %10p %10p"
@@ -636 +639 @@
                 printf("%-8" PRIu64 " %9" PRIu64 "%c %9" PRIu64 "%c "
                     "%9zu\n", task->taskid, ucycles, usuffix, kcycles,
-                    ksuffix, atomic_load(&task->refcount));
+                    ksuffix, atomic_load(&task->lifecount));
         else
                 printf("%-8" PRIu64 " %-14s %-5" PRIu32 " %18p %18p\n",