[ros-diffs] [reactos] 06/11: [NTOS:CM] Do not acquire the lock twice when the Object Manager calls CmpSecurityMethod

[George Bișoc]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=697a52aa33350b8b1667a…

commit 697a52aa33350b8b1667a48f968438ffef0ab016
Author:     George Bișoc <george.bisoc(a)reactos.org>
AuthorDate: Sat Feb 25 23:33:19 2023 +0100
Commit:     George Bișoc <george.bisoc(a)reactos.org>
CommitDate: Sun Oct 1 20:06:02 2023 +0200

    [NTOS:CM] Do not acquire the lock twice when the Object Manager calls
CmpSecurityMethod
    
    Whenever a security request is invoked into a key object, such as when requesting
    information from its security descriptor, the Object Manager will execute
    the CmpSecurityMethod method to do the job.
    
    The problem is that CmpSecurityMethod is not aware if the key control block
    of the key body already has a lock acquired which means the function will attempt
    to acquire a lock again, leading to a deadlock. This happens if the same
    calling thread locks the KCB but it also wants to acquire security information
    with ObCheckObjectAccess in CmpDoOpen.
    
    Windows has a hack in CmpSecurityMethod where the passed KCB pointer is ORed
    with a bitfield mask to avoid locking in all cases. This is ugly because it negates
    every thread to acquire a lock if at least one has it.
---
 ntoskrnl/config/cmparse.c      | 16 ++++++++++++++++
 ntoskrnl/config/cmse.c         | 17 ++++++++++++++---
 ntoskrnl/config/cmsysini.c     |  1 +
 ntoskrnl/include/internal/cm.h |  3 +++
 4 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/ntoskrnl/config/cmparse.c b/ntoskrnl/config/cmparse.c
index 52a3219babb..53f2d3adbab 100644
--- a/ntoskrnl/config/cmparse.c
+++ b/ntoskrnl/config/cmparse.c
@@ -284,6 +284,7 @@ CmpDoCreateChild(IN PHHIVE Hive,
     KeyBody = (PCM_KEY_BODY)(*Object);
     KeyBody->Type = CM_KEY_BODY_TYPE;
     KeyBody->KeyControlBlock = NULL;
+    KeyBody->KcbLocked = FALSE;
 
     /* Check if we had a class */
     if (ParseContext->Class.Length > 0)
@@ -702,6 +703,15 @@ CmpDoOpen(IN PHHIVE Hive,
         /* Link to the KCB */
         EnlistKeyBodyWithKCB(KeyBody, 0);
 
+        /*
+         * We are already holding a lock against the KCB that is assigned
+         * to this key body. This is to prevent a potential deadlock on
+         * CmpSecurityMethod as ObCheckObjectAccess will invoke the Object
+         * Manager to call that method, of which CmpSecurityMethod would
+         * attempt to acquire a lock again.
+         */
+        KeyBody->KcbLocked = TRUE;
+
         if (!ObCheckObjectAccess(*Object,
                                  AccessState,
                                  FALSE,
@@ -711,6 +721,12 @@ CmpDoOpen(IN PHHIVE Hive,
             /* Access check failed */
             ObDereferenceObject(*Object);
         }
+
+        /*
+         * We are done, the lock we are holding will be released
+         * once the registry parsing is done.
+         */
+        KeyBody->KcbLocked = FALSE;
     }
     else
     {
diff --git a/ntoskrnl/config/cmse.c b/ntoskrnl/config/cmse.c
index 1d5aec4e99e..6c8c55472b8 100644
--- a/ntoskrnl/config/cmse.c
+++ b/ntoskrnl/config/cmse.c
@@ -281,10 +281,15 @@ CmpSecurityMethod(IN PVOID ObjectBody,
     /* Acquire the KCB lock */
     if (OperationCode == QuerySecurityDescriptor)
     {
-        CmpAcquireKcbLockShared(Kcb);
+        /* Avoid recursive locking if somebody already holds it */
+        if (!((PCM_KEY_BODY)ObjectBody)->KcbLocked)
+        {
+            CmpAcquireKcbLockShared(Kcb);
+        }
     }
     else
     {
+        ASSERT(!((PCM_KEY_BODY)ObjectBody)->KcbLocked);
         CmpAcquireKcbLockExclusive(Kcb);
     }
 
@@ -334,8 +339,14 @@ CmpSecurityMethod(IN PVOID ObjectBody,
             KeBugCheckEx(SECURITY_SYSTEM, 0, STATUS_INVALID_PARAMETER, 0, 0);
     }
 
-    /* Release the KCB lock */
-    CmpReleaseKcbLock(Kcb);
+    /*
+     * Release the KCB lock, but only if we locked it ourselves and
+     * nobody else was locking it by themselves.
+     */
+    if (!((PCM_KEY_BODY)ObjectBody)->KcbLocked)
+    {
+        CmpReleaseKcbLock(Kcb);
+    }
 
     /* Release the hive lock */
     CmpUnlockRegistry();
diff --git a/ntoskrnl/config/cmsysini.c b/ntoskrnl/config/cmsysini.c
index 282e422b759..df6ebbd90d4 100644
--- a/ntoskrnl/config/cmsysini.c
+++ b/ntoskrnl/config/cmsysini.c
@@ -1126,6 +1126,7 @@ CmpCreateRegistryRoot(VOID)
     RootKey->Type = CM_KEY_BODY_TYPE;
     RootKey->NotifyBlock = NULL;
     RootKey->ProcessID = PsGetCurrentProcessId();
+    RootKey->KcbLocked = FALSE;
 
     /* Link with KCB */
     EnlistKeyBodyWithKCB(RootKey, 0);
diff --git a/ntoskrnl/include/internal/cm.h b/ntoskrnl/include/internal/cm.h
index 36b640f2034..a0921e4eb3b 100644
--- a/ntoskrnl/include/internal/cm.h
+++ b/ntoskrnl/include/internal/cm.h
@@ -235,6 +235,9 @@ typedef struct _CM_KEY_BODY
     struct _CM_NOTIFY_BLOCK *NotifyBlock;
     HANDLE ProcessID;
     LIST_ENTRY KeyBodyList;
+
+    /* ReactOS specific -- boolean flag to avoid recursive locking of the KCB */
+    BOOLEAN KcbLocked;
 } CM_KEY_BODY, *PCM_KEY_BODY;
 
 //