Changeset 07d4271 in mainline for kernel/generic/src/ipc/kbox.c

Timestamp:

2024-01-25T16:22:55Z (16 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/ipc/kbox.c (modified) (5 diffs)

kernel/generic/src/ipc/kbox.c

@@ -200 +200 @@
 /** Connect phone to a task kernel-box specified by id.
  *
- * Note that this is not completely atomic. For optimisation reasons, the task
- * might start cleaning up kbox after the phone has been connected and before
- * a kbox thread has been created. This must be taken into account in the
- * cleanup code.
- *
  * @param[out] out_phone  Phone capability handle on success.
  * @return Error code.
@@ -211 +206 @@
 errno_t ipc_connect_kbox(task_id_t taskid, cap_phone_handle_t *out_phone)
 {
-        irq_spinlock_lock(&tasks_lock, true);
-
         task_t *task = task_find_by_id(taskid);
-        if (task == NULL) {
-                irq_spinlock_unlock(&tasks_lock, true);
+        if (!task)
                 return ENOENT;
-        }
-
-        atomic_inc(&task->refcount);
-
-        irq_spinlock_unlock(&tasks_lock, true);
 
         mutex_lock(&task->kb.cleanup_lock);
-
-        if (atomic_predec(&task->refcount) == 0) {
-                mutex_unlock(&task->kb.cleanup_lock);
-                task_destroy(task);
-                return ENOENT;
-        }
 
         if (task->kb.finished) {
                 mutex_unlock(&task->kb.cleanup_lock);
+                task_release(task);
                 return EINVAL;
         }
@@ -243 +225 @@
                 if (!kb_thread) {
                         mutex_unlock(&task->kb.cleanup_lock);
+                        task_release(task);
                         return ENOMEM;
                 }
@@ -255 +238 @@
         if (rc != EOK) {
                 mutex_unlock(&task->kb.cleanup_lock);
+                task_release(task);
                 return rc;
         }
@@ -265 +249 @@
 
         mutex_unlock(&task->kb.cleanup_lock);
+        task_release(task);
         *out_phone = phone_handle;
         return EOK;