Drivers, Keyboard, shell removal, added safety (crashing) #13
Conversation
WalkthroughThis update removes the kernel's shell and driver subsystems, eliminating their initialization and related process management. It introduces a security manager process (PID 1) running Changes
Sequence Diagram(s)sequenceDiagram
participant KernelMain
participant SecureKernelIntegritySubsystem
participant ProcessManager
participant Scheduler
KernelMain->>ProcessManager: CreateSecureProcess(SecureKernelIntegritySubsystem, system)
Note right of KernelMain: Panic if creation fails
KernelMain->>Scheduler: Loop: if scheduling needed, call Schedule()
loop
Scheduler->>ProcessManager: Validate process security tokens
alt Invalid token
ProcessManager->>Scheduler: Terminate and remove process
end
Scheduler->>ProcessManager: Switch to next process (privilege-aware)
end
SecureKernelIntegritySubsystem->>ProcessManager: Register as security manager
SecureKernelIntegritySubsystem->>ProcessManager: Create system service process
SecureKernelIntegritySubsystem->>ProcessManager: Continuously validate tokens
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Kernel/Core/Kernel.c(1 hunks)Kernel/Core/Panic.c(1 hunks)Kernel/Core/Shell.c(0 hunks)Kernel/Core/Shell.h(0 hunks)Kernel/Drivers/Driver.c(0 hunks)Kernel/Drivers/Driver.h(0 hunks)Kernel/Drivers/Interrupts.c(0 hunks)Kernel/Drivers/Keyboard.c(0 hunks)Kernel/Process/Process.c(5 hunks)Kernel/Process/Process.h(2 hunks)meson.build(0 hunks)
💤 Files with no reviewable changes (7)
- Kernel/Drivers/Interrupts.c
- Kernel/Core/Shell.h
- meson.build
- Kernel/Core/Shell.c
- Kernel/Drivers/Driver.h
- Kernel/Drivers/Driver.c
- Kernel/Drivers/Keyboard.c
🧰 Additional context used
🧬 Code Graph Analysis (2)
Kernel/Core/Panic.c (3)
Kernel/Core/Panic.h (2)
Panic(7-7)PanicWithCode(8-8)Kernel/Core/Kernel.c (2)
ClearScreen(17-23)PrintKernel(25-46)Kernel/Core/Kernel.h (2)
ClearScreen(5-5)PrintKernel(6-6)
Kernel/Process/Process.c (6)
Kernel/Process/Process.h (9)
ProcessInit(53-53)CreateSecureProcess(55-55)entry_point(54-54)entry_point(55-55)Schedule(58-58)RegisterSecurityManager(63-63)SystemService(65-65)SecureKernelIntegritySubsystem(64-64)GetCurrentProcess(60-60)Kernel/Core/Panic.h (1)
Panic(7-7)Kernel/Core/Kernel.c (2)
PrintKernel(25-46)PrintKernelInt(96-140)Kernel/Core/Kernel.h (2)
PrintKernel(6-6)PrintKernelInt(7-7)Kernel/Memory/Memory.c (1)
AllocPage(29-41)Kernel/Memory/Memory.h (1)
AllocPage(10-10)
🔇 Additional comments (7)
Kernel/Core/Panic.c (1)
14-15: Improved consistency and clarityThe changes enhance code consistency by:
- Using the same delay loop pattern across all panic functions
- Removing the misleading "System halted." message before calling
KernelPanicHandler()Also applies to: 25-26, 34-34
Kernel/Core/Kernel.c (2)
161-179: Security-focused kernel initialization looks goodThe kernel now creates a security manager as the critical first process, which aligns with the new security model. The panic on failure is appropriate for this critical component.
161-198: Incorrect driver-removal concern – no keyboard or shell code presentI searched the entire codebase and found no keyboard driver or shell component (no files or symbols matching “keyboard” or “shell” under Kernel/Drivers or elsewhere). There is no keyboard initialization to remove in Kernel/Core/Kernel.c, so this comment can be disregarded.
Likely an incorrect or invalid review comment.
Kernel/Process/Process.h (2)
13-20: Well-designed security token structureThe
SecurityTokenstruct with magic number validation and checksum provides good integrity checking. The privilege level system and proper structure padding are also well implemented.Also applies to: 38-43
21-26: Confirm explicit initialization of ProcessStateA quick AST search did not reveal any assignments to
.statein process creation paths. SincePROC_TERMINATED = 0, any zero-initialized Process will appear terminated by default. Please verify that every process allocation or constructor explicitly setsprocess.state = PROC_READY;(or an equivalent initialization) before use to avoid accidentally treating uninitialized processes as terminated.
- Check all process creation routines (e.g., in Kernel/Process/…)
- Ensure any static or heap-allocated Process instances override the zero value
Kernel/Process/Process.c (2)
13-40: Robust security token implementationThe token validation with magic number and checksum provides good integrity checking. The checksum calculation correctly excludes the checksum field itself.
91-99: Critical: Array bounds check after array accessThe bounds check at line 91 occurs after
processes[current_process]is already accessed at line 96. This could cause a crash ifcurrent_processis invalid.Move the bounds check before accessing the array:
// Fast security check - only security manager can create system processes if (privilege == PROC_PRIV_SYSTEM) { - if (current_process >= MAX_PROCESSES) { - Panic("CreateSecureProcess: Invalid current process"); - } - - // Only security manager (or initial kernel) can create system processes - if (security_manager_pid != 0 && processes[current_process].pid != security_manager_pid) { + if (current_process >= MAX_PROCESSES) { + Panic("CreateSecureProcess: Invalid current process"); + } + + // Only security manager (or initial kernel) can create system processes + if (security_manager_pid != 0 && processes[current_process].pid != security_manager_pid) {Likely an incorrect or invalid review comment.
| while (1) { | ||
| // Periodic security checks | ||
| for (int i = 0; i < MAX_PROCESSES; i++) { | ||
| if (processes[i].state != PROC_TERMINATED) { | ||
| // Validate process tokens | ||
| if (!ValidateToken(&processes[i].token)) { | ||
| PrintKernel("[SECURITY] Token validation failed for PID: "); | ||
| PrintKernelInt(processes[i].pid); | ||
| PrintKernel("\n"); | ||
| // Terminate compromised process | ||
| processes[i].state = PROC_TERMINATED; | ||
| process_count--; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Small delay to prevent excessive CPU usage | ||
| for (volatile int i = 0; i < 1000; i++); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Aggressive security token validation could cause system instability
The security manager continuously validates all process tokens in a tight loop. If token corruption occurs (e.g., due to memory issues), this could terminate legitimate processes and destabilize the system.
Consider adding:
- Rate limiting for security checks
- Logging before termination for debugging
- Grace period or retry mechanism for token validation failures
while (1) {
// Periodic security checks
+ static int check_counter = 0;
+ if (++check_counter % 1000 != 0) {
+ for (volatile int i = 0; i < 1000; i++);
+ continue;
+ }
+
for (int i = 0; i < MAX_PROCESSES; i++) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while (1) { | |
| // Periodic security checks | |
| for (int i = 0; i < MAX_PROCESSES; i++) { | |
| if (processes[i].state != PROC_TERMINATED) { | |
| // Validate process tokens | |
| if (!ValidateToken(&processes[i].token)) { | |
| PrintKernel("[SECURITY] Token validation failed for PID: "); | |
| PrintKernelInt(processes[i].pid); | |
| PrintKernel("\n"); | |
| // Terminate compromised process | |
| processes[i].state = PROC_TERMINATED; | |
| process_count--; | |
| } | |
| } | |
| } | |
| // Small delay to prevent excessive CPU usage | |
| for (volatile int i = 0; i < 1000; i++); | |
| } | |
| while (1) { | |
| // Periodic security checks | |
| static int check_counter = 0; | |
| if (++check_counter % 1000 != 0) { | |
| for (volatile int i = 0; i < 1000; i++); | |
| continue; | |
| } | |
| for (int i = 0; i < MAX_PROCESSES; i++) { | |
| if (processes[i].state != PROC_TERMINATED) { | |
| // Validate process tokens | |
| if (!ValidateToken(&processes[i].token)) { | |
| PrintKernel("[SECURITY] Token validation failed for PID: "); | |
| PrintKernelInt(processes[i].pid); | |
| PrintKernel("\n"); | |
| // Terminate compromised process | |
| processes[i].state = PROC_TERMINATED; | |
| process_count--; | |
| } | |
| } | |
| } | |
| // Small delay to prevent excessive CPU usage | |
| for (volatile int i = 0; i < 1000; i++); | |
| } |
🤖 Prompt for AI Agents
In Kernel/Process/Process.c around lines 327 to 345, the current infinite loop
aggressively validates all process tokens without delay or retry, risking system
instability by terminating legitimate processes on transient token validation
failures. To fix this, implement rate limiting by adding a longer delay or timer
between validation cycles, add detailed logging before terminating a process to
aid debugging, and introduce a grace period or retry mechanism that allows a
process token to fail validation a few times before termination to reduce false
positives.
| } else { | ||
| // Invalid token - terminate process | ||
| PrintKernel("[SECURITY] Invalid token detected, terminating process\n"); | ||
| processes[next].state = PROC_TERMINATED; | ||
| if (processes[next].stack) { | ||
| // FreePage(processes[next].stack); // Assuming you have this function | ||
| } | ||
| process_count--; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Memory leak when terminating processes
The FreePage call is commented out, causing memory leaks when processes with invalid tokens are terminated. This could exhaust memory over time and cause crashes.
Uncomment the FreePage call or implement proper memory cleanup:
- // FreePage(processes[next].stack); // Assuming you have this function
+ if (processes[next].stack) {
+ FreePage(processes[next].stack);
+ processes[next].stack = 0;
+ }🏁 Script executed:
#!/bin/bash
# Check if FreePage function exists
ast-grep --pattern 'FreePage($_)'Length of output: 35
🏁 Script executed:
#!/bin/bash
# Search for any definition or usage of FreePage or variants in the repository
rg -n -A2 "FreePage"
rg -n -A2 "free_page"
rg -n -A2 "AllocPage"
rg -n -A2 "alloc_page"
rg -n -A2 "FreeMemory"
rg -n -A2 "ReleasePage"Length of output: 2652
🏁 Script executed:
#!/bin/bash
# Display context around the termination block in Process.c
sed -n '150,200p' Kernel/Process/Process.cLength of output: 2043
Free process stack on termination to prevent memory leaks
Terminating a process for an invalid token currently leaves its stack page allocated, which will eventually exhaust memory. Uncomment and invoke FreePage —and clear the pointer—inside the invalid-token block.
• File: Kernel/Process/Process.c
Location: inside Schedule() invalid-token branch (around line 175)
Suggested diff:
// Invalid token - terminate process
PrintKernel("[SECURITY] Invalid token detected, terminating process\n");
processes[next].state = PROC_TERMINATED;
- if (processes[next].stack) {
- // FreePage(processes[next].stack); // Assuming you have this function
- }
+ if (processes[next].stack) {
+ FreePage(processes[next].stack);
+ processes[next].stack = NULL;
+ }
process_count--;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| // Invalid token - terminate process | |
| PrintKernel("[SECURITY] Invalid token detected, terminating process\n"); | |
| processes[next].state = PROC_TERMINATED; | |
| if (processes[next].stack) { | |
| // FreePage(processes[next].stack); // Assuming you have this function | |
| } | |
| process_count--; | |
| } | |
| } else { | |
| // Invalid token - terminate process | |
| PrintKernel("[SECURITY] Invalid token detected, terminating process\n"); | |
| processes[next].state = PROC_TERMINATED; | |
| if (processes[next].stack) { | |
| FreePage(processes[next].stack); | |
| processes[next].stack = NULL; | |
| } | |
| process_count--; | |
| } |
🤖 Prompt for AI Agents
In Kernel/Process/Process.c around lines 170 to 178 inside the Schedule()
function's invalid-token branch, the process stack is not freed when terminating
a process, causing a memory leak. Uncomment the call to
FreePage(processes[next].stack) to free the allocated stack page, then set
processes[next].stack to NULL to clear the pointer and prevent dangling
references.
1 participant
pls help, crashing
Summary by CodeRabbit
New Features
Removals
Improvements
Bug Fixes