Implement stack/heap collision protection for processes

.github/workflows/ci.yml

@@ -23,7 +23,9 @@ jobs:
       - uses: actions/checkout@v6
 
       - name: Install dependencies
-        run: sudo apt-get install -y build-essential gcc-multilib g++-multilib
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y build-essential gcc-multilib g++-multilib
 
       - name: Install gcovr
         run: pip install gcovr

src/kernel/include/paging.h

@@ -45,13 +45,15 @@ size_t paging_temp_available();
 /**
  * @brief Identity map a range of pages.
  *
- * The range is end inclusive, ie a page will be added for end.
+ * The range is end exclusive, ie a page will be added for every index in
+ * [start, end) but not for end itself. A range where `start == end` is a
+ * valid empty range and is a no-op.
  *
- * start and end must be < MMU_TABLE_SIZE (ie. only the firs table can be
- * identity mapped).
+ * start must be < MMU_TABLE_SIZE and end must be <= MMU_TABLE_SIZE (ie. only
+ * the firs table can be identity mapped).
  *
- * @param start first page index
- * @param end last page index (inclusive)
+ * @param start first page index (inclusive)
+ * @param end one past the last page index (exclusive)
  * @return int 0 for success
  */
 int paging_id_map_range(size_t start, size_t end);
@@ -70,11 +72,13 @@ int paging_id_map_page(size_t page);
 /**
  * @brief Allocate physical memory and map it to a range of pages.
  *
- * The range is end inclusive, ie a page will be added for end.
+ * The range is end exclusive, ie a page will be added for every index in
+ * [start, end) but not for end itself. `end - start` pages are added in
+ * total. A range where `start == end` is a valid empty range and is a no-op.
  *
  * @param dir pointer to the page directory
- * @param start first page index
- * @param end last page index (inclusive)
+ * @param start first page index (inclusive)
+ * @param end one past the last page index (exclusive)
  * @param flags mmu table (and directory) flags, eg. MMU_TABLE_RW or MMU_TABLE_RW_USER
  * @return int 0 for success
  */
@@ -83,13 +87,15 @@ int paging_add_pages(mmu_dir_t * dir, size_t start, size_t end, uint32_t flags);
 /**
  * @brief Free physical memory for a range of pages.
  *
- * The range is end inclusive, ie a page will be freed for end.
+ * The range is end exclusive, ie a page will be freed for every index in
+ * [start, end) but not for end itself. `end - start` pages are freed in
+ * total. A range where `start == end` is a valid empty range and is a no-op.
  *
  * This function does not free the page tables, it only frees their pages.
  *
  * @param dir pointer to the page directory
- * @param start first page index
- * @param end last page index (inclusive)
+ * @param start first page index (inclusive)
+ * @param end one past the last page index (exclusive)
  * @return int 0 for success
  */
 int paging_remove_pages(mmu_dir_t * dir, size_t start, size_t end);

src/kernel/include/process.h

@@ -72,8 +72,6 @@ typedef struct _process {
     /// PID of parent process (0 is no parent)
     uint32_t parent_pid;
 
-    // TODO heap & stack limits
-
     /// String path to executable file on filesystem
     char * filepath;
     /// Number of arguments, at least 1 (for filename)
@@ -151,17 +149,25 @@ int process_resume(process_t * proc, const ebus_event_t * event);
 /**
  * @brief Add `count` pages to the process heap.
  *
+ * Fails without allocating anything if the new pages would collide with (or
+ * come within HEAP_STACK_GUARD_PAGES of) the process' own user stack, so the
+ * heap and stack can never overlap.
+ *
  * @param proc pointer to the process object
  * @param count number of pages to add
- * @return pointer to the first new page in virtual memory
+ * @return pointer to the first new page in virtual memory, or 0 for failure
  */
 void * process_add_pages(process_t * proc, size_t count);
 
 /**
  * @brief Add a single page to expand the process stack
  *
+ * Fails without allocating anything if the new page would collide with (or
+ * come within HEAP_STACK_GUARD_PAGES of) the process' own heap, so the stack
+ * and heap can never overlap.
+ *
  * @param proc pointer to the process object
- * @return int 0 for success
+ * @return int 0 for success, -1 if it would collide with the heap or on error
  */
 int process_grow_stack(process_t * proc);
 

src/kernel/src/kernel/io_device.c

@@ -73,7 +73,7 @@ int io_device_close(io_device_t * dev) {
     }
 
     int close_res = dev->close_fn(dev->device_data);
-    if (!close_res) {
+    if (close_res) {
         KLOG_DEBUG("Device close function returned an error %d", close_res);
     }
 

src/kernel/src/kernel/memory.c

@@ -42,7 +42,7 @@ void * kernel_alloc_page(size_t count) {
         return 0;
     }
 
-    if (__next_heap_page + count >= MMU_DIR_SIZE * MMU_TABLE_SIZE) {
+    if (__next_heap_page + count > MMU_DIR_SIZE * MMU_TABLE_SIZE) {
         KLOG_WARNING("Tried to allocate past 4 GB");
         return 0;
     }

src/kernel/src/kernel/system_call_proc.c

@@ -80,17 +80,17 @@ int sys_call_proc_cb(uint32_t call_id, void * args_data, registers_t * regs) {
 
             // Default empty string if not provided by process
             const char * file = "";
-            if (!file) {
+            if (args->file) {
                 file = args->file;
             }
 
             // Default empty string if not provided by process
             const char * msg = "";
-            if (!msg) {
+            if (args->msg) {
                 msg = args->msg;
             }
 
-            KLOG_WARNING("Process %u panicked in %s at line %s: %s", proc->pid, file, args->line, msg);
+            KLOG_WARNING("Process %u panicked in %s at line %u: %s", proc->pid, file, args->line, msg);
 
             proc->state = PROCESS_STATE_DEAD;
 

src/kernel/src/paging.c

@@ -120,7 +120,7 @@ int paging_id_map_range(size_t start, size_t end) {
         return -1;
     }
 
-    while (start <= end) {
+    while (start < end) {
         if (paging_id_map_page(start++)) {
             // Error logged in paging_id_map_page
             return -1;
@@ -152,19 +152,24 @@ int paging_add_pages(mmu_dir_t * dir, size_t start, size_t end, uint32_t flags)
         return -1;
     }
 
-    uint32_t table_end = end / MMU_TABLE_SIZE;
+    // Empty range, nothing to do
+    if (start == end) {
+        return 0;
+    }
+
+    uint32_t table_end = (end - 1) / MMU_TABLE_SIZE;
 
     if (table_end >= MMU_DIR_SIZE) {
         KLOG_WARNING("End %u is after last table end %u", end, MMU_DIR_SIZE * MMU_TABLE_SIZE);
         return -1;
     }
 
     // Add pages to tables
-    for (size_t page_i = start; page_i <= end; page_i++) {
+    for (size_t page_i = start; page_i < end; page_i++) {
         uint32_t addr = ram_page_alloc();
 
         if (!addr) {
-            if (paging_remove_pages(dir, start, page_i - 1)) {
+            if (paging_remove_pages(dir, start, page_i)) {
                 KLOG_DEBUG("Failed to remove page from directory");
             }
             return -1;
@@ -177,7 +182,7 @@ int paging_add_pages(mmu_dir_t * dir, size_t start, size_t end, uint32_t flags)
         // already used for supervisor-only pages may also need to become
         // accessible to user pages sharing the same table)
         if (paging_add_table(dir, dir_i, flags)) {
-            if (paging_remove_pages(dir, start, page_i - 1)) {
+            if (paging_remove_pages(dir, start, page_i)) {
                 KLOG_DEBUG("Failed to remove page from directory %p", dir);
             }
             if (ram_page_free(addr)) {
@@ -192,7 +197,7 @@ int paging_add_pages(mmu_dir_t * dir, size_t start, size_t end, uint32_t flags)
 
         if (!table) {
             KLOG_ERROR("Failed to create temporary map for %p", table_addr);
-            if (paging_remove_pages(dir, start, page_i - 1)) {
+            if (paging_remove_pages(dir, start, page_i)) {
                 KLOG_DEBUG("Failed to remove page from directory %p while unwinding failed temp map", dir);
             }
             if (ram_page_free(addr)) {
@@ -225,15 +230,20 @@ int paging_remove_pages(mmu_dir_t * dir, size_t start, size_t end) {
         return -1;
     }
 
-    uint32_t table_end = end / MMU_TABLE_SIZE;
+    // Empty range, nothing to do
+    if (start == end) {
+        return 0;
+    }
+
+    uint32_t table_end = (end - 1) / MMU_TABLE_SIZE;
 
     if (table_end >= MMU_DIR_SIZE) {
         KLOG_WARNING("End %u is after last table end %u", end, MMU_TABLE_SIZE);
         return -1;
     }
 
     // Remove pages from tables
-    for (size_t page_i = start; page_i <= end; page_i++) {
+    for (size_t page_i = start; page_i < end; page_i++) {
         uint32_t dir_i   = page_i / MMU_TABLE_SIZE;
         uint32_t table_i = page_i % MMU_TABLE_SIZE;
 

src/kernel/src/process.c

@@ -74,7 +74,7 @@ int process_create(process_t * proc) {
     // as the TSS esp0 target so a ring3->ring0 transition always starts fresh
     // here.
     uint32_t isr_stack_start = ADDR2PAGE(proc->esp0) - ISR_STACK_PAGES + 1;
-    if (paging_add_pages(dir, isr_stack_start, ADDR2PAGE(proc->esp0), MMU_TABLE_RW)) {
+    if (paging_add_pages(dir, isr_stack_start, ADDR2PAGE(proc->esp0) + 1, MMU_TABLE_RW)) {
         KLOG_DEBUG("Failed to create pages for isr stack");
         paging_temp_free(proc->cr3);
         ram_page_free(proc->cr3);
@@ -83,7 +83,7 @@ int process_create(process_t * proc) {
 
     // Allocate the first page of the user (ring 3) stack. This must be
     // user-accessible so the process can use it once running in ring 3.
-    if (paging_add_pages(dir, ADDR2PAGE(VADDR_USER_STACK), ADDR2PAGE(VADDR_USER_STACK), MMU_TABLE_RW_USER)) {
+    if (paging_add_pages(dir, ADDR2PAGE(VADDR_USER_STACK), ADDR2PAGE(VADDR_USER_STACK) + 1, MMU_TABLE_RW_USER)) {
         KLOG_DEBUG("Failed to create page for user stack");
         paging_temp_free(proc->cr3);
         ram_page_free(proc->cr3);
@@ -386,11 +386,16 @@ void * process_add_pages(process_t * proc, size_t count) {
         return 0;
     }
 
-    if (proc->next_heap_page + count >= MMU_DIR_SIZE * MMU_TABLE_SIZE) {
+    if (proc->next_heap_page + count > MMU_DIR_SIZE * MMU_TABLE_SIZE) {
         KLOG_WARNING("Cannot allocate %u pages after %u, will exceed max size of %d", count, proc->next_heap_page, MMU_DIR_SIZE * MMU_TABLE_SIZE);
         return 0;
     }
 
+    if (proc->next_heap_page + count + HEAP_STACK_GUARD_PAGES + proc->stack_page_count > (size_t)ADDR2PAGE(VADDR_USER_STACK) + 1) {
+        KLOG_WARNING("Cannot allocate %u pages after %u, would collide with process stack (%u pages)", count, proc->next_heap_page, proc->stack_page_count);
+        return 0;
+    }
+
     mmu_dir_t * dir = paging_temp_map(proc->cr3);
 
     if (!dir) {
@@ -421,13 +426,6 @@ int process_grow_stack(process_t * proc) {
         return -1;
     }
 
-    mmu_dir_t * dir = paging_temp_map(proc->cr3);
-
-    if (!dir) {
-        KLOG_ERROR("Failed to create temporary map of process page directory");
-        return -1;
-    }
-
     // Stack pages grow down starting immediately below the first user stack
     // page allocated by process_create (at ADDR2PAGE(VADDR_USER_STACK)).
     // proc->stack_page_count starts at 1 (that first page), so the Nth call
@@ -437,7 +435,19 @@ int process_grow_stack(process_t * proc) {
     // to user-accessible) the supervisor-only ISR stack pages above it.
     size_t new_stack_page_i = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count;
 
-    if (paging_add_pages(dir, new_stack_page_i, new_stack_page_i, MMU_TABLE_RW_USER)) {
+    if (new_stack_page_i < proc->next_heap_page + HEAP_STACK_GUARD_PAGES) {
+        KLOG_WARNING("Cannot grow stack to page %u, would collide with process heap (next heap page %u)", new_stack_page_i, proc->next_heap_page);
+        return -1;
+    }
+
+    mmu_dir_t * dir = paging_temp_map(proc->cr3);
+
+    if (!dir) {
+        KLOG_ERROR("Failed to create temporary map of process page directory");
+        return -1;
+    }
+
+    if (paging_add_pages(dir, new_stack_page_i, new_stack_page_i + 1, MMU_TABLE_RW_USER)) {
         KLOG_DEBUG("Failed to add pages for process stack");
         paging_temp_free(proc->cr3);
         return -1;

src/util/include/addr.h

@@ -39,4 +39,6 @@ typedef void * v_ptr_t;
 #define VADDR_ISR_STACK    0xffffffff
 #define ISR_STACK_PAGES    0x10
 
+#define HEAP_STACK_GUARD_PAGES 1
+
 #endif // ADDR_H