https://git.reactos.org/?p=reactos.git;a=commitdiff;h=75125228be048a899bb9d0...
commit 75125228be048a899bb9d03efb2a55c8c3a8b60e Author: Jérôme Gardou jerome.gardou@reactos.org AuthorDate: Fri Sep 9 19:17:14 2022 +0200 Commit: Jérôme Gardou zefklop@users.noreply.github.com CommitDate: Sat Sep 17 13:48:56 2022 +0200
[NTOS] Add some sanity checks when synchronizing PDEs --- ntoskrnl/include/internal/mm.h | 12 ++--- ntoskrnl/mm/ARM3/miarm.h | 18 +++++-- ntoskrnl/mm/i386/page.c | 117 +++++++++++++++++++++-------------------- ntoskrnl/mm/marea.c | 7 ++- ntoskrnl/mm/rmap.c | 18 +++++-- ntoskrnl/mm/section.c | 14 ++++- 6 files changed, 112 insertions(+), 74 deletions(-)
diff --git a/ntoskrnl/include/internal/mm.h b/ntoskrnl/include/internal/mm.h index af281b26ff3..c7bf663d1c8 100644 --- a/ntoskrnl/include/internal/mm.h +++ b/ntoskrnl/include/internal/mm.h @@ -1288,13 +1288,13 @@ NTSTATUS NTAPI MmGetExecuteOptions(IN PULONG ExecuteOptions);
-VOID -NTAPI +_Success_(return) +BOOLEAN MmDeleteVirtualMapping( - struct _EPROCESS *Process, - PVOID Address, - BOOLEAN* WasDirty, - PPFN_NUMBER Page + _In_opt_ PEPROCESS Process, + _In_ PVOID Address, + _Out_opt_ BOOLEAN* WasDirty, + _Out_opt_ PPFN_NUMBER Page );
/* arch/procsup.c ************************************************************/ diff --git a/ntoskrnl/mm/ARM3/miarm.h b/ntoskrnl/mm/ARM3/miarm.h index b5fd1de3409..60197cda74e 100644 --- a/ntoskrnl/mm/ARM3/miarm.h +++ b/ntoskrnl/mm/ARM3/miarm.h @@ -2428,21 +2428,29 @@ FORCEINLINE BOOLEAN MiSynchronizeSystemPde(PMMPDE PointerPde) { - MMPDE SystemPde; ULONG Index;
/* Get the Index from the PDE */ Index = ((ULONG_PTR)PointerPde & (SYSTEM_PD_SIZE - 1)) / sizeof(MMPTE); + if (PointerPde->u.Hard.Valid != 0) + { + NT_ASSERT(PointerPde->u.Long == MmSystemPagePtes[Index].u.Long); + return TRUE; + } + + if (MmSystemPagePtes[Index].u.Hard.Valid == 0) + { + return FALSE; + }
/* Copy the PDE from the double-mapped system page directory */ - SystemPde = MmSystemPagePtes[Index]; - *PointerPde = SystemPde; + MI_WRITE_VALID_PDE(PointerPde, MmSystemPagePtes[Index]);
/* Make sure we re-read the PDE and PTE */ KeMemoryBarrierWithoutFence();
- /* Return, if we had success */ - return SystemPde.u.Hard.Valid != 0; + /* Return success */ + return TRUE; } #endif
diff --git a/ntoskrnl/mm/i386/page.c b/ntoskrnl/mm/i386/page.c index a9ba2eb1179..791aeb93490 100644 --- a/ntoskrnl/mm/i386/page.c +++ b/ntoskrnl/mm/i386/page.c @@ -119,7 +119,12 @@ BOOLEAN MiIsPageTablePresent(PVOID Address) { #if _MI_PAGING_LEVELS == 2 - return MmWorkingSetList->UsedPageTableEntries[MiGetPdeOffset(Address)] != 0; + BOOLEAN Ret = MmWorkingSetList->UsedPageTableEntries[MiGetPdeOffset(Address)] != 0; + + /* Some sanity check while we're here */ + ASSERT(Ret == (MiAddressToPde(Address)->u.Hard.Valid != 0)); + + return Ret; #else PMMPDE PointerPde; PMMPPE PointerPpe; @@ -217,16 +222,29 @@ MmGetPfnForProcess(PEPROCESS Process, return Page; }
-VOID -NTAPI -MmDeleteVirtualMapping(PEPROCESS Process, PVOID Address, - BOOLEAN* WasDirty, PPFN_NUMBER Page) -/* - * FUNCTION: Delete a virtual mapping +/** + * @brief Deletes the virtual mapping and optionally gives back the page & dirty bit. + * + * @param Process - The process this address belongs to, or NULL if system address. + * @param Address - The address to unmap. + * @param WasDirty - Optional param receiving the dirty bit of the PTE. + * @param Page - Optional param receiving the page number previously mapped to this address. + * + * @return Whether there was actually a page mapped at the given address. */ +_Success_(return) +BOOLEAN +MmDeleteVirtualMapping( + _In_opt_ PEPROCESS Process, + _In_ PVOID Address, + _Out_opt_ BOOLEAN* WasDirty, + _Out_opt_ PPFN_NUMBER Page) { PMMPTE PointerPte; MMPTE OldPte; + BOOLEAN ValidPde; + + OldPte.u.Long = 0;
DPRINT("MmDeleteVirtualMapping(%p, %p, %p, %p)\n", Process, Address, WasDirty, Page);
@@ -244,18 +262,10 @@ MmDeleteVirtualMapping(PEPROCESS Process, PVOID Address, KeBugCheck(MEMORY_MANAGEMENT); } #if (_MI_PAGING_LEVELS == 2) - if (!MiSynchronizeSystemPde(MiAddressToPde(Address))) + ValidPde = MiSynchronizeSystemPde(MiAddressToPde(Address)); #else - if (!MiIsPdeForAddressValid(Address)) + ValidPde = MiIsPdeForAddressValid(Address); #endif - { - /* There can't be a page if there is no PDE */ - if (WasDirty) - *WasDirty = FALSE; - if (Page) - *Page = 0; - return; - } } else { @@ -266,61 +276,56 @@ MmDeleteVirtualMapping(PEPROCESS Process, PVOID Address, }
/* Only for current process !!! */ - ASSERT(Process = PsGetCurrentProcess()); + ASSERT(Process == PsGetCurrentProcess()); MiLockProcessWorkingSetUnsafe(Process, PsGetCurrentThread());
- /* No PDE --> No page */ - if (!MiIsPageTablePresent(Address)) + ValidPde = MiIsPageTablePresent(Address); + if (ValidPde) { - MiUnlockProcessWorkingSetUnsafe(Process, PsGetCurrentThread()); - if (WasDirty) - *WasDirty = 0; - if (Page) - *Page = 0; - return; + MiMakePdeExistAndMakeValid(MiAddressToPde(Address), Process, MM_NOIRQL); } - - MiMakePdeExistAndMakeValid(MiAddressToPde(Address), Process, MM_NOIRQL); }
- PointerPte = MiAddressToPte(Address); - OldPte.u.Long = InterlockedExchangePte(PointerPte, 0); - - if (OldPte.u.Long == 0) + /* Get the PTE if we're having anything */ + if (ValidPde) { - /* There was nothing here */ - if (Address < MmSystemRangeStart) - MiUnlockProcessWorkingSetUnsafe(Process, PsGetCurrentThread()); - if (WasDirty) - *WasDirty = 0; - if (Page) - *Page = 0; - return; - } + PointerPte = MiAddressToPte(Address); + OldPte.u.Long = InterlockedExchangePte(PointerPte, 0);
- /* It must have been present, or not a swap entry */ - ASSERT(OldPte.u.Hard.Valid || !FlagOn(OldPte.u.Long, 0x800)); - - if (OldPte.u.Hard.Valid) KeInvalidateTlbEntry(Address);
- if (Address < MmSystemRangeStart) + if (OldPte.u.Long != 0) + { + /* It must have been present, or not a swap entry */ + ASSERT(OldPte.u.Hard.Valid || !FlagOn(OldPte.u.Long, 0x800)); + if (WasDirty != NULL) + { + *WasDirty = !!OldPte.u.Hard.Dirty; + } + if (Page != NULL) + { + *Page = OldPte.u.Hard.PageFrameNumber; + } + } + } + + if (Process != NULL) { - /* Remove PDE reference */ - if (MiDecrementPageTableReferences(Address) == 0) + /* Remove PDE reference, if needed */ + if (OldPte.u.Long != 0) { - KIRQL OldIrql = MiAcquirePfnLock(); - MiDeletePde(MiAddressToPde(Address), Process); - MiReleasePfnLock(OldIrql); + if (MiDecrementPageTableReferences(Address) == 0) + { + KIRQL OldIrql = MiAcquirePfnLock(); + MiDeletePde(MiAddressToPde(Address), Process); + MiReleasePfnLock(OldIrql); + } }
MiUnlockProcessWorkingSetUnsafe(Process, PsGetCurrentThread()); }
- if (WasDirty) - *WasDirty = !!OldPte.u.Hard.Dirty; - if (Page) - *Page = OldPte.u.Hard.PageFrameNumber; + return OldPte.u.Long != 0; }
@@ -631,7 +636,7 @@ MmCreateVirtualMappingUnsafe(PEPROCESS Process, }
/* Only for current process !!! */ - ASSERT(Process = PsGetCurrentProcess()); + ASSERT(Process == PsGetCurrentProcess()); MiLockProcessWorkingSetUnsafe(Process, PsGetCurrentThread());
MiMakePdeExistAndMakeValid(MiAddressToPde(Address), Process, MM_NOIRQL); diff --git a/ntoskrnl/mm/marea.c b/ntoskrnl/mm/marea.c index 78d07c75cd6..8bf8ab72630 100644 --- a/ntoskrnl/mm/marea.c +++ b/ntoskrnl/mm/marea.c @@ -314,16 +314,19 @@ MmFreeMemoryArea( BOOLEAN Dirty = FALSE; SWAPENTRY SwapEntry = 0; PFN_NUMBER Page = 0; + BOOLEAN DoFree;
if (MmIsPageSwapEntry(Process, (PVOID)Address)) { MmDeletePageFileMapping(Process, (PVOID)Address, &SwapEntry); + /* We'll have to do some cleanup when we're on the page file */ + DoFree = TRUE; } else { - MmDeleteVirtualMapping(Process, (PVOID)Address, &Dirty, &Page); + DoFree = MmDeleteVirtualMapping(Process, (PVOID)Address, &Dirty, &Page); } - if (FreePage != NULL) + if (DoFree && (FreePage != NULL)) { FreePage(FreePageContext, MemoryArea, (PVOID)Address, Page, SwapEntry, (BOOLEAN)Dirty); diff --git a/ntoskrnl/mm/rmap.c b/ntoskrnl/mm/rmap.c index e27e31de820..9c2f00eff22 100644 --- a/ntoskrnl/mm/rmap.c +++ b/ntoskrnl/mm/rmap.c @@ -135,6 +135,7 @@ GetEntry: PFN_NUMBER MapPage; LARGE_INTEGER Offset; BOOLEAN Released; + BOOLEAN Unmapped;
Offset.QuadPart = MemoryArea->SectionData.ViewOffset + ((ULONG_PTR)Address - MA_GetStartingAddress(MemoryArea)); @@ -158,10 +159,19 @@ GetEntry:
/* Delete this virtual mapping in the process */ MmDeleteRmap(Page, Process, Address); - MmDeleteVirtualMapping(Process, Address, &Dirty, &MapPage); - - /* We checked this earlier */ - ASSERT(MapPage == Page); + Unmapped = MmDeleteVirtualMapping(Process, Address, &Dirty, &MapPage); + if (!Unmapped || (MapPage != Page)) + { + /* + * Something's corrupted, we got there because this process is + * supposed to be mapping this page there. + */ + KeBugCheckEx(MEMORY_MANAGEMENT, + (ULONG_PTR)Process, + (ULONG_PTR)Address, + (ULONG_PTR)__FILE__, + __LINE__); + }
if (Page != PFN_FROM_SSE(Entry)) { diff --git a/ntoskrnl/mm/section.c b/ntoskrnl/mm/section.c index 44b5710f6f4..0ab7da4a648 100644 --- a/ntoskrnl/mm/section.c +++ b/ntoskrnl/mm/section.c @@ -1897,6 +1897,7 @@ MmAccessFaultSectionView(PMMSUPPORT AddressSpace, PMM_SECTION_SEGMENT Segment; PFN_NUMBER OldPage; PFN_NUMBER NewPage; + PFN_NUMBER UnmappedPage; PVOID PAddress; LARGE_INTEGER Offset; PMM_REGION Region; @@ -1904,6 +1905,7 @@ MmAccessFaultSectionView(PMMSUPPORT AddressSpace, PEPROCESS Process = MmGetAddressSpaceOwner(AddressSpace); BOOLEAN Cow = FALSE; ULONG NewProtect; + BOOLEAN Unmapped;
DPRINT("MmAccessFaultSectionView(%p, %p, %p)\n", AddressSpace, MemoryArea, Address);
@@ -2003,7 +2005,17 @@ MmAccessFaultSectionView(PMMSUPPORT AddressSpace, * Unshare the old page. */ DPRINT("Swapping page (Old %x New %x)\n", OldPage, NewPage); - MmDeleteVirtualMapping(Process, PAddress, NULL, NULL); + Unmapped = MmDeleteVirtualMapping(Process, PAddress, NULL, &UnmappedPage); + if (!Unmapped || (UnmappedPage != OldPage)) + { + /* Uh , we had a page just before, but suddenly it changes. Someone corrupted us. */ + KeBugCheckEx(MEMORY_MANAGEMENT, + (ULONG_PTR)Process, + (ULONG_PTR)PAddress, + (ULONG_PTR)__FILE__, + __LINE__); + } + if (Process) MmDeleteRmap(OldPage, Process, PAddress); MmUnsharePageEntrySectionSegment(MemoryArea, Segment, &Offset, FALSE, FALSE, NULL);
-
Jérôme Gardou