--- /dev/null
+# AdrOS Audit Fix Plan
+
+Consolidated from the 2026-05-20 technical audit. Issues ordered by severity
+and exploitability. Each entry references the exact code location and proposed
+fix.
+
+---
+
+## Round 1 — CRITICAL: Kernel Memory Isolation & W^X
+
+### K01: mmap MAP_FIXED end address not validated
+
+**File**: `src/kernel/syscall.c:3040-3043`
+
+**Bug**: Only `addr` is checked against `kernel_base`. The end address
+`addr + aligned_len` can cross into kernel space, allowing a user process to
+overwrite kernel page tables.
+
+**Fix**: After the existing `addr >= kernel_base` check, add:
+```c
+if (addr + aligned_len > hal_mm_kernel_virt_base())
+ return (uintptr_t)-EINVAL;
+```
+Also validate that `addr + aligned_len` does not wrap around (overflow).
+
+---
+
+### K02: mprotect range crosses kernel boundary
+
+**File**: `src/kernel/syscall.c:4176-4222`
+
+**Bug**: The permissive stack check at line 4209 (`addr >= 0x08000000U &&
+addr < kern_base`) allows any range in that region, even if `addr + aligned_len`
+extends past `kern_base` into kernel space. `vmm_protect_range` would then
+modify kernel page flags.
+
+**Fix**:
+1. Add end-address validation: `if (addr + aligned_len > kern_base) return -ENOMEM;`
+2. Remove the permissive fallback or restrict it to only the actual stack
+ region (track stack bottom in `struct process`).
+
+---
+
+### K03: shm_at maps without address validation
+
+**File**: `src/kernel/shm.c:150-162`
+
+**Bug**: User-supplied `shmaddr` is used directly without checking alignment
+or kernel boundary. The auto-assigned address (`0x40000000U + mslot * ...`)
+can also overlap existing mappings or exceed user space.
+
+**Fix**:
+1. If `shmaddr != 0`, validate alignment (`shmaddr & 0xFFF == 0`) and
+ `shmaddr + seg->npages * PAGE_SIZE <= kernel_base`.
+2. For auto-assignment, use `vmm_find_free_area` instead of a fixed formula.
+3. Check that the range doesn't overlap existing mmap entries.
+
+---
+
+### A01: NX flag lost on COW clone and COW fault
+
+**File**: `src/arch/x86/vmm.c:446-459` (clone), `:505,525` (fault handler)
+
+**Bug**: `vmm_as_clone_user_cow` builds `new_pte` with only
+`PRESENT | USER | COW`, discarding the NX bit from the original PTE.
+`vmm_handle_cow_fault` maps the new page with `PRESENT | RW | USER`,
+also losing NX. This breaks W^X: executable+writable pages in the parent
+become writable+executable in the child after COW resolution.
+
+**Fix**:
+
+In `vmm_as_clone_user_cow` (line 446):
+```c
+uint64_t new_pte = (uint64_t)frame_phys | X86_PTE_PRESENT | X86_PTE_USER;
+if (pte & X86_PTE_RW) {
+ new_pte |= X86_PTE_COW;
+ // Preserve NX from original
+ if (pte & X86_PTE_NX) new_pte |= X86_PTE_NX;
+ src_pt[ti] = new_pte;
+ invlpg(va);
+} else {
+ new_pte = pte; // already preserves NX
+}
+```
+
+And in `vmm_as_map_page_nolock`, pass `VMM_FLAG_NX` when `new_pte & X86_PTE_NX`.
+
+In `vmm_handle_cow_fault` (lines 505 and 525):
+```c
+// Preserve NX from original PTE
+uint64_t nx = pte & X86_PTE_NX;
+// rc <= 1 path:
+pt[ti] = (uint64_t)old_frame | X86_PTE_PRESENT | X86_PTE_RW | X86_PTE_USER | nx;
+// copy path:
+pt[ti] = (uint64_t)(uintptr_t)new_frame | X86_PTE_PRESENT | X86_PTE_RW | X86_PTE_USER | nx;
+```
+
+---
+
+## Round 2 — HIGH: Permission & Access Control
+
+### A03: read/write ignore fd open mode
+
+**File**: `src/kernel/syscall.c:2740-2806` (read), `:2810-2861` (write)
+
+**Bug**: `syscall_read_impl` does not check if the fd was opened `O_WRONLY`,
+and `syscall_write_impl` does not check if the fd was opened `O_RDONLY`.
+
+**Fix**:
+
+In `syscall_read_impl`, after `fd_get`:
+```c
+if ((f->flags & 3U) == 1U) return -EBADF; /* O_WRONLY */
+```
+
+In `syscall_write_impl`, after `fd_get`:
+```c
+if ((f->flags & 3U) == 0U) return -EBADF; /* O_RDONLY */
+```
+
+---
+
+### A06: kill syscall lacks permission checks
+
+**File**: `src/kernel/scheduler.c:365-424`
+
+**Bug**: `process_kill` sends signals to any process without checking
+caller credentials. Any user can kill any other user's processes (or root's).
+
+**Fix**: Add permission check in `process_kill` before setting
+`sig_pending_mask`:
+```c
+/* Permission check (POSIX): sender must be root, or same uid, or same euid */
+if (current_process && current_process->euid != 0) {
+ if (current_process->euid != p->uid && current_process->uid != p->uid)
+ return -EPERM;
+}
+/* SIGCONT is special: can always be sent if same session (skip for now) */
+```
+Same check needed in `process_kill_pgrp`.
+
+---
+
+### SYSENTER: user stack pointer not validated
+
+**File**: `src/arch/x86/sysenter.S:84-85`
+
+**Bug**: `mov 4(%ecx), %edx` and `mov 8(%ecx), %ecx` read from the user ESP
+without validating that ECX points to user space. A malicious ECX pointing
+into kernel memory leaks kernel data into syscall arguments.
+
+**Fix**: Add validation in the assembly entry or in `syscall_handler`:
+- In `sysenter_entry`, after pushing the iret frame, check that ECX is
+ below `KERNEL_VIRT_BASE` before dereferencing. If not, set ECX/EDX to 0
+ and set EAX to -EFAULT.
+- Alternatively, validate in C before the `mov 4(%ecx)` / `mov 8(%ecx)`
+ by using `copy_from_user` to read arg2/arg3 from the saved user ESP
+ in the registers struct.
+
+---
+
+### AIO: passes user buffer directly to VFS
+
+**File**: `src/kernel/syscall.c:1656-1666`
+
+**Bug**: `syscall_aio_rw_impl` passes `cb.aio_buf` (a user-space pointer)
+directly to `f->node->f_ops->read/write`. The VFS function then reads/writes
+from/to user memory without `copy_from_user`/`copy_to_user`, bypassing SMAP.
+
+**Fix**: Allocate a kernel bounce buffer, `copy_from_user` into it (for
+writes), call VFS with the kernel buffer, then `copy_to_user` (for reads).
+
+---
+
+### Socket send/recv: passes user buffer directly to lwIP
+
+**File**: `src/kernel/syscall.c:4695,4711` → `src/kernel/socket.c:377-432`
+
+**Bug**: `ksocket_send` passes the user buffer directly to `tcp_write` and
+`memcpy(p->payload, buf, len)`. `ksocket_recv` writes directly to the user
+buffer via `rxbuf_read`. With SMAP enabled, the `user_range_ok` check is
+done but the actual access bypasses `copy_from_user`/`copy_to_user`.
+
+**Fix**:
+- For send: `copy_from_user` into a kernel buffer, then pass to lwIP.
+- For recv: `rxbuf_read` into a kernel buffer, then `copy_to_user`.
+
+---
+
+### vDSO tick_hz mismatch
+
+**File**: `src/kernel/vdso.c:35` vs `include/timer.h:15`
+
+**Bug**: `vdso_kptr->tick_hz = 50` but `TIMER_HZ = 100`. User-space time
+calculations using vDSO will be off by 2x.
+
+**Fix**: Replace hardcoded 50 with `TIMER_HZ`:
+```c
+vdso_kptr->tick_hz = TIMER_HZ;
+```
+Add `#include "timer.h"` to `vdso.c`.
+
+---
+
+## Round 3 — MEDIUM: POSIX Compliance & Robustness
+
+### truncate/ftruncate: no write permission check
+
+**File**: `src/kernel/syscall.c:4299-4323`
+
+**Bug**: Neither syscall checks that the fd was opened for writing, or that
+the caller has write permission on the file.
+
+**Fix**:
+- `ftruncate`: check `(f->flags & 3U) == 0U` (O_RDONLY) → return -EBADF.
+- `truncate`: `vfs_check_permission(node, 2)` (write) → return -EACCES.
+
+---
+
+### O_EXCL not enforced
+
+**File**: `src/kernel/syscall.c:2361-2365`
+
+**Bug**: When `O_CREAT | O_EXCL` is specified and the file already exists,
+the open should fail with `-EEXIST`. Currently it succeeds (falls through
+to the O_TRUNC check or just opens normally).
+
+**Fix**: After `vfs_lookup` finds the node:
+```c
+if (node && (flags & 0x40U) && (flags & 0x80U)) /* O_CREAT | O_EXCL */
+ return -EEXIST;
+```
+
+---
+
+### O_NOFOLLOW not enforced
+
+**File**: `src/kernel/syscall.c:2341-2404`
+
+**Bug**: `O_NOFOLLOW` (0x20000) should cause open to fail with `-ELOOP`
+if the path refers to a symlink. AdrOS doesn't have symlinks yet, so this
+is a no-op for now but the flag should be accepted silently.
+
+**Status**: Deferred (no symlink support yet).
+
+---
+
+### O_DIRECTORY not enforced
+
+**File**: `src/kernel/syscall.c:2341-2404`
+
+**Bug**: `O_DIRECTORY` (0x10000) should fail with `-ENOTDIR` if the path
+is not a directory.
+
+**Fix**: After `vfs_lookup`:
+```c
+if ((flags & 0x10000U) && node && !(node->flags & FS_DIRECTORY))
+ return -ENOTDIR;
+```
+
+---
+
+### posix_spawn wrapper broken
+
+**File**: `user/ulibc/src/spawn.c:22`
+
+**Bug**: Uses `_syscall2(SYS_POSIX_SPAWN, path, argv)` but the kernel
+handler expects 4 arguments (pid_out, path, argv, envp). The missing
+arguments cause the child to exec with garbage envp, and the parent
+doesn't get the child PID.
+
+**Fix**: Change to `_syscall4`:
+```c
+int ret = _syscall4(SYS_POSIX_SPAWN, (int)pid, (int)path, (int)argv, (int)envp);
+```
+
+---
+
+### SYSCALL_MKDIR ignores mode argument
+
+**File**: `src/kernel/syscall.c:3693-3696`
+
+**Bug**: `syscall_mkdir_impl` doesn't receive or pass the mode argument.
+
+**Fix**: Read mode from `sc_arg1(regs)` and pass it through to `vfs_mkdir`.
+Update `vfs_mkdir` signature to accept mode_t.
+
+---
+
+### CLONE_VM address space refcount missing
+
+**File**: `src/kernel/scheduler.c:772-774,338-344`
+
+**Bug**: When `CLONE_VM` is set, the child shares the parent's `addr_space`
+pointer. When any thread exits, `process_reap` checks `PROCESS_FLAG_THREAD`
+and skips `vmm_as_destroy`. But if the thread group leader exits before
+other threads, its `process_reap` destroys the address space while other
+threads still reference it.
+
+**Fix**: Add a `uint32_t as_refcount` to `struct process` (or a separate
+refcount table). Increment on CLONE_VM, decrement on reap. Only destroy
+when refcount reaches 0.
+
+---
+
+## Round 4 — LOW: Hardening & Cleanup
+
+### Saved set-user-ID not implemented
+
+**Bug**: `setuid`/`seteuid` don't maintain the POSIX saved set-user-ID.
+After `seteuid(uid)`, there's no way back without being root.
+
+**Fix**: Add `suid`/`sgid` fields to `struct process`. On `setuid`/`seteuid`,
+save the old euid to suid. Allow `seteuid(suid)` without root.
+
+---
+
+### No kernel reboot() syscall
+
+**Bug**: `init` handles SIGUSR1/SIGUSR2 by killing all processes and
+calling `_exit(0)`, but the kernel never actually reboots or powers off.
+
+**Fix**: Add `SYSCALL_REBOOT` that calls ACPI shutdown or keyboard
+controller reset.
+
+---
+
+### socket_syscall_dispatch misnamed
+
+**File**: `src/kernel/syscall.c`
+
+**Bug**: `socket_syscall_dispatch` handles MQ, SEM, DLOPEN, EPOLL, INOTIFY,
+AIO, MOUNT, PIVOT_ROOT, etc. — not just sockets.
+
+**Fix**: Rename to `extended_syscall_dispatch`.
+
+---
+
+## Test Plan
+
+After each round, run:
+```bash
+make iso && rm -f serial.log && make test # QEMU smoke
+make test-battery # Extended
+make test-host # Host unit tests
+cppcheck --enable=all --suppress=unusedFunction \
+ --suppress=missingIncludeSystem \
+ -I include -I user/ulibc/include src/ user/ # Static analysis
+```
+
+### New regression tests to add:
+
+| Test | Validates |
+|------|-----------|
+| mmap MAP_FIXED crossing kernel base | K01 |
+| mprotect range crossing kernel base | K02 |
+| shmat with kernel-space address | K03 |
+| fork preserves NX on code pages | A01 |
+| read on O_WRONLY fd returns EBADF | A03 |
+| write on O_RDONLY fd returns EBADF | A03 |
+| kill from non-root to root returns EPERM | A06 |
+| O_CREAT+O_EXCL on existing file returns EEXIST | O_EXCL |
+| ftruncate on O_RDONLY fd returns EBADF | truncate |
+| posix_spawn returns child PID and execs | posix_spawn |
projects / AdrOS.git / commitdiff