Skip to content

Development #2

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 8, 2025
Merged

Development #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Kernel/Idt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ struct IdtPtr {


void IdtInstall();

void IdtSetGate(uint8_t num, uint64_t base, uint16_t sel, uint8_t flags);
#endif
19 changes: 17 additions & 2 deletions Kernel/Kernel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "Kernel.h"
#include "Memory.h"
#include "Process.h"
#include "Syscall.h"
#include "Io.h"
int CurrentLine = 0;
int CurrentColumn = 0;
Expand Down Expand Up @@ -120,16 +121,30 @@ void task2(void) {
return;
}
void task3(void) {
// Test syscall
asm volatile(
"mov $2, %%rax\n" // SYS_WRITE
"mov $1, %%rbx\n" // stdout
"mov %0, %%rcx\n" // message
"mov $13, %%r8\n" // length
"int $0x80\n"
:
: "r"("Syscall test!")
: "rax", "rbx", "rcx", "r8"
);
Comment on lines +124 to +134
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix hardcoded syscall number and add error handling.

The syscall test has several issues:

  1. Hardcodes mov $2, %%rax instead of using SYS_WRITE constant
  2. No error handling or result checking
  3. Assumes specific register calling convention that may not match the actual handler

Apply this diff to use the defined constant and add basic error handling:

 // Test syscall
+uint64_t result;
 asm volatile(
-    "mov $2, %%rax\n"          // SYS_WRITE
+    "mov %1, %%rax\n"          // SYS_WRITE
     "mov $1, %%rbx\n"          // stdout
-    "mov %0, %%rcx\n"          // message
+    "mov %2, %%rcx\n"          // message
     "mov $13, %%r8\n"          // length
     "int $0x80\n"
-    :
-    : "r"("Syscall test!")
+    : "=a"(result)
+    : "i"(SYS_WRITE), "r"("Syscall test!")
     : "rax", "rbx", "rcx", "r8"
 );
+if (result != 0) {
+    PrintKernel("Syscall failed!\n");
+}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In Kernel/Kernel.c around lines 124 to 134, replace the hardcoded syscall number
2 with the defined SYS_WRITE constant to improve clarity and maintainability.
Add error handling by checking the return value of the syscall in the
appropriate register and handle errors accordingly. Also, verify and adjust the
register usage to match the actual syscall calling convention on the target
architecture to ensure correctness.


while (1) {
PrintKernelAt("T3 running", 12, 0);
PrintKernelAt("T3 syscall", 12, 0);
for(volatile int i = 0; i < 50000; i++);
}
return;
}
void KernelMain(uint32_t magic, uint32_t info) {
ClearScreen();
PrintKernel("VoidFrame Kernel - Version 0.0.1-alpha\n");
PrintKernel("Initializing IDT...\n");
IdtInstall();
PrintKernel("Initializing System Calls...\n");
SyscallInit();
PrintKernel("Initializing PIC...\n");
PicInstall();
PrintKernel("Initializing Memory...\n");
Expand Down
4 changes: 2 additions & 2 deletions Kernel/Kernel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#ifndef VOIDFRAME_KERNEL_H
#define VOIDFRAME_KERNEL_H
#ifndef KERNEL_H
#define KERNEL_H
extern int CurrentLine;
extern int CurrentColumn;
void ClearScreen();
Expand Down
45 changes: 45 additions & 0 deletions Kernel/Syscall.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#include "Syscall.h"
#include "Kernel.h"
#include "Process.h"
#include "Idt.h"
extern void SyscallEntry(void);
uint64_t SyscallHandler(uint64_t syscall_num, uint64_t arg1, uint64_t arg2, uint64_t arg3) {
switch (syscall_num) {
case SYS_EXIT:
// Terminate current process
GetCurrentProcess()->state = PROC_TERMINATED;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null pointer check for GetCurrentProcess().

The code assumes GetCurrentProcess() always returns a valid pointer. Add a null check to prevent potential kernel panic.

Apply this diff:

-            GetCurrentProcess()->state = PROC_TERMINATED;
+            Process* current = GetCurrentProcess();
+            if (current) {
+                current->state = PROC_TERMINATED;
+            }
🤖 Prompt for AI Agents 
In Kernel/Syscall.c at line 10, add a null pointer check for the result of
GetCurrentProcess() before accessing its state field. If GetCurrentProcess()
returns null, avoid dereferencing it to prevent kernel panic. Implement a
conditional check to ensure the pointer is valid before setting the state to
PROC_TERMINATED.

Schedule(); // Switch to next process
return 0;

case SYS_WRITE:
// arg1 = fd (ignored for now), arg2 = buffer, arg3 = count
if (arg1 == 1) { // stdout
char* buffer = (char*)arg2;
for (uint64_t i = 0; i < arg3; i++) {
if (buffer[i] == '\0') break;
PrintKernelAt(&buffer[i], CurrentLine, CurrentColumn++);
if (CurrentColumn >= 80) {
CurrentLine++;
CurrentColumn = 0;
}
}
return arg3;
Comment on lines +17 to +26
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Critical issues in SYS_WRITE implementation.

Several problems need to be addressed:

  1. Security: The buffer pointer is not validated before dereferencing, which could cause a kernel panic if userspace passes an invalid pointer.
  2. Logic error: PrintKernelAt(&buffer[i], ...) passes the address of a single character, but based on the function signature in the relevant snippets, it expects a null-terminated string.
  3. Missing declarations: CurrentLine and CurrentColumn are used but not declared or imported.
  4. Missing bounds check: No validation that CurrentLine doesn't exceed screen boundaries.

Run the following script to verify the missing declarations:

To fix the immediate issues, consider:

-            if (arg1 == 1) { // stdout
-                char* buffer = (char*)arg2;
-                for (uint64_t i = 0; i < arg3; i++) {
-                    if (buffer[i] == '\0') break;
-                    PrintKernelAt(&buffer[i], CurrentLine, CurrentColumn++);
-                    if (CurrentColumn >= 80) {
-                        CurrentLine++;
-                        CurrentColumn = 0;
-                    }
-                }
-                return arg3;
-            }
+            if (arg1 == 1) { // stdout
+                // TODO: Validate buffer pointer is in valid user memory range
+                char* buffer = (char*)arg2;
+                char single_char[2] = {0, 0};
+                static int CurrentLine = 0;    // TODO: Move to proper location
+                static int CurrentColumn = 0;  // TODO: Move to proper location
+                
+                for (uint64_t i = 0; i < arg3; i++) {
+                    if (buffer[i] == '\0') break;
+                    single_char[0] = buffer[i];
+                    PrintKernelAt(single_char, CurrentLine, CurrentColumn++);
+                    if (CurrentColumn >= 80) {
+                        CurrentLine++;
+                        CurrentColumn = 0;
+                        if (CurrentLine >= 25) { // Assuming 25 lines on screen
+                            CurrentLine = 0; // Wrap around or implement scrolling
+                        }
+                    }
+                }
+                return arg3;
+            }

🏁 Script executed:

#!/bin/bash
# Description: Check for CurrentLine and CurrentColumn declarations

# Search for CurrentLine and CurrentColumn declarations
rg -A 2 "CurrentLine|CurrentColumn" --type c --type h

Length of output: 1410


---

🏁 Script executed:

```shell
#!/bin/bash
# 1. Check if Kernel/Syscall.c includes Kernel/Kernel.h
echo "=== Includes in Syscall.c ==="
grep -E "#include .*Kernel.h" -n Kernel/Syscall.c || echo "No Kernel.h include found."

# 2. Show first 50 lines of Kernel/Syscall.c
echo -e "\n=== Top of Kernel/Syscall.c ==="
sed -n '1,50p' Kernel/Syscall.c

# 3. Find PrintKernelAt declaration/signature
echo -e "\n=== PrintKernelAt in headers ==="
rg -n "PrintKernelAt" --type h

# 4. Show PrintKernelAt definition in C files
echo -e "\n=== PrintKernelAt definition in C files ==="
rg -n "PrintKernelAt" --type c

Length of output: 3511

Address SYS_WRITE buffer validation, single-char printing, and screen bounds

The SYS_WRITE path in Kernel/Syscall.c still needs:

  • User‐pointer validation before dereference
  • Correct use of PrintKernelAt (it expects a null-terminated string)
  • Bounds checking on CurrentLine (wrap or scroll at screen height)

Locations to update:

  • Kernel/Syscall.c, lines ~17–26 (case SYS_WRITE)

Suggested patch:

--- a/Kernel/Syscall.c
+++ b/Kernel/Syscall.c
@@ case SYS_WRITE:
-    if (arg1 == 1) { // stdout
-        char* buffer = (char*)arg2;
-        for (uint64_t i = 0; i < arg3; i++) {
-            if (buffer[i] == '\0') break;
-            PrintKernelAt(&buffer[i], CurrentLine, CurrentColumn++);
-            if (CurrentColumn >= 80) {
-                CurrentLine++;
-                CurrentColumn = 0;
-            }
-        }
-        return arg3;
-    }
+    if (arg1 == 1) { // stdout
+        // 1) Validate user buffer before use
+        if (!IsValidUserPtr(arg2, arg3)) return -1;
+        char* buffer = (char*)arg2;
+        char tmp[2] = {0, 0};
+
+        // 2) Print one char at a time using a null-terminated string
+        for (uint64_t i = 0; i < arg3 && buffer[i] != '\0'; i++) {
+            tmp[0] = buffer[i];
+            PrintKernelAt(tmp, CurrentLine, CurrentColumn++);
+
+            // 3) Handle column wrap
+            if (CurrentColumn >= 80) {
+                CurrentLine++;
+                CurrentColumn = 0;
+                // 4) Clamp or scroll at screen height
+                if (CurrentLine >= SCREEN_HEIGHT) {
+                    CurrentLine = 0; // or invoke ScrollScreen()
+                }
+            }
+        }
+        return arg3;
+    }

You’ll also need to:

  • Define or import IsValidUserPtr, SCREEN_HEIGHT (e.g. 25), or use your existing screen‐size/constants.
  • Optionally document that only fd 1 (stdout) is supported.
📝 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.

Suggested change
char* buffer = (char*)arg2;
for (uint64_t i = 0; i < arg3; i++) {
if (buffer[i] == '\0') break;
PrintKernelAt(&buffer[i], CurrentLine, CurrentColumn++);
if (CurrentColumn >= 80) {
CurrentLine++;
CurrentColumn = 0;
}
}
return arg3;
case SYS_WRITE:
if (arg1 == 1) { // stdout
// 1) Validate user buffer before use
if (!IsValidUserPtr(arg2, arg3)) {
return -1;
}
char* buffer = (char*)arg2;
char tmp[2] = {0, 0};
// 2) Print one char at a time using a null-terminated string
for (uint64_t i = 0; i < arg3 && buffer[i] != '\0'; i++) {
tmp[0] = buffer[i];
PrintKernelAt(tmp, CurrentLine, CurrentColumn++);
// 3) Handle column wrap
if (CurrentColumn >= 80) {
CurrentLine++;
CurrentColumn = 0;
// 4) Clamp or scroll at screen height
if (CurrentLine >= SCREEN_HEIGHT) {
CurrentLine = 0; // or invoke ScrollScreen()
}
}
}
return arg3;
}
break;
🤖 Prompt for AI Agents 
In Kernel/Syscall.c around lines 17 to 26, the SYS_WRITE case lacks validation
of the user pointer before accessing the buffer, incorrectly uses PrintKernelAt
which expects a null-terminated string instead of single characters, and does
not check or handle screen line bounds. Fix this by first validating arg2 as a
user pointer using IsValidUserPtr before dereferencing. Then, modify the
printing logic to print single characters properly rather than passing pointers
to PrintKernelAt. Finally, add bounds checking for CurrentLine to wrap or scroll
when reaching SCREEN_HEIGHT (e.g., 25). Also, ensure to define or include
IsValidUserPtr and SCREEN_HEIGHT constants, and optionally document that only fd
1 (stdout) is supported in this syscall path.

}
return -1;

case SYS_READ:
// Not implemented yet
return 0;

case SYS_GETPID:
return GetCurrentProcess()->pid;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null pointer check for GetCurrentProcess().

Similar to the SYS_EXIT case, validate the process pointer before dereferencing.

Apply this diff:

-            return GetCurrentProcess()->pid;
+            Process* current = GetCurrentProcess();
+            return current ? current->pid : -1;
📝 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.

Suggested change
return GetCurrentProcess()->pid;
Process* current = GetCurrentProcess();
return current ? current->pid : -1;
🤖 Prompt for AI Agents 
In Kernel/Syscall.c at line 35, add a null pointer check for the result of
GetCurrentProcess() before accessing its pid field. This prevents potential null
dereference errors. Implement a conditional check to verify that
GetCurrentProcess() returns a valid pointer, and handle the null case
appropriately, similar to the existing SYS_EXIT case.


default:
return -1;
}
}

void SyscallInit(void) {
// Install syscall interrupt (0x80)
IdtSetGate(0x80, (uint64_t)SyscallEntry, 0x08, 0x8E);
Copy link

@coderabbitai coderabbitai bot Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Define constants for interrupt vector and flags.

Consider defining constants for better maintainability and documentation.

Add these definitions at the top of the file:

#define SYSCALL_INTERRUPT_VECTOR 0x80
#define IDT_INTERRUPT_GATE_KERNEL 0x8E  // Present, DPL=0, 64-bit interrupt gate

Then update the code:

-    IdtSetGate(0x80, (uint64_t)SyscallEntry, 0x08, 0x8E);
+    IdtSetGate(SYSCALL_INTERRUPT_VECTOR, (uint64_t)SyscallEntry, 0x08, IDT_INTERRUPT_GATE_KERNEL);
🤖 Prompt for AI Agents 
In Kernel/Syscall.c at line 44, the interrupt vector 0x80 and flags 0x8E are
used as magic numbers. Define constants SYSCALL_INTERRUPT_VECTOR as 0x80 and
IDT_INTERRUPT_GATE_KERNEL as 0x8E at the top of the file, then replace the
literal values in the IdtSetGate call with these constants for clarity and
maintainability.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i thought the 3rd args of idtsetgate is 0x08 but SYSCALL_INTERRUPT_VECTOR is 0x80?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@assembler-0, you're absolutely correct! I made an error in my previous suggestion. The third argument (0x08) is the selector, not the interrupt vector. The interrupt vector 0x80 is the first argument.

The correct constant definitions should be:

#define SYSCALL_INTERRUPT_VECTOR 0x80
#define KERNEL_CODE_SELECTOR 0x08
#define IDT_INTERRUPT_GATE_KERNEL 0x8E

And the corrected code:

IdtSetGate(SYSCALL_INTERRUPT_VECTOR, (uint64_t)SyscallEntry, KERNEL_CODE_SELECTOR, IDT_INTERRUPT_GATE_KERNEL);

Thank you for catching that mistake!

✏️ Learnings added 
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.290Z
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.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}
16 changes: 16 additions & 0 deletions Kernel/Syscall.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#ifndef SYSCALL_H
#define SYSCALL_H

#include "stdint.h"

// System call numbers
#define SYS_EXIT 1
#define SYS_WRITE 2
#define SYS_READ 3
#define SYS_GETPID 4

// System call handler
void SyscallInit(void);
uint64_t SyscallHandler(uint64_t syscall_num, uint64_t arg1, uint64_t arg2, uint64_t arg3);

#endif
52 changes: 52 additions & 0 deletions Kernel/SyscallEntry.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
bits 64

extern SyscallHandler

global SyscallEntry

SyscallEntry:
; Save all registers
push rax
push rbx
push rcx
push rdx
push rsi
push rdi
push rbp
push r8
push r9
push r10
push r11
push r12
push r13
push r14
push r15
; System call convention: rax=syscall_num, rdi=arg1, rsi=arg2, rdx=arg3
mov rdi, rax ; syscall number
mov rsi, rbx ; arg1
mov rdx, rcx ; arg2
mov rcx, r8 ; arg3
Comment on lines +25 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Clarify syscall convention in comments.

The comment on line 25 is misleading as it describes the target C calling convention, not the incoming syscall convention. Also, the syscall convention used here differs from the standard x86-64 Linux convention.

Update the comments to clarify both conventions:

-    ; System call convention: rax=syscall_num, rdi=arg1, rsi=arg2, rdx=arg3
+    ; Incoming syscall convention: rax=syscall_num, rbx=arg1, rcx=arg2, r8=arg3
+    ; C calling convention target: rdi=syscall_num, rsi=arg1, rdx=arg2, rcx=arg3
     mov rdi, rax    ; syscall number
     mov rsi, rbx    ; arg1 
     mov rdx, rcx    ; arg2
     mov rcx, r8     ; arg3

Note: The standard x86-64 Linux syscall convention uses: rax=syscall_num, rdi=arg1, rsi=arg2, rdx=arg3, r10=arg4, r8=arg5, r9=arg6. Consider aligning with this standard for compatibility.

📝 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.

Suggested change
; System call convention: rax=syscall_num, rdi=arg1, rsi=arg2, rdx=arg3
mov rdi, rax ; syscall number
mov rsi, rbx ; arg1
mov rdx, rcx ; arg2
mov rcx, r8 ; arg3
; Incoming syscall convention: rax=syscall_num, rbx=arg1, rcx=arg2, r8=arg3
; C calling convention target: rdi=syscall_num, rsi=arg1, rdx=arg2, rcx=arg3
mov rdi, rax ; syscall number
mov rsi, rbx ; arg1
mov rdx, rcx ; arg2
mov rcx, r8 ; arg3
🤖 Prompt for AI Agents 
In Kernel/SyscallEntry.asm around lines 25 to 29, the comment describing the
syscall convention is misleading and does not match the actual registers used in
the code or the standard x86-64 Linux syscall convention. Update the comment to
clearly distinguish between the incoming syscall register usage and the target C
calling convention. Additionally, revise the code or comments to align with the
standard Linux syscall convention (rax=syscall_num, rdi=arg1, rsi=arg2,
rdx=arg3, r10=arg4, r8=arg5, r9=arg6) for better compatibility, or explicitly
document the custom convention used if not aligning.

call SyscallHandler
; Return value in rax is already set
; Restore registers
pop r15
pop r14
pop r13
pop r12
pop r11
pop r10
pop r9
pop r8
pop rbp
pop rdi
pop rsi
pop rdx
pop rcx
pop rbx
add rsp, 8 ; Skip original rax (return value)
iretq
4 changes: 3 additions & 1 deletion meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ asm_sources = [
'Kernel/IdtLoad.asm',
'Kernel/Interrupts.asm',
'Kernel/Switch.asm',
'Kernel/SyscallEntry.asm',
]

# C sources
Expand All @@ -46,7 +47,8 @@ c_sources = [
'Kernel/Cpu.c',
'Kernel/MemOps.c',
'Kernel/VMem.c',
'Kernel/Process.c'
'Kernel/Process.c',
'Kernel/Syscall.c'
]

# Compile assembly files
Expand Down