[edk2-devel] [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Ma, Hua posted 1 patch 1 week, 3 days ago
Failed in applying to current master (apply log)
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(-)

[edk2-devel] [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Posted by Ma, Hua 1 week, 3 days ago
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81812): https://edk2.groups.io/g/devel/message/81812
Mute This Topic: https://groups.io/mt/86257717/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Posted by Wang, Jian J 1 week, 2 days ago
Hi Hua,

It looks a bit odd to me to add 'IsLocked' parameter and acquire lock
inside CoreValidateHandle() if it's FALSE. Maybe we can keep the
function prototype as-is but do something like below:

    a) Just keep ASSERT_LOCKED(&gProtocolDatabaseLock) in CoreValidateHandle()
    b) Call CoreAcquireProtocolLock() before any calling of CoreValidateHandle()
         and CoreReleaseProtocolLock() afterwards.

Actually, CoreAcquireProtocolLock() is always called wherever CoreValidateHandle()
is called. The problem is that, in many cases, CoreAcquireProtocolLock() is called
after CoreValidateHandle(). We can simply move the calling of CoreAcquireProtocolLock()
before CoreValidateHandle() to fix this problem.

For those cases CoreAcquireProtocolLock() is not called at all, just simply add it.

Regards,
Jian

> -----Original Message-----
> From: Ma, Hua <hua.ma@intel.com>
> Sent: Tuesday, October 12, 2021 4:34 PM
> To: devel@edk2.groups.io
> Cc: Ma, Hua <hua.ma@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Liming Gao <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>;
> Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating
> gHandleList
> 
> 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81850): https://edk2.groups.io/g/devel/message/81850
Mute This Topic: https://groups.io/mt/86257717/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Posted by Ma, Hua 1 week, 2 days ago
Hi Jian,

Thanks for the comment.
Patch v3 is just sent with the update.
Please help to review.

Thank you,
Ma Hua

> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Wednesday, October 13, 2021 12:26 PM
> To: Ma, Hua <hua.ma@intel.com>; devel@edk2.groups.io
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Bi, Dandan
> <dandan.bi@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when
> iterating gHandleList
> 
> Hi Hua,
> 
> It looks a bit odd to me to add 'IsLocked' parameter and acquire lock inside
> CoreValidateHandle() if it's FALSE. Maybe we can keep the function
> prototype as-is but do something like below:
> 
>     a) Just keep ASSERT_LOCKED(&gProtocolDatabaseLock) in
> CoreValidateHandle()
>     b) Call CoreAcquireProtocolLock() before any calling of
> CoreValidateHandle()
>          and CoreReleaseProtocolLock() afterwards.
> 
> Actually, CoreAcquireProtocolLock() is always called wherever
> CoreValidateHandle() is called. The problem is that, in many cases,
> CoreAcquireProtocolLock() is called after CoreValidateHandle(). We can
> simply move the calling of CoreAcquireProtocolLock() before
> CoreValidateHandle() to fix this problem.
> 
> For those cases CoreAcquireProtocolLock() is not called at all, just simply add
> it.
> 
> Regards,
> Jian
> 
> > -----Original Message-----
> > From: Ma, Hua <hua.ma@intel.com>
> > Sent: Tuesday, October 12, 2021 4:34 PM
> > To: devel@edk2.groups.io
> > Cc: Ma, Hua <hua.ma@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> > Liming Gao <gaoliming@byosoft.com.cn>; Bi, Dandan
> > <dandan.bi@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Subject: [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when
> > iterating gHandleList
> >
> > 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81865): https://edk2.groups.io/g/devel/message/81865
Mute This Topic: https://groups.io/mt/86257717/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Posted by Dandan Bi 1 week, 2 days ago
Reviewed-by: Dandan Bi <dandan.bi@intel.com>



Thanks,
Dandan

> -----Original Message-----
> From: Ma, Hua <hua.ma@intel.com>
> Sent: Tuesday, October 12, 2021 4:34 PM
> To: devel@edk2.groups.io
> Cc: Ma, Hua <hua.ma@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Liming Gao <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>;
> Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating
> gHandleList
> 
> 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81849): https://edk2.groups.io/g/devel/message/81849
Mute This Topic: https://groups.io/mt/86257717/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-