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 (#81866): https://edk2.groups.io/g/devel/message/81866 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] -=-=-=-=-=-=-=-=-=-=-=-
