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

Ma, Hua posted 1 patch 1 week, 4 days ago
Failed in applying to current master (apply log)
MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++---
MdeModulePkg/Core/Dxe/Hand/Handle.c        | 47 ++++++++++++++--------
MdeModulePkg/Core/Dxe/Hand/Handle.h        |  3 +-
MdeModulePkg/Core/Dxe/Hand/Notify.c        |  2 +-
4 files changed, 39 insertions(+), 23 deletions(-)

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

Posted by Ma, Hua 1 week, 4 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.

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        | 47 ++++++++++++++--------
 MdeModulePkg/Core/Dxe/Hand/Handle.h        |  3 +-
 MdeModulePkg/Core/Dxe/Hand/Notify.c        |  2 +-
 4 files changed, 39 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..b4f389bb35 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,7 @@ CoreGetProtocolInterface (
   IHANDLE             *Handle;
   LIST_ENTRY          *Link;
 
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, TRUE);
   if (EFI_ERROR (Status)) {
     return NULL;
   }
@@ -1013,7 +1028,7 @@ CoreOpenProtocol (
   //
   // Check for invalid UserHandle
   //
-  Status = CoreValidateHandle (UserHandle);
+  Status = CoreValidateHandle (UserHandle, FALSE);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -1023,11 +1038,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 +1052,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 +1264,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 +1458,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..b962d80a29 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
@@ -251,7 +251,8 @@ CoreReleaseProtocolLock (
 **/
 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 (#81747): https://edk2.groups.io/g/devel/message/81747
Mute This Topic: https://groups.io/mt/86233569/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Dandan Bi 1 week, 3 days ago
Hi Hua,

One minor comment inline, please check.



Thanks,
Dandan

> -----Original Message-----
> From: Ma, Hua <hua.ma@intel.com>
> Sent: Monday, October 11, 2021 6:45 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] 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.
> 
> 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        | 47 ++++++++++++++--------
>  MdeModulePkg/Core/Dxe/Hand/Handle.h        |  3 +-
>  MdeModulePkg/Core/Dxe/Hand/Notify.c        |  2 +-
>  4 files changed, 39 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..b4f389bb35 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,7 @@ CoreGetProtocolInterface (
>    IHANDLE             *Handle;
>    LIST_ENTRY          *Link;
> 
> -  Status = CoreValidateHandle (UserHandle);
> +  Status = CoreValidateHandle (UserHandle, TRUE);
[Dandan] As current all callers of CoreGetProtocolInterface () has acquired the lock, it's OK to pass TRUE to indicate the lock is acquired. 
I am just thinking about following small question.
Could we add some comments to explain why it's true here and also notify that caller should also acquire the lock firstly by itself if there is any new caller added to call this function?

>    if (EFI_ERROR (Status)) {
>      return NULL;
>    }
> @@ -1013,7 +1028,7 @@ CoreOpenProtocol (
>    //
>    // Check for invalid UserHandle
>    //
> -  Status = CoreValidateHandle (UserHandle);
> +  Status = CoreValidateHandle (UserHandle, FALSE);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -1023,11 +1038,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 +1052,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 +1264,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 +1458,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..b962d80a29 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
> +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
> @@ -251,7 +251,8 @@ CoreReleaseProtocolLock (  **/  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 (#81800): https://edk2.groups.io/g/devel/message/81800
Mute This Topic: https://groups.io/mt/86233569/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Ma, Hua 1 week, 3 days ago
Hi Dandan,

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

Thank you,
Ma Hua

> -----Original Message-----
> From: Bi, Dandan <dandan.bi@intel.com>
> Sent: Tuesday, October 12, 2021 12:13 PM
> To: Ma, Hua <hua.ma@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when
> iterating gHandleList
> 
> Hi Hua,
> 
> One minor comment inline, please check.
> 
> 
> 
> Thanks,
> Dandan
> 
> > -----Original Message-----
> > From: Ma, Hua <hua.ma@intel.com>
> > Sent: Monday, October 11, 2021 6:45 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] 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.
> >
> > 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        | 47 ++++++++++++++------
> --
> >  MdeModulePkg/Core/Dxe/Hand/Handle.h        |  3 +-
> >  MdeModulePkg/Core/Dxe/Hand/Notify.c        |  2 +-
> >  4 files changed, 39 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..b4f389bb35 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,7 @@ CoreGetProtocolInterface (
> >    IHANDLE             *Handle;
> >    LIST_ENTRY          *Link;
> >
> > -  Status = CoreValidateHandle (UserHandle);
> > +  Status = CoreValidateHandle (UserHandle, TRUE);
> [Dandan] As current all callers of CoreGetProtocolInterface () has acquired
> the lock, it's OK to pass TRUE to indicate the lock is acquired.
> I am just thinking about following small question.
> Could we add some comments to explain why it's true here and also notify
> that caller should also acquire the lock firstly by itself if there is any new caller
> added to call this function?
> 
> >    if (EFI_ERROR (Status)) {
> >      return NULL;
> >    }
> > @@ -1013,7 +1028,7 @@ CoreOpenProtocol (
> >    //
> >    // Check for invalid UserHandle
> >    //
> > -  Status = CoreValidateHandle (UserHandle);
> > +  Status = CoreValidateHandle (UserHandle, FALSE);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > @@ -1023,11 +1038,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 +1052,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 +1264,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 +1458,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..b962d80a29 100644
> > --- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
> > +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
> > @@ -251,7 +251,8 @@ CoreReleaseProtocolLock (  **/  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 (#81813): https://edk2.groups.io/g/devel/message/81813
Mute This Topic: https://groups.io/mt/86233569/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


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

Posted by Michael Brown 1 week, 4 days ago
On 11/10/2021 11:45, Ma, Hua wrote:
> 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.

At a first glance, it looks as though if the caller does not already 
hold the lock, then the result from CoreValidateHandle() may be invalid 
by the time that control returns to the caller.

Under what circumstances is it valid to call CoreValidateHandle() when 
the caller does not _already_ hold the lock (i.e. IsLocked==FALSE)?

Thanks,

Michael


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


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

Posted by Ma, Hua 1 week, 4 days ago
> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org>
> Sent: Monday, October 11, 2021 7:28 PM
> To: devel@edk2.groups.io; Ma, Hua <hua.ma@intel.com>
> Cc: 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: Re: [edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock
> when iterating gHandleList
> 
> On 11/10/2021 11:45, Ma, Hua wrote:
> > 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.
> 
> At a first glance, it looks as though if the caller does not already
> hold the lock, then the result from CoreValidateHandle() may be invalid
> by the time that control returns to the caller.
> 
> Under what circumstances is it valid to call CoreValidateHandle() when
> the caller does not _already_ hold the lock (i.e. IsLocked==FALSE)?
> 
> Thanks,
> 
> Michael

Hi Michael,

Thanks for the review,
In current CoreValidateHandle implementation:

  for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink) {
    Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
    if (Handle == (IHANDLE *) UserHandle) {
      return EFI_SUCCESS;
    }
  }

Let's say, if the list have 4 entries, (A->B->C->D), and the caller is trying to validate if entry C is valid,
When caller task just set local variable Link to B, and before it calls CR() macro to validate the signature,
At this point, if other task just deletes other entry, entry B, and reset B's signature. 
Then when back to caller task,  There is a signature mismatch assertion for B.
But the result should be still valid for C if call CoreValidateHandle() again.
The patch is trying to fix this kind of problem.

I feel one valid kind of calling CoreValidateHandle() without holding the lock is:
when the handle is passed in as parameter, and do a parameter checking,
and later, if need, validated the handle again when use the handle.
for example in current code, in AddSortedDriverBindingProtocol, CoreConnectController,
This patch do not change where to add a handle validation.

Thank you,
Ma Hua


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