From 72650ee47b60f13633e62217d2a03573da3e2cf3 Mon Sep 17 00:00:00 2001 From: Torin Date: Sun, 22 Mar 2026 18:22:14 +0200 Subject: [PATCH] fix(thread-safety): Address TOCTOU races in global state initialization using Windows synchronization primitives --- .claude/settings.local.json | 3 +- include/Resource.h | 4 +- .../.openspec.yaml | 2 + .../fix-global-state-thread-safety/design.md | 60 +++++++++++++++ .../proposal.md | 29 +++++++ .../specs/thread-safe-init/spec.md | 30 ++++++++ .../fix-global-state-thread-safety/tasks.md | 10 +++ src/KernelHeap.c | 64 +++++++++------- src/Resource.c | 76 +++++++++---------- 9 files changed, 210 insertions(+), 68 deletions(-) create mode 100644 openspec/changes/fix-global-state-thread-safety/.openspec.yaml create mode 100644 openspec/changes/fix-global-state-thread-safety/design.md create mode 100644 openspec/changes/fix-global-state-thread-safety/proposal.md create mode 100644 openspec/changes/fix-global-state-thread-safety/specs/thread-safe-init/spec.md create mode 100644 openspec/changes/fix-global-state-thread-safety/tasks.md diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 5d4c767..10a145e 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -13,7 +13,8 @@ "Bash(grep -r \"#include\" /c/projects/winkernellite/include/*.h)", "Bash(grep -r \"^#ifndef\\\\|^#define\" /c/projects/winkernellite/include/*.h)", "Bash(grep:*)", - "Bash(./build/bin/runTests.exe)" + "Bash(./build/bin/runTests.exe)", + "Bash(git branch:*)" ] } } diff --git a/include/Resource.h b/include/Resource.h index e3b89a8..8cea984 100644 --- a/include/Resource.h +++ b/include/Resource.h @@ -82,12 +82,12 @@ extern "C" { /* Global critical region counter (protected by its own critical section) */ extern LONG g_WinKernelLite_KernelApcDisableCount; extern CRITICAL_SECTION g_WinKernelLite_KernelApcDisableLock; -extern BOOLEAN g_WinKernelLite_KernelApcDisableLockInitialized; +extern INIT_ONCE g_WinKernelLite_KernelApcDisableInitOnce; /* Global system resources list */ extern LIST_ENTRY g_WinKernelLite_SystemResourcesList; extern CRITICAL_SECTION g_WinKernelLite_SystemResourcesLock; -extern BOOLEAN g_WinKernelLite_SystemResourcesInitialized; +extern INIT_ONCE g_WinKernelLite_SystemResourcesInitOnce; #define IsOwnedExclusive(R) ((R)->Flag & ResourceOwnedExclusive) diff --git a/openspec/changes/fix-global-state-thread-safety/.openspec.yaml b/openspec/changes/fix-global-state-thread-safety/.openspec.yaml new file mode 100644 index 0000000..caac517 --- /dev/null +++ b/openspec/changes/fix-global-state-thread-safety/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-03-22 diff --git a/openspec/changes/fix-global-state-thread-safety/design.md b/openspec/changes/fix-global-state-thread-safety/design.md new file mode 100644 index 0000000..59e6a83 --- /dev/null +++ b/openspec/changes/fix-global-state-thread-safety/design.md @@ -0,0 +1,60 @@ +## Context + +Three separate initialization paths in WinKernelLite use check-then-act patterns without synchronization. All are in `.c` files after the recent refactor, making them straightforward to fix. + +**Pattern 1 - Pointer initialization** (`GetGlobalState`): +```c +if (g_WinKernelLite_GlobalState == NULL) { + temp = HeapAlloc(...); + // ... initialize temp ... + g_WinKernelLite_GlobalState = temp; // RACE: two threads both write +} +``` + +**Pattern 2 - Boolean-guarded one-time init** (`EnsureSystemResourcesListInitialized`, `KeEnterCriticalRegion`, etc.): +```c +if (!g_Initialized) { + InitializeCriticalSection(&g_Lock); // RACE: double init + g_Initialized = TRUE; +} +``` + +## Goals / Non-Goals + +**Goals:** +- Eliminate all TOCTOU races in global state initialization +- Lock-free on the fast path (after initialization completes) +- Use Windows-native primitives only + +**Non-Goals:** +- Making heap tracking operations (TrackAllocation, UntrackAllocation, etc.) thread-safe - that is a separate change +- Adding thread safety to debug state initialization (Debug.h/Debug.c) - separate change + +## Decisions + +### 1. Use InterlockedCompareExchangePointer for GetGlobalState + +**Decision:** Allocate and fully initialize a `GLOBAL_STATE` into a local temp pointer, then atomically CAS it into `g_WinKernelLite_GlobalState`. If CAS fails (another thread won the race), free the temp. + +**Rationale:** This is the standard Windows pattern for lazy pointer initialization. It's lock-free, requires no additional global state, and the fast path is a single pointer read. The kernel itself uses this pattern (`ExInterlockedCompareExchangePointer`). + +**Alternative considered:** `InitOnceExecuteOnce` - works but adds an `INIT_ONCE` variable and a callback function for what is fundamentally a pointer-swap operation. CAS is simpler here. + +### 2. Use InitOnceExecuteOnce for CRITICAL_SECTION initialization + +**Decision:** Replace the boolean flags (`g_WinKernelLite_KernelApcDisableLockInitialized`, `g_WinKernelLite_SystemResourcesInitialized`) with `INIT_ONCE` variables. Use `InitOnceExecuteOnce` to guarantee exactly-once initialization. + +**Rationale:** `InitializeCriticalSection` cannot be safely called twice on the same `CRITICAL_SECTION`. `INIT_ONCE` is the Windows-blessed mechanism for this exact scenario. It handles all synchronization internally and the fast path (after init) is a single interlocked read. + +**Alternative considered:** Manual CAS on the boolean flag - fragile because `InitializeCriticalSection` can throw an exception, leaving the flag in an inconsistent state. `InitOnceExecuteOnce` handles errors correctly. + +### 3. Remove boolean flag globals, add INIT_ONCE globals + +**Decision:** Replace the 2 boolean globals (`g_WinKernelLite_KernelApcDisableLockInitialized`, `g_WinKernelLite_SystemResourcesInitialized`) with 2 `INIT_ONCE` globals. Initialize them with `INIT_ONCE_STATIC_INIT` (zero-init, no runtime constructor needed). + +**Rationale:** Cleaner API surface, removes the race-prone boolean pattern entirely. + +## Risks / Trade-offs + +- **[Risk] CAS loop on high contention** -> Mitigation: In practice, `GetGlobalState` is called once early in process lifetime. The losing thread just frees its allocation. No spin loop. +- **[Risk] InitOnceExecuteOnce callback complexity** -> Mitigation: Callbacks are trivial 3-line functions. The alternative (manual CAS + exception handling) is more complex and error-prone. diff --git a/openspec/changes/fix-global-state-thread-safety/proposal.md b/openspec/changes/fix-global-state-thread-safety/proposal.md new file mode 100644 index 0000000..4997b3c --- /dev/null +++ b/openspec/changes/fix-global-state-thread-safety/proposal.md @@ -0,0 +1,29 @@ +## Why + +Code review identified TOCTOU (Time-of-Check-Time-of-Use) race conditions in all global state initialization paths. Multiple threads calling `GetGlobalState()`, `EnsureSystemResourcesListInitialized()`, or `KeEnterCriticalRegion()` simultaneously can double-initialize, leaking memory or corrupting state. Now that these implementations are in `.c` files (after the refactor-headers-to-source change), we can apply proper Windows synchronization primitives. + +## What Changes + +- Fix `GetGlobalState()` in `src/KernelHeap.c`: use `InterlockedCompareExchangePointer` to atomically publish the global state pointer. Allocate to a temp variable, initialize fully, then CAS into the global. If CAS fails (another thread won), free the temp. +- Fix `EnsureSystemResourcesListInitialized()` in `src/Resource.c`: replace the `BOOLEAN` flag check with `InitOnceExecuteOnce` using a static `INIT_ONCE` variable. +- Fix `KeEnterCriticalRegion()`, `KeLeaveCriticalRegion()`, and `GetKernelApcDisableCount()` in `src/Resource.c`: replace the `BOOLEAN` flag check with `InitOnceExecuteOnce` using a static `INIT_ONCE` variable. +- Remove the now-unnecessary `g_WinKernelLite_KernelApcDisableLockInitialized` and `g_WinKernelLite_SystemResourcesInitialized` boolean flags from `Resource.h` and `Resource.c`. + +## Capabilities + +### New Capabilities +- `thread-safe-init`: Requirements for thread-safe one-time initialization of global state + +### Modified Capabilities + + +## Impact + +- **src/KernelHeap.c**: `GetGlobalState()` modified to use CAS pattern +- **src/Resource.c**: 4 functions modified to use `InitOnceExecuteOnce`; 2 boolean globals removed +- **include/Resource.h**: Remove `extern` declarations for 2 boolean flags; add `INIT_ONCE` externs +- **Tests**: No changes - behavior is identical, just thread-safe + +## Version Impact + +**PATCH** - Bug fix. No API changes. Existing single-threaded behavior is identical; multi-threaded behavior is corrected from broken to correct. diff --git a/openspec/changes/fix-global-state-thread-safety/specs/thread-safe-init/spec.md b/openspec/changes/fix-global-state-thread-safety/specs/thread-safe-init/spec.md new file mode 100644 index 0000000..f30f029 --- /dev/null +++ b/openspec/changes/fix-global-state-thread-safety/specs/thread-safe-init/spec.md @@ -0,0 +1,30 @@ +## ADDED Requirements + +### Requirement: GetGlobalState SHALL be thread-safe +`GetGlobalState()` SHALL use `InterlockedCompareExchangePointer` to atomically publish the global state pointer. If two threads race, exactly one SHALL succeed and the other SHALL free its local allocation without leaking memory. + +#### Scenario: Concurrent first call from two threads +- **WHEN** two threads call `GetGlobalState()` simultaneously while `g_WinKernelLite_GlobalState` is NULL +- **THEN** exactly one `GLOBAL_STATE` allocation SHALL survive, the other SHALL be freed, and both threads SHALL receive the same valid pointer + +#### Scenario: Fast path after initialization +- **WHEN** `GetGlobalState()` is called after the global state is already initialized +- **THEN** the function SHALL return the existing pointer without allocating or performing any synchronization beyond a pointer read + +### Requirement: CRITICAL_SECTION initialization SHALL use InitOnceExecuteOnce +All `CRITICAL_SECTION` globals that are lazily initialized SHALL use `InitOnceExecuteOnce` with a static `INIT_ONCE` variable to guarantee exactly-once initialization. + +#### Scenario: Concurrent EnsureSystemResourcesListInitialized +- **WHEN** two threads call `EnsureSystemResourcesListInitialized()` simultaneously before the system resources list is initialized +- **THEN** `InitializeCriticalSection` SHALL be called exactly once, and both threads SHALL proceed safely after initialization completes + +#### Scenario: Concurrent KeEnterCriticalRegion +- **WHEN** two threads call `KeEnterCriticalRegion()` simultaneously before the APC disable lock is initialized +- **THEN** `InitializeCriticalSection` for the APC disable lock SHALL be called exactly once + +### Requirement: Boolean initialization flags SHALL be removed +The `g_WinKernelLite_KernelApcDisableLockInitialized` and `g_WinKernelLite_SystemResourcesInitialized` boolean globals SHALL be replaced by `INIT_ONCE` variables. No race-prone boolean check-then-act patterns SHALL remain. + +#### Scenario: No boolean flags in Resource.h or Resource.c +- **WHEN** the codebase is searched for `KernelApcDisableLockInitialized` or `SystemResourcesInitialized` +- **THEN** no boolean declarations or checks SHALL be found; only `INIT_ONCE` variables SHALL control initialization state diff --git a/openspec/changes/fix-global-state-thread-safety/tasks.md b/openspec/changes/fix-global-state-thread-safety/tasks.md new file mode 100644 index 0000000..f96dd02 --- /dev/null +++ b/openspec/changes/fix-global-state-thread-safety/tasks.md @@ -0,0 +1,10 @@ +## 1. KernelHeap thread-safe init + +- [x] 1.1 Fix `GetGlobalState()` in `src/KernelHeap.c`: allocate and initialize `GLOBAL_STATE` into a local `temp`, then use `InterlockedCompareExchangePointer` to atomically set `g_WinKernelLite_GlobalState`. If CAS fails (another thread won), free `temp` and return the winner's pointer. +- [x] 1.2 Build and run all tests to verify KernelHeap fix + +## 2. Resource thread-safe init + +- [x] 2.1 In `src/Resource.c` and `include/Resource.h`: replace `g_WinKernelLite_SystemResourcesInitialized` (BOOLEAN) with a static `INIT_ONCE` variable. Rewrite `EnsureSystemResourcesListInitialized()` to use `InitOnceExecuteOnce` with a callback that calls `InitializeCriticalSection` and `InitializeListHead`. +- [x] 2.2 In `src/Resource.c` and `include/Resource.h`: replace `g_WinKernelLite_KernelApcDisableLockInitialized` (BOOLEAN) with a static `INIT_ONCE` variable. Rewrite `KeEnterCriticalRegion()`, `KeLeaveCriticalRegion()`, and `GetKernelApcDisableCount()` to use `InitOnceExecuteOnce` with a callback that calls `InitializeCriticalSection`. Remove the inline init checks from all three functions. +- [x] 2.3 Build and run all tests to verify Resource fixes diff --git a/src/KernelHeap.c b/src/KernelHeap.c index 7e70635..e89f2d7 100644 --- a/src/KernelHeap.c +++ b/src/KernelHeap.c @@ -21,34 +21,44 @@ GLOBAL_STATE* g_WinKernelLite_GlobalState = NULL; GLOBAL_STATE* GetGlobalState(void) { - if (g_WinKernelLite_GlobalState == NULL) { - HEAP_TRACE("GetGlobalState: Creating new global state"); - GLOBAL_STATE* temp = (GLOBAL_STATE*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(GLOBAL_STATE)); - if (temp != NULL) { - HEAP_VERBOSE("GetGlobalState: Allocated global state at %p (size: %zu)", temp, sizeof(GLOBAL_STATE)); - /* CriticalSection initialization removed for performance */ - temp->HeapHandle = GetProcessHeap(); - HEAP_VERBOSE("GetGlobalState: Using heap handle %p", temp->HeapHandle); - /* Initialize the linked lists immediately when creating global state */ - InitializeListHead(&temp->MemoryAllocations); - InitializeListHead(&temp->FreedMemoryList); - /* Initialize other fields to safe defaults */ - temp->AllocationCount = 0; - temp->TotalBytesAllocated = 0; - temp->CurrentBytesAllocated = 0; - temp->PeakBytesAllocated = 0; - temp->DoubleFreeCount = 0; - temp->FreedEntryCount = 0; - temp->MaxFreedEntries = 1000; /* Default: keep track of last 1000 freed allocations */ - temp->NextAllocationId = 1; /* Start allocation IDs at 1 */ - temp->SuppressErrors = FALSE; - temp->TrackFreedMemory = TRUE; /* Enable double-free tracking by default */ - g_WinKernelLite_GlobalState = temp; - HEAP_INFO("GetGlobalState: Global state initialized successfully"); - } else { - HEAP_ERROR("GetGlobalState: Failed to allocate global state"); - } + GLOBAL_STATE* state = g_WinKernelLite_GlobalState; + if (state != NULL) { + return state; /* Fast path - already initialized */ + } + + HEAP_TRACE("GetGlobalState: Creating new global state"); + GLOBAL_STATE* temp = (GLOBAL_STATE*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(GLOBAL_STATE)); + if (temp == NULL) { + HEAP_ERROR("GetGlobalState: Failed to allocate global state"); + return NULL; } + + HEAP_VERBOSE("GetGlobalState: Allocated global state at %p (size: %zu)", temp, sizeof(GLOBAL_STATE)); + temp->HeapHandle = GetProcessHeap(); + HEAP_VERBOSE("GetGlobalState: Using heap handle %p", temp->HeapHandle); + InitializeListHead(&temp->MemoryAllocations); + InitializeListHead(&temp->FreedMemoryList); + temp->AllocationCount = 0; + temp->TotalBytesAllocated = 0; + temp->CurrentBytesAllocated = 0; + temp->PeakBytesAllocated = 0; + temp->DoubleFreeCount = 0; + temp->FreedEntryCount = 0; + temp->MaxFreedEntries = 1000; + temp->NextAllocationId = 1; + temp->SuppressErrors = FALSE; + temp->TrackFreedMemory = TRUE; + + /* Atomically publish: if another thread won the race, free our copy */ + if (InterlockedCompareExchangePointer( + (PVOID*)&g_WinKernelLite_GlobalState, temp, NULL) != NULL) { + /* Another thread initialized first - free our allocation */ + HEAP_TRACE("GetGlobalState: Lost race, freeing duplicate allocation at %p", temp); + HeapFree(GetProcessHeap(), 0, temp); + } else { + HEAP_INFO("GetGlobalState: Global state initialized successfully"); + } + return g_WinKernelLite_GlobalState; } diff --git a/src/Resource.c b/src/Resource.c index e4ce9ee..d90e3b6 100644 --- a/src/Resource.c +++ b/src/Resource.c @@ -21,35 +21,47 @@ /* Critical region counter (protected by its own critical section) */ LONG g_WinKernelLite_KernelApcDisableCount = 0; CRITICAL_SECTION g_WinKernelLite_KernelApcDisableLock; -BOOLEAN g_WinKernelLite_KernelApcDisableLockInitialized = FALSE; +INIT_ONCE g_WinKernelLite_KernelApcDisableInitOnce = INIT_ONCE_STATIC_INIT; /* Global system resources list */ LIST_ENTRY g_WinKernelLite_SystemResourcesList; CRITICAL_SECTION g_WinKernelLite_SystemResourcesLock; -BOOLEAN g_WinKernelLite_SystemResourcesInitialized = FALSE; +INIT_ONCE g_WinKernelLite_SystemResourcesInitOnce = INIT_ONCE_STATIC_INIT; + +static BOOL CALLBACK InitSystemResourcesCallback(PINIT_ONCE InitOnce, PVOID Parameter, PVOID* Context) +{ + UNREFERENCED_PARAMETER(InitOnce); + UNREFERENCED_PARAMETER(Parameter); + UNREFERENCED_PARAMETER(Context); + InitializeCriticalSection(&g_WinKernelLite_SystemResourcesLock); + InitializeListHead(&g_WinKernelLite_SystemResourcesList); + return TRUE; +} + +static BOOL CALLBACK InitKernelApcDisableLockCallback(PINIT_ONCE InitOnce, PVOID Parameter, PVOID* Context) +{ + UNREFERENCED_PARAMETER(InitOnce); + UNREFERENCED_PARAMETER(Parameter); + UNREFERENCED_PARAMETER(Context); + InitializeCriticalSection(&g_WinKernelLite_KernelApcDisableLock); + return TRUE; +} void EnsureSystemResourcesListInitialized(void) { - if (!g_WinKernelLite_SystemResourcesInitialized) { - InitializeCriticalSection(&g_WinKernelLite_SystemResourcesLock); - InitializeListHead(&g_WinKernelLite_SystemResourcesList); - g_WinKernelLite_SystemResourcesInitialized = TRUE; - } + InitOnceExecuteOnce(&g_WinKernelLite_SystemResourcesInitOnce, + InitSystemResourcesCallback, NULL, NULL); } void CleanupGlobalResources(void) { - /* Cleanup APC disable lock if initialized */ - if (g_WinKernelLite_KernelApcDisableLockInitialized) { - DeleteCriticalSection(&g_WinKernelLite_KernelApcDisableLock); - g_WinKernelLite_KernelApcDisableLockInitialized = FALSE; - } + /* Cleanup APC disable lock */ + DeleteCriticalSection(&g_WinKernelLite_KernelApcDisableLock); + g_WinKernelLite_KernelApcDisableInitOnce = (INIT_ONCE)INIT_ONCE_STATIC_INIT; - /* Cleanup system resources lock if initialized */ - if (g_WinKernelLite_SystemResourcesInitialized) { - DeleteCriticalSection(&g_WinKernelLite_SystemResourcesLock); - g_WinKernelLite_SystemResourcesInitialized = FALSE; - } + /* Cleanup system resources lock */ + DeleteCriticalSection(&g_WinKernelLite_SystemResourcesLock); + g_WinKernelLite_SystemResourcesInitOnce = (INIT_ONCE)INIT_ONCE_STATIC_INIT; } NTSTATUS @@ -249,19 +261,18 @@ ExReleaseResourceLite( LeaveCriticalSection(&Resource->CriticalSection); } +static void EnsureKernelApcDisableLockInitialized(void) +{ + InitOnceExecuteOnce(&g_WinKernelLite_KernelApcDisableInitOnce, + InitKernelApcDisableLockCallback, NULL, NULL); +} + VOID KeEnterCriticalRegion( VOID ) { - /* Initialize the critical section if needed */ - if (!g_WinKernelLite_KernelApcDisableLockInitialized) { - InitializeCriticalSection(&g_WinKernelLite_KernelApcDisableLock); - g_WinKernelLite_KernelApcDisableLockInitialized = TRUE; - } - - /* In kernel mode, this disables normal kernel APCs */ - /* In our user-mode implementation, we'll use a critical section for thread safety */ + EnsureKernelApcDisableLockInitialized(); EnterCriticalSection(&g_WinKernelLite_KernelApcDisableLock); InterlockedIncrement(&g_WinKernelLite_KernelApcDisableCount); LeaveCriticalSection(&g_WinKernelLite_KernelApcDisableLock); @@ -272,13 +283,7 @@ KeLeaveCriticalRegion( VOID ) { - /* Initialize the critical section if needed (safety check) */ - if (!g_WinKernelLite_KernelApcDisableLockInitialized) { - InitializeCriticalSection(&g_WinKernelLite_KernelApcDisableLock); - g_WinKernelLite_KernelApcDisableLockInitialized = TRUE; - } - /* Re-enables normal kernel APCs */ - /* In our user-mode implementation, we'll use a critical section for thread safety */ + EnsureKernelApcDisableLockInitialized(); EnterCriticalSection(&g_WinKernelLite_KernelApcDisableLock); InterlockedDecrement(&g_WinKernelLite_KernelApcDisableCount); LeaveCriticalSection(&g_WinKernelLite_KernelApcDisableLock); @@ -288,12 +293,7 @@ LONG GetKernelApcDisableCount(void) { LONG currentValue; - /* Initialize the critical section if needed */ - if (!g_WinKernelLite_KernelApcDisableLockInitialized) { - InitializeCriticalSection(&g_WinKernelLite_KernelApcDisableLock); - g_WinKernelLite_KernelApcDisableLockInitialized = TRUE; - } - + EnsureKernelApcDisableLockInitialized(); EnterCriticalSection(&g_WinKernelLite_KernelApcDisableLock); currentValue = g_WinKernelLite_KernelApcDisableCount; LeaveCriticalSection(&g_WinKernelLite_KernelApcDisableLock);