From e403aceb12622a9c97b99d7ccadaf3c5f0b9953e 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 | 270 ++++++++++++++---- 1 file changed, 215 insertions(+), 55 deletions(-) diff --git a/src/schedule/zephyr_dp_schedule_application.c b/src/schedule/zephyr_dp_schedule_application.c index 1ce105392c2f..1355bf44fe41 100644 --- a/src/schedule/zephyr_dp_schedule_application.c +++ b/src/schedule/zephyr_dp_schedule_application.c @@ -147,6 +147,96 @@ 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; + uint32_t gen; /* unique per record, see dp_task_kobj_take() */ +}; + +static struct list_item dp_task_kobj_list = LIST_INIT(dp_task_kobj_list); +static struct k_spinlock dp_task_kobj_lock; +/* generation of the last added record, under dp_task_kobj_lock */ +static uint32_t dp_task_kobj_gen; + +static void dp_task_kobj_add(struct scheduler_dp_task_kobj *kobj) +{ + k_spinlock_key_t key = k_spin_lock(&dp_task_kobj_lock); + + kobj->gen = ++dp_task_kobj_gen; + list_item_append(&kobj->list, &dp_task_kobj_list); + k_spin_unlock(&dp_task_kobj_lock, key); +} + +/* Find the record for @task, caller holds dp_task_kobj_lock */ +static struct scheduler_dp_task_kobj *dp_task_kobj_find(const struct task *task) +{ + struct scheduler_dp_task_kobj *kobj; + 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) + return kobj; + } + + return NULL; +} + +/* Copy the record for @task to @copy without unlinking it */ +static bool dp_task_kobj_peek(const struct task *task, struct scheduler_dp_task_kobj *copy) +{ + k_spinlock_key_t key = k_spin_lock(&dp_task_kobj_lock); + struct scheduler_dp_task_kobj *kobj = dp_task_kobj_find(task); + + if (kobj) + *copy = *kobj; + + k_spin_unlock(&dp_task_kobj_lock, key); + + return kobj != NULL; +} + +/* + * Find the record for @task and unlink it, so it can only be freed once. + * If @expected is given, only take the record it was copied from, so a + * record replaced since @expected was peeked is left alone. + */ +static struct scheduler_dp_task_kobj * +dp_task_kobj_take(const struct task *task, const struct scheduler_dp_task_kobj *expected) +{ + k_spinlock_key_t key = k_spin_lock(&dp_task_kobj_lock); + struct scheduler_dp_task_kobj *kobj = dp_task_kobj_find(task); + + /* + * Compare the generation, not the object addresses: the task could + * have been freed and recreated at the same addresses meanwhile. + */ + if (kobj && expected && kobj->gen != expected->gen) + kobj = NULL; + + if (kobj) + list_item_del(&kobj->list); + + k_spin_unlock(&dp_task_kobj_lock, key); + + return kobj; +} + /* Unpack IPC data and execute a callback */ static void ipc_thread_unflatten_run(struct processing_module *pmod, struct ipc4_flat *flat) { @@ -441,17 +531,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 +563,35 @@ 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, + const struct scheduler_dp_task_kobj *expected) { - struct task_dp_pdata *pdata = task->priv_data; + struct scheduler_dp_task_kobj *kobj = dp_task_kobj_take(task, expected); + + if (!kobj) + return false; + + /* 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); - k_object_free(pdata->event); - k_object_free(pdata->thread); - scheduler_dp_domain_free(pdata); + mod_free(kobj->mod, container_of(task, struct scheduler_dp_task_memory, task)); + rfree(kobj); - mod_free(pdata->mod, container_of(task, struct scheduler_dp_task_memory, task)); + return true; +} + +void z_impl_scheduler_dp_internal_free(struct task *task) +{ + if (!scheduler_dp_internal_free_kobj(task, NULL)) + tr_err(&dp_tr, "unknown DP task %p", task); } #ifdef CONFIG_THREAD_NAME @@ -507,6 +615,9 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, { k_thread_stack_t *p_stack; struct scheduler_dp_task_memory *task_memory; + uint8_t *ipc_config_data = NULL; + k_tid_t thread_id; + uint32_t comp_id; int ret; @@ -552,36 +663,56 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, goto e_stack; } + /* + * task_memory, including pdata, is in the module heap, which user + * threads can write while this function runs. Keep everything the + * kernel uses here or at teardown in kobj and locals, and only mirror + * it into pdata for the DP thread. + */ 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; + } + + kobj->task = ptask; + kobj->mod = mod; + comp_id = mod->dev->ipc_config.id; + kobj->comp_id = comp_id; 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, - 0); - if (!pdata->ipc_config_data) { + ipc_config_data = mod_alloc_ext(mod, SOF_MEM_FLAG_USER, SOF_IPC_MSG_MAX_SIZE, 0); + if (!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; } + pdata->ipc_config_data = ipc_config_data; pdata->flat = &task_memory->flat; - pdata->event = k_object_alloc(K_OBJ_EVENT); - if (!pdata->event) { + kobj->event = k_object_alloc(K_OBJ_EVENT); + if (!kobj->event) { tr_err(&dp_tr, "Event object allocation failed"); ret = -ENOMEM; goto e_config; } - pdata->thread = k_object_alloc(K_OBJ_THREAD); - if (!pdata->thread) { + pdata->event = kobj->event; + + kobj->thread = k_object_alloc(K_OBJ_THREAD); + if (!kobj->thread) { tr_err(&dp_tr, "Thread object allocation failed"); ret = -ENOMEM; goto e_event; } - memset(&pdata->thread->arch, 0, sizeof(pdata->thread->arch)); + memset(&kobj->thread->arch, 0, sizeof(kobj->thread->arch)); + pdata->thread = kobj->thread; /* success, fill the structures */ pdata->p_stack = p_stack; @@ -594,10 +725,11 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, list_init(&ptask->list); /* create a zephyr thread for the task */ - pdata->thread_id = k_thread_create(pdata->thread, p_stack, - stack_size, dp_thread_fn, ptask, NULL, NULL, - CONFIG_DP_THREAD_PRIORITY, ptask->flags, K_FOREVER); - scheduler_dp_thread_name_set(pdata->thread_id, mod); + thread_id = k_thread_create(kobj->thread, p_stack, + stack_size, dp_thread_fn, ptask, NULL, NULL, + CONFIG_DP_THREAD_PRIORITY, ptask->flags, K_FOREVER); + pdata->thread_id = thread_id; + scheduler_dp_thread_name_set(thread_id, mod); unsigned int pidx; size_t size; @@ -606,19 +738,19 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, #ifdef CONFIG_SCHED_CPU_MASK /* pin the thread to specific core */ - ret = k_thread_cpu_pin(pdata->thread_id, core); + ret = k_thread_cpu_pin(thread_id, core); if (ret < 0) { tr_err(&dp_tr, "zephyr task pin to core failed"); goto e_thread; } #endif - k_thread_access_grant(pdata->thread_id, pdata->event, &dp_sync[core]); - scheduler_dp_grant(pdata->thread_id, core); + k_thread_access_grant(thread_id, kobj->event, &dp_sync[core]); + scheduler_dp_grant(thread_id, core); #if CONFIG_SOF_USERSPACE_LL struct k_thread *thread_ipc = ipc_thread_user(core); - k_thread_access_grant(thread_ipc, pdata->event, pdata->thread_id, p_stack, &dp_sync[core]); + k_thread_access_grant(thread_ipc, kobj->event, thread_id, p_stack, &dp_sync[core]); scheduler_dp_grant(thread_ipc, core); scheduler_dp_grant(zephyr_ll_domain_thread(), core); #endif @@ -635,6 +767,7 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, on_pool = true; mod->mdom = mdom; + kobj->mdom = mdom; if (!mdom->arch.ptables) { ret = k_mem_domain_init(mdom, 0, NULL); @@ -644,35 +777,37 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, /* Module heap partition */ mod_heap_info(mod, &size, &start); - pdata->mpart[SOF_DP_PART_HEAP] = (struct k_mem_partition){ + kobj->mpart[SOF_DP_PART_HEAP] = (struct k_mem_partition){ .start = (uintptr_t)sys_cache_uncached_ptr_get((void *)start), .size = size, .attr = K_MEM_PARTITION_P_RW_U_RW, }; - pdata->mpart[SOF_DP_PART_HEAP_CACHE] = (struct k_mem_partition){ + kobj->mpart[SOF_DP_PART_HEAP_CACHE] = (struct k_mem_partition){ .start = start, .size = size, .attr = K_MEM_PARTITION_P_RW_U_RW | XTENSA_MMU_CACHED_WB, }; /* Host mailbox partition for additional IPC parameters: read-only */ - pdata->mpart[SOF_DP_PART_CFG] = (struct k_mem_partition){ + kobj->mpart[SOF_DP_PART_CFG] = (struct k_mem_partition){ .start = (uintptr_t)sys_cache_uncached_ptr_get((void *)MAILBOX_HOSTBOX_BASE), .size = MAILBOX_HOSTBOX_SIZE, .attr = K_MEM_PARTITION_P_RO_U_RO, }; - pdata->mpart[SOF_DP_PART_CFG_CACHE] = (struct k_mem_partition){ + kobj->mpart[SOF_DP_PART_CFG_CACHE] = (struct k_mem_partition){ .start = (uintptr_t)MAILBOX_HOSTBOX_BASE, .size = MAILBOX_HOSTBOX_SIZE, .attr = K_MEM_PARTITION_P_RO_U_RO | XTENSA_MMU_CACHED_WB, }; for (pidx = 0; pidx < SOF_DP_PART_TYPE_COUNT; pidx++) { - ret = k_mem_domain_add_partition(mdom, pdata->mpart + pidx); + ret = k_mem_domain_add_partition(mdom, kobj->mpart + pidx); if (ret < 0) goto e_dom; } - ret = llext_manager_add_domain(mod->dev->ipc_config.id, mdom); + memcpy(pdata->mpart, kobj->mpart, sizeof(pdata->mpart)); + + ret = llext_manager_add_domain(comp_id, mdom); if (ret < 0) { tr_err(&dp_tr, "failed to add LLEXT to domain %d", ret); goto e_dom; @@ -682,15 +817,18 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, * Keep this call last, able to fail, otherwise domain will be removed * before its thread */ - ret = k_mem_domain_add_thread(mdom, pdata->thread_id); + ret = k_mem_domain_add_thread(mdom, thread_id); if (ret < 0) { tr_err(&dp_tr, "failed to add thread to domain %d", ret); goto e_dom; } + /* publish the record before the thread can run */ + 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); + k_event_init(kobj->event); + k_thread_start(thread_id); /* ptask points into task_memory, so only publish it once it cannot be freed */ *task = ptask; @@ -698,17 +836,19 @@ 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, kobj->mpart, comp_id); on_pool = false; e_thread: if (on_pool) objpool_free(&dp_mdom_head, mdom); - k_thread_abort(pdata->thread_id); - k_object_free(pdata->thread); + k_thread_abort(thread_id); + k_object_free(kobj->thread); e_event: - k_object_free(pdata->event); + k_object_free(kobj->event); e_config: - mod_free(mod, pdata->ipc_config_data); + mod_free(mod, ipc_config_data); +e_kobj: + rfree(kobj); e_stack: user_stack_free(p_stack); e_tmem: @@ -721,6 +861,11 @@ int scheduler_dp_task_init(struct task **task, const struct sof_uuid_entry *uid, #ifdef CONFIG_USERSPACE #include +/* + * mod_free() follows the module's resource and allocator pointers in + * supervisor mode. They are user-writable, so at least check that they + * point to memory the caller can access. + */ static void scheduler_dp_mod_vrfy(struct processing_module *mod) { K_OOPS(K_SYSCALL_MEMORY_WRITE(mod, sizeof(*mod))); @@ -749,15 +894,30 @@ static void scheduler_dp_mod_vrfy(struct processing_module *mod) void z_vrfy_scheduler_dp_internal_free(struct task *task) { - K_OOPS(K_SYSCALL_MEMORY_WRITE(task, sizeof(*task))); + struct scheduler_dp_task_kobj kobj; - struct task_dp_pdata *pdata = task->priv_data; + /* + * Do not read task->priv_data: the kernel objects come from the + * kernel-only record, and an unknown task pointer is rejected. + */ + K_OOPS(!dp_task_kobj_peek(task, &kobj)); + + /* + * A DP thread holds its own thread and event objects, so it would pass + * the checks below for its own task. Freeing would then abort the + * caller after the record is unlinked, leaking everything it holds. + */ + K_OOPS(kobj.thread == k_current_get()); + + /* + * The caller must have been granted the task's objects. Check before + * unlinking the record, so an unauthorized call leaves it intact. + */ + K_OOPS(K_SYSCALL_OBJ(kobj.event, K_OBJ_EVENT)); + K_OOPS(K_SYSCALL_OBJ_INIT(kobj.thread, K_OBJ_THREAD)); + scheduler_dp_mod_vrfy(kobj.mod); - 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); + K_OOPS(!scheduler_dp_internal_free_kobj(task, &kobj)); } #include #endif /* CONFIG_USERSPACE */