Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Teardown removes caller authorization checks and still follows user-writable allocator metadata in privileged context.
2 open issues
What changed in this PR
This PR hardens DP scheduler teardown by recording kernel-object ownership outside user-writable task memory.
Changes:
- Adds a kernel-only task registry protected by a spinlock.
- Uses recorded objects, domain partitions, and component IDs during cleanup.
- Rejects unknown or already-freed task pointers and adds allocation-failure cleanup for records.
| File | Description |
|---|---|
src/schedule/zephyr_dp_schedule_application.c |
Adds trusted task records and routes teardown through registry lookup. |
Review effort: Balanced
| 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)); |
There was a problem hiding this comment.
Sorry, that was not correct, this was not fixed in V2 or subsequent PR updates. Agreed this is a real issue, but it is not introduced by this PR: the base code runs the same mod_free() with the same checks. The root cause is that module resource bookkeeping lives in user-writable memory, and it affects all mod_* syscalls and other kernel-side sof_ctx_free() users, not only DP teardown. A partial fix here would leave a stale entry in the module's resource list. I propose to handle this in a follow-up that moves the module allocation context and resource bookkeeping to kernel-owned storage.
There was a problem hiding this comment.
What's your plan for splitting this further? I'm plan moving the fields intended exclusively for kernel-side usage out of struct processing_module into a separate pipeline_module structure. The pipeline would then maintain a list of pipeline_module instances, while individual modules would use a minimal processing_module containing module-specific data.
PR 11360: test resultsRun date: 2026-10-28 15:10 UTC Tested commit: e403aceb12622a9c97b99d7ccadaf3c5f0b9953e
|
2d0e980 to
8168e00
Compare
|
V2 pushed:
|
There was a problem hiding this comment.
🟡 Changes recommended
Active issues allow a userspace race to substitute a thread pointer later used by supervisor teardown, while self-teardown unlinks the record before aborting the caller and leaks resources.
3 open issues
1 resolved since last review
Review effort: Balanced
8168e00 to
5d63e01
Compare
|
V3 pushed:
|
There was a problem hiding this comment.
🟡 Changes recommended
The expected-record check can accept a replacement task when allocation addresses are reused, allowing a paused free syscall to tear down objects it never validated.
2 open issues
2 resolved since last review
Review effort: Balanced
| if (kobj && expected && | ||
| (kobj->event != expected->event || kobj->thread != expected->thread || | ||
| kobj->mod != expected->mod)) | ||
| kobj = NULL; |
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 <kai.vehmanen@linux.intel.com>
5d63e01 to
e403ace
Compare
|
V4 pushed:
|
@kv2019i can you check CI thanks ! |
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.