[reactos] 06/11: [NTOS:CM] Do not acquire the lock twice when the Object Manager calls CmpSecurityMethod
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=697a52aa33350b8b1667a4...
commit 697a52aa33350b8b1667a48f968438ffef0ab016 Author: George Bișoc george.bisoc@reactos.org AuthorDate: Sat Feb 25 23:33:19 2023 +0100 Commit: George Bișoc george.bisoc@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;
//
-
George Bișoc