public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList
@ 2021-10-12  8:34 Ma, Hua
  2021-10-13  3:28 ` Dandan Bi
  2021-10-13  4:25 ` Wang, Jian J
  0 siblings, 2 replies; 4+ messages in thread
From: Ma, Hua @ 2021-10-12  8:34 UTC (permalink / raw)
  To: devel; +Cc: Hua Ma, Jian J Wang, Liming Gao, Dandan Bi, Ray Ni

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680

This patch fixes the following issue:

The global variable gHandleList is a linked list.
This list is locked when a entry is added or removed from the list,
but there is no lock when iterating this list in function
CoreValidateHandle().
It can lead to "Handle.c (76): CR has Bad Signature" assertion if the
iterated entry in the list is just removed by other task during iterating.
Locking the list when iterating can fix this issue.

v2 changes:
 - Add lock check and comments in CoreGetProtocolInterface() before
calling CoreValidateHandle()
 - Update the comments in CoreValidateHandle() header file

v1: https://edk2.groups.io/g/devel/topic/86233569

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Hua Ma <hua.ma@intel.com>
---
 MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++--
 MdeModulePkg/Core/Dxe/Hand/Handle.c        | 56 +++++++++++++++-------
 MdeModulePkg/Core/Dxe/Hand/Handle.h        |  5 +-
 MdeModulePkg/Core/Dxe/Hand/Notify.c        |  2 +-
 4 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index feabf12faf..eb8a765d2c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -68,7 +68,7 @@ CoreConnectController (
   //
   // Make sure ControllerHandle is valid
   //
-  Status = CoreValidateHandle (ControllerHandle);
+  Status = CoreValidateHandle (ControllerHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -154,7 +154,7 @@ CoreConnectController (
     //
     // Make sure the DriverBindingHandle is valid
     //
-    Status = CoreValidateHandle (ControllerHandle);
+    Status = CoreValidateHandle (ControllerHandle, TRUE);
     if (EFI_ERROR (Status)) {
       //
       // Release the protocol lock on the handle database
@@ -268,7 +268,7 @@ AddSortedDriverBindingProtocol (
   //
   // Make sure the DriverBindingHandle is valid
   //
-  Status = CoreValidateHandle (DriverBindingHandle);
+  Status = CoreValidateHandle (DriverBindingHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return;
   }
@@ -746,7 +746,7 @@ CoreDisconnectController (
   //
   // Make sure ControllerHandle is valid
   //
-  Status = CoreValidateHandle (ControllerHandle);
+  Status = CoreValidateHandle (ControllerHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -755,7 +755,7 @@ CoreDisconnectController (
   // Make sure ChildHandle is valid if it is not NULL
   //
   if (ChildHandle != NULL) {
-    Status = CoreValidateHandle (ChildHandle);
+    Status = CoreValidateHandle (ChildHandle, FALSE);
     if (EFI_ERROR (Status)) {
       return Status;
     }
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 6eccb41ecb..46f67d3d6a 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -55,31 +55,46 @@ CoreReleaseProtocolLock (
   Check whether a handle is a valid EFI_HANDLE
 
   @param  UserHandle             The handle to check
+  @param  IsLocked               The protocol lock is acquried or not
 
   @retval EFI_INVALID_PARAMETER  The handle is NULL or not a valid EFI_HANDLE.
+  @retval EFI_NOT_FOUND          The handle is not found in the handle database.
   @retval EFI_SUCCESS            The handle is valid EFI_HANDLE.
 
 **/
 EFI_STATUS
 CoreValidateHandle (
-  IN  EFI_HANDLE                UserHandle
+  IN  EFI_HANDLE                UserHandle,
+  IN  BOOLEAN                   IsLocked
   )
 {
   IHANDLE             *Handle;
   LIST_ENTRY          *Link;
+  EFI_STATUS          Status;
 
   if (UserHandle == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
+  if (IsLocked) {
+    ASSERT_LOCKED(&gProtocolDatabaseLock);
+  } else {
+    CoreAcquireProtocolLock ();
+  }
+
+  Status = EFI_NOT_FOUND;
   for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink) {
     Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
     if (Handle == (IHANDLE *) UserHandle) {
-      return EFI_SUCCESS;
+      Status = EFI_SUCCESS;
+      break;
     }
   }
 
-  return EFI_INVALID_PARAMETER;
+  if (!IsLocked) {
+    CoreReleaseProtocolLock ();
+  }
+  return Status;
 }
 
 
@@ -428,7 +443,7 @@ CoreInstallProtocolInterfaceNotify (
     //
     InsertTailList (&gHandleList, &Handle->AllHandles);
   } else {
-    Status = CoreValidateHandle (Handle);
+    Status = CoreValidateHandle (Handle, TRUE);
     if (EFI_ERROR (Status)) {
       DEBUG((DEBUG_ERROR, "InstallProtocolInterface: input handle at 0x%x is invalid\n", Handle));
       goto Done;
@@ -723,7 +738,7 @@ CoreUninstallProtocolInterface (
   //
   // Check that UserHandle is a valid handle
   //
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -899,7 +914,16 @@ CoreGetProtocolInterface (
   IHANDLE             *Handle;
   LIST_ENTRY          *Link;
 
-  Status = CoreValidateHandle (UserHandle);
+  //
+  // The gProtocolDatabaseLock must be owned
+  //
+  ASSERT_LOCKED(&gProtocolDatabaseLock);
+
+  //
+  // The gProtocolDatabaseLock is owned
+  // Call CoreValidateHandle () with IsLocked == TRUE
+  //
+  Status = CoreValidateHandle (UserHandle, TRUE);
   if (EFI_ERROR (Status)) {
     return NULL;
   }
@@ -1013,7 +1037,7 @@ CoreOpenProtocol (
   //
   // Check for invalid UserHandle
   //
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -1023,11 +1047,11 @@ CoreOpenProtocol (
   //
   switch (Attributes) {
   case EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER :
-    Status = CoreValidateHandle (ImageHandle);
+    Status = CoreValidateHandle (ImageHandle, FALSE);
     if (EFI_ERROR (Status)) {
       return Status;
     }
-    Status = CoreValidateHandle (ControllerHandle);
+    Status = CoreValidateHandle (ControllerHandle, FALSE);
     if (EFI_ERROR (Status)) {
       return Status;
     }
@@ -1037,17 +1061,17 @@ CoreOpenProtocol (
     break;
   case EFI_OPEN_PROTOCOL_BY_DRIVER :
   case EFI_OPEN_PROTOCOL_BY_DRIVER | EFI_OPEN_PROTOCOL_EXCLUSIVE :
-    Status = CoreValidateHandle (ImageHandle);
+    Status = CoreValidateHandle (ImageHandle, FALSE);
     if (EFI_ERROR (Status)) {
       return Status;
     }
-    Status = CoreValidateHandle (ControllerHandle);
+    Status = CoreValidateHandle (ControllerHandle, FALSE);
     if (EFI_ERROR (Status)) {
       return Status;
     }
     break;
   case EFI_OPEN_PROTOCOL_EXCLUSIVE :
-    Status = CoreValidateHandle (ImageHandle);
+    Status = CoreValidateHandle (ImageHandle, FALSE);
     if (EFI_ERROR (Status)) {
       return Status;
     }
@@ -1249,16 +1273,16 @@ CoreCloseProtocol (
   //
   // Check for invalid parameters
   //
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return Status;
   }
-  Status = CoreValidateHandle (AgentHandle);
+  Status = CoreValidateHandle (AgentHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return Status;
   }
   if (ControllerHandle != NULL) {
-    Status = CoreValidateHandle (ControllerHandle);
+    Status = CoreValidateHandle (ControllerHandle, FALSE);
     if (EFI_ERROR (Status)) {
       return Status;
     }
@@ -1443,7 +1467,7 @@ CoreProtocolsPerHandle (
   UINTN                               ProtocolCount;
   EFI_GUID                            **Buffer;
 
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return Status;
   }
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.h b/MdeModulePkg/Core/Dxe/Hand/Handle.h
index 83eb2b9f3a..20d886beca 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
@@ -244,14 +244,17 @@ CoreReleaseProtocolLock (
   Check whether a handle is a valid EFI_HANDLE
 
   @param  UserHandle             The handle to check
+  @param  IsLocked               The protocol lock is acquried or not
 
   @retval EFI_INVALID_PARAMETER  The handle is NULL or not a valid EFI_HANDLE.
+  @retval EFI_NOT_FOUND          The handle is not found in the handle database.
   @retval EFI_SUCCESS            The handle is valid EFI_HANDLE.
 
 **/
 EFI_STATUS
 CoreValidateHandle (
-  IN  EFI_HANDLE                UserHandle
+  IN  EFI_HANDLE                UserHandle,
+  IN  BOOLEAN                   IsLocked
   );
 
 //
diff --git a/MdeModulePkg/Core/Dxe/Hand/Notify.c b/MdeModulePkg/Core/Dxe/Hand/Notify.c
index 553413a350..f4e7c01e96 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Notify.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Notify.c
@@ -188,7 +188,7 @@ CoreReinstallProtocolInterface (
   PROTOCOL_INTERFACE        *Prot;
   PROTOCOL_ENTRY            *ProtEntry;
 
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return Status;
   }
-- 
2.32.0.windows.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-10-13  7:47 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-12  8:34 [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList Ma, Hua
2021-10-13  3:28 ` Dandan Bi
2021-10-13  4:25 ` Wang, Jian J
2021-10-13  7:47   ` Ma, Hua

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox