From fe23a4aaeb1e760c7ebff858602910c61f7e9ee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?George=20Bi=C8=99oc?= Date: Sun, 26 May 2024 21:05:37 +0200 Subject: [PATCH] [NTOS:CM] Lock the entire registry down when we unload a hive The PR #6649 which fixed an issue with orphaned KCBs leaking in memory which also pointed to unloaded registry hives, it also brought a problem. In CmpEnumerateOpenSubKeys there is a risk of getting hit by a deadlock as we enumerate the cache table to remove empty cache entries. Fundamentally CmpEnumerateOpenSubKeys locks down a KCB from cache for exclusive use in order to tear down its contents from memory but it doesn't address the fact a KCB might have already been locked in the same calling thread, leading to a recursion. This leads to random hangs when unloading a hive during system startup (tipically on a clean install). The solution here is to simply lock the whole registry when we unload a hive so that we don't have to worry the KCBs are getting tampered by anybody else. This also simplifies the code. Although locking the entire registry while other apps are doing registry related operations to other hives can cause overhead. If this turns out to be bad then we have to rethink the locking mechanism here. CORE-19539 --- ntoskrnl/config/cmapi.c | 35 ++++++------------- ntoskrnl/config/ntapi.c | 64 ++++++++-------------------------- ntoskrnl/include/internal/cm.h | 1 - 3 files changed, 25 insertions(+), 75 deletions(-) diff --git a/ntoskrnl/config/cmapi.c b/ntoskrnl/config/cmapi.c index 383956655b0..2b36c10b3e5 100644 --- a/ntoskrnl/config/cmapi.c +++ b/ntoskrnl/config/cmapi.c @@ -2216,6 +2216,9 @@ CmUnloadKey( DPRINT("CmUnloadKey(%p, %lx)\n", Kcb, Flags); + /* Ensure the registry is locked exclusively for the calling thread */ + CMP_ASSERT_EXCLUSIVE_REGISTRY_LOCK(); + /* Get the hive */ Hive = Kcb->KeyHive; Cell = Kcb->KeyCell; @@ -2243,7 +2246,7 @@ CmUnloadKey( { if (Flags != REG_FORCE_UNLOAD) { - if (CmpEnumerateOpenSubKeys(Kcb, FALSE, TRUE, FALSE) != 0) + if (CmpEnumerateOpenSubKeys(Kcb, TRUE, FALSE) != 0) { /* There are open subkeys but we don't force hive unloading, fail */ Hive->HiveFlags &= ~HIVE_IS_UNLOADING; @@ -2252,7 +2255,7 @@ CmUnloadKey( } else { - if (CmpEnumerateOpenSubKeys(Kcb, TRUE, TRUE, TRUE) != 0) + if (CmpEnumerateOpenSubKeys(Kcb, TRUE, TRUE) != 0) { /* There are open subkeys that we cannot force to unload, fail */ Hive->HiveFlags &= ~HIVE_IS_UNLOADING; @@ -2293,14 +2296,8 @@ CmUnloadKey( Kcb->Delete = TRUE; CmpRemoveKeyControlBlock(Kcb); - if (Flags != REG_FORCE_UNLOAD) - { - /* Release the KCB locks */ - CmpReleaseTwoKcbLockByKey(Kcb->ConvKey, Kcb->ParentKcb->ConvKey); - - /* Release the hive loading lock */ - ExReleasePushLockExclusive(&CmpLoadHiveLock); - } + /* Release the hive loading lock */ + ExReleasePushLockExclusive(&CmpLoadHiveLock); /* Release hive lock */ CmpUnlockRegistry(); @@ -2341,7 +2338,6 @@ ULONG NTAPI CmpEnumerateOpenSubKeys( _In_ PCM_KEY_CONTROL_BLOCK RootKcb, - _In_ BOOLEAN LockHeldExclusively, _In_ BOOLEAN RemoveEmptyCacheEntries, _In_ BOOLEAN DereferenceOpenedEntries) { @@ -2354,6 +2350,9 @@ CmpEnumerateOpenSubKeys( DPRINT("CmpEnumerateOpenSubKeys() called\n"); + /* Ensure the registry is locked exclusively for the calling thread */ + CMP_ASSERT_EXCLUSIVE_REGISTRY_LOCK(); + /* The root key is the only referenced key. There are no referenced sub keys. */ if (RootKcb->RefCount == 1) { @@ -2401,9 +2400,6 @@ CmpEnumerateOpenSubKeys( if (DereferenceOpenedEntries && !(CachedKcb->ExtFlags & CM_KCB_READ_ONLY_KEY)) { - /* Registry needs to be locked down */ - CMP_ASSERT_EXCLUSIVE_REGISTRY_LOCK(); - /* Flush any notifications */ CmpFlushNotifiesOnKeyBodyList(CachedKcb, TRUE); // Lock is already held @@ -2431,20 +2427,11 @@ CmpEnumerateOpenSubKeys( } else if ((CachedKcb->RefCount == 0) && RemoveEmptyCacheEntries) { - /* Lock the cached KCB of subkey before removing it from cache entries */ - if (!LockHeldExclusively) - CmpAcquireKcbLockExclusive(CachedKcb); - /* Remove the current key from the delayed close list */ CmpRemoveFromDelayedClose(CachedKcb); /* Remove the current cache entry */ - // Lock is either held by ourselves or registry is locked exclusively - CmpCleanUpKcbCacheWithLock(CachedKcb, LockHeldExclusively); - - /* Unlock the cached KCB if it was done by ourselves */ - if (!LockHeldExclusively) - CmpReleaseKcbLock(CachedKcb); + CmpCleanUpKcbCacheWithLock(CachedKcb, TRUE); /* Restart, because the hash list has changed */ Entry = CmpCacheTable[i].Entry; diff --git a/ntoskrnl/config/ntapi.c b/ntoskrnl/config/ntapi.c index f425b3a79e0..ba456908e3f 100644 --- a/ntoskrnl/config/ntapi.c +++ b/ntoskrnl/config/ntapi.c @@ -1565,7 +1565,7 @@ NtQueryOpenSubKeys(IN POBJECT_ATTRIBUTES TargetKey, /* Call the internal API */ SubKeys = CmpEnumerateOpenSubKeys(KeyBody->KeyControlBlock, - TRUE, FALSE, FALSE); + FALSE, FALSE); /* Unlock the registry */ CmpUnlockRegistry(); @@ -1803,7 +1803,6 @@ NtUnloadKey2(IN POBJECT_ATTRIBUTES TargetKey, CM_PARSE_CONTEXT ParseContext = {0}; KPROCESSOR_MODE PreviousMode = ExGetPreviousMode(); PCM_KEY_BODY KeyBody = NULL; - ULONG ParentConv = 0, ChildConv = 0; HANDLE Handle; PAGED_CODE(); @@ -1895,30 +1894,17 @@ NtUnloadKey2(IN POBJECT_ATTRIBUTES TargetKey, if (!NT_SUCCESS(Status)) return Status; - /* Acquire the lock depending on flags */ - if (Flags == REG_FORCE_UNLOAD) - { - /* Lock registry exclusively */ - CmpLockRegistryExclusive(); - } - else - { - /* Lock registry */ - CmpLockRegistry(); - - /* Acquire the hive loading lock */ - ExAcquirePushLockExclusive(&CmpLoadHiveLock); - - /* Lock parent and child */ - if (KeyBody->KeyControlBlock->ParentKcb) - ParentConv = KeyBody->KeyControlBlock->ParentKcb->ConvKey; - else - ParentConv = KeyBody->KeyControlBlock->ConvKey; - - ChildConv = KeyBody->KeyControlBlock->ConvKey; - - CmpAcquireTwoKcbLocksExclusiveByKey(ChildConv, ParentConv); - } + /* + * Lock down the entire registry when we unload a hive. + * + * NOTE: We might block other threads of other processes that do + * operations with unrelated keys of other hives when we lock + * the registry for exclusive use by the calling thread that does + * the unloading. If this turns out to cause a major overhead we + * have to rethink the locking mechanism here (prior commit - f1d2a44). + */ + CmpLockRegistryExclusive(); + ExAcquirePushLockExclusive(&CmpLoadHiveLock); /* Check if it's being deleted already */ if (KeyBody->KeyControlBlock->Delete) @@ -1939,34 +1925,12 @@ NtUnloadKey2(IN POBJECT_ATTRIBUTES TargetKey, /* Call the internal API. Note that CmUnloadKey() unlocks the registry only on success. */ Status = CmUnloadKey(KeyBody->KeyControlBlock, Flags); - /* Check if we failed, but really need to succeed */ - if ((Status == STATUS_CANNOT_DELETE) && (Flags == REG_FORCE_UNLOAD)) - { - /* TODO: We should perform another attempt here */ - _SEH2_TRY - { - DPRINT1("NtUnloadKey2(%wZ): We want to force-unload the hive but couldn't unload it: Retrying is UNIMPLEMENTED!\n", TargetKey->ObjectName); - } - _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER) - { - } - _SEH2_END; - } - Quit: /* If CmUnloadKey() failed we need to unlock registry ourselves */ if (!NT_SUCCESS(Status)) { - if (Flags != REG_FORCE_UNLOAD) - { - /* Release the KCB locks */ - CmpReleaseTwoKcbLockByKey(ChildConv, ParentConv); - - /* Release the hive loading lock */ - ExReleasePushLockExclusive(&CmpLoadHiveLock); - } - - /* Unlock the registry */ + /* Unlock the hive loading and registry locks */ + ExReleasePushLockExclusive(&CmpLoadHiveLock); CmpUnlockRegistry(); } diff --git a/ntoskrnl/include/internal/cm.h b/ntoskrnl/include/internal/cm.h index fc5eb365465..f40d8420895 100644 --- a/ntoskrnl/include/internal/cm.h +++ b/ntoskrnl/include/internal/cm.h @@ -1337,7 +1337,6 @@ ULONG NTAPI CmpEnumerateOpenSubKeys( _In_ PCM_KEY_CONTROL_BLOCK RootKcb, - _In_ BOOLEAN LockHeldExclusively, _In_ BOOLEAN RemoveEmptyCacheEntries, _In_ BOOLEAN DereferenceOpenedEntries );