From e1d149c925cfcc2656086bee2416fb319e1bd9ca Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Wed, 14 Nov 2012 07:02:01 -0500 Subject: [PATCH] Windows: Hold ProcessTreeLock across AFSValidateProcessEntry AFSValidateProcessEntry() is called from AFSProcessCreate() which holds the ProcessTree.TreeLock exclusive across the call and from AFSCreate() and AFSRetrieveAuthGroup() where it is not held at all. Add a parameter to AFSValidateProcessEntry() that indicates whether or not the ProcessTree.TreeLock is held. If it is held, it must be held exclusive. If it is not held, the lock must be acquired within AFSValidateProcessEntry() and it must be held for the entire lifetime of the pParentProcessCB reference. Failure to hold the TreeLock for the lifetime of the reference permits a race with AFSProcessDestroy() that can result in the parent ProcessCB being destroyed while its Resource is held. Change-Id: I7cf0dff6bd541b0588a060d677a8e3d724858b96 Reviewed-on: http://gerrit.openafs.org/8455 Tested-by: BuildBot Reviewed-by: Rod Widdowson Tested-by: Jeffrey Altman Reviewed-by: Jeffrey Altman --- src/WINNT/afsrdr/kernel/fs/AFSAuthGroupSupport.cpp | 4 +- src/WINNT/afsrdr/kernel/fs/AFSCreate.cpp | 3 +- src/WINNT/afsrdr/kernel/fs/AFSProcessSupport.cpp | 52 ++++++++++++++-------- src/WINNT/afsrdr/kernel/fs/Include/AFSCommon.h | 3 +- 4 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/WINNT/afsrdr/kernel/fs/AFSAuthGroupSupport.cpp b/src/WINNT/afsrdr/kernel/fs/AFSAuthGroupSupport.cpp index 105bf09..5656ad7 100644 --- a/src/WINNT/afsrdr/kernel/fs/AFSAuthGroupSupport.cpp +++ b/src/WINNT/afsrdr/kernel/fs/AFSAuthGroupSupport.cpp @@ -135,6 +135,7 @@ AFSRetrieveAuthGroup( IN ULONGLONG ProcessId, ThreadId); AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock); + try_return( ntStatus); } @@ -224,7 +225,8 @@ AFSRetrieveAuthGroup( IN ULONGLONG ProcessId, ProcessId, ThreadId); - pAuthGroup = AFSValidateProcessEntry( PsGetCurrentProcessId()); + pAuthGroup = AFSValidateProcessEntry( PsGetCurrentProcessId(), + FALSE); if( pAuthGroup != NULL) { diff --git a/src/WINNT/afsrdr/kernel/fs/AFSCreate.cpp b/src/WINNT/afsrdr/kernel/fs/AFSCreate.cpp index fde3259..2ecf0e3 100644 --- a/src/WINNT/afsrdr/kernel/fs/AFSCreate.cpp +++ b/src/WINNT/afsrdr/kernel/fs/AFSCreate.cpp @@ -120,7 +120,8 @@ AFSCommonCreate( IN PDEVICE_OBJECT DeviceObject, // Validate the process entry // - pAuthGroup = AFSValidateProcessEntry( PsGetCurrentProcessId()); + pAuthGroup = AFSValidateProcessEntry( PsGetCurrentProcessId(), + FALSE); if( pAuthGroup != NULL) { diff --git a/src/WINNT/afsrdr/kernel/fs/AFSProcessSupport.cpp b/src/WINNT/afsrdr/kernel/fs/AFSProcessSupport.cpp index 88b7a3c..35326b2 100644 --- a/src/WINNT/afsrdr/kernel/fs/AFSProcessSupport.cpp +++ b/src/WINNT/afsrdr/kernel/fs/AFSProcessSupport.cpp @@ -131,7 +131,8 @@ AFSProcessCreate( IN HANDLE ParentId, // Now assign the AuthGroup ACE // - AFSValidateProcessEntry( ProcessId); + AFSValidateProcessEntry( ProcessId, + TRUE); } else { @@ -243,7 +244,8 @@ AFSProcessDestroy( IN HANDLE ProcessId) // GUID * -AFSValidateProcessEntry( IN HANDLE ProcessId) +AFSValidateProcessEntry( IN HANDLE ProcessId, + IN BOOLEAN bProcessTreeLocked) { GUID *pAuthGroup = NULL; @@ -263,18 +265,22 @@ AFSValidateProcessEntry( IN HANDLE ProcessId) __Enter { - AFSDbgLogMsg( AFS_SUBSYSTEM_LOCK_PROCESSING, - AFS_TRACE_LEVEL_VERBOSE, - "AFSValidateProcessEntry Acquiring Control ProcessTree.TreeLock lock %08lX SHARED %08lX\n", - pDeviceExt->Specific.Control.ProcessTree.TreeLock, - PsGetCurrentThread()); - uniSIDString.Length = 0; uniSIDString.MaximumLength = 0; uniSIDString.Buffer = NULL; - AFSAcquireShared( pDeviceExt->Specific.Control.ProcessTree.TreeLock, - TRUE); + if ( !bProcessTreeLocked) + { + + AFSDbgLogMsg( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSValidateProcessEntry Acquiring Control ProcessTree.TreeLock lock %08lX SHARED %08lX\n", + pDeviceExt->Specific.Control.ProcessTree.TreeLock, + PsGetCurrentThread()); + + AFSAcquireShared( pDeviceExt->Specific.Control.ProcessTree.TreeLock, + TRUE); + } AFSDbgLogMsg( AFS_SUBSYSTEM_AUTHGROUP_PROCESSING, AFS_TRACE_LEVEL_VERBOSE, @@ -290,10 +296,14 @@ AFSValidateProcessEntry( IN HANDLE ProcessId) pProcessCB == NULL) { - AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock); + if ( !bProcessTreeLocked) + { - AFSAcquireExcl( pDeviceExt->Specific.Control.ProcessTree.TreeLock, - TRUE); + AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock); + + AFSAcquireExcl( pDeviceExt->Specific.Control.ProcessTree.TreeLock, + TRUE); + } ntStatus = AFSLocateHashEntry( pDeviceExt->Specific.Control.ProcessTree.TreeHead, ullProcessID, @@ -319,12 +329,14 @@ AFSValidateProcessEntry( IN HANDLE ProcessId) __FUNCTION__, ullProcessID); - AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock); - try_return( ntStatus = STATUS_UNSUCCESSFUL); } - AFSConvertToShared( pDeviceExt->Specific.Control.ProcessTree.TreeLock); + if ( !bProcessTreeLocked) + { + + AFSConvertToShared( pDeviceExt->Specific.Control.ProcessTree.TreeLock); + } } // @@ -370,8 +382,6 @@ AFSValidateProcessEntry( IN HANDLE ProcessId) AFSAcquireExcl( &pProcessCB->Lock, TRUE); - AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock); - #if defined(_WIN64) // @@ -782,6 +792,12 @@ try_exit: { RtlFreeUnicodeString( &uniSIDString); } + + if ( !bProcessTreeLocked) + { + + AFSReleaseResource( pDeviceExt->Specific.Control.ProcessTree.TreeLock); + } } return pAuthGroup; diff --git a/src/WINNT/afsrdr/kernel/fs/Include/AFSCommon.h b/src/WINNT/afsrdr/kernel/fs/Include/AFSCommon.h index 65c14af..9af1fa7 100644 --- a/src/WINNT/afsrdr/kernel/fs/Include/AFSCommon.h +++ b/src/WINNT/afsrdr/kernel/fs/Include/AFSCommon.h @@ -815,7 +815,8 @@ void AFSProcessDestroy( IN HANDLE ProcessId); GUID * -AFSValidateProcessEntry( IN HANDLE ProcessId); +AFSValidateProcessEntry( IN HANDLE ProcessId, + IN BOOLEAN bProcessTreeLocked); BOOLEAN AFSIs64BitProcess( IN ULONGLONG ProcessId); -- 1.9.4