From: Tulio A M Mendes Date: Tue, 10 Feb 2026 08:04:52 +0000 (-0300) Subject: fix: correct 5 bugs in shared memory IPC (Fase 5 audit) X-Git-Url: https://projects.tadryanom.me/docs/static/gitweb.js?a=commitdiff_plain;h=60f30d25160b96eb41d744ae4e3dc687794cc2c7;p=AdrOS.git 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 324dad2..a8e6dca 100644 --- 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 71884e7..fbdd6a7 100644 --- 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 b2b1c93..eda14e6 100644 --- 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 @@ -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 71b9763..7f90fb4 100644 --- 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; }