Author: tkreuzer
Date: Mon Nov 25 00:18:33 2013
New Revision: 61095
URL: http://svn.reactos.org/svn/reactos?rev=61095&view=rev
Log:
[NTOSKRNL]
Windows / ReactOS uses a software protection field called protection mask,
which is stored inside invalid (Software) PTEs to provide information about the
desired protection, when a page is made valid by the page fault handler. The
mask consists of the values 0-7 specifying the read/write/execute rights, 0
being inaccessible aka MM_ZERO_ACCESS, plus 2 flag-like bits, for uncached and
writecombine memory respectively. Both flags together don't make sense, so this
combination is used to mark guard pages. Since all these flags only make sense
when used together with a proper access (i.e. not MM_ZERO_ACCESS), the
combination of these flags together with MM_ZERO_ACCESS was given special
meaning: MM_DECOMMIT, which equals MM_GUARDPAGE | MM_ZERO_ACCESS is for
decommitted pages, that are not yet erased to zero, MM_NOACCESS, which is the
mask for pages that are mapped with PAGE_NOACCESS (this is to make sure that a
software PTE of a committed page is never completely 0, which it could be, whe
n MM_ZERO_ACCESS was used), and finally MM_OUTSWAPPED_KSTACK for outswapped
kernel stacks. See also
https://www.reactos.org/wiki/Techwiki:Memory_Protection_constants.
The next thing to know is that the number of PTEs that are not null is counted
for each PDE. So once a page gets committed, a software PTE is written and the
reference count is incremented. When the page is made valid by the fault
handler, the count is not changed, when the page is decommitted, the
MM_DECOMMIT software PTE is written and again the PTE stays non-null and
nothing is changed. Only when the range is cleaned up totally, the PTEs get
erased and the reference count is decremented. Now it happened that our page
fault handler missed to validate the access rights of protection constants. The
problem that came up with this is a major one: since a decommitted page is a
software PTE with MM_DECOMMIT as the protection mask (which we remember has the
MM_GUARDPAGE bit set), the fault handler considered faults on decommitted PTEs
as faults on guard pages and simply removed the guard page flag, leaving a
completely empty PTE behind! So the decommitted page got erased without
decrement
ing the reference count. This lead to CORE-7445.
- Add protection flags (MM_GUARDPAGE, MM_WRITECOMBINE, MM_OUTSWAPPED_KSTACK)
- Instead of writing 0 to a PTE, use MI_WRITE_INVALID_PTE with MmZeroPte
- Implement MiIsAccessAllowed that checks for read/write/execute access and use
it in MiAccessCheck
- Add some more ASSERTs
CORE-7445 #resolve
Modified:
trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
trunk/reactos/ntoskrnl/mm/ARM3/section.c
trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
Modified: trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/miarm.h?rev=61095&r1=61094&r2=61095&view=diff
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/miarm.h [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/miarm.h [iso-8859-1] Mon Nov 25
00:18:33 2013
@@ -96,6 +96,7 @@
//
// Protection Bits part of the internal memory manager Protection Mask, from:
// http://reactos.org/wiki/Techwiki:Memory_management_in_the_Windows_XP_kernel
+// https://www.reactos.org/wiki/Techwiki:Memory_Protection_constants
// and public assertions.
//
#define MM_ZERO_ACCESS 0
@@ -106,9 +107,20 @@
#define MM_WRITECOPY 5
#define MM_EXECUTE_READWRITE 6
#define MM_EXECUTE_WRITECOPY 7
-#define MM_NOCACHE 8
-#define MM_DECOMMIT 0x10
-#define MM_NOACCESS (MM_DECOMMIT | MM_NOCACHE)
+
+//
+// These are flags on top of the actual protection mask
+//
+#define MM_NOCACHE 0x08
+#define MM_GUARDPAGE 0x10
+#define MM_WRITECOMBINE 0x18
+
+//
+// These are special cases
+//
+#define MM_DECOMMIT (MM_ZERO_ACCESS | MM_GUARDPAGE)
+#define MM_NOACCESS (MM_ZERO_ACCESS | MM_WRITECOMBINE)
+#define MM_OUTSWAPPED_KSTACK (MM_EXECUTE_WRITECOPY | MM_WRITECOMBINE)
#define MM_INVALID_PROTECTION 0xFFFFFFFF
//
@@ -576,6 +588,7 @@
LONG ImageLoadingCount;
} MM_SESSION_SPACE, *PMM_SESSION_SPACE;
+static const MMPTE MmZeroPte = {{0}};
extern PMM_SESSION_SPACE MmSessionSpace;
extern MMPTE HyperTemplatePte;
extern MMPDE ValidKernelPde;
Modified: trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c?rev=61095&r1=61094&r2=61095&view=diff
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c [iso-8859-1] Mon Nov 25
00:18:33 2013
@@ -112,12 +112,41 @@
return STATUS_STACK_OVERFLOW;
}
+FORCEINLINE
+BOOLEAN
+MiIsAccessAllowed(
+ _In_ ULONG ProtectionMask,
+ _In_ BOOLEAN Write,
+ _In_ BOOLEAN Execute)
+{
+ #define _BYTE_MASK(Bit0, Bit1, Bit2, Bit3, Bit4, Bit5, Bit6, Bit7) \
+ (Bit0) | ((Bit1) << 1) | ((Bit2) << 2) | ((Bit3) << 3) | \
+ ((Bit4) << 4) | ((Bit5) << 5) | ((Bit6) << 6) | ((Bit7) << 7)
+ static const UCHAR MiAccessAllowedMask[2][2] =
+ {
+ { // Protect 0 1 2 3 4 5 6 7
+ _BYTE_MASK(0, 1, 1, 1, 1, 1, 1, 1), // READ
+ _BYTE_MASK(0, 0, 1, 1, 0, 0, 1, 1), // EXECUTE READ
+ },
+ {
+ _BYTE_MASK(0, 0, 0, 0, 1, 1, 1, 1), // WRITE
+ _BYTE_MASK(0, 0, 0, 0, 0, 0, 1, 1), // EXECUTE WRITE
+ }
+ };
+
+ /* We want only the low 3 bits */
+ ProtectionMask &= 7;
+
+ /* Look it up in the table */
+ return (MiAccessAllowedMask[Write != 0][Execute != 0] >> ProtectionMask) &
1;
+}
+
NTSTATUS
NTAPI
MiAccessCheck(IN PMMPTE PointerPte,
IN BOOLEAN StoreInstruction,
IN KPROCESSOR_MODE PreviousMode,
- IN ULONG_PTR ProtectionCode,
+ IN ULONG_PTR ProtectionMask,
IN PVOID TrapFrame,
IN BOOLEAN LockHeld)
{
@@ -151,20 +180,17 @@
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)
+ if (!MiIsAccessAllowed(ProtectionMask, StoreInstruction, FALSE))
{
return STATUS_ACCESS_VIOLATION;
}
-#endif
/* Check if this is a guard page */
- if (ProtectionCode & MM_DECOMMIT)
- {
+ if (ProtectionMask & MM_GUARDPAGE)
+ {
+ NT_ASSERT(ProtectionMask != MM_DECOMMIT);
+
/* Attached processes can't expand their stack */
if (KeIsAttachedProcess()) return STATUS_ACCESS_VIOLATION;
@@ -173,7 +199,9 @@
(TempPte.u.Soft.Prototype == 0)) == FALSE);
/* Remove the guard page bit, and return a guard page violation */
- PointerPte->u.Soft.Protection = ProtectionCode & ~MM_DECOMMIT;
+ TempPte.u.Soft.Protection = ProtectionMask & ~MM_GUARDPAGE;
+ NT_ASSERT(TempPte.u.Long != 0);
+ MI_WRITE_INVALID_PTE(PointerPte, TempPte);
return STATUS_GUARD_PAGE_VIOLATION;
}
@@ -950,6 +978,9 @@
KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql);
return STATUS_ACCESS_VIOLATION;
}
+
+ /* There is no such thing as a decommitted prototype PTE */
+ NT_ASSERT(TempPte.u.Long != MmDecommittedPte.u.Long);
/* Check for access rights on the PTE proper */
PteContents = *PointerPte;
@@ -1863,10 +1894,14 @@
}
/* Is this a guard page? */
- if (ProtectionCode & MM_DECOMMIT)
- {
+ if (ProtectionCode & MM_GUARDPAGE)
+ {
+ /* The VAD protection cannot be MM_DECOMMIT! */
+ NT_ASSERT(ProtectionCode != MM_DECOMMIT);
+
/* Remove the bit */
- PointerPte->u.Soft.Protection = ProtectionCode & ~MM_DECOMMIT;
+ TempPte.u.Soft.Protection = ProtectionCode & ~MM_GUARDPAGE;
+ MI_WRITE_INVALID_PTE(PointerPte, TempPte);
/* Not supported */
ASSERT(ProtoPte == NULL);
@@ -1892,7 +1927,8 @@
else
{
/* No, create a new PTE. First, write the protection */
- PointerPte->u.Soft.Protection = ProtectionCode;
+ TempPte.u.Soft.Protection = ProtectionCode;
+ MI_WRITE_INVALID_PTE(PointerPte, TempPte);
}
/* Lock the PFN database since we're going to grab a page */
@@ -1972,6 +2008,7 @@
/* Write the prototype PTE */
TempPte = PrototypePte;
TempPte.u.Soft.Protection = ProtectionCode;
+ NT_ASSERT(TempPte.u.Long != 0);
MI_WRITE_INVALID_PTE(PointerPte, TempPte);
}
else
Modified: trunk/reactos/ntoskrnl/mm/ARM3/section.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/section.c?rev=61095&r1=61094&r2=61095&view=diff
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/section.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/section.c [iso-8859-1] Mon Nov 25
00:18:33 2013
@@ -198,7 +198,7 @@
}
/* This actually turns on guard page in this scenario! */
- ProtectMask |= MM_DECOMMIT;
+ ProtectMask |= MM_GUARDPAGE;
}
/* Check for nocache option */
Modified: trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/virtual.c?rev=61095&r1=61094&r2=61095&view=diff
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] Mon Nov 25
00:18:33 2013
@@ -350,7 +350,7 @@
KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql);
/* Destroy the PTE */
- PointerPte->u.Long = 0;
+ MI_WRITE_INVALID_PTE(PointerPte, MmZeroPte);
}
/* Actual legitimate pages */
@@ -368,7 +368,7 @@
ASSERT(PointerPte->u.Soft.PageFileHigh == 0);
/* Destroy the PTE */
- PointerPte->u.Long = 0;
+ MI_WRITE_INVALID_PTE(PointerPte, MmZeroPte);
}
/* Keep going */
@@ -486,7 +486,7 @@
}
/* Destroy the PTE and flush the TLB */
- PointerPte->u.Long = 0;
+ MI_WRITE_INVALID_PTE(PointerPte, MmZeroPte);
KeFlushCurrentTb();
}
@@ -618,7 +618,7 @@
(TempPte.u.Soft.Prototype == 1))
{
/* Just nuke it */
- PointerPte->u.Long = 0;
+ MI_WRITE_INVALID_PTE(PointerPte, MmZeroPte);
}
else
{
@@ -632,7 +632,7 @@
else
{
/* The PTE was never mapped, just nuke it here */
- PointerPte->u.Long = 0;
+ MI_WRITE_INVALID_PTE(PointerPte, MmZeroPte);
}
}
@@ -2113,7 +2113,8 @@
ASSERT(PteContents.u.Soft.Transition == 0);
/* The PTE is already demand-zero, just update the protection
mask */
- PointerPte->u.Soft.Protection = ProtectionMask;
+ PteContents.u.Soft.Protection = ProtectionMask;
+ MI_WRITE_INVALID_PTE(PointerPte, PteContents);
ASSERT(PointerPte->u.Long != 0);
}
