From 2d0e980434088b49ec5be58ec9dbba9807d4f929 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Wed, 21 Oct 2026 16:17:08 +0300 Subject: [PATCH] schedule: dp: keep DP task kernel objects in a kernel-only record scheduler_dp_internal_free() freed the event and thread objects, removed memory domain partitions and deinitialized the memory domain using pointers read from task_memory. That memory is in the module heap, which user threads can write, and the validator did not check mod->mdom at all. A caller could make the kernel free arbitrary kernel objects or operate on a forged memory domain. Record the event, thread, memory domain, partitions and component id in kernel memory when the DP task is created. The free path, including the syscall, now only uses the task pointer as a lookup key, and rejects unknown or already freed tasks. Signed-off-by: Kai Vehmanen --- src/schedule/zephyr_dp_schedule_application.c | 163 ++++++++++++------ 1 file changed, 111 insertions(+), 52 deletions(-) diff --git a/src/schedule/zephyr_dp_schedule_application.c b/src/schedule/zephyr_dp_schedule_application.c index 1ce105392c2f..6c48aa6b5159 100644 --- a/src/schedule/zephyr_dp_schedule_application.c +++ b/src/schedule/zephyr_dp_schedule_application.c @@ -147,6 +147,58 @@ struct scheduler_dp_task_memory { struct ipc4_flat flat; }; +/* + * Kernel-only record of the kernel objects owned by a DP task. + * + * struct scheduler_dp_task_memory lives in the module's heap, which is mapped + * into the DP thread's memory domain, and is also writable by the LL and IPC + * user threads. Freeing must therefore never use the pointers stored there: + * scheduler_dp_internal_free() takes them from this record instead. The task + * pointer is only used as a lookup key. + */ +struct scheduler_dp_task_kobj { + struct list_item list; + struct task *task; + struct processing_module *mod; + struct k_event *event; + struct k_thread *thread; + struct k_mem_domain *mdom; + struct k_mem_partition mpart[SOF_DP_PART_TYPE_COUNT]; + uint32_t comp_id; +}; + +static struct list_item dp_task_kobj_list = LIST_INIT(dp_task_kobj_list); +static struct k_spinlock dp_task_kobj_lock; + +static void dp_task_kobj_add(struct scheduler_dp_task_kobj *kobj) +{ + k_spinlock_key_t key = k_spin_lock(&dp_task_kobj_lock); + + list_item_append(&kobj->list, &dp_task_kobj_list); + k_spin_unlock(&dp_task_kobj_lock, key); +} + +/* Find the record for @task and unlink it, so it can only be freed once */ +static struct scheduler_dp_task_kobj *dp_task_kobj_take(const struct task *task) +{ + struct scheduler_dp_task_kobj *kobj, *found = NULL; + k_spinlock_key_t key = k_spin_lock(&dp_task_kobj_lock); + struct list_item *item; + + list_for_item(item, &dp_task_kobj_list) { + kobj = container_of(item, struct scheduler_dp_task_kobj, list); + if (kobj->task == task) { + list_item_del(&kobj->list); + found = kobj; + break; + } + } + + k_spin_unlock(&dp_task_kobj_lock, key); + + return found; +} + /* Unpack IPC data and execute a callback */ static void ipc_thread_unflatten_run(struct processing_module *pmod, struct ipc4_flat *flat) { @@ -441,17 +493,15 @@ void dp_thread_fn(void *p1, void *p2, void *p3) * Safe to call with partial successful initialisation, * k_mem_domain_remove_partition() then just returns -ENOENT */ -static void scheduler_dp_domain_free(struct task_dp_pdata *pdata) +static void scheduler_dp_domain_free(struct processing_module *pmod, struct k_mem_domain *mdom, + struct k_mem_partition *mpart, uint32_t comp_id) { - struct processing_module *pmod = pdata->mod; - struct k_mem_domain *mdom = pmod->mdom; - - llext_manager_rm_domain(pmod->dev->ipc_config.id, mdom); + llext_manager_rm_domain(comp_id, mdom); - k_mem_domain_remove_partition(mdom, pdata->mpart + SOF_DP_PART_HEAP); - k_mem_domain_remove_partition(mdom, pdata->mpart + SOF_DP_PART_HEAP_CACHE); - k_mem_domain_remove_partition(mdom, pdata->mpart + SOF_DP_PART_CFG); - k_mem_domain_remove_partition(mdom, pdata->mpart + SOF_DP_PART_CFG_CACHE); + k_mem_domain_remove_partition(mdom, mpart + SOF_DP_PART_HEAP); + k_mem_domain_remove_partition(mdom, mpart + SOF_DP_PART_HEAP_CACHE); + k_mem_domain_remove_partition(mdom, mpart + SOF_DP_PART_CFG); + k_mem_domain_remove_partition(mdom, mpart + SOF_DP_PART_CFG_CACHE); if (mdom->num_partitions) { /* Violation: all partitions should be freed by now */ @@ -475,15 +525,34 @@ static void scheduler_dp_domain_free(struct task_dp_pdata *pdata) objpool_free(&dp_mdom_head, mdom); } -void z_impl_scheduler_dp_internal_free(struct task *task) +/* + * Free the kernel objects of @task, using only the kernel-only record. + * + * @return false if @task has no record (unknown task or already freed) + */ +static bool scheduler_dp_internal_free_kobj(struct task *task) { - struct task_dp_pdata *pdata = task->priv_data; + struct scheduler_dp_task_kobj *kobj = dp_task_kobj_take(task); - k_object_free(pdata->event); - k_object_free(pdata->thread); - scheduler_dp_domain_free(pdata); + if (!kobj) + return false; - mod_free(pdata->mod, container_of(task, struct scheduler_dp_task_memory, task)); + /* normally already terminated by scheduler_dp_task_free(), no-op then */ + k_thread_abort(kobj->thread); + k_object_free(kobj->event); + k_object_free(kobj->thread); + scheduler_dp_domain_free(kobj->mod, kobj->mdom, kobj->mpart, kobj->comp_id); + + mod_free(kobj->mod, container_of(task, struct scheduler_dp_task_memory, task)); + rfree(kobj); + + return true; +} + +void z_impl_scheduler_dp_internal_free(struct task *task) +{ + if (!scheduler_dp_internal_free_kobj(task)) + tr_err(&dp_tr, "unknown DP task %p", task); } #ifdef CONFIG_THREAD_NAME @@ -553,6 +622,14 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, } struct task_dp_pdata *pdata = &task_memory->pdata; + struct scheduler_dp_task_kobj *kobj = rmalloc(SOF_MEM_FLAG_KERNEL | SOF_MEM_FLAG_COHERENT, + sizeof(*kobj)); + + if (!kobj) { + tr_err(&dp_tr, "task record alloc failed"); + ret = -ENOMEM; + goto e_stack; + } if (mod->dev->drv->adapter_ops->get_configuration) { pdata->ipc_config_data = mod_alloc_ext(mod, SOF_MEM_FLAG_USER, SOF_IPC_MSG_MAX_SIZE, @@ -560,7 +637,7 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, if (!pdata->ipc_config_data) { tr_err(&dp_tr, "Failed to allocate memory for get-config"); ret = -ENOMEM; - goto e_stack; + goto e_kobj; } } else { pdata->ipc_config_data = NULL; @@ -688,6 +765,16 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, goto e_dom; } + /* record the objects to free, before the thread can touch task_memory */ + kobj->task = ptask; + kobj->mod = mod; + kobj->event = pdata->event; + kobj->thread = pdata->thread; + kobj->mdom = mdom; + memcpy(kobj->mpart, pdata->mpart, sizeof(kobj->mpart)); + kobj->comp_id = mod->dev->ipc_config.id; + dp_task_kobj_add(kobj); + /* start the thread, it should immediately stop at the semaphore */ k_event_init(pdata->event); k_thread_start(pdata->thread_id); @@ -698,7 +785,7 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, return 0; e_dom: - scheduler_dp_domain_free(pdata); + scheduler_dp_domain_free(mod, mdom, pdata->mpart, mod->dev->ipc_config.id); on_pool = false; e_thread: if (on_pool) @@ -709,6 +796,8 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, k_object_free(pdata->event); e_config: mod_free(mod, pdata->ipc_config_data); +e_kobj: + rfree(kobj); e_stack: user_stack_free(p_stack); e_tmem: @@ -721,43 +810,13 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, #ifdef CONFIG_USERSPACE #include -static void scheduler_dp_mod_vrfy(struct processing_module *mod) -{ - K_OOPS(K_SYSCALL_MEMORY_WRITE(mod, sizeof(*mod))); - K_OOPS(K_SYSCALL_MEMORY_WRITE(mod->dev, sizeof(*mod->dev))); - K_OOPS(K_SYSCALL_MEMORY_READ(mod->dev->drv, sizeof(*mod->dev->drv))); - - struct mod_alloc_ctx *alloc = mod->priv.resources.alloc; - - K_OOPS(K_SYSCALL_MEMORY_WRITE(alloc, sizeof(*alloc))); - if (alloc->heap) { - size_t h_size = 0; - uintptr_t h_start; - - mod_heap_info(mod, &h_size, &h_start); - if (h_size) - K_OOPS(K_SYSCALL_MEMORY_WRITE(h_start, h_size)); - } - if (alloc->vreg) { - size_t vr_size = 0; - uintptr_t vr_start; - - vregion_mem_info(alloc->vreg, &vr_size, &vr_start); - K_OOPS(K_SYSCALL_MEMORY_WRITE(vr_start, vr_size)); - } -} - void z_vrfy_scheduler_dp_internal_free(struct task *task) { - K_OOPS(K_SYSCALL_MEMORY_WRITE(task, sizeof(*task))); - - struct task_dp_pdata *pdata = task->priv_data; - - K_OOPS(K_SYSCALL_MEMORY_WRITE(pdata, sizeof(*pdata))); - K_OOPS(K_SYSCALL_OBJ(pdata->event, K_OBJ_EVENT)); - K_OOPS(K_SYSCALL_OBJ_INIT(pdata->thread, K_OBJ_THREAD)); - scheduler_dp_mod_vrfy(pdata->mod); - return z_impl_scheduler_dp_internal_free(task); + /* + * Do not read task->priv_data: everything needed comes from the + * kernel-only record, and an unknown task pointer is rejected. + */ + K_OOPS(!scheduler_dp_internal_free_kobj(task)); } #include #endif /* CONFIG_USERSPACE */