Reviewed-by: Dandan Bi <dandan...@intel.com>
Thanks, Dandan > -----Original Message----- > From: Wang, Jian J <jian.j.w...@intel.com> > Sent: Wednesday, October 13, 2021 4:11 PM > To: Ma, Hua <hua...@intel.com>; devel@edk2.groups.io > Cc: Liming Gao <gaolim...@byosoft.com.cn>; Bi, Dandan > <dandan...@intel.com>; Ni, Ray <ray...@intel.com> > Subject: RE: [PATCH v3] MdeModulePkg/Core/Dxe: Acquire a lock when > iterating gHandleList > > Reviewed-by: Jian J Wang <jian.j.w...@intel.com> > > Regards, > Jian > > > -----Original Message----- > > From: Ma, Hua <hua...@intel.com> > > Sent: Wednesday, October 13, 2021 3:45 PM > > To: devel@edk2.groups.io > > Cc: Ma, Hua <hua...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; > > Liming Gao <gaolim...@byosoft.com.cn>; Bi, Dandan > > <dandan...@intel.com>; Ni, Ray <ray...@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.w...@intel.com> > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > Cc: Dandan Bi <dandan...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Signed-off-by: Hua Ma <hua...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#81931): https://edk2.groups.io/g/devel/message/81931 Mute This Topic: https://groups.io/mt/86282804/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
