From 1bc59379598fd5fad8ef85120f48da3cd5339b8b Mon Sep 17 00:00:00 2001 From: Alex Ionescu Date: Thu, 29 Aug 2013 07:33:10 +0000 Subject: [PATCH] CORE-6639 #resolve #time 1d #comment Guard pages now work ;-) [NDK]: Fix definition of a global flag. [RTL]: RtlpCreateUserStack: 1) If the image is invalid, bail out. 2) If the stack commit is higher than reserve, adjust reserve. 3) After allocating a guard page, the stack limit is now ABOVE the guard page, not BELOW it (stack grows backward!). [RTL]: Remove the hack which always Commited the "StackReserve" value. Threads now have a 4-64KB stack, instead of a 1MB stack. [NTOSKRNL]: Implement MiAccessCheck and MiCheckForUserStackOverflow, which handle guard page + stack expansion. [USER32]: Because threads now correctly run with a smaller stack than usual (and expand as needed), remove some checks in user-mode callbacks which forced larger stacks. svn path=/trunk/; revision=59868 --- reactos/include/ndk/pstypes.h | 2 +- reactos/lib/rtl/thread.c | 15 +- reactos/ntoskrnl/mm/ARM3/pagfault.c | 220 +++++++++++++++++- reactos/win32ss/user/user32/windows/message.c | 17 -- 4 files changed, 223 insertions(+), 31 deletions(-) diff --git a/reactos/include/ndk/pstypes.h b/reactos/include/ndk/pstypes.h index caeb127e080..ebe43c07b96 100644 --- a/reactos/include/ndk/pstypes.h +++ b/reactos/include/ndk/pstypes.h @@ -69,7 +69,7 @@ extern POBJECT_TYPE NTSYSAPI PsJobType; #define FLG_KERNEL_STACK_TRACE_DB 0x00002000 #define FLG_MAINTAIN_OBJECT_TYPELIST 0x00004000 #define FLG_HEAP_ENABLE_TAG_BY_DLL 0x00008000 -#define FLG_IGNORE_DEBUG_PRIV 0x00010000 +#define FLG_DISABLE_STACK_EXTENSION 0x00010000 #define FLG_ENABLE_CSRDEBUG 0x00020000 #define FLG_ENABLE_KDEBUG_SYMBOL_LOAD 0x00040000 #define FLG_DISABLE_PAGE_KERNEL_STACKS 0x00080000 diff --git a/reactos/lib/rtl/thread.c b/reactos/lib/rtl/thread.c index 717cdb2e45c..93e6fe09953 100644 --- a/reactos/lib/rtl/thread.c +++ b/reactos/lib/rtl/thread.c @@ -46,6 +46,7 @@ RtlpCreateUserStack(IN HANDLE hProcess, { /* Get the Image Headers */ Headers = RtlImageNtHeader(NtCurrentPeb()->ImageBaseAddress); + if (!Headers) return STATUS_INVALID_IMAGE_FORMAT; /* If we didn't get the parameters, find them ourselves */ if (!StackReserve) StackReserve = Headers->OptionalHeader. @@ -60,15 +61,17 @@ RtlpCreateUserStack(IN HANDLE hProcess, if (!StackCommit) StackCommit = SystemBasicInfo.PageSize; } + /* Check if the commit is higher than the reserve*/ + if (StackCommit >= StackReserve) + { + /* Grow the reserve beyond the commit, up to 1MB alignment */ + StackReserve = ROUND_UP(StackCommit, 1024 * 1024); + } + /* Align everything to Page Size */ StackReserve = ROUND_UP(StackReserve, SystemBasicInfo.AllocationGranularity); StackCommit = ROUND_UP(StackCommit, SystemBasicInfo.PageSize); - // FIXME: Remove once Guard Page support is here - #if 1 - StackCommit = StackReserve; - #endif - /* Reserve memory for the stack */ Status = ZwAllocateVirtualMemory(hProcess, (PVOID*)&Stack, @@ -121,7 +124,7 @@ RtlpCreateUserStack(IN HANDLE hProcess, if (!NT_SUCCESS(Status)) return Status; /* Update the Stack Limit keeping in mind the Guard Page */ - InitialTeb->StackLimit = (PVOID)((ULONG_PTR)InitialTeb->StackLimit - + InitialTeb->StackLimit = (PVOID)((ULONG_PTR)InitialTeb->StackLimit + GuardPageSize); } diff --git a/reactos/ntoskrnl/mm/ARM3/pagfault.c b/reactos/ntoskrnl/mm/ARM3/pagfault.c index d217616b10c..3a5e9484789 100644 --- a/reactos/ntoskrnl/mm/ARM3/pagfault.c +++ b/reactos/ntoskrnl/mm/ARM3/pagfault.c @@ -24,6 +24,162 @@ BOOLEAN UserPdeFault = FALSE; /* PRIVATE FUNCTIONS **********************************************************/ +NTSTATUS +NTAPI +MiCheckForUserStackOverflow(IN PVOID Address, + IN PVOID TrapInformation) +{ + PETHREAD CurrentThread = PsGetCurrentThread(); + PTEB Teb = CurrentThread->Tcb.Teb; + PVOID StackBase, DeallocationStack, NextStackAddress; + ULONG GuranteedSize; + NTSTATUS Status; + + /* Do we own the address space lock? */ + if (CurrentThread->AddressSpaceOwner == 1) + { + /* This isn't valid */ + DPRINT1("Process owns address space lock\n"); + ASSERT(KeAreAllApcsDisabled() == TRUE); + return STATUS_GUARD_PAGE_VIOLATION; + } + + /* Are we attached? */ + if (KeIsAttachedProcess()) + { + /* This isn't valid */ + DPRINT1("Process is attached\n"); + return STATUS_GUARD_PAGE_VIOLATION; + } + + /* Read the current settings */ + StackBase = Teb->NtTib.StackBase; + DeallocationStack = Teb->DeallocationStack; + GuranteedSize = Teb->GuaranteedStackBytes; + DPRINT1("Handling guard page fault with Stacks Addresses 0x%p and 0x%p, guarantee: %lx\n", + StackBase, DeallocationStack, GuranteedSize); + + /* Guarantees make this code harder, for now, assume there aren't any */ + ASSERT(GuranteedSize == 0); + + /* So allocate only the minimum guard page size */ + GuranteedSize = PAGE_SIZE; + + /* Does this faulting stack address actually exist in the stack? */ + if ((Address >= StackBase) || (Address < DeallocationStack)) + { + /* That's odd... */ + DPRINT1("Faulting address outside of stack bounds\n"); + return STATUS_GUARD_PAGE_VIOLATION; + } + + /* This is where the stack will start now */ + NextStackAddress = (PVOID)((ULONG_PTR)PAGE_ALIGN(Address) - GuranteedSize); + + /* Do we have at least one page between here and the end of the stack? */ + if (((ULONG_PTR)NextStackAddress - PAGE_SIZE) <= (ULONG_PTR)DeallocationStack) + { + /* We don't -- Windows would try to make this guard page valid now */ + DPRINT1("Close to our death...\n"); + ASSERT(FALSE); + return STATUS_STACK_OVERFLOW; + } + + /* Don't handle this flag yet */ + ASSERT((PsGetCurrentProcess()->Peb->NtGlobalFlag & FLG_DISABLE_STACK_EXTENSION) == 0); + + /* Update the stack limit */ + Teb->NtTib.StackLimit = (PVOID)((ULONG_PTR)NextStackAddress + GuranteedSize); + + /* Now move the guard page to the next page */ + Status = ZwAllocateVirtualMemory(NtCurrentProcess(), + &NextStackAddress, + 0, + &GuranteedSize, + MEM_COMMIT, + PAGE_READWRITE | PAGE_GUARD); + if ((NT_SUCCESS(Status) || (Status == STATUS_ALREADY_COMMITTED))) + { + /* We did it! */ + DPRINT1("Guard page handled successfully for %p\n", Address); + return STATUS_PAGE_FAULT_GUARD_PAGE; + } + + /* Fail, we couldn't move the guard page */ + DPRINT1("Guard page failure: %lx\n", Status); + ASSERT(FALSE); + return STATUS_STACK_OVERFLOW; +} + +NTSTATUS +NTAPI +MiAccessCheck(IN PMMPTE PointerPte, + IN BOOLEAN StoreInstruction, + IN KPROCESSOR_MODE PreviousMode, + IN ULONG ProtectionCode, + IN PVOID TrapFrame, + IN BOOLEAN LockHeld) +{ + MMPTE TempPte; + + /* Check for invalid user-mode access */ + if ((PreviousMode == UserMode) && (PointerPte > MiHighestUserPte)) + { + return STATUS_ACCESS_VIOLATION; + } + + /* Capture the PTE -- is it valid? */ + TempPte = *PointerPte; + if (TempPte.u.Hard.Valid) + { + /* Was someone trying to write to it? */ + if (StoreInstruction) + { + /* Is it writable?*/ + if ((TempPte.u.Hard.Write) || (TempPte.u.Hard.CopyOnWrite)) + { + /* Then there's nothing to worry about */ + return STATUS_SUCCESS; + } + + /* Oops! This isn't allowed */ + return STATUS_ACCESS_VIOLATION; + } + + /* Someone was trying to read from a valid PTE, that's fine too */ + return STATUS_SUCCESS; + } + + /* Convert any fault flag to 1 only */ + if (StoreInstruction) StoreInstruction = 1; + +#if 0 + /* Check if the protection on the page allows what is being attempted */ + if ((MmReadWrite[Protection] - StoreInstruction) < 10) + { + return STATUS_ACCESS_VIOLATION; + } +#endif + + /* Check if this is a guard page */ + if (ProtectionCode & MM_DECOMMIT) + { + /* Attached processes can't expand their stack */ + if (KeIsAttachedProcess()) return STATUS_ACCESS_VIOLATION; + + /* No support for transition PTEs yet */ + ASSERT(((TempPte.u.Soft.Transition == 1) && + (TempPte.u.Soft.Prototype == 0)) == FALSE); + + /* Remove the guard page bit, and return a guard page violation */ + PointerPte->u.Soft.Protection = ProtectionCode & ~MM_DECOMMIT; + return STATUS_GUARD_PAGE_VIOLATION; + } + + /* Nothing to do */ + return STATUS_SUCCESS; +} + PMMPTE NTAPI MiCheckVirtualAddress(IN PVOID VirtualAddress, @@ -800,7 +956,14 @@ MiResolveProtoPteFault(IN BOOLEAN StoreInstruction, { if (!PteContents.u.Proto.ReadOnly) { - /* FIXME: CHECK FOR ACCESS */ + /* Check for page acess in software */ + Status = MiAccessCheck(PointerProtoPte, + StoreInstruction, + KernelMode, + TempPte.u.Soft.Protection, + TrapInformation, + TRUE); + ASSERT(Status == STATUS_SUCCESS); /* Check for copy on write page */ if ((TempPte.u.Soft.Protection & MM_WRITECOPY) == MM_WRITECOPY) @@ -1682,9 +1845,6 @@ UserFault: return Status; } - /* No guard page support yet */ - ASSERT((ProtectionCode & MM_DECOMMIT) == 0); - /* * Check if this is a real user-mode address or actually a kernel-mode * page table for a user mode address @@ -1695,6 +1855,24 @@ UserFault: MiIncrementPageTableReferences(Address); } + /* Is this a guard page? */ + if (ProtectionCode & MM_DECOMMIT) + { + /* Remove the bit */ + PointerPte->u.Soft.Protection = ProtectionCode & ~MM_DECOMMIT; + + /* Not supported */ + ASSERT(ProtoPte == NULL); + ASSERT(CurrentThread->ApcNeeded == 0); + + /* Drop the working set lock */ + MiUnlockProcessWorkingSet(CurrentProcess, CurrentThread); + ASSERT(KeGetCurrentIrql() == OldIrql); + + /* Handle stack expansion */ + return MiCheckForUserStackOverflow(Address, TrapInformation); + } + /* Did we get a prototype PTE back? */ if (!ProtoPte) { @@ -1781,8 +1959,7 @@ UserFault: return STATUS_PAGE_FAULT_DEMAND_ZERO; } - /* No guard page support yet */ - ASSERT((ProtectionCode & MM_DECOMMIT) == 0); + /* We should have a valid protection here */ ASSERT(ProtectionCode != 0x100); /* Write the prototype PTE */ @@ -1831,7 +2008,36 @@ UserFault: } } - /* FIXME: Run MiAccessCheck */ + /* Do we have a valid protection code? */ + if (ProtectionCode != 0x100) + { + /* Run a software access check first, including to detect guard pages */ + Status = MiAccessCheck(PointerPte, + StoreInstruction, + Mode, + ProtectionCode, + TrapInformation, + FALSE); + if (Status != STATUS_SUCCESS) + { + /* Not supported */ + ASSERT(CurrentThread->ApcNeeded == 0); + + /* Drop the working set lock */ + MiUnlockProcessWorkingSet(CurrentProcess, CurrentThread); + ASSERT(KeGetCurrentIrql() == OldIrql); + + /* Did we hit a guard page? */ + if (Status == STATUS_GUARD_PAGE_VIOLATION) + { + /* Handle stack expansion */ + return MiCheckForUserStackOverflow(Address, TrapInformation); + } + + /* Otherwise, fail back to the caller directly */ + return Status; + } + } /* Dispatch the fault */ Status = MiDispatchFault(StoreInstruction, diff --git a/reactos/win32ss/user/user32/windows/message.c b/reactos/win32ss/user/user32/windows/message.c index 89ddb6dd5b6..f7c7ccfe90a 100644 --- a/reactos/win32ss/user/user32/windows/message.c +++ b/reactos/win32ss/user/user32/windows/message.c @@ -1288,7 +1288,6 @@ IntCallWindowProcW(BOOL IsAnsiProc, { MSG AnsiMsg; MSG UnicodeMsg; - ULONG_PTR LowLimit; BOOL Hook = FALSE, MsgOverride = FALSE, Dialog; LRESULT Result = 0, PreResult = 0; DWORD Data = 0; @@ -1299,14 +1298,6 @@ IntCallWindowProcW(BOOL IsAnsiProc, return FALSE; } - // Safeguard against excessive recursions. - LowLimit = (ULONG_PTR)NtCurrentTeb()->NtTib.StackLimit; - if (((ULONG_PTR)&lParam - LowLimit) < PAGE_SIZE ) - { - ERR("IntCallWindowsProcW() Exceeded Stack!\n"); - return FALSE; - } - if (pWnd) Dialog = (pWnd->fnid == FNID_DIALOG); else @@ -2766,7 +2757,6 @@ User32CallWindowProcFromKernel(PVOID Arguments, ULONG ArgumentLength) PWINDOWPROC_CALLBACK_ARGUMENTS CallbackArgs; MSG KMMsg, UMMsg; PWND pWnd = NULL; - ULONG_PTR LowLimit; PCLIENTINFO pci = GetWin32ClientInfo(); /* Make sure we don't try to access mem beyond what we were given */ @@ -2775,13 +2765,6 @@ User32CallWindowProcFromKernel(PVOID Arguments, ULONG ArgumentLength) return STATUS_INFO_LENGTH_MISMATCH; } - LowLimit = (ULONG_PTR)NtCurrentTeb()->NtTib.StackLimit; - if (((ULONG_PTR)&ArgumentLength - LowLimit) < PAGE_SIZE ) - { - ERR("Callback from Win32k Exceeded Stack!\n"); - return STATUS_BAD_STACK; - } - CallbackArgs = (PWINDOWPROC_CALLBACK_ARGUMENTS) Arguments; KMMsg.hwnd = CallbackArgs->Wnd; KMMsg.message = CallbackArgs->Msg; -- 2.17.1