https://git.reactos.org/?p=reactos.git;a=commitdiff;h=697a52aa33350b8b1667a48f968438ffef0ab016
commit 697a52aa33350b8b1667a48f968438ffef0ab016
Author: George Bișoc <george.bi...@reactos.org>
AuthorDate: Sat Feb 25 23:33:19 2023 +0100
Commit: George Bișoc <george.bi...@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;
//
