* [PATCH v3] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList
@ 2021-10-13 7:45 Ma, Hua
2021-10-13 8:11 ` Wang, Jian J
0 siblings, 1 reply; 3+ messages in thread
From: Ma, Hua @ 2021-10-13 7:45 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.
Currently some caller functions of CoreValidateHandle() have
CoreAcquireProtocolLock(), but some caller functions of
CoreValidateHandle() do not CoreAcquireProtocolLock().
Add CoreAcquireProtocolLock() always when CoreValidateHandle() is called,
Also, A lock check is added in the CoreValidateHandle().
v3 changes:
- keep ASSERT_LOCKED(&gProtocolDatabaseLock) in CoreValidateHandle()
- Call CoreAcquireProtocolLock() before any calling of
CoreValidateHandle() and CoreReleaseProtocolLock() afterwards
- Update the commit message
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 | 16 +++++
MdeModulePkg/Core/Dxe/Hand/Handle.c | 75 ++++++++++++----------
MdeModulePkg/Core/Dxe/Hand/Handle.h | 1 +
MdeModulePkg/Core/Dxe/Hand/Notify.c | 13 ++--
4 files changed, 64 insertions(+), 41 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index feabf12faf..12a202417c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -68,7 +68,12 @@ CoreConnectController (
//
// Make sure ControllerHandle is valid
//
+ CoreAcquireProtocolLock ();
+
Status = CoreValidateHandle (ControllerHandle);
+
+ CoreReleaseProtocolLock ();
+
if (EFI_ERROR (Status)) {
return Status;
}
@@ -268,7 +273,12 @@ AddSortedDriverBindingProtocol (
//
// Make sure the DriverBindingHandle is valid
//
+ CoreAcquireProtocolLock ();
+
Status = CoreValidateHandle (DriverBindingHandle);
+
+ CoreReleaseProtocolLock ();
+
if (EFI_ERROR (Status)) {
return;
}
@@ -746,8 +756,11 @@ CoreDisconnectController (
//
// Make sure ControllerHandle is valid
//
+ CoreAcquireProtocolLock ();
+
Status = CoreValidateHandle (ControllerHandle);
if (EFI_ERROR (Status)) {
+ CoreReleaseProtocolLock ();
return Status;
}
@@ -757,10 +770,13 @@ CoreDisconnectController (
if (ChildHandle != NULL) {
Status = CoreValidateHandle (ChildHandle);
if (EFI_ERROR (Status)) {
+ CoreReleaseProtocolLock ();
return Status;
}
}
+ CoreReleaseProtocolLock ();
+
Handle = ControllerHandle;
//
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 6eccb41ecb..92979281b7 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -53,6 +53,7 @@ CoreReleaseProtocolLock (
/**
Check whether a handle is a valid EFI_HANDLE
+ The gProtocolDatabaseLock must be owned
@param UserHandle The handle to check
@@ -72,6 +73,8 @@ CoreValidateHandle (
return EFI_INVALID_PARAMETER;
}
+ ASSERT_LOCKED(&gProtocolDatabaseLock);
+
for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink) {
Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
if (Handle == (IHANDLE *) UserHandle) {
@@ -720,19 +723,19 @@ CoreUninstallProtocolInterface (
return EFI_INVALID_PARAMETER;
}
+ //
+ // Lock the protocol database
+ //
+ CoreAcquireProtocolLock ();
+
//
// Check that UserHandle is a valid handle
//
Status = CoreValidateHandle (UserHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
- //
- // Lock the protocol database
- //
- CoreAcquireProtocolLock ();
-
//
// Check that Protocol exists on UserHandle, and Interface matches the interface in the database
//
@@ -1010,12 +1013,17 @@ CoreOpenProtocol (
return EFI_INVALID_PARAMETER;
}
+ //
+ // Lock the protocol database
+ //
+ CoreAcquireProtocolLock ();
+
//
// Check for invalid UserHandle
//
Status = CoreValidateHandle (UserHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
//
@@ -1025,31 +1033,32 @@ CoreOpenProtocol (
case EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER :
Status = CoreValidateHandle (ImageHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
Status = CoreValidateHandle (ControllerHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
if (UserHandle == ControllerHandle) {
- return EFI_INVALID_PARAMETER;
+ Status = EFI_INVALID_PARAMETER;
+ goto Done;
}
break;
case EFI_OPEN_PROTOCOL_BY_DRIVER :
case EFI_OPEN_PROTOCOL_BY_DRIVER | EFI_OPEN_PROTOCOL_EXCLUSIVE :
Status = CoreValidateHandle (ImageHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
Status = CoreValidateHandle (ControllerHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
break;
case EFI_OPEN_PROTOCOL_EXCLUSIVE :
Status = CoreValidateHandle (ImageHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
break;
case EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL :
@@ -1057,13 +1066,10 @@ CoreOpenProtocol (
case EFI_OPEN_PROTOCOL_TEST_PROTOCOL :
break;
default:
- return EFI_INVALID_PARAMETER;
+ Status = EFI_INVALID_PARAMETER;
+ goto Done;
}
- //
- // Lock the protocol database
- //
- CoreAcquireProtocolLock ();
//
// Look at each protocol interface for a match
@@ -1246,32 +1252,33 @@ CoreCloseProtocol (
LIST_ENTRY *Link;
OPEN_PROTOCOL_DATA *OpenData;
+ //
+ // Lock the protocol database
+ //
+ CoreAcquireProtocolLock ();
+
//
// Check for invalid parameters
//
Status = CoreValidateHandle (UserHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
Status = CoreValidateHandle (AgentHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
if (ControllerHandle != NULL) {
Status = CoreValidateHandle (ControllerHandle);
if (EFI_ERROR (Status)) {
- return Status;
+ goto Done;
}
}
if (Protocol == NULL) {
- return EFI_INVALID_PARAMETER;
+ Status = EFI_INVALID_PARAMETER;
+ goto Done;
}
- //
- // Lock the protocol database
- //
- CoreAcquireProtocolLock ();
-
//
// Look at each protocol interface for a match
//
@@ -1443,13 +1450,6 @@ CoreProtocolsPerHandle (
UINTN ProtocolCount;
EFI_GUID **Buffer;
- Status = CoreValidateHandle (UserHandle);
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Handle = (IHANDLE *)UserHandle;
-
if (ProtocolBuffer == NULL) {
return EFI_INVALID_PARAMETER;
}
@@ -1464,6 +1464,13 @@ CoreProtocolsPerHandle (
CoreAcquireProtocolLock ();
+ Status = CoreValidateHandle (UserHandle);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ Handle = (IHANDLE *)UserHandle;
+
for (Link = Handle->Protocols.ForwardLink; Link != &Handle->Protocols; Link = Link->ForwardLink) {
ProtocolCount++;
}
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.h b/MdeModulePkg/Core/Dxe/Hand/Handle.h
index 83eb2b9f3a..3f83e3af15 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
@@ -242,6 +242,7 @@ CoreReleaseProtocolLock (
/**
Check whether a handle is a valid EFI_HANDLE
+ The gProtocolDatabaseLock must be owned
@param UserHandle The handle to check
diff --git a/MdeModulePkg/Core/Dxe/Hand/Notify.c b/MdeModulePkg/Core/Dxe/Hand/Notify.c
index 553413a350..d05f95207f 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Notify.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Notify.c
@@ -188,22 +188,21 @@ CoreReinstallProtocolInterface (
PROTOCOL_INTERFACE *Prot;
PROTOCOL_ENTRY *ProtEntry;
- Status = CoreValidateHandle (UserHandle);
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
if (Protocol == NULL) {
return EFI_INVALID_PARAMETER;
}
- Handle = (IHANDLE *) UserHandle;
-
//
// Lock the protocol database
//
CoreAcquireProtocolLock ();
+ Status = CoreValidateHandle (UserHandle);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ Handle = (IHANDLE *) UserHandle;
//
// Check that Protocol exists on UserHandle, and Interface matches the interface in the database
//
--
2.32.0.windows.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList
2021-10-13 7:45 [PATCH v3] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList Ma, Hua
@ 2021-10-13 8:11 ` Wang, Jian J
2021-10-14 2:21 ` Dandan Bi
0 siblings, 1 reply; 3+ messages in thread
From: Wang, Jian J @ 2021-10-13 8:11 UTC (permalink / raw)
To: Ma, Hua, devel@edk2.groups.io; +Cc: Liming Gao, Bi, Dandan, Ni, Ray
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Regards,
Jian
> -----Original Message-----
> From: Ma, Hua <hua.ma@intel.com>
> Sent: Wednesday, October 13, 2021 3: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 v3] 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.
>
> Currently some caller functions of CoreValidateHandle() have
> CoreAcquireProtocolLock(), but some caller functions of
> CoreValidateHandle() do not CoreAcquireProtocolLock().
> Add CoreAcquireProtocolLock() always when CoreValidateHandle() is called,
> Also, A lock check is added in the CoreValidateHandle().
>
> v3 changes:
> - keep ASSERT_LOCKED(&gProtocolDatabaseLock) in CoreValidateHandle()
> - Call CoreAcquireProtocolLock() before any calling of
> CoreValidateHandle() and CoreReleaseProtocolLock() afterwards
> - Update the commit message
>
> 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 | 16 +++++
> MdeModulePkg/Core/Dxe/Hand/Handle.c | 75 ++++++++++++----------
> MdeModulePkg/Core/Dxe/Hand/Handle.h | 1 +
> MdeModulePkg/Core/Dxe/Hand/Notify.c | 13 ++--
> 4 files changed, 64 insertions(+), 41 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> index feabf12faf..12a202417c 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> +++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> @@ -68,7 +68,12 @@ CoreConnectController (
> //
> // Make sure ControllerHandle is valid
> //
> + CoreAcquireProtocolLock ();
> +
> Status = CoreValidateHandle (ControllerHandle);
> +
> + CoreReleaseProtocolLock ();
> +
> if (EFI_ERROR (Status)) {
> return Status;
> }
> @@ -268,7 +273,12 @@ AddSortedDriverBindingProtocol (
> //
> // Make sure the DriverBindingHandle is valid
> //
> + CoreAcquireProtocolLock ();
> +
> Status = CoreValidateHandle (DriverBindingHandle);
> +
> + CoreReleaseProtocolLock ();
> +
> if (EFI_ERROR (Status)) {
> return;
> }
> @@ -746,8 +756,11 @@ CoreDisconnectController (
> //
> // Make sure ControllerHandle is valid
> //
> + CoreAcquireProtocolLock ();
> +
> Status = CoreValidateHandle (ControllerHandle);
> if (EFI_ERROR (Status)) {
> + CoreReleaseProtocolLock ();
> return Status;
> }
>
> @@ -757,10 +770,13 @@ CoreDisconnectController (
> if (ChildHandle != NULL) {
> Status = CoreValidateHandle (ChildHandle);
> if (EFI_ERROR (Status)) {
> + CoreReleaseProtocolLock ();
> return Status;
> }
> }
>
> + CoreReleaseProtocolLock ();
> +
> Handle = ControllerHandle;
>
> //
> diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> index 6eccb41ecb..92979281b7 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> @@ -53,6 +53,7 @@ CoreReleaseProtocolLock (
>
> /**
> Check whether a handle is a valid EFI_HANDLE
> + The gProtocolDatabaseLock must be owned
>
> @param UserHandle The handle to check
>
> @@ -72,6 +73,8 @@ CoreValidateHandle (
> return EFI_INVALID_PARAMETER;
> }
>
> + ASSERT_LOCKED(&gProtocolDatabaseLock);
> +
> for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink) {
> Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
> if (Handle == (IHANDLE *) UserHandle) {
> @@ -720,19 +723,19 @@ CoreUninstallProtocolInterface (
> return EFI_INVALID_PARAMETER;
> }
>
> + //
> + // Lock the protocol database
> + //
> + CoreAcquireProtocolLock ();
> +
> //
> // Check that UserHandle is a valid handle
> //
> Status = CoreValidateHandle (UserHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
>
> - //
> - // Lock the protocol database
> - //
> - CoreAcquireProtocolLock ();
> -
> //
> // Check that Protocol exists on UserHandle, and Interface matches the
> interface in the database
> //
> @@ -1010,12 +1013,17 @@ CoreOpenProtocol (
> return EFI_INVALID_PARAMETER;
> }
>
> + //
> + // Lock the protocol database
> + //
> + CoreAcquireProtocolLock ();
> +
> //
> // Check for invalid UserHandle
> //
> Status = CoreValidateHandle (UserHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
>
> //
> @@ -1025,31 +1033,32 @@ CoreOpenProtocol (
> case EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER :
> Status = CoreValidateHandle (ImageHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
> Status = CoreValidateHandle (ControllerHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
> if (UserHandle == ControllerHandle) {
> - return EFI_INVALID_PARAMETER;
> + Status = EFI_INVALID_PARAMETER;
> + goto Done;
> }
> break;
> case EFI_OPEN_PROTOCOL_BY_DRIVER :
> case EFI_OPEN_PROTOCOL_BY_DRIVER | EFI_OPEN_PROTOCOL_EXCLUSIVE :
> Status = CoreValidateHandle (ImageHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
> Status = CoreValidateHandle (ControllerHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
> break;
> case EFI_OPEN_PROTOCOL_EXCLUSIVE :
> Status = CoreValidateHandle (ImageHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
> break;
> case EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL :
> @@ -1057,13 +1066,10 @@ CoreOpenProtocol (
> case EFI_OPEN_PROTOCOL_TEST_PROTOCOL :
> break;
> default:
> - return EFI_INVALID_PARAMETER;
> + Status = EFI_INVALID_PARAMETER;
> + goto Done;
> }
>
> - //
> - // Lock the protocol database
> - //
> - CoreAcquireProtocolLock ();
>
> //
> // Look at each protocol interface for a match
> @@ -1246,32 +1252,33 @@ CoreCloseProtocol (
> LIST_ENTRY *Link;
> OPEN_PROTOCOL_DATA *OpenData;
>
> + //
> + // Lock the protocol database
> + //
> + CoreAcquireProtocolLock ();
> +
> //
> // Check for invalid parameters
> //
> Status = CoreValidateHandle (UserHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
> Status = CoreValidateHandle (AgentHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
> if (ControllerHandle != NULL) {
> Status = CoreValidateHandle (ControllerHandle);
> if (EFI_ERROR (Status)) {
> - return Status;
> + goto Done;
> }
> }
> if (Protocol == NULL) {
> - return EFI_INVALID_PARAMETER;
> + Status = EFI_INVALID_PARAMETER;
> + goto Done;
> }
>
> - //
> - // Lock the protocol database
> - //
> - CoreAcquireProtocolLock ();
> -
> //
> // Look at each protocol interface for a match
> //
> @@ -1443,13 +1450,6 @@ CoreProtocolsPerHandle (
> UINTN ProtocolCount;
> EFI_GUID **Buffer;
>
> - Status = CoreValidateHandle (UserHandle);
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> - Handle = (IHANDLE *)UserHandle;
> -
> if (ProtocolBuffer == NULL) {
> return EFI_INVALID_PARAMETER;
> }
> @@ -1464,6 +1464,13 @@ CoreProtocolsPerHandle (
>
> CoreAcquireProtocolLock ();
>
> + Status = CoreValidateHandle (UserHandle);
> + if (EFI_ERROR (Status)) {
> + goto Done;
> + }
> +
> + Handle = (IHANDLE *)UserHandle;
> +
> for (Link = Handle->Protocols.ForwardLink; Link != &Handle->Protocols; Link =
> Link->ForwardLink) {
> ProtocolCount++;
> }
> diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.h
> b/MdeModulePkg/Core/Dxe/Hand/Handle.h
> index 83eb2b9f3a..3f83e3af15 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
> +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
> @@ -242,6 +242,7 @@ CoreReleaseProtocolLock (
>
> /**
> Check whether a handle is a valid EFI_HANDLE
> + The gProtocolDatabaseLock must be owned
>
> @param UserHandle The handle to check
>
> diff --git a/MdeModulePkg/Core/Dxe/Hand/Notify.c
> b/MdeModulePkg/Core/Dxe/Hand/Notify.c
> index 553413a350..d05f95207f 100644
> --- a/MdeModulePkg/Core/Dxe/Hand/Notify.c
> +++ b/MdeModulePkg/Core/Dxe/Hand/Notify.c
> @@ -188,22 +188,21 @@ CoreReinstallProtocolInterface (
> PROTOCOL_INTERFACE *Prot;
> PROTOCOL_ENTRY *ProtEntry;
>
> - Status = CoreValidateHandle (UserHandle);
> - if (EFI_ERROR (Status)) {
> - return Status;
> - }
> -
> if (Protocol == NULL) {
> return EFI_INVALID_PARAMETER;
> }
>
> - Handle = (IHANDLE *) UserHandle;
> -
> //
> // Lock the protocol database
> //
> CoreAcquireProtocolLock ();
>
> + Status = CoreValidateHandle (UserHandle);
> + if (EFI_ERROR (Status)) {
> + goto Done;
> + }
> +
> + Handle = (IHANDLE *) UserHandle;
> //
> // Check that Protocol exists on UserHandle, and Interface matches the
> interface in the database
> //
> --
> 2.32.0.windows.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList
2021-10-13 8:11 ` Wang, Jian J
@ 2021-10-14 2:21 ` Dandan Bi
0 siblings, 0 replies; 3+ messages in thread
From: Dandan Bi @ 2021-10-14 2:21 UTC (permalink / raw)
To: Wang, Jian J, Ma, Hua, devel@edk2.groups.io; +Cc: Liming Gao, Ni, Ray
Reviewed-by: Dandan Bi <dandan.bi@intel.com>
Thanks,
Dandan
> -----Original Message-----
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Wednesday, October 13, 2021 4:11 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 v3] MdeModulePkg/Core/Dxe: Acquire a lock when
> iterating gHandleList
>
> Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
>
> Regards,
> Jian
>
> > -----Original Message-----
> > From: Ma, Hua <hua.ma@intel.com>
> > Sent: Wednesday, October 13, 2021 3: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 v3] 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.
> >
> > Currently some caller functions of CoreValidateHandle() have
> > CoreAcquireProtocolLock(), but some caller functions of
> > CoreValidateHandle() do not CoreAcquireProtocolLock().
> > Add CoreAcquireProtocolLock() always when CoreValidateHandle() is
> > called, Also, A lock check is added in the CoreValidateHandle().
> >
> > v3 changes:
> > - keep ASSERT_LOCKED(&gProtocolDatabaseLock) in CoreValidateHandle()
> > - Call CoreAcquireProtocolLock() before any calling of
> > CoreValidateHandle() and CoreReleaseProtocolLock() afterwards
> > - Update the commit message
> >
> > 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 | 16 +++++
> > MdeModulePkg/Core/Dxe/Hand/Handle.c | 75 ++++++++++++----------
> > MdeModulePkg/Core/Dxe/Hand/Handle.h | 1 +
> > MdeModulePkg/Core/Dxe/Hand/Notify.c | 13 ++--
> > 4 files changed, 64 insertions(+), 41 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > index feabf12faf..12a202417c 100644
> > --- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > +++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
> > @@ -68,7 +68,12 @@ CoreConnectController (
> > //
> > // Make sure ControllerHandle is valid
> > //
> > + CoreAcquireProtocolLock ();
> > +
> > Status = CoreValidateHandle (ControllerHandle);
> > +
> > + CoreReleaseProtocolLock ();
> > +
> > if (EFI_ERROR (Status)) {
> > return Status;
> > }
> > @@ -268,7 +273,12 @@ AddSortedDriverBindingProtocol (
> > //
> > // Make sure the DriverBindingHandle is valid
> > //
> > + CoreAcquireProtocolLock ();
> > +
> > Status = CoreValidateHandle (DriverBindingHandle);
> > +
> > + CoreReleaseProtocolLock ();
> > +
> > if (EFI_ERROR (Status)) {
> > return;
> > }
> > @@ -746,8 +756,11 @@ CoreDisconnectController (
> > //
> > // Make sure ControllerHandle is valid
> > //
> > + CoreAcquireProtocolLock ();
> > +
> > Status = CoreValidateHandle (ControllerHandle);
> > if (EFI_ERROR (Status)) {
> > + CoreReleaseProtocolLock ();
> > return Status;
> > }
> >
> > @@ -757,10 +770,13 @@ CoreDisconnectController (
> > if (ChildHandle != NULL) {
> > Status = CoreValidateHandle (ChildHandle);
> > if (EFI_ERROR (Status)) {
> > + CoreReleaseProtocolLock ();
> > return Status;
> > }
> > }
> >
> > + CoreReleaseProtocolLock ();
> > +
> > Handle = ControllerHandle;
> >
> > //
> > diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > index 6eccb41ecb..92979281b7 100644
> > --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
> > @@ -53,6 +53,7 @@ CoreReleaseProtocolLock (
> >
> > /**
> > Check whether a handle is a valid EFI_HANDLE
> > + The gProtocolDatabaseLock must be owned
> >
> > @param UserHandle The handle to check
> >
> > @@ -72,6 +73,8 @@ CoreValidateHandle (
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > + ASSERT_LOCKED(&gProtocolDatabaseLock);
> > +
> > for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink)
> {
> > Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
> > if (Handle == (IHANDLE *) UserHandle) { @@ -720,19 +723,19 @@
> > CoreUninstallProtocolInterface (
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > + //
> > + // Lock the protocol database
> > + //
> > + CoreAcquireProtocolLock ();
> > +
> > //
> > // Check that UserHandle is a valid handle
> > //
> > Status = CoreValidateHandle (UserHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> >
> > - //
> > - // Lock the protocol database
> > - //
> > - CoreAcquireProtocolLock ();
> > -
> > //
> > // Check that Protocol exists on UserHandle, and Interface matches
> > the interface in the database
> > //
> > @@ -1010,12 +1013,17 @@ CoreOpenProtocol (
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > + //
> > + // Lock the protocol database
> > + //
> > + CoreAcquireProtocolLock ();
> > +
> > //
> > // Check for invalid UserHandle
> > //
> > Status = CoreValidateHandle (UserHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> >
> > //
> > @@ -1025,31 +1033,32 @@ CoreOpenProtocol (
> > case EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER :
> > Status = CoreValidateHandle (ImageHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> > Status = CoreValidateHandle (ControllerHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> > if (UserHandle == ControllerHandle) {
> > - return EFI_INVALID_PARAMETER;
> > + Status = EFI_INVALID_PARAMETER;
> > + goto Done;
> > }
> > break;
> > case EFI_OPEN_PROTOCOL_BY_DRIVER :
> > case EFI_OPEN_PROTOCOL_BY_DRIVER |
> EFI_OPEN_PROTOCOL_EXCLUSIVE :
> > Status = CoreValidateHandle (ImageHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> > Status = CoreValidateHandle (ControllerHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> > break;
> > case EFI_OPEN_PROTOCOL_EXCLUSIVE :
> > Status = CoreValidateHandle (ImageHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> > break;
> > case EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL :
> > @@ -1057,13 +1066,10 @@ CoreOpenProtocol (
> > case EFI_OPEN_PROTOCOL_TEST_PROTOCOL :
> > break;
> > default:
> > - return EFI_INVALID_PARAMETER;
> > + Status = EFI_INVALID_PARAMETER;
> > + goto Done;
> > }
> >
> > - //
> > - // Lock the protocol database
> > - //
> > - CoreAcquireProtocolLock ();
> >
> > //
> > // Look at each protocol interface for a match @@ -1246,32 +1252,33
> > @@ CoreCloseProtocol (
> > LIST_ENTRY *Link;
> > OPEN_PROTOCOL_DATA *OpenData;
> >
> > + //
> > + // Lock the protocol database
> > + //
> > + CoreAcquireProtocolLock ();
> > +
> > //
> > // Check for invalid parameters
> > //
> > Status = CoreValidateHandle (UserHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> > Status = CoreValidateHandle (AgentHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> > if (ControllerHandle != NULL) {
> > Status = CoreValidateHandle (ControllerHandle);
> > if (EFI_ERROR (Status)) {
> > - return Status;
> > + goto Done;
> > }
> > }
> > if (Protocol == NULL) {
> > - return EFI_INVALID_PARAMETER;
> > + Status = EFI_INVALID_PARAMETER;
> > + goto Done;
> > }
> >
> > - //
> > - // Lock the protocol database
> > - //
> > - CoreAcquireProtocolLock ();
> > -
> > //
> > // Look at each protocol interface for a match
> > //
> > @@ -1443,13 +1450,6 @@ CoreProtocolsPerHandle (
> > UINTN ProtocolCount;
> > EFI_GUID **Buffer;
> >
> > - Status = CoreValidateHandle (UserHandle);
> > - if (EFI_ERROR (Status)) {
> > - return Status;
> > - }
> > -
> > - Handle = (IHANDLE *)UserHandle;
> > -
> > if (ProtocolBuffer == NULL) {
> > return EFI_INVALID_PARAMETER;
> > }
> > @@ -1464,6 +1464,13 @@ CoreProtocolsPerHandle (
> >
> > CoreAcquireProtocolLock ();
> >
> > + Status = CoreValidateHandle (UserHandle); if (EFI_ERROR (Status))
> > + {
> > + goto Done;
> > + }
> > +
> > + Handle = (IHANDLE *)UserHandle;
> > +
> > for (Link = Handle->Protocols.ForwardLink; Link !=
> > &Handle->Protocols; Link =
> > Link->ForwardLink) {
> > ProtocolCount++;
> > }
> > diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.h
> > b/MdeModulePkg/Core/Dxe/Hand/Handle.h
> > index 83eb2b9f3a..3f83e3af15 100644
> > --- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
> > +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
> > @@ -242,6 +242,7 @@ CoreReleaseProtocolLock (
> >
> > /**
> > Check whether a handle is a valid EFI_HANDLE
> > + The gProtocolDatabaseLock must be owned
> >
> > @param UserHandle The handle to check
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Hand/Notify.c
> > b/MdeModulePkg/Core/Dxe/Hand/Notify.c
> > index 553413a350..d05f95207f 100644
> > --- a/MdeModulePkg/Core/Dxe/Hand/Notify.c
> > +++ b/MdeModulePkg/Core/Dxe/Hand/Notify.c
> > @@ -188,22 +188,21 @@ CoreReinstallProtocolInterface (
> > PROTOCOL_INTERFACE *Prot;
> > PROTOCOL_ENTRY *ProtEntry;
> >
> > - Status = CoreValidateHandle (UserHandle);
> > - if (EFI_ERROR (Status)) {
> > - return Status;
> > - }
> > -
> > if (Protocol == NULL) {
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > - Handle = (IHANDLE *) UserHandle;
> > -
> > //
> > // Lock the protocol database
> > //
> > CoreAcquireProtocolLock ();
> >
> > + Status = CoreValidateHandle (UserHandle); if (EFI_ERROR (Status))
> > + {
> > + goto Done;
> > + }
> > +
> > + Handle = (IHANDLE *) UserHandle;
> > //
> > // Check that Protocol exists on UserHandle, and Interface matches
> > the interface in the database
> > //
> > --
> > 2.32.0.windows.2
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-14 2:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-13 7:45 [PATCH v3] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList Ma, Hua
2021-10-13 8:11 ` Wang, Jian J
2021-10-14 2:21 ` Dandan Bi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox