Development #72
Development #72
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds higher-half kernel entry and boot flow, introduces an ELF loader and shell elfload command, extends syscalls (file ops, input, SET_FREQ) with SMAP-aware entry, replaces SystemService/SystemIntegrityVerificationAgent with DynamoX/Astra (fixed-point frequency controller) and adjusts process termination and related macros; several formatting and build updates. Changes
Sequence Diagram(s)sequenceDiagram
participant DynamoX
participant PIT as PIT/Timer
participant Proc as ProcessTable
DynamoX->>PIT: read ticks / ctx-switch rate (SAMPLING_INTERVAL)
DynamoX->>Proc: inspect queues, context-switch history
DynamoX->>DynamoX: compute target freq (fixed-point)
DynamoX->>PIT: PitSetFrequency(target)
DynamoX->>Proc: cleanup terminated processes
sequenceDiagram
participant User
participant CPU
participant SysEntry as SyscallEntry
participant Kernel as SyscallHandler
participant VFS
User->>CPU: int 0x80 (syscall)
CPU->>SysEntry: entry (stac -> prepare args rdi/rsi/rdx)
SysEntry->>Kernel: call Syscall (SMAP disabled)
Kernel->>VFS: handle file ops / Pit for SET_FREQ / PS2 input
Kernel-->>SysEntry: return value (rax)
SysEntry->>CPU: restore SMAP (clac) and return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (2)
kernel/process/Process.h (1)
89-105: Fixed-point implementation looks good, but improve the comment.The fixed-point arithmetic implementation correctly addresses the kernel space floating-point concerns from previous reviews. However, the comment on line 92 is unprofessional.
// ============================================================================= -// SystemService (aka dynamic frequency controller) modify as needed +// Dynamic Frequency Controller (DynamoX) - Fixed-Point Configuration // ============================================================================= -// Fixed points, if you found it annoying (ofc it is what are you asking), remove and pray GCC how to handle floats. +// Using fixed-point arithmetic to avoid floating-point operations in kernel space #define FXP_SHIFT 10 // Use 10 bits for the fractional partkernel/process/Process.c (1)
1137-1262: Fixed-point implementation correctly avoids floating-point operations.The conversion from floating-point to fixed-point arithmetic properly addresses the kernel space constraints. The calculations are mathematically sound with proper scaling and shifting.
However, defining the PITController struct inside the function (lines 1139-1150) is unusual. Consider moving it to the header file or at least to file scope for better code organization.
Consider moving the PITController struct definition to Process.h or at least to file scope rather than defining it inside the function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
drivers/Pic.c(1 hunks)drivers/ethernet/RTL8139.c(1 hunks)kernel/core/Kernel.c(1 hunks)kernel/etc/Editor.h(1 hunks)kernel/process/Process.c(12 hunks)kernel/process/Process.h(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-10T02:19:46.917Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.917Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:32.692Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:32.692Z
Learning: In the VoidFrame kernel (kernel/process/Process.c), avoid using floating-point arithmetic in kernel space. Use fixed-point arithmetic instead with integer scaling (e.g., multiplying by 1000 for 3 decimal places) to avoid FPU state management overhead and potential exceptions.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:51.116Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:51.116Z
Learning: In VoidFrame kernel, avoid using floating-point arithmetic in kernel space functions like SystemService. Use fixed-point arithmetic instead (e.g., 16.16 format with integer operations) to prevent FPU state corruption, improve performance, and ensure predictable behavior. This is especially important for system-level processes running with PROC_PRIV_SYSTEM privileges.
Applied to files:
kernel/core/Kernel.ckernel/process/Process.h
📚 Learning: 2025-07-16T01:04:23.790Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#47
File: kernel/core/Kernel.c:434-434
Timestamp: 2025-07-16T01:04:23.790Z
Learning: VMemAlloc() in VoidFrame kernel fails with error code -2 (VMEM_ERROR_INVALID_ADDR) due to virtual address space layout conflict where the heap start address (_bss_end + KERNEL_VIRTUAL_OFFSET + 1MB margin) exceeds VIRT_ADDR_SPACE_END (0xFFFFFFFF80000000ULL), causing VMemMap validation to fail.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T08:52:13.291Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T08:52:13.291Z
Learning: VoidFrame's TerminateProcess function in Process.c only checks if the caller has PROC_PRIV_SYSTEM privilege but doesn't compare caller vs target privilege levels, allowing supervisor processes like the shell to incorrectly kill system-level processes.
Applied to files:
kernel/process/Process.c
📚 Learning: 2025-08-10T08:52:13.291Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T08:52:13.291Z
Learning: The VoidFrame kernel's KillProcess function lacks proper privilege enforcement - supervisor-level processes can incorrectly kill system-level processes and vice versa, indicating missing privilege validation checks.
Applied to files:
kernel/process/Process.c
🧬 Code Graph Analysis (2)
kernel/etc/Editor.h (1)
kernel/etc/Editor.c (1)
EditorOpen(229-286)
kernel/process/Process.c (2)
kernel/etc/Console.c (5)
PrintKernelError(205-209)PrintKernelInt(243-267)PrintKernelSuccess(199-203)PrintKernelWarning(211-215)PrintKernel(173-189)include/stdlib.h (1)
ABSi(4-9)
🔇 Additional comments (13)
drivers/Pic.c (1)
42-42: LGTM!Adding a blank line between functions improves code readability.
kernel/core/Kernel.c (2)
1-1: Good documentation addition.Adding a file-level comment improves code documentation.
500-500: Minor formatting adjustment.kernel/etc/Editor.h (1)
4-4: Style adjustment for pointer declaration.Formatting change aligns with pointer declaration style.
drivers/ethernet/RTL8139.c (1)
119-120: Good practice: Adding trailing newline.Files should end with a newline character for POSIX compliance.
kernel/process/Process.c (8)
275-314: Renaming from SIVA to AS (Astra) is consistent.The function correctly maintains its role as a security bypass termination function.
316-330: Security violation handler updated consistently.Proper integration with the renamed AS system.
1284-1439: Astra security monitor properly refactored.The security monitoring functionality is preserved with consistent renaming from SIVA to AS. The integration with DynamoX as the frequency controller is appropriate.
1468-1476: Process initialization correctly creates Astra.The security monitor initialization is properly updated with consistent naming.
1226-1235: Check for potential integer overflow in momentum calculation.The momentum calculation on line 1229 uses 64-bit intermediate to prevent overflow, which is good. However, ensure that the adjustment values don't grow unbounded over time.
The fixed-point arithmetic looks correct, but consider adding bounds checking for the adjustment value to prevent potential accumulation of errors over long runtime.
1170-1170: Good addition of cleanup in the control loop.Having the frequency controller also handle terminated process cleanup ensures regular maintenance.
189-227: Privilege validation properly implemented.The termination privilege checks now correctly enforce that only system-level processes can terminate other system-level processes, addressing the previous security concerns.
168-175: Incorrect summary: self-termination is allowed
TheTerminateProcessimplementation does not include an early‐return forcaller->pid == proc->pid; it only skips the cross‐process security checks in that case. In other words, a process can terminate itself—there is no special “do nothing” path for self‐termination. Please update the summary to reflect that self‐termination remains supported.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
arch/x86_64/syscall/Syscall.c (1)
32-49: Add bounds checking for user-provided buffer addresses.The syscall copies data directly from user-provided addresses without validating that these addresses are within the user's address space. This could allow malicious processes to read kernel memory.
Consider adding user address validation before copying:
case SYS_WRITE: // arg1 = fd (ignored for now), arg2 = buffer, arg3 = count if (likely(arg1 == 1)) { // stdout if (unlikely(!arg2)) { return -1; // NULL buffer } + // Validate that the buffer is in user space + if (!IsUserAddress((void*)arg2, arg3)) { + return -1; // Invalid user buffer + } // Limit the size to prevent buffer overflows in kernel space if (unlikely(arg3 > MAX_SYSCALL_BUFFER_SIZE)) { return -1; // Buffer too large }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
arch/x86_64/syscall/Syscall.c(4 hunks)arch/x86_64/syscall/Syscall.h(1 hunks)arch/x86_64/syscall/SyscallEntry.asm(1 hunks)kernel/core/Kernel.c(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:32.692Z
Learning: In the VoidFrame kernel (kernel/process/Process.c), avoid using floating-point arithmetic in kernel space. Use fixed-point arithmetic instead with integer scaling (e.g., multiplying by 1000 for 3 decimal places) to avoid FPU state management overhead and potential exceptions.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:51.116Z
Learning: In VoidFrame kernel, avoid using floating-point arithmetic in kernel space functions like SystemService. Use fixed-point arithmetic instead (e.g., 16.16 format with integer operations) to prevent FPU state corruption, improve performance, and ensure predictable behavior. This is especially important for system-level processes running with PROC_PRIV_SYSTEM privileges.
📚 Learning: 2025-08-10T06:22:28.788Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.788Z
Learning: In VoidFrame kernel, panic functions must call "asm volatile("cli")" as the very first instruction to disable interrupts immediately. Without this, if interrupts are enabled (after "sti"), an interrupt can fire during panic handling causing the panic to fail, hang, or behave unpredictably. This must be done in ALL panic entry points: Panic(), PanicWithCode(), PanicWithContext(), PanicFromInterrupt(), and at the start of KernelPanicHandler().
Applied to files:
arch/x86_64/syscall/Syscall.c
📚 Learning: 2025-07-08T00:36:50.297Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.297Z
Learning: In IdtSetGate function calls, the parameters are: (interrupt_vector, handler_address, selector, flags). The interrupt vector (like 0x80) is the first argument, and the selector (like 0x08 for kernel code segment) is the third argument.
Applied to files:
arch/x86_64/syscall/Syscall.h
📚 Learning: 2025-08-10T02:19:46.917Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.917Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:32.692Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:32.692Z
Learning: In the VoidFrame kernel (kernel/process/Process.c), avoid using floating-point arithmetic in kernel space. Use fixed-point arithmetic instead with integer scaling (e.g., multiplying by 1000 for 3 decimal places) to avoid FPU state management overhead and potential exceptions.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:51.116Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:51.116Z
Learning: In VoidFrame kernel, avoid using floating-point arithmetic in kernel space functions like SystemService. Use fixed-point arithmetic instead (e.g., 16.16 format with integer operations) to prevent FPU state corruption, improve performance, and ensure predictable behavior. This is especially important for system-level processes running with PROC_PRIV_SYSTEM privileges.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-07-16T01:04:23.790Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#47
File: kernel/core/Kernel.c:434-434
Timestamp: 2025-07-16T01:04:23.790Z
Learning: VMemAlloc() in VoidFrame kernel fails with error code -2 (VMEM_ERROR_INVALID_ADDR) due to virtual address space layout conflict where the heap start address (_bss_end + KERNEL_VIRTUAL_OFFSET + 1MB margin) exceeds VIRT_ADDR_SPACE_END (0xFFFFFFFF80000000ULL), causing VMemMap validation to fail.
Applied to files:
kernel/core/Kernel.c
🧬 Code Graph Analysis (2)
arch/x86_64/syscall/Syscall.c (5)
drivers/PS2.c (1)
GetChar(101-108)kernel/memory/MemOps.c (1)
FastMemcpy(78-101)kernel/memory/KernelHeap.c (1)
KernelMemoryAlloc(185-227)fs/VFS.c (2)
VfsReadFile(126-165)VfsCreateDir(227-245)drivers/Pic.c (1)
PitSetFrequency(25-41)
kernel/core/Kernel.c (1)
kernel/process/Process.c (1)
CreateProcess(1058-1060)
🔇 Additional comments (5)
arch/x86_64/syscall/Syscall.h (1)
7-21: LGTM! Well-organized syscall constants.The new syscall definitions follow a consistent naming convention and are logically grouped. The numbering sequence is maintained without gaps.
kernel/core/Kernel.c (3)
1-1: LGTM! Helpful file documentation.The header comment clearly identifies the purpose of this file.
559-559: Verify process creation in user mode.The
CreateProcessfunction creates processes withPROC_PRIV_USERprivileges by default (as seen in the relevant code snippets). Ensure that the UserTestProcess will execute in user mode with appropriate privilege restrictions.
533-544: Ignore incorrect syscall argument concernThe inline assembly in UserTestProcess already matches the kernel’s int 0x80 convention (rax=syscall number, rdi=arg1, rsi=arg2, rdx=arg3), as defined in arch/x86_64/syscall/SyscallEntry.asm. No changes are required here.
Likely an incorrect or invalid review comment.
arch/x86_64/syscall/Syscall.c (1)
108-113: LGTM! Well-implemented frequency control syscall.The implementation correctly validates the frequency range and uses the appropriate fixed-point arithmetic through
PitSetFrequency, avoiding floating-point operations in kernel space as per the retrieved learnings.
| case SYS_STAT: | ||
| (void)arg3; | ||
| char ls_path[MAX_SYSCALL_BUFFER_SIZE + 1]; // +1 for null terminator | ||
| FastMemcpy(ls_path, (const void*)arg1, arg2); | ||
| ls_path[arg2] = '\0'; // Ensure null termination | ||
| VfsCreateDir(ls_path); | ||
| return 0; |
There was a problem hiding this comment.
Critical: Wrong VFS function called in SYS_STAT and SYS_RM syscalls.
Both SYS_STAT and SYS_RM syscalls incorrectly call VfsCreateDir instead of their intended functions. This is a severe functional bug that will cause stat operations to create directories and remove operations to also create directories instead of removing files.
Apply these fixes:
For SYS_STAT:
case SYS_STAT:
(void)arg3;
char ls_path[MAX_SYSCALL_BUFFER_SIZE + 1]; // +1 for null terminator
FastMemcpy(ls_path, (const void*)arg1, arg2);
ls_path[arg2] = '\0'; // Ensure null termination
- VfsCreateDir(ls_path);
+ // TODO: Implement VfsStat or similar function
+ // VfsStat(ls_path, stat_buffer);
return 0;For SYS_RM:
case SYS_RM:
(void)arg3;
char rm_path[MAX_SYSCALL_BUFFER_SIZE + 1]; // +1 for null terminator
FastMemcpy(rm_path, (const void*)arg1, arg2);
rm_path[arg2] = '\0'; // Ensure null termination
- VfsCreateDir(rm_path);
+ // TODO: Implement VfsRemove or similar function
+ // return VfsRemove(rm_path);
return 0;Also applies to: 100-106
🤖 Prompt for AI Agents
In arch/x86_64/syscall/Syscall.c around lines 92-98 (and similarly 100-106), the
SYS_STAT and SYS_RM cases call VfsCreateDir incorrectly; replace those calls
with the proper VFS operations: for SYS_STAT after null-terminating the copied
path call the VFS stat function (e.g., VfsStat or equivalent) passing the path
and the user-provided stat/result buffer (cast arg3 appropriately) and return
its result; for SYS_RM call the VFS remove/unlink function (e.g., VfsRemove or
VfsUnlink) with the null-terminated path and return that result. Ensure you
preserve the existing path copy and null-termination, cast arg3 to the correct
pointer type for the stat call, and return the VFS call return values instead of
0.
| ; Stop SMAP temporarily | ||
| stac |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify SMAP instructions are conditionally executed.
The stac and clac instructions will cause an invalid opcode exception if SMAP is not supported by the CPU. The kernel should check for SMAP support before executing these instructions.
Also applies to: 33-34
🏁 Script executed:
#!/bin/bash
# Check if SMAP support is verified before using stac/clac instructions
rg -A 10 "SMAP|stac|clac" --type c --type asm | head -50
# Check if there's conditional execution of SMAP instructions
ast-grep --pattern 'if ($_ & $_) {
$$$
stac
$$$
}'Length of output: 3294
To verify whether SMAP support is properly guarded around the stac/clac in the syscall entry, let’s locate and inspect the assembly snippet directly:
#!/bin/bash
# 1. Find the SyscallEntry.asm path
entry_file=$(fd SyscallEntry.asm)
echo "Located syscall entry at: $entry_file"
echo
# 2. Show context around stac/clac
grep -n -C4 "stac" "$entry_file" || echo "No stac found"
echo
grep -n -C4 "clac" "$entry_file" || echo "No clac found"
echo
# 3. Search for any CPUID-based SMAP checks in the same directory
grep -R -n "cpuid" "$(dirname "$entry_file")" || echo "No CPUID checks found"#!/bin/bash
set -e
# 1. Locate the SyscallEntry.asm file
entry_file=$(fd SyscallEntry.asm)
echo "Located SyscallEntry.asm at: $entry_file"
echo
# 2. Show context around stac
echo "---- stac context ----"
grep -n -C3 "stac" "$entry_file" || echo "No 'stac' found"
# 3. Show context around clac
echo
echo "---- clac context ----"
grep -n -C3 "clac" "$entry_file" || echo "No 'clac' found"
# 4. Check for any SMAP identifiers in the same directory
echo
echo "---- SMAP references ----"
grep -R -n "SMAP" "$(dirname "$entry_file")" || echo "No SMAP references found"
# 5. Look for CPUID-based feature checks nearby
echo
echo "---- CPUID or feature-check code ----"
grep -R -n "cpuid" "$(dirname "$entry_file")" || echo "No CPUID checks found"🏁 Script executed:
#!/bin/bash
set -e
# Locate the SyscallEntry.asm file
entry_file=$(find . -type f -name SyscallEntry.asm | head -n1)
if [[ -z "$entry_file" ]]; then
echo "SyscallEntry.asm not found"
exit 1
fi
echo "Found SyscallEntry.asm at: $entry_file"
echo
# Show context around 'stac'
echo "---- stac context ----"
grep -n -C5 "stac" "$entry_file" || echo "'stac' not found"
# Show context around 'clac'
echo
echo "---- clac context ----"
grep -n -C5 "clac" "$entry_file" || echo "'clac' not found"
# Search for CPUID or SMAP feature checks in the same directory
echo
echo "---- SMAP references in directory ----"
grep -R -n "SMAP" "$(dirname "$entry_file")" || echo "No SMAP references found"
echo
echo "---- CPUID or feature-check code ----"
grep -R -n "cpuid" "$(dirname "$entry_file")" || echo "No CPUID checks found"Length of output: 1642
Guard SMAP instructions with a CPU‐feature check
We currently execute stac (line 25) and clac (line 34) unconditionally in
arch/x86_64/syscall/SyscallEntry.asm. On CPUs without SMAP, these will #UD.
Please add a CPUID-leaf-7 EBX[21] test (or use the kernel’s global SMAP flag) before each SMAP instruction:
• Wrap lines 24–25 (stac) and 33–34 (clac) in a runtime feature check
• Cache the result at boot (e.g. in a per-CPU or global variable) to avoid repeated CPUID calls
For example:
--- a/arch/x86_64/syscall/SyscallEntry.asm
+++ b/arch/x86_64/syscall/SyscallEntry.asm
@@ -23,6 +23,13 @@ SyscallEntry:
push r15
; Stop SMAP temporarily
+
+ ;— check SMAP support via CPUID leaf 7, EBX[21] —#
+ xor ecx, ecx
+ cpuid ; eax=7, ecx=0 → EBX[21]=SMAP
+ test ebx, 1<<21
+ jz .Lskip_stac
stac
+.Lskip_stac:
; System call convention: rax=syscall_num…
@@ -30,6 +37,13 @@ SyscallEntry:
call Syscall
; Reset SMAP
+
+ ;— re-enable SMAP if supported —#
+ xor ecx, ecx
+ cpuid
+ test ebx, 1<<21
+ jz .Lskip_clac
clac
+.Lskip_clac:
; Return value in rax is already set
pop r15This ensures stac/clac only execute on CPUs that actually support SMAP.
📝 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.
| ; Stop SMAP temporarily | |
| stac | |
| push r15 | |
| ; Stop SMAP temporarily | |
| ;— check SMAP support via CPUID leaf 7, EBX[21] —# | |
| xor ecx, ecx | |
| cpuid ; eax=7, ecx=0 → EBX[21]=SMAP | |
| test ebx, 1<<21 | |
| jz .Lskip_stac | |
| stac | |
| .Lskip_stac: | |
| ; System call convention: rax=syscall_num… | |
| call Syscall | |
| ; Reset SMAP | |
| ;— re-enable SMAP if supported —# | |
| xor ecx, ecx | |
| cpuid | |
| test ebx, 1<<21 | |
| jz .Lskip_clac | |
| clac | |
| .Lskip_clac: | |
| ; Return value in rax is already set | |
| pop r15 |
🤖 Prompt for AI Agents
In arch/x86_64/syscall/SyscallEntry.asm around lines 24–25 and 33–34, the
STAC/CLAC instructions are executed unconditionally which will #UD on CPUs
without SMAP; add a runtime guard so those instructions only execute when SMAP
is supported: introduce and use a cached boot-time feature flag (per-CPU or
global) set once at boot by testing CPUID leaf 7 EBX[21] (or reuse the kernel’s
existing SMAP global flag), then wrap the STAC at lines 24–25 and the CLAC at
lines 33–34 with conditional checks that jump over the instruction when the
cached flag is clear to avoid repeated CPUID calls.
There was a problem hiding this comment.
what, check static void SetupMemoryProtection(void) ?
There was a problem hiding this comment.
🧩 Analysis chain
@assembler-0, let me check for that function to see if there's already existing SMAP detection logic.
🏁 Script executed:
#!/bin/bash
# Search for SetupMemoryProtection function
echo "=== Searching for SetupMemoryProtection function ==="
ast-grep --pattern 'void SetupMemoryProtection($_) {
$$$
}'
echo -e "\n=== Alternative search with rg ==="
rg -A 20 "SetupMemoryProtection" --type c --type asm
echo -e "\n=== Search for SMAP-related code ==="
rg -B 5 -A 10 "SMAP|smap" --type c --type asm
echo -e "\n=== Search for existing feature flags ==="
rg -B 3 -A 3 "feature.*flag|FLAG.*SMAP|SMAP.*flag" --type c --type asm
Length of output: 6200
@assembler-0, you're absolutely right! I found the SetupMemoryProtection() function in kernel/core/Kernel.c. It already performs the CPUID leaf 7 check for SMAP support (bit 20 in EBX) and enables CR4.SMAP if supported.
However, the issue remains that while SMAP is being detected and conditionally enabled at boot time, the syscall entry assembly still executes stac and clac unconditionally. Even though SetupMemoryProtection() won't enable SMAP on unsupported CPUs, the assembly instructions will still cause #UD.
The cleanest solution would be to:
- Export a global SMAP flag from the existing
SetupMemoryProtection()function - Check that flag in the assembly code instead of doing redundant CPUID calls
For example, in kernel/core/Kernel.c:
bool smap_supported = false; // Global flag
static void SetupMemoryProtection(void) {
// ... existing CPUID code ...
// Enable SMAP if supported (bit 20 in EBX from CPUID leaf 7)
if (ebx & (1 << 20)) {
cr4 |= (1ULL << 21); // CR4.SMAP
PrintKernel("[SYSTEM] SMAP enabled\n");
smap_supported = true; // Set the global flag
protection_enabled = true;
}
// ...
}Then in SyscallEntry.asm, check this flag instead of doing CPUID each time. This leverages your existing detection logic without duplication.
🧠 Learnings used
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.788Z
Learning: In VoidFrame kernel, panic functions must call "asm volatile("cli")" as the very first instruction to disable interrupts immediately. Without this, if interrupts are enabled (after "sti"), an interrupt can fire during panic handling causing the panic to fail, hang, or behave unpredictably. This must be done in ALL panic entry points: Panic(), PanicWithCode(), PanicWithContext(), PanicFromInterrupt(), and at the start of KernelPanicHandler().
1 participant
Summary by CodeRabbit
New Features
Refactor
Style