Changeset 07d4271 in mainline for kernel/generic/src/sysinfo/stats.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/sysinfo/stats.c (modified) (4 diffs)

kernel/generic/src/sysinfo/stats.c

@@ -221 +221 @@
         stats_task->virtmem = get_task_virtmem(task->as);
         stats_task->resmem = get_task_resmem(task->as);
-        stats_task->threads = atomic_load(&task->refcount);
+        stats_task->threads = atomic_load(&task->lifecount);
         task_get_accounting(task, &(stats_task->ucycles),
             &(stats_task->kcycles));
@@ -511 +511 @@
 {
         /* Initially no return value */
-        sysinfo_return_t ret;
-        ret.tag = SYSINFO_VAL_UNDEFINED;
+        sysinfo_return_t ret = {
+                .tag = SYSINFO_VAL_UNDEFINED,
+        };
 
         /* Parse the task ID */
@@ -519 +520 @@
                 return ret;
 
-        /* Messing with task structures, avoid deadlock */
-        irq_spinlock_lock(&tasks_lock, true);
-
         task_t *task = task_find_by_id(task_id);
-        if (task == NULL) {
-                /* No task with this ID */
-                irq_spinlock_unlock(&tasks_lock, true);
+        if (!task)
                 return ret;
-        }
 
         if (dry_run) {
@@ -533 +528 @@
                 ret.data.data = NULL;
                 ret.data.size = sizeof(stats_task_t);
-
-                irq_spinlock_unlock(&tasks_lock, true);
         } else {
                 /* Allocate stats_task_t structure */
-                stats_task_t *stats_task =
-                    (stats_task_t *) malloc(sizeof(stats_task_t));
-                if (stats_task == NULL) {
-                        irq_spinlock_unlock(&tasks_lock, true);
-                        return ret;
+                stats_task_t *stats_task = malloc(sizeof(stats_task_t));
+
+                if (stats_task != NULL) {
+                        /* Correct return value */
+                        ret.tag = SYSINFO_VAL_FUNCTION_DATA;
+                        ret.data.data = stats_task;
+                        ret.data.size = sizeof(stats_task_t);
+
+                        irq_spinlock_lock(&task->lock, true);
+                        produce_stats_task(task, stats_task);
+                        irq_spinlock_unlock(&task->lock, true);
                 }
-
-                /* Correct return value */
-                ret.tag = SYSINFO_VAL_FUNCTION_DATA;
-                ret.data.data = (void *) stats_task;
-                ret.data.size = sizeof(stats_task_t);
-
-                /* Hand-over-hand locking */
-                irq_spinlock_exchange(&tasks_lock, &task->lock);
-
-                produce_stats_task(task, stats_task);
-
-                irq_spinlock_unlock(&task->lock, true);
-        }
-
+        }
+
+        task_release(task);
         return ret;
 }