-
Notifications
You must be signed in to change notification settings - Fork 32
Add U-mode task isolation and fix self-termination #76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="kernel/task.c">
<violation number="1" location="kernel/task.c:1003">
P1: Marking the current task as TASK_ZOMBIE before _yield() lets dispatch() free the running task in task_cleanup_zombies(), then dereference kcb->task_current->data during context save/scheduling. This introduces a use-after-free on self-termination. Defer freeing the current task until after the context switch or skip cleanup of the current task.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
8f6f132 to
ad15807
Compare
kernel/syscall.c
Outdated
| { | ||
| if (unlikely(id <= 0)) | ||
| return -EINVAL; | ||
| if (id <= 0 || (uint16_t) id != mo_task_id()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the syscall interface takes int id (32-bit), but mo_task_id() returns uint16_t, there is a potential aliasing issue here due to the cast.
If a user accidentally passes a large ID like 0x10001 (65537), the check (uint16_t)0x10001 != 1 becomes 1 != 1 (False), allowing the call to proceed. Then, mo_task_cancel(uint16_t id) will receive the truncated value 1 and terminate the current task unexpectedly.
I think it would be safer to enforce an upper bound check or perform the comparison without the cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Adopted the upper bound check approach and preserved Linmo's original error code semantics.
ad15807 to
b9d44fd
Compare
kernel/syscall.c
Outdated
| static int _tsuspend(int id) | ||
| { | ||
| if (unlikely(id <= 0)) | ||
| if (unlikely(id <= 0 || id > (int) UINT16_MAX)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I don't think we need the (int) cast here due to integer promotion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To satisfy -Wsign-compare without the (int) cast, adopted the single-check idiom.
b9d44fd to
f76afa7
Compare
U-mode tasks could previously control other tasks and had no way to properly terminate themselves. This adds permission checks to restrict task control syscalls to self-only operations and enables safe self-termination through the existing zombie task mechanism.
f76afa7 to
d34b392
Compare
U-mode tasks could previously control other tasks and had no way to properly terminate themselves. This adds permission checks to restrict task control syscalls to self-only operations and enables safe self-termination through the existing zombie task mechanism.
Summary by cubic
Locks down U‑mode task control to self-only and adds safe self-termination using the zombie task path. This prevents user tasks from affecting others and ensures proper cleanup.
Written for commit d34b392. Summary will update on new commits.