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

Timestamp:

2024-01-25T16:22:55Z (17 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/ddi/ddi.c (modified) (2 diffs)

kernel/generic/src/ddi/ddi.c

@@ -336 +336 @@
                 return EPERM;
 
-        irq_spinlock_lock(&tasks_lock, true);
-
         task_t *task = task_find_by_id(id);
 
-        if ((!task) || (!container_check(CONTAINER, task->container))) {
-                /*
-                 * There is no task with the specified ID
-                 * or the task belongs to a different security
-                 * context.
-                 */
-                irq_spinlock_unlock(&tasks_lock, true);
+        if (!task)
                 return ENOENT;
-        }
-
-        /* Lock the task and release the lock protecting tasks dictionary. */
-        irq_spinlock_exchange(&tasks_lock, &task->lock);
-        errno_t rc = ddi_iospace_enable_arch(task, ioaddr, size);
+
+        errno_t rc = ENOENT;
+
+        irq_spinlock_lock(&task->lock, true);
+
+        /* Check that the task belongs to the correct security context. */
+        if (container_check(CONTAINER, task->container))
+                rc = ddi_iospace_enable_arch(task, ioaddr, size);
+
         irq_spinlock_unlock(&task->lock, true);
-
+        task_release(task);
         return rc;
 }
@@ -377 +373 @@
                 return EPERM;
 
-        irq_spinlock_lock(&tasks_lock, true);
-
         task_t *task = task_find_by_id(id);
 
-        if ((!task) || (!container_check(CONTAINER, task->container))) {
-                /*
-                 * There is no task with the specified ID
-                 * or the task belongs to a different security
-                 * context.
-                 */
-                irq_spinlock_unlock(&tasks_lock, true);
+        if (!task)
                 return ENOENT;
-        }
-
-        /* Lock the task and release the lock protecting tasks dictionary. */
-        irq_spinlock_exchange(&tasks_lock, &task->lock);
-        errno_t rc = ddi_iospace_disable_arch(task, ioaddr, size);
+
+        errno_t rc = ENOENT;
+
+        irq_spinlock_lock(&task->lock, true);
+
+        /* Check that the task belongs to the correct security context. */
+        if (container_check(CONTAINER, task->container))
+                rc = ddi_iospace_disable_arch(task, ioaddr, size);
+
         irq_spinlock_unlock(&task->lock, true);
-
+        task_release(task);
         return rc;
 }