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

@@ -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

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);
     }
 

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 */
+        /* We should have a valid protection here */
-        ASSERT((ProtectionCode & MM_DECOMMIT) == 0);
         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,

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;