fix: correct 5 bugs in shared memory IPC (Fase 5 audit)

Bugs found and fixed during deep audit of the Fase 5 commit
(implemented during WSL2/GCC instability):

BUG 1 (CRITICAL): vmm_map_page args were inverted in shm_at().
  Signature is vmm_map_page(phys, virt, flags) but code passed
  (virt, phys, flags). Would map physical pages at wrong addresses
  causing memory corruption. Fixed both code paths.

BUG 2 (CRITICAL): shm_dt() used broken heuristic to find segment.
  Matched by npages count — if two segments had same page count,
  wrong one got decremented. Added shmid field to mmap entry struct
  for direct O(1) lookup. Removed dead code loop that computed
  expected_va and discarded it.

BUG 3: shm_at() with shmaddr!=0 didn't register in mmaps[].
  shm_dt() would never find the slot, returning -EINVAL.
  Now always registers in mmap table regardless of shmaddr.

BUG 4: shm_destroy() only cleared 'used' flag, leaving stale
  key/size/npages/nattch. Now memset()s entire struct to zero.

BUG 5: shm_ctl(IPC_STAT) wrote directly to userspace pointer
  while holding spinlock. Page fault under spinlock = deadlock.
  Now copies to local struct, releases lock, then copy_to_user().

Additional fixes:
- Added shmid field to process mmap entry (process.h)
- Initialize mmaps[].shmid = -1 in all 3 process creation paths
  (process_init, process_create_kernel, process_fork_create)
- Set shmid = -1 in syscall_mmap_impl and syscall_munmap_impl
- Fork now copies parent's mmap table to child (with shmid)

Passes: make, cppcheck, QEMU smoke test (all init tests OK).

diff --git a/include/process.h b/include/process.h
index 324dad2c0bd355512c5de4a7369b0a759b4e95b2..a8e6dca802918c3a925476a802278aae1efa5a05 100644 (file)
--- a/include/process.h
+++ b/include/process.h
@@ -60,6 +60,7 @@ struct process {
     struct {
         uintptr_t base;
         uint32_t  length;
+        int       shmid;       /* shm segment id, or -1 if not shm */
     } mmaps[PROCESS_MAX_MMAPS];
 
     uintptr_t heap_start;

diff --git a/src/kernel/scheduler.c b/src/kernel/scheduler.c
index 71884e7e28dd99464e73a63921325c962dfd9e92..fbdd6a75de51dd7557b91f51446ead27afe493f8 100644 (file)
--- a/src/kernel/scheduler.c
+++ b/src/kernel/scheduler.c
@@ -384,6 +384,10 @@ struct process* process_fork_create(uintptr_t child_as, const struct registers*
     for (int i = 0; i < PROCESS_MAX_FILES; i++) {
         proc->files[i] = NULL;
     }
+    for (int i = 0; i < PROCESS_MAX_MMAPS; i++) {
+        proc->mmaps[i] = current_process ? current_process->mmaps[i]
+                                         : (typeof(proc->mmaps[i])){0, 0, -1};
+    }
 
     void* stack = kmalloc(4096);
     if (!stack) {
@@ -452,6 +456,9 @@ void process_init(void) {
     for (int i = 0; i < PROCESS_MAX_FILES; i++) {
         kernel_proc->files[i] = NULL;
     }
+    for (int i = 0; i < PROCESS_MAX_MMAPS; i++) {
+        kernel_proc->mmaps[i].shmid = -1;
+    }
     
     current_process = kernel_proc;
     ready_queue_head = kernel_proc;
@@ -500,6 +507,9 @@ struct process* process_create_kernel(void (*entry_point)(void)) {
     for (int i = 0; i < PROCESS_MAX_FILES; i++) {
         proc->files[i] = NULL;
     }
+    for (int i = 0; i < PROCESS_MAX_MMAPS; i++) {
+        proc->mmaps[i].shmid = -1;
+    }
     
     void* stack = kmalloc(4096);
     if (!stack) {

diff --git a/src/kernel/shm.c b/src/kernel/shm.c
index b2b1c93c056e98318385f7ede3a7bf0cf375528d..eda14e613e9d5cfb30b3aace659e4a2ff67fbd9c 100644 (file)
--- a/src/kernel/shm.c
+++ b/src/kernel/shm.c
@@ -3,7 +3,9 @@
 #include "vmm.h"
 #include "process.h"
 #include "spinlock.h"
+#include "uaccess.h"
 #include "errno.h"
+#include "utils.h"
 
 #include <stddef.h>
 
@@ -24,7 +26,7 @@ static spinlock_t shm_lock = {0};
 
 void shm_init(void) {
     for (int i = 0; i < SHM_MAX_SEGMENTS; i++) {
-        segments[i].used = 0;
+        memset(&segments[i], 0, sizeof(segments[i]));
     }
 }
 
@@ -32,10 +34,9 @@ static void shm_destroy(struct shm_segment* seg) {
     for (uint32_t i = 0; i < seg->npages; i++) {
         if (seg->pages[i]) {
             pmm_free_page((void*)seg->pages[i]);
-            seg->pages[i] = 0;
         }
     }
-    seg->used = 0;
+    memset(seg, 0, sizeof(*seg));
 }
 
 int shm_get(uint32_t key, uint32_t size, int flags) {
@@ -112,48 +113,43 @@ void* shm_at(int shmid, uintptr_t shmaddr) {
         return (void*)(uintptr_t)-EINVAL;
     }
 
-    /* Find a free virtual address range in user space for mapping.
-     * Use the process mmap slots. */
     if (!current_process) {
         spin_unlock_irqrestore(&shm_lock, irqf);
         return (void*)(uintptr_t)-EINVAL;
     }
 
+    /* Find a free mmap slot (always needed to track the mapping) */
+    int mslot = -1;
+    for (int i = 0; i < PROCESS_MAX_MMAPS; i++) {
+        if (current_process->mmaps[i].length == 0) {
+            mslot = i;
+            break;
+        }
+    }
+    if (mslot < 0) {
+        spin_unlock_irqrestore(&shm_lock, irqf);
+        return (void*)(uintptr_t)-ENOMEM;
+    }
+
     /* If shmaddr == 0, kernel picks address */
     uintptr_t vaddr = shmaddr;
     if (vaddr == 0) {
-        /* Find free mmap slot and pick address starting at 0x40000000 */
-        int mslot = -1;
-        for (int i = 0; i < PROCESS_MAX_MMAPS; i++) {
-            if (current_process->mmaps[i].length == 0) {
-                mslot = i;
-                break;
-            }
-        }
-        if (mslot < 0) {
-            spin_unlock_irqrestore(&shm_lock, irqf);
-            return (void*)(uintptr_t)-ENOMEM;
-        }
-
-        /* Simple address allocation: 0x40000000 + slot * 64KB */
         vaddr = 0x40000000U + (uint32_t)mslot * (SHM_MAX_PAGES * PAGE_SIZE);
+    }
 
-        /* Map physical pages into user address space */
-        for (uint32_t i = 0; i < seg->npages; i++) {
-            vmm_map_page(vaddr + i * PAGE_SIZE, seg->pages[i],
-                         VMM_FLAG_PRESENT | VMM_FLAG_RW | VMM_FLAG_USER);
-        }
-
-        current_process->mmaps[mslot].base = vaddr;
-        current_process->mmaps[mslot].length = seg->npages * PAGE_SIZE;
-    } else {
-        /* Map at requested address */
-        for (uint32_t i = 0; i < seg->npages; i++) {
-            vmm_map_page(vaddr + i * PAGE_SIZE, seg->pages[i],
-                         VMM_FLAG_PRESENT | VMM_FLAG_RW | VMM_FLAG_USER);
-        }
+    /* Map physical pages into user address space.
+     * vmm_map_page signature: (phys, virt, flags) */
+    for (uint32_t i = 0; i < seg->npages; i++) {
+        vmm_map_page((uint64_t)seg->pages[i],
+                     (uint64_t)(vaddr + i * PAGE_SIZE),
+                     VMM_FLAG_PRESENT | VMM_FLAG_RW | VMM_FLAG_USER);
     }
 
+    /* Record mapping in process mmap table with shmid for detach lookup */
+    current_process->mmaps[mslot].base = vaddr;
+    current_process->mmaps[mslot].length = seg->npages * PAGE_SIZE;
+    current_process->mmaps[mslot].shmid = shmid;
+
     seg->nattch++;
     spin_unlock_irqrestore(&shm_lock, irqf);
     return (void*)vaddr;
@@ -180,36 +176,25 @@ int shm_dt(const void* shmaddr) {
 
     uint32_t len = current_process->mmaps[mslot].length;
     uint32_t npages = len / PAGE_SIZE;
+    int shmid = current_process->mmaps[mslot].shmid;
 
     /* Unmap pages (but don't free — they belong to the shm segment) */
     for (uint32_t i = 0; i < npages; i++) {
         vmm_unmap_page((uint64_t)(addr + i * PAGE_SIZE));
     }
 
+    /* Clear the mmap slot */
     current_process->mmaps[mslot].base = 0;
     current_process->mmaps[mslot].length = 0;
+    current_process->mmaps[mslot].shmid = -1;
 
-    /* Find the segment and decrement attach count */
-    for (int i = 0; i < SHM_MAX_SEGMENTS; i++) {
-        if (!segments[i].used) continue;
-        /* Check if any page matches */
-        for (uint32_t p = 0; p < segments[i].npages; p++) {
-            uintptr_t expected_va = addr + p * PAGE_SIZE;
-            (void)expected_va;
-            /* We can't easily reverse-map. Just decrement nattch for
-             * segments with matching page count. This is a simplification. */
+    /* Decrement attach count using the stored shmid */
+    if (shmid >= 0 && shmid < SHM_MAX_SEGMENTS && segments[shmid].used) {
+        if (segments[shmid].nattch > 0) {
+            segments[shmid].nattch--;
         }
-    }
-
-    /* Simplified: decrement nattch by scanning for segments whose pages
-     * were mapped at this address range. For now, just scan by npages match. */
-    for (int i = 0; i < SHM_MAX_SEGMENTS; i++) {
-        if (segments[i].used && segments[i].npages == npages && segments[i].nattch > 0) {
-            segments[i].nattch--;
-            if (segments[i].nattch == 0 && segments[i].marked_rm) {
-                shm_destroy(&segments[i]);
-            }
-            break;
+        if (segments[shmid].nattch == 0 && segments[shmid].marked_rm) {
+            shm_destroy(&segments[shmid]);
         }
     }
 
@@ -229,12 +214,18 @@ int shm_ctl(int shmid, int cmd, struct shmid_ds* buf) {
     }
 
     if (cmd == IPC_STAT) {
+        /* Copy to local struct first, then release lock before
+         * writing to userspace to avoid deadlock on page fault. */
+        struct shmid_ds local;
+        local.shm_segsz = seg->size;
+        local.shm_nattch = seg->nattch;
+        local.shm_key = seg->key;
+        spin_unlock_irqrestore(&shm_lock, irqf);
         if (buf) {
-            buf->shm_segsz = seg->size;
-            buf->shm_nattch = seg->nattch;
-            buf->shm_key = seg->key;
+            if (copy_to_user(buf, &local, sizeof(local)) < 0) {
+                return -EFAULT;
+            }
         }
-        spin_unlock_irqrestore(&shm_lock, irqf);
         return 0;
     }
 

diff --git a/src/kernel/syscall.c b/src/kernel/syscall.c
index 71b976316971798b6ac8e55071a49b34a280d6f0..7f90fb4482273f73e9ff28515ee47724ca537253 100644 (file)
--- a/src/kernel/syscall.c
+++ b/src/kernel/syscall.c
@@ -1528,6 +1528,7 @@ static uintptr_t syscall_mmap_impl(uintptr_t addr, uint32_t length, uint32_t pro
 
     current_process->mmaps[slot].base = base;
     current_process->mmaps[slot].length = aligned_len;
+    current_process->mmaps[slot].shmid = -1;
 
     return base;
 }
@@ -1555,6 +1556,7 @@ static int syscall_munmap_impl(uintptr_t addr, uint32_t length) {
 
     current_process->mmaps[found].base = 0;
     current_process->mmaps[found].length = 0;
+    current_process->mmaps[found].shmid = -1;
     return 0;
 }