diff --git a/CMakeLists.txt b/CMakeLists.txt index e3d534dd..de6eed09 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,6 +10,9 @@ set(CMAKE_TOOLCHAIN_FILE cmake/toolchain.cmake) # Extra checks in malloc set(KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED 1) +# Default maximum size (in KB) a process' stack is allowed to grow to. +set(KERNEL_MAX_STACK_SIZE_KB 8192) + execute_process( COMMAND git describe --tags --abbrev=0 OUTPUT_VARIABLE OS_VERSION diff --git a/cmake/config.h.in b/cmake/config.h.in index e4ec1acb..e36e56cf 100644 --- a/cmake/config.h.in +++ b/cmake/config.h.in @@ -11,6 +11,7 @@ #define CPU_ARCH "@CPU_ARCH@" #define FILE_PREFIX_LENGTH @FILE_PREFIX_LENGTH@ #define KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED @KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED@ +#define KERNEL_MAX_STACK_SIZE_KB @KERNEL_MAX_STACK_SIZE_KB@ #define PROJECT_DESCRIPTION PROJECT_NAME \ " version=" PROJECT_VERSION \ diff --git a/design/kernel_parameters.md b/design/kernel_parameters.md index 9b9b0eaa..15e36b76 100644 --- a/design/kernel_parameters.md +++ b/design/kernel_parameters.md @@ -16,4 +16,5 @@ generated from `cmake/config.h.in`). | CPU_ARCH | CPU Architecture | | FILE_PREFIX_LENGTH | Length of absolute path to `src/`, used in logger macro to isolate relative file path | | KERNEL_MEMORY_INTEGRITY_CHECKS_ENABLED | Perform extra checks during malloc operations | +| KERNEL_MAX_STACK_SIZE_KB | Default maximum size in KB a process' stack may grow to | | PROJECT_DESCRIPTION | String that combines name, version and arguments to the kernel | diff --git a/src/cpu/i386/src/isr.c b/src/cpu/i386/src/isr.c index 10375b65..cfbcc9db 100644 --- a/src/cpu/i386/src/isr.c +++ b/src/cpu/i386/src/isr.c @@ -2,8 +2,13 @@ #include "cpu/isr.h" +#include + +#include "addr.h" #include "cpu/idt.h" +#include "cpu/mmu.h" #include "cpu/ports.h" +#include "defs.h" #include "kernel.h" #include "kernel/logs.h" #include "libc/proc.h" @@ -13,6 +18,8 @@ static isr_t __interrupt_handlers[256]; +static int handle_stack_growth_fault(registers_t * r); + /* Can't do this with a loop because we need the address * of the function names */ void isr_install() { @@ -125,6 +132,12 @@ char * exception_messages[] = { }; void isr_handler(registers_t r) { + if (r.int_no == 14 && !handle_stack_growth_fault(&r)) { + // Stack grown enough to retry the faulting instruction (possibly + // not all the way to the fault yet - see handle_stack_growth_fault). + return; + } + print_trace(&r); printf("ISR %u (err 0x%X)\n", r.int_no, r.err_code); printf("%s\n", exception_messages[r.int_no]); @@ -264,6 +277,78 @@ void print_trace(registers_t * r) { print_cr0(r->cr0); putc('\n'); } + +// A faulting PUSH/PUSHA is restartable: on fault, the CPU rolls back to the +// pre-instruction ESP (the useresp saved on the ring3->ring0 transition), not +// to wherever the in-flight instruction had reached. So the lowest address an +// otherwise-legitimate stack write can fault on is up to this many bytes +// below useresp - PUSHA being the worst case (8 dwords). Without this slack, +// a fault landing just below a page boundary (CR2 one page lower than +// useresp) would be wrongly rejected as illegitimate. +#define MAX_STACK_FAULT_SLACK_BYTES 32 + +// Caps how many pages a single page fault will grow the stack by. Larger +// requests (eg. a huge single access after `sub esp, `) are only +// grown up to this many pages per fault; if that isn't enough to reach the +// faulting page, the faulting instruction is retried, re-faults at the same +// address, and growth continues from there on the next call - bounding how +// much work (and RAM) one fault can commit while interrupts are disabled, +// instead of one fault being able to claim the whole configured stack size +// limit at once. Either way, total stack growth remains bounded by the +// process' own proc->max_stack_pages, enforced in process_grow_stack. +#define MAX_STACK_GROWTH_PAGES_PER_FAULT 16 + +/** + * @brief Try to handle a page fault by growing the faulting process' stack + * down to (or partway to, see MAX_STACK_GROWTH_PAGES_PER_FAULT) the faulting + * page. + * + * Only handles a user-mode fault on a non-present page that is at or below + * the current stack end and at or above (allowing for + * MAX_STACK_FAULT_SLACK_BYTES) the saved user ESP - anything else + * (protection fault, supervisor fault, or a fault outside that range) is + * left for the caller to report and panic on. + * + * @param r page fault registers (int_no == 14) + * @return 0 if the fault was handled (stack grown far enough to retry, even + * if not all the way to the fault - see MAX_STACK_GROWTH_PAGES_PER_FAULT), + * non-zero otherwise + */ +static int handle_stack_growth_fault(registers_t * r) { + bool user_mode = (r->err_code & MMU_DIR_FLAG_USER_SUPERVISOR) != 0; + bool page_present = (r->err_code & MMU_DIR_FLAG_PRESENT) != 0; + + if (!user_mode || page_present) { + return -1; + } + + process_t * proc = get_current_process(); + if (!proc) { + return -1; + } + + size_t fault_page = ADDR2PAGE(r->cr2); + size_t next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; + + if (fault_page > next_stack_page) { + return -1; + } + + size_t lowest_valid_page = ADDR2PAGE(r->useresp - MAX_STACK_FAULT_SLACK_BYTES); + if (fault_page < lowest_valid_page) { + return -1; + } + + for (int i = 0; i < MAX_STACK_GROWTH_PAGES_PER_FAULT && next_stack_page >= fault_page; i++) { + if (process_grow_stack(proc)) { + return -1; + } + next_stack_page = ADDR2PAGE(VADDR_USER_STACK) - proc->stack_page_count; + } + + return 0; +} + /* Bit Label Description 0 PE Protected Mode Enable diff --git a/src/kernel/include/process.h b/src/kernel/include/process.h index 5a417172..d40b5002 100644 --- a/src/kernel/include/process.h +++ b/src/kernel/include/process.h @@ -68,6 +68,8 @@ typedef struct _process { uint32_t next_heap_page; /// Number of pages allocated to the stack uint32_t stack_page_count; + /// Maximum number of pages the stack may grow to + uint32_t max_stack_pages; /// PID of parent process (0 is no parent) uint32_t parent_pid; @@ -167,10 +169,29 @@ void * process_add_pages(process_t * proc, size_t count); * and heap can never overlap. * * @param proc pointer to the process object - * @return int 0 for success, -1 if it would collide with the heap or on error + * @return 0 for success, -1 if it would collide with the heap, would + * exceed proc->max_stack_pages, or on error */ int process_grow_stack(process_t * proc); +/** + * @brief Change the maximum size (in KB) a process' stack may grow to, + * rounding up to a whole page if `max_stack_size_kb` is not already page + * aligned. + * + * @todo This currently checks against *allocated* stack pages + * (stack_page_count), not how much of the stack is actually *in use*. Once + * that's tracked, this should instead check against actual stack usage, and + * shrinking should free any allocated pages that fall outside the new, + * smaller limit. + * + * @param proc pointer to the process object + * @param max_stack_size_kb new maximum stack size in KB + * @return 0 for success, -1 if null process or the rounded page count is + * smaller than the currently allocated stack page count + */ +int process_set_max_stack_size_kb(process_t * proc, uint32_t max_stack_size_kb); + /** * @brief Allocate pages in the heap and copy data from `buff` into the pages. * diff --git a/src/kernel/src/exec.c b/src/kernel/src/exec.c index 6ca1f8d5..02136890 100644 --- a/src/kernel/src/exec.c +++ b/src/kernel/src/exec.c @@ -54,18 +54,6 @@ int command_exec(uint8_t * buff, const char * filepath, size_t size, size_t argc return -1; } - KLOG_TRACE("Increasing stack by 1022 pages"); - for (size_t i = 0; i < 1022; i++) { - if (process_grow_stack(proc)) { - KLOG_DEBUG("Failed to add page %u", i); - if (process_free(proc)) { - KPANIC("Failed to free process while handling stack grow error"); - } - kfree(proc); - return -1; - } - } - if (process_copy_args(proc, filepath, argc, argv)) { KLOG_DEBUG("Failed to copy args"); if (process_free(proc)) { diff --git a/src/kernel/src/kernel.c b/src/kernel/src/kernel.c index 184827d1..34c50686 100644 --- a/src/kernel/src/kernel.c +++ b/src/kernel/src/kernel.c @@ -167,7 +167,7 @@ int kernel_exec(const char * filename, size_t argc, char ** argv) { return -1; } - KLOG_DEBUG("Process for exec %d finished for filename %s", pid); + KLOG_DEBUG("Process for exec %d finished for filename %s", pid, filename); io_device_close(file); KLOG_TRACE("File device closed"); diff --git a/src/kernel/src/loader.c b/src/kernel/src/loader.c index 29ad658d..d6aecc71 100644 --- a/src/kernel/src/loader.c +++ b/src/kernel/src/loader.c @@ -237,15 +237,6 @@ static process_t * load_init() { return 0; } - for (size_t i = 0; i < 1022; i++) { - if (process_grow_stack(proc)) { - KLOG_DEBUG("Failed to grow process stack"); - return 0; - } - } - - KLOG_TRACE("Stack allocated for process %u", proc->pid); - process_copy_args(proc, filename, 0, 0); KLOG_TRACE("Copied filename %s to process %u", filename, proc->pid); diff --git a/src/kernel/src/process.c b/src/kernel/src/process.c index d2138c4e..2415e5cd 100644 --- a/src/kernel/src/process.c +++ b/src/kernel/src/process.c @@ -2,6 +2,7 @@ #include "process.h" +#include "config.h" #include "cpu/gdt.h" #include "cpu/mmu.h" #include "cpu/tss.h" @@ -16,9 +17,10 @@ static int open_stdio_handles(process_t * proc); -static char * copy_string(const char * str); -static int copy_to_process_pages(process_t * proc, uint32_t page_start, size_t count, const char * buff, size_t size); -static int write_process_dwords(process_t * proc, uint32_t first_addr, const uint32_t * values, size_t count); +static char * copy_string(const char * str); +static int copy_to_process_pages(process_t * proc, uint32_t page_start, size_t count, const char * buff, size_t size); +static int write_process_dwords(process_t * proc, uint32_t first_addr, const uint32_t * values, size_t count); +static uint32_t stack_size_kb_to_pages(uint32_t size_kb); static uint32_t next_pid(); static uint32_t next_handle_id(); @@ -93,6 +95,7 @@ int process_create(process_t * proc) { proc->pid = next_pid(); proc->next_heap_page = ADDR2PAGE(VADDR_USER_MEM); proc->stack_page_count = 1; + proc->max_stack_pages = stack_size_kb_to_pages(KERNEL_MAX_STACK_SIZE_KB); // TODO parent process pid @@ -426,6 +429,11 @@ int process_grow_stack(process_t * proc) { return -1; } + if (proc->stack_page_count >= proc->max_stack_pages) { + KLOG_WARNING("Cannot grow stack past %u pages (max_stack_pages)", proc->max_stack_pages); + 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 @@ -463,6 +471,28 @@ int process_grow_stack(process_t * proc) { return 0; } +int process_set_max_stack_size_kb(process_t * proc, uint32_t max_stack_size_kb) { + if (!proc) { + KLOG_WARNING("Process struct is null pointer"); + return -1; + } + + uint32_t max_stack_pages = stack_size_kb_to_pages(max_stack_size_kb); + + // TODO this should check against actual stack usage rather than pages + // allocated so far (proc->stack_page_count) - once usage is tracked, + // shrinking below the current allocation should free the pages that + // fall outside the new, smaller limit instead of being rejected outright. + if (max_stack_pages < proc->stack_page_count) { + KLOG_WARNING("Cannot set max stack size to %u KB (%u pages), %u pages are already allocated", max_stack_size_kb, max_stack_pages, proc->stack_page_count); + return -1; + } + + proc->max_stack_pages = max_stack_pages; + + return 0; +} + /** * @brief Copy `size` bytes from `buff` (kernel memory) into `count` pages of * the process' own address space, starting at page index `page_start`. The @@ -881,3 +911,27 @@ void set_next_handle_id(uint32_t next) { next_handle_id(); // Force handle_id_set to true so it doesn't override this value __handle_id = next; } + +/** + * @brief Convert a stack size in KB to a whole number of pages, rounding up + * if `size_kb` is not already page aligned. + * + * Works entirely in KB (never converting to bytes) so that a large + * `size_kb` cannot overflow uint32_t during the conversion - multiplying by + * 1024 (or adding PAGE_SIZE - 1 to the result) can wrap for inputs + * approaching UINT32_MAX, which would silently turn a large configured + * limit into a much smaller (or zero) page count. + * + * @param size_kb stack size in KB + * @return number of pages + */ +static uint32_t stack_size_kb_to_pages(uint32_t size_kb) { + const uint32_t page_size_kb = PAGE_SIZE / 1024; + + uint32_t pages = size_kb / page_size_kb; + if (size_kb % page_size_kb != 0) { + pages++; + } + + return pages; +} diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 20c8b9c7..60098f3e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -12,6 +12,9 @@ set(CMAKE_C_STANDARD 17) # Set the global FFF_GCC_FUNCTION_ATTRIBUTES for config.h set(FFF_GCC_FUNCTION_ATTRIBUTES "__attribute__((weak))") +# Mirrors the default in the root CMakeLists.txt so process.c (which reads +# this to seed process_t.max_stack_pages) compiles the same way under test. +set(KERNEL_MAX_STACK_SIZE_KB 8192) configure_file(${CMAKE_CURRENT_SOURCE_DIR}/config/config.h.in ${CMAKE_BINARY_DIR}/include/config.h) include_directories(PRIVATE ${CMAKE_BINARY_DIR}/include) diff --git a/tests/config/config.h.in b/tests/config/config.h.in index 30121ce2..59bdf902 100644 --- a/tests/config/config.h.in +++ b/tests/config/config.h.in @@ -3,3 +3,5 @@ */ #cmakedefine FFF_GCC_FUNCTION_ATTRIBUTES @FFF_GCC_FUNCTION_ATTRIBUTES@ + +#define KERNEL_MAX_STACK_SIZE_KB @KERNEL_MAX_STACK_SIZE_KB@ diff --git a/tests/src/kernel/test_process.cpp b/tests/src/kernel/test_process.cpp index 3398ce83..b4107ff5 100644 --- a/tests/src/kernel/test_process.cpp +++ b/tests/src/kernel/test_process.cpp @@ -5,6 +5,7 @@ extern "C" { #include "addr.h" +#include "config.h" #include "cpu/gdt.h" #include "cpu/mmu.h" #include "libc/datastruct/array.h" @@ -45,6 +46,13 @@ int custom_mmu_table_set(mmu_table_t * table, size_t i, uint32_t addr, uint32_t std::array temp_page; std::array heap_data; +// Mirrors process.c's static stack_size_kb_to_pages helper, so tests can +// compute the expected page count for a given KB size without depending on +// kernel-internal (non-exported) code. +static uint32_t kb_to_pages(uint32_t size_kb) { + return (size_kb * 1024 + PAGE_SIZE - 1) / PAGE_SIZE; +} + class Process : public ::testing::Test { protected: void SetUp() override { @@ -63,7 +71,8 @@ class Process : public ::testing::Test { heap_data[i] = i % 0xff; } - proc.next_heap_page = 2; + proc.next_heap_page = 2; + proc.max_stack_pages = kb_to_pages(KERNEL_MAX_STACK_SIZE_KB); mmu_dir_set_fake.custom_fake = custom_mmu_dir_set; mmu_table_set_fake.custom_fake = custom_mmu_table_set; @@ -160,6 +169,7 @@ TEST_F(Process, process_create) { EXPECT_EQ(12, proc.pid); EXPECT_EQ(1024, proc.next_heap_page); EXPECT_EQ(1, proc.stack_page_count); + EXPECT_EQ(kb_to_pages(KERNEL_MAX_STACK_SIZE_KB), proc.max_stack_pages); EXPECT_EQ(0xffffffff, proc.esp); EXPECT_EQ(0xffffffff, proc.esp0); @@ -518,8 +528,11 @@ TEST_F(Process, process_grow_stack_CollidesWithHeap) { // Simulate a stack that has already grown down to meet the process' // current heap top (next_heap_page defaults to 2 in SetUp). The next // grown page would land exactly on next_heap_page, so it must be - // rejected instead of overlapping the heap. + // rejected instead of overlapping the heap. max_stack_pages is set high + // enough here that only the heap collision check can be responsible for + // the rejection. proc.stack_page_count = (uint32_t)ADDR2PAGE(VADDR_USER_STACK) - proc.next_heap_page; + proc.max_stack_pages = proc.stack_page_count + 1; paging_temp_map_fake.return_val = &dir; uint32_t stack_page_count_before = proc.stack_page_count; @@ -531,6 +544,55 @@ TEST_F(Process, process_grow_stack_CollidesWithHeap) { ASSERT_TEMP_MAP_BALANCED(); } +TEST_F(Process, process_grow_stack_AtMaxStackPages) { + // Stack has already grown to the process' configured limit, so the next + // grow must be rejected without touching paging at all. + proc.max_stack_pages = 4; + proc.stack_page_count = proc.max_stack_pages; + paging_temp_map_fake.return_val = &dir; + + EXPECT_NE(0, process_grow_stack(&proc)); + EXPECT_EQ(0, paging_temp_map_fake.call_count); + EXPECT_EQ(0, paging_add_pages_fake.call_count); + EXPECT_EQ(4u, proc.stack_page_count); +} + +// Process Set Max Stack Size (KB) + +TEST_F(Process, process_set_max_stack_size_kb_InvalidParameters) { + EXPECT_NE(0, process_set_max_stack_size_kb(0, 100)); +} + +TEST_F(Process, process_set_max_stack_size_kb_TooSmall) { + proc.stack_page_count = 5; + proc.max_stack_pages = kb_to_pages(KERNEL_MAX_STACK_SIZE_KB); + + // 16 KB is exactly 4 pages, less than the 5 pages already allocated + EXPECT_NE(0, process_set_max_stack_size_kb(&proc, 16)); + EXPECT_EQ(kb_to_pages(KERNEL_MAX_STACK_SIZE_KB), proc.max_stack_pages); +} + +TEST_F(Process, process_set_max_stack_size_kb) { + proc.stack_page_count = 5; + + // 20 KB is exactly 5 pages + EXPECT_EQ(0, process_set_max_stack_size_kb(&proc, 20)); + EXPECT_EQ(5u, proc.max_stack_pages); + + // 400 KB is exactly 100 pages + EXPECT_EQ(0, process_set_max_stack_size_kb(&proc, 400)); + EXPECT_EQ(100u, proc.max_stack_pages); +} + +TEST_F(Process, process_set_max_stack_size_kb_RoundsUpToPageBoundary) { + proc.stack_page_count = 0; + + // 5 KB is not page aligned (PAGE_SIZE is 4 KB), so it must round up to 2 + // whole pages (8 KB) rather than truncate to 1. + EXPECT_EQ(0, process_set_max_stack_size_kb(&proc, 5)); + EXPECT_EQ(2u, proc.max_stack_pages); +} + // Process Load Heap TEST_F(Process, process_load_heap_InvalidParameters) {