-
Notifications
You must be signed in to change notification settings - Fork 31
Implement kernel stack isolation for U-mode tasks #62
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
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.
No issues found across 5 files
9801050 to
afcfd14
Compare
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 7 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="arch/riscv/boot.c">
<violation number="1" location="arch/riscv/boot.c:333">
P3: Comment claims `ISR_CONTEXT_SIZE` is "used inline" but the macro is not actually used - the value `144` is hardcoded. Consider either using the macro via extended asm operands (as the original code did) or updating the comment to reflect the actual implementation.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
afcfd14 to
fe3574c
Compare
fe3574c to
e31f722
Compare
1930417 to
a903be2
Compare
98ec3ef to
6411a74
Compare
|
This PR now implements per-task kernel stack allocation for U-mode tasks. Previously, all U-mode tasks shared a single global kernel stack ( Regarding validation, as |
6411a74 to
28dcddd
Compare
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.
2 issues found across 11 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="arch/riscv/hal.c">
<violation number="1" location="arch/riscv/hal.c:84">
P3: The ISR frame size comment is now outdated. With `FRAME_SP = 33` added, the frame contains 34 words (indices 0-33), not 33 as documented.</violation>
<violation number="2" location="arch/riscv/hal.c:866">
P2: U-mode mscratch initialization uses global `_stack` instead of `current_kernel_stack_top`. This means the first trap after initial dispatch will use the global stack rather than the task's per-task kernel stack, which is inconsistent with boot.c's ISR exit path design. Should load from `current_kernel_stack_top` with fallback to `_stack` if NULL.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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.
3 issues found across 11 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="Documentation/hal-calling-convention.md">
<violation number="1" location="Documentation/hal-calling-convention.md:234">
P2: Trap entry description incorrectly claims the hardware swaps SP with `mscratch`; the swap is performed manually in `_isr` via `csrrw`, so the documentation should describe it as an ISR software operation, not a hardware feature.</violation>
</file>
<file name="kernel/syscall.c">
<violation number="1" location="kernel/syscall.c:400">
P1: `sys_tputs` can block the scheduler indefinitely because it keeps the timer interrupt disabled while emitting an unbounded user-controlled string, allowing a U-mode caller to freeze all other tasks.</violation>
</file>
<file name="arch/riscv/hal.c">
<violation number="1" location="arch/riscv/hal.c:866">
P1: Kernel stack isolation bypassed on initial dispatch. For U-mode tasks, `mscratch` is set to `_stack` (global stack) instead of `current_kernel_stack_top` (per-task kernel stack). This causes the first trap after initial dispatch to use the wrong kernel stack, inconsistent with boot.c's ISR restore path which correctly loads `current_kernel_stack_top`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
28dcddd to
aeb7cd3
Compare
aeb7cd3 to
185b4a0
Compare
|
512 bytes with 144-byte ISR frame leaves only 368 bytes for trap handler execution. If do_trap() → syscall_handler() → any function with local variables, stack overflow is possible. Consider: |
|
The initial dispatch path restores from frame but doesn't handle frame[33] (FRAME_SP) for the first task. Looking at the code: For U-mode, this sets SP to kernel stack top (wrong). This contradicts _isr's U-mode restore path which correctly loads SP from frame[33]. Impact: First U-mode task starts executing with SP pointing to kernel stack instead of user stack. Security violation and likely crash. |
|
Location: Problem: The blind-swap relies on mscratch == 0 to detect M-mode. At reset, mscratch contains undefined garbage. If non-zero, the first M-mode interrupt misidentifies as U-mode entry, causing:
Fix: Add to _entry before enabling traps: "csrw mideleg, zero\n"
"csrw medeleg, zero\n"
"csrw mscratch, zero\n" /* ADD THIS */ |
|
Location: Problem: Task is added to
Current flow: CRITICAL_LEAVE();
// <-- Window: task visible but kernel_stack not allocated
tcb->kernel_stack = malloc(); // UAF if TCB freedFix: Move list_pushback after all allocations complete: /* Allocate kernel stack BEFORE adding to task list */
if (user_mode) {
tcb->kernel_stack = malloc(KERNEL_STACK_SIZE);
if (!tcb->kernel_stack) {
free(tcb->stack);
free(tcb);
panic(ERR_STACK_ALLOC);
}
tcb->kernel_stack_size = KERNEL_STACK_SIZE;
}
CRITICAL_ENTER();
node = list_pushback(kcb->tasks, tcb); // Now safe
// ...
CRITICAL_LEAVE(); |
jserv
left a comment
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.
Rebase latest main branch.
Fixed. The initial dispatch now restores SP from |
185b4a0 to
0a6cd74
Compare
Fixed. Moved |
Fixed. Added |
Finished. |
0a6cd74 to
32dbe79
Compare
app/umode.c
Outdated
| umode_printf("\n"); | ||
|
|
||
| /* Test 1: sys_tid() - Simplest read-only syscall. */ | ||
| /* Test 1a: Baseline - Syscall with normal SP */ |
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.
Instead of 1a and 1b, use numbers.
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.
Now test 1 is renumbered to test 1-1, 1-2, 1-3.
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.
Now test 1 is renumbered to test 1-1, 1-2, 1-3.
No, simply count Test 1, Test 2, and so on.
32dbe79 to
fac7e93
Compare
User mode tasks require kernel stack isolation to prevent malicious or corrupted user stack pointers from compromising kernel memory during interrupt handling. Without this protection, a user task could set its stack pointer to an invalid or controlled address, causing the ISR to write trap frames to arbitrary memory locations. This commit implements stack isolation using the mscratch register as a discriminator between machine mode and user mode execution contexts. The ISR entry performs a blind swap with mscratch: for machine mode tasks (mscratch=0), the swap is immediately undone to restore the kernel stack pointer. For user mode tasks (mscratch=kernel_stack), the swap provides the kernel stack while preserving the user stack pointer in mscratch. Each user mode task is allocated a dedicated 1024-byte kernel stack to ensure complete isolation between tasks and prevent stack overflow attacks. The task control block is extended to track per-task kernel stack allocations. A global pointer references the current task's kernel stack and is updated during each context switch. The ISR loads this pointer to access the appropriate per-task kernel stack through mscratch, replacing the previous approach of using a single global kernel stack shared by all user mode tasks. The interrupt frame structure is extended to include dedicated storage for the stack pointer. Task initialization zeroes the entire frame and correctly sets the initial stack pointer to support the new restoration path. For user mode tasks, the initial ISR frame is constructed on the kernel stack rather than the user stack, ensuring the frame is protected from user manipulation. Enumeration constants replace magic number usage for improved code clarity and consistency. The ISR implementation now includes separate entry and restoration paths for each privilege mode. The M-mode path maintains mscratch=0 throughout execution. The U-mode path saves the user stack pointer from mscratch immediately after frame allocation and restores mscratch to the current task's kernel stack address before returning to user mode, enabling the next trap to use the correct per-task kernel stack. Task initialization was updated to configure mscratch appropriately during the first dispatch. The dispatcher checks the current privilege level and sets mscratch to zero for machine mode tasks. For user mode tasks, it loads the current task's kernel stack pointer if available, with a fallback to the global kernel stack for initial dispatch before the first task switch. The main scheduler initialization ensures the first task's kernel stack pointer is set before entering the scheduling loop. The user mode output system call was modified to bypass the asynchronous logger queue and implement task-level synchronization. Direct output ensures strict FIFO ordering for test output clarity, while preventing task preemption during character transmission avoids interleaving when multiple user tasks print concurrently. This ensures each string is output atomically with respect to other tasks. A test helper function was added to support stack pointer manipulation during validation. Following the Linux kernel's context switching pattern, this provides precise control over stack operations without compiler interference. The validation harness uses this to verify syscall stability under corrupted stack pointer conditions. Documentation updates include the calling convention guide's stack layout section, which now distinguishes between machine mode and user mode task stack organization with detailed diagrams of the dual-stack design. The context switching guide's task initialization section reflects the updated function signature for building initial interrupt frames with per-task kernel stack parameters. Testing validates that system calls succeed even when invoked with a malicious stack pointer (0xDEADBEEF), confirming the ISR correctly uses the per-task kernel stack from mscratch rather than the user-controlled stack pointer.
fac7e93 to
a41d543
Compare
|
Thank @HeatCrab for contributing! |
User mode tasks require kernel stack isolation to prevent malicious or corrupted user stack pointers from compromising kernel memory during interrupt handling. Without this protection, a user task could set its stack pointer to an invalid or controlled address, causing the ISR to write trap frames to arbitrary memory locations.
Stack isolation is implemented by using the
mscratchregister as a discriminator between machine mode and user mode execution contexts. The ISR entry performs a blind swap withmscratch: for machine mode tasks (mscratch=0), the swap is immediately undone to restore the kernel stack pointer. For user mode tasks, the swap provides the kernel stack while preserving the user stack pointer inmscratch. The interrupt frame structure is extended to 36 words withframe[33]dedicated to stack pointer storage. Task initialization configuresmscratchappropriately during the first dispatch by checking theMPPfield in mstatus.Test code and the outputs validates that system calls succeed even when invoked with a malicious stack pointer (
0xDEADBEEF), confirming the ISR correctly uses the kernel stack frommscratchrather than the user-controlled stack pointer. All existing tests continue to pass, demonstrating that the isolation mechanism does not affect machine mode task execution.Related to #53