* [edk2-devel] [PATCH v4 0/6] Support to unregister SMI handler in SMI handlers
@ 2024-04-16 2:41 Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 1/6] Revert 2ec8f0c6407f062441b205b900038933865c7b3c Zhiguang Liu
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: Zhiguang Liu @ 2024-04-16 2:41 UTC (permalink / raw)
To: devel; +Cc: Zhiguang Liu
Months ago, I sent patch set to fix potential issues in the usage of SMI handler unregistering SMI handler. Discussion can be found in below
link:
https://edk2.groups.io/g/devel/topic/103925794#114251
The conclusion was to only support SMI handler unregistering itself, and not allow SMI handler unregistering other handlers, because we thought there would be no such usage.
However, recently, I find in some platform, there is kind of usage.
To also support SMI handler unregistering other handlers, this patch set use a totally different design. So I revert the former ones first to make the code more readable.
Thank Laszlo for bring up the initial idea for the new design.
V2&V3:
Code refine based on Ray's comments
V4:
Add NULL pointer check to pass CI
Refine commit message
Zhiguang Liu (6):
Revert 2ec8f0c6407f062441b205b900038933865c7b3c
Revert 049ff6c39c73edd3709c05bd0e46184320471358
Revert 17b28722008eab745ce186b72cd325944cbe6bf0
Revert ae1079b386a597108a8070652bf7cdaa4ec3dda3
MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers
StandaloneMmPkg: Support to unregister MMI handler in MMI handlers
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 1 +
MdeModulePkg/Core/PiSmmCore/Smi.c | 201 +++++++++++++++++-------
StandaloneMmPkg/Core/Mmi.c | 200 ++++++++++++++++-------
3 files changed, 294 insertions(+), 108 deletions(-)
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117817): https://edk2.groups.io/g/devel/message/117817
Mute This Topic: https://groups.io/mt/105550114/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH v4 1/6] Revert 2ec8f0c6407f062441b205b900038933865c7b3c
2024-04-16 2:41 [edk2-devel] [PATCH v4 0/6] Support to unregister SMI handler in SMI handlers Zhiguang Liu
@ 2024-04-16 2:41 ` Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 2/6] Revert 049ff6c39c73edd3709c05bd0e46184320471358 Zhiguang Liu
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Zhiguang Liu @ 2024-04-16 2:41 UTC (permalink / raw)
To: devel
Cc: Zhiguang Liu, Liming Gao, Jiaxin Wu, Ray Ni, Laszlo Ersek,
Ard Biesheuvel, Sami Mujawar
This reverts commit "StandaloneMmPkg: Disallow unregister MMI
handler in other MMI handler" for better design later.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
StandaloneMmPkg/Core/Mmi.c | 32 +++++++++-----------------------
1 file changed, 9 insertions(+), 23 deletions(-)
diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index 9e52072bf7..c1a1d76e85 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -36,9 +36,8 @@ typedef struct {
MMI_ENTRY *MmiEntry;
} MMI_HANDLER;
-LIST_ENTRY mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
-LIST_ENTRY mMmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
-MMI_HANDLER *mCurrentMmiHandler = NULL;
+LIST_ENTRY mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
+LIST_ENTRY mMmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
/**
Finds the MMI entry for the requested handler type.
@@ -162,19 +161,13 @@ MmiManage (
// get next node before handler is executed, since LIST_ENTRY that
// Link points to may be freed if unregister MMI handler.
//
- Link = Link->ForwardLink;
- //
- // Assign gCurrentMmiHandle before calling the MMI handler and
- // set to NULL when it returns.
- //
- mCurrentMmiHandler = MmiHandler;
- Status = MmiHandler->Handler (
- (EFI_HANDLE)MmiHandler,
- Context,
- CommBuffer,
- CommBufferSize
- );
- mCurrentMmiHandler = NULL;
+ Link = Link->ForwardLink;
+ Status = MmiHandler->Handler (
+ (EFI_HANDLE)MmiHandler,
+ Context,
+ CommBuffer,
+ CommBufferSize
+ );
switch (Status) {
case EFI_INTERRUPT_PENDING:
@@ -321,13 +314,6 @@ MmiHandlerUnRegister (
return EFI_INVALID_PARAMETER;
}
- //
- // Do not allow to unregister MMI Handler inside other MMI Handler
- //
- if ((mCurrentMmiHandler != NULL) && (mCurrentMmiHandler != MmiHandler)) {
- return EFI_INVALID_PARAMETER;
- }
-
MmiEntry = MmiHandler->MmiEntry;
RemoveEntryList (&MmiHandler->Link);
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117818): https://edk2.groups.io/g/devel/message/117818
Mute This Topic: https://groups.io/mt/105550115/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH v4 2/6] Revert 049ff6c39c73edd3709c05bd0e46184320471358
2024-04-16 2:41 [edk2-devel] [PATCH v4 0/6] Support to unregister SMI handler in SMI handlers Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 1/6] Revert 2ec8f0c6407f062441b205b900038933865c7b3c Zhiguang Liu
@ 2024-04-16 2:41 ` Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 3/6] Revert 17b28722008eab745ce186b72cd325944cbe6bf0 Zhiguang Liu
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Zhiguang Liu @ 2024-04-16 2:41 UTC (permalink / raw)
To: devel
Cc: Zhiguang Liu, Liming Gao, Jiaxin Wu, Ray Ni, Laszlo Ersek,
Ard Biesheuvel, Sami Mujawar
This reverts commit "StandaloneMmPkg: Support to unregister
MMI handler inside MMI handler" for better design later.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
StandaloneMmPkg/Core/Mmi.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index c1a1d76e85..0de6fd17fc 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -154,14 +154,9 @@ MmiManage (
Head = &MmiEntry->MmiHandlers;
}
- for (Link = Head->ForwardLink; Link != Head;) {
+ for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
- //
- // To support unregister MMI handler inside MMI handler itself,
- // get next node before handler is executed, since LIST_ENTRY that
- // Link points to may be freed if unregister MMI handler.
- //
- Link = Link->ForwardLink;
+
Status = MmiHandler->Handler (
(EFI_HANDLE)MmiHandler,
Context,
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117819): https://edk2.groups.io/g/devel/message/117819
Mute This Topic: https://groups.io/mt/105550116/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH v4 3/6] Revert 17b28722008eab745ce186b72cd325944cbe6bf0
2024-04-16 2:41 [edk2-devel] [PATCH v4 0/6] Support to unregister SMI handler in SMI handlers Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 1/6] Revert 2ec8f0c6407f062441b205b900038933865c7b3c Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 2/6] Revert 049ff6c39c73edd3709c05bd0e46184320471358 Zhiguang Liu
@ 2024-04-16 2:41 ` Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 4/6] Revert ae1079b386a597108a8070652bf7cdaa4ec3dda3 Zhiguang Liu
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Zhiguang Liu @ 2024-04-16 2:41 UTC (permalink / raw)
To: devel
Cc: Zhiguang Liu, Liming Gao, Jiaxin Wu, Ray Ni, Laszlo Ersek,
Ard Biesheuvel, Sami Mujawar
This reverts commit "MdeModulePkg/SMM: Disallow unregister
SMI handler in other SMI handler" for better design later.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Core/PiSmmCore/Smi.c | 29 ++++++++---------------------
1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
index b3a81ac877..3489c130fd 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -8,8 +8,7 @@
#include "PiSmmCore.h"
-SMI_HANDLER *mCurrentSmiHandler = NULL;
-LIST_ENTRY mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
+LIST_ENTRY mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
SMI_ENTRY mRootSmiEntry = {
SMI_ENTRY_SIGNATURE,
@@ -143,18 +142,13 @@ SmiManage (
// Link points to may be freed if unregister SMI handler.
//
Link = Link->ForwardLink;
- //
- // Assign gCurrentSmiHandle before calling the SMI handler and
- // set to NULL when it returns.
- //
- mCurrentSmiHandler = SmiHandler;
- Status = SmiHandler->Handler (
- (EFI_HANDLE)SmiHandler,
- Context,
- CommBuffer,
- CommBufferSize
- );
- mCurrentSmiHandler = NULL;
+
+ Status = SmiHandler->Handler (
+ (EFI_HANDLE)SmiHandler,
+ Context,
+ CommBuffer,
+ CommBufferSize
+ );
switch (Status) {
case EFI_INTERRUPT_PENDING:
@@ -334,13 +328,6 @@ SmiHandlerUnRegister (
return EFI_INVALID_PARAMETER;
}
- //
- // Do not allow to unregister SMI Handler inside other SMI Handler
- //
- if ((mCurrentSmiHandler != NULL) && (mCurrentSmiHandler != SmiHandler)) {
- return EFI_INVALID_PARAMETER;
- }
-
SmiEntry = SmiHandler->SmiEntry;
RemoveEntryList (&SmiHandler->Link);
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117820): https://edk2.groups.io/g/devel/message/117820
Mute This Topic: https://groups.io/mt/105550117/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH v4 4/6] Revert ae1079b386a597108a8070652bf7cdaa4ec3dda3
2024-04-16 2:41 [edk2-devel] [PATCH v4 0/6] Support to unregister SMI handler in SMI handlers Zhiguang Liu
` (2 preceding siblings ...)
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 3/6] Revert 17b28722008eab745ce186b72cd325944cbe6bf0 Zhiguang Liu
@ 2024-04-16 2:41 ` Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers Zhiguang Liu
5 siblings, 0 replies; 9+ messages in thread
From: Zhiguang Liu @ 2024-04-16 2:41 UTC (permalink / raw)
To: devel
Cc: Zhiguang Liu, Liming Gao, Jiaxin Wu, Ray Ni, Laszlo Ersek,
Ard Biesheuvel, Sami Mujawar
This reverts "MdeModulePkg/SMM: Support to unregister
SMI handler inside SMI handler" for better design later.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Core/PiSmmCore/Smi.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
index 3489c130fd..2985f989c3 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -134,14 +134,8 @@ SmiManage (
Head = &SmiEntry->SmiHandlers;
- for (Link = Head->ForwardLink; Link != Head;) {
+ for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
- //
- // To support unregister SMI handler inside SMI handler itself,
- // get next node before handler is executed, since LIST_ENTRY that
- // Link points to may be freed if unregister SMI handler.
- //
- Link = Link->ForwardLink;
Status = SmiHandler->Handler (
(EFI_HANDLE)SmiHandler,
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117821): https://edk2.groups.io/g/devel/message/117821
Mute This Topic: https://groups.io/mt/105550118/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers
2024-04-16 2:41 [edk2-devel] [PATCH v4 0/6] Support to unregister SMI handler in SMI handlers Zhiguang Liu
` (3 preceding siblings ...)
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 4/6] Revert ae1079b386a597108a8070652bf7cdaa4ec3dda3 Zhiguang Liu
@ 2024-04-16 2:41 ` Zhiguang Liu
2024-04-16 3:52 ` Ni, Ray
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers Zhiguang Liu
5 siblings, 1 reply; 9+ messages in thread
From: Zhiguang Liu @ 2024-04-16 2:41 UTC (permalink / raw)
To: devel; +Cc: Zhiguang Liu, Liming Gao, Jiaxin Wu, Ray Ni, Laszlo Ersek
This patch fix a use-after-free issue where unregistering an
SMI handler could lead to the deletion of the SMI_HANDLER while it is
still in use by SmiManage(). The fix involves modifying
SmiHandlerUnRegister() to detect whether it is being called from
within the SmiManage() stack. If so, the removal of the SMI_HANDLER
is deferred until SmiManage() has finished executing.
Additionally, due to the possibility of recursive SmiManage() calls,
the unregistration and subsequent removal of the SMI_HANDLER are
ensured to occur only after the outermost SmiManage() invocation has
completed.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 1 +
MdeModulePkg/Core/PiSmmCore/Smi.c | 164 ++++++++++++++++++++----
2 files changed, 139 insertions(+), 26 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
index b8a490a8c3..60073c78b4 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
@@ -93,6 +93,7 @@ typedef struct {
SMI_ENTRY *SmiEntry;
VOID *Context; // for profile
UINTN ContextSize; // for profile
+ BOOLEAN ToRemove; // To remove this SMI_HANDLER later
} SMI_HANDLER;
//
diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
index 2985f989c3..a84a1f48d3 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -8,6 +8,11 @@
#include "PiSmmCore.h"
+//
+// mSmiManageCallingDepth is used to track the depth of recursive calls of SmiManage.
+//
+UINTN mSmiManageCallingDepth = 0;
+
LIST_ENTRY mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
SMI_ENTRY mRootSmiEntry = {
@@ -79,6 +84,40 @@ SmmCoreFindSmiEntry (
return SmiEntry;
}
+/**
+ Remove SmiHandler and free the memory it used.
+ If SmiEntry is empty, remove SmiEntry and free the memory it used.
+
+ @param SmiHandler Points to SMI handler.
+ @param SmiEntry Points to SMI Entry or NULL for root SMI handlers.
+
+ @retval TRUE SmiEntry is removed.
+ @retval FALSE SmiEntry is not removed.
+**/
+BOOLEAN
+RemoveSmiHandler (
+ IN SMI_HANDLER *SmiHandler,
+ IN SMI_ENTRY *SmiEntry
+ )
+{
+ ASSERT (SmiHandler->ToRemove);
+ RemoveEntryList (&SmiHandler->Link);
+ FreePool (SmiHandler);
+
+ //
+ // Remove the SMI_ENTRY if all handlers have been removed.
+ //
+ if (SmiEntry != NULL) {
+ if (IsListEmpty (&SmiEntry->SmiHandlers)) {
+ RemoveEntryList (&SmiEntry->AllEntries);
+ FreePool (SmiEntry);
+ return TRUE;
+ }
+ }
+
+ return FALSE;
+}
+
/**
Manage SMI of a particular type.
@@ -104,15 +143,17 @@ SmiManage (
{
LIST_ENTRY *Link;
LIST_ENTRY *Head;
+ LIST_ENTRY *EntryLink;
SMI_ENTRY *SmiEntry;
SMI_HANDLER *SmiHandler;
- BOOLEAN SuccessReturn;
+ EFI_STATUS ReturnStatus;
+ BOOLEAN WillReturn;
EFI_STATUS Status;
PERF_FUNCTION_BEGIN ();
-
- Status = EFI_NOT_FOUND;
- SuccessReturn = FALSE;
+ mSmiManageCallingDepth++;
+ Status = EFI_NOT_FOUND;
+ ReturnStatus = Status;
if (HandlerType == NULL) {
//
// Root SMI handler
@@ -152,7 +193,16 @@ SmiManage (
//
if (HandlerType != NULL) {
PERF_FUNCTION_END ();
- return EFI_INTERRUPT_PENDING;
+ ReturnStatus = EFI_INTERRUPT_PENDING;
+ WillReturn = TRUE;
+ } else {
+ //
+ // If any other handler's result sets ReturnStatus as EFI_SUCCESS, the return status
+ // will be EFI_SUCCESS.
+ //
+ if (ReturnStatus != EFI_SUCCESS) {
+ ReturnStatus = Status;
+ }
}
break;
@@ -165,10 +215,10 @@ SmiManage (
//
if (HandlerType != NULL) {
PERF_FUNCTION_END ();
- return EFI_SUCCESS;
+ WillReturn = TRUE;
}
- SuccessReturn = TRUE;
+ ReturnStatus = EFI_SUCCESS;
break;
case EFI_WARN_INTERRUPT_SOURCE_QUIESCED:
@@ -176,7 +226,7 @@ SmiManage (
// If at least one of the handlers returns EFI_WARN_INTERRUPT_SOURCE_QUIESCED
// then the function will return EFI_SUCCESS.
//
- SuccessReturn = TRUE;
+ ReturnStatus = EFI_SUCCESS;
break;
case EFI_WARN_INTERRUPT_SOURCE_PENDING:
@@ -184,6 +234,10 @@ SmiManage (
// If all the handlers returned EFI_WARN_INTERRUPT_SOURCE_PENDING
// then EFI_WARN_INTERRUPT_SOURCE_PENDING will be returned.
//
+ if (ReturnStatus != EFI_SUCCESS) {
+ ReturnStatus = Status;
+ }
+
break;
default:
@@ -193,14 +247,74 @@ SmiManage (
ASSERT (FALSE);
break;
}
+
+ if (WillReturn) {
+ break;
+ }
}
- if (SuccessReturn) {
- Status = EFI_SUCCESS;
+ ASSERT (mSmiManageCallingDepth > 0);
+ mSmiManageCallingDepth--;
+
+ //
+ // SmiHandlerUnRegister() calls from SMI handlers are deferred till this point.
+ // Before returned from SmiManage, delete the SmiHandler which is
+ // marked as ToRemove.
+ // Note that SmiManage can be called recursively.
+ //
+ if (mSmiManageCallingDepth == 0) {
+ //
+ // Go through all SmiHandler in root SMI handlers
+ //
+ for ( Link = GetFirstNode (&mRootSmiEntry.SmiHandlers)
+ ; !IsNull (&mRootSmiEntry.SmiHandlers, Link);
+ )
+ {
+ //
+ // SmiHandler might be removed in below, so cache the next link in Link
+ //
+ SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
+ Link = GetNextNode (&mRootSmiEntry.SmiHandlers, Link);
+ if (SmiHandler->ToRemove) {
+ //
+ // Remove SmiHandler if the ToRemove is set.
+ //
+ RemoveSmiHandler (SmiHandler, NULL);
+ }
+ }
+
+ //
+ // Go through all SmiHandler in non-root SMI handlers
+ //
+ for ( EntryLink = GetFirstNode (&mSmiEntryList)
+ ; !IsNull (&mSmiEntryList, EntryLink);
+ )
+ {
+ //
+ // SmiEntry might be removed in below, so cache the next link in EntryLink
+ //
+ SmiEntry = CR (EntryLink, SMI_ENTRY, AllEntries, SMI_ENTRY_SIGNATURE);
+ EntryLink = GetNextNode (&mSmiEntryList, EntryLink);
+ for ( Link = GetFirstNode (&SmiEntry->SmiHandlers)
+ ; !IsNull (&SmiEntry->SmiHandlers, Link);
+ )
+ {
+ //
+ // SmiHandler might be removed in below, so cache the next link in Link
+ //
+ SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
+ Link = GetNextNode (&SmiEntry->SmiHandlers, Link);
+ if (SmiHandler->ToRemove) {
+ if (RemoveSmiHandler (SmiHandler, SmiEntry)) {
+ break;
+ }
+ }
+ }
+ }
}
PERF_FUNCTION_END ();
- return Status;
+ return ReturnStatus;
}
/**
@@ -238,6 +352,7 @@ SmiHandlerRegister (
SmiHandler->Signature = SMI_HANDLER_SIGNATURE;
SmiHandler->Handler = Handler;
SmiHandler->CallerAddr = (UINTN)RETURN_ADDRESS (0);
+ SmiHandler->ToRemove = FALSE;
if (HandlerType == NULL) {
//
@@ -318,30 +433,27 @@ SmiHandlerUnRegister (
}
}
- if ((EFI_HANDLE)SmiHandler != DispatchHandle) {
+ if (SmiHandler == NULL) {
return EFI_INVALID_PARAMETER;
}
- SmiEntry = SmiHandler->SmiEntry;
-
- RemoveEntryList (&SmiHandler->Link);
- FreePool (SmiHandler);
+ ASSERT ((EFI_HANDLE)SmiHandler == DispatchHandle);
+ SmiHandler->ToRemove = TRUE;
- if (SmiEntry == NULL) {
+ if (mSmiManageCallingDepth > 0) {
//
- // This is root SMI handler
+ // This function is called from SmiManage()
+ // Do not delete or remove SmiHandler or SmiEntry now.
+ // SmiManage will handle it later
//
return EFI_SUCCESS;
}
- if (IsListEmpty (&SmiEntry->SmiHandlers)) {
- //
- // No handler registered for this interrupt now, remove the SMI_ENTRY
- //
- RemoveEntryList (&SmiEntry->AllEntries);
-
- FreePool (SmiEntry);
- }
+ SmiEntry = SmiHandler->SmiEntry;
+ //
+ // For root SMI handler, use NULL for SmiEntry
+ //
+ RemoveSmiHandler (SmiHandler, (SmiEntry == &mRootSmiEntry) ? NULL : SmiEntry);
return EFI_SUCCESS;
}
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117822): https://edk2.groups.io/g/devel/message/117822
Mute This Topic: https://groups.io/mt/105550121/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers
2024-04-16 2:41 [edk2-devel] [PATCH v4 0/6] Support to unregister SMI handler in SMI handlers Zhiguang Liu
` (4 preceding siblings ...)
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers Zhiguang Liu
@ 2024-04-16 2:41 ` Zhiguang Liu
2024-04-16 3:52 ` Ni, Ray
5 siblings, 1 reply; 9+ messages in thread
From: Zhiguang Liu @ 2024-04-16 2:41 UTC (permalink / raw)
To: devel
Cc: Zhiguang Liu, Liming Gao, Jiaxin Wu, Ray Ni, Laszlo Ersek,
Ard Biesheuvel, Sami Mujawar
This patch fix a use-after-free issue where unregistering an
MMI handler could lead to the deletion of the MMI_HANDLER while it is
still in use by MmiManage(). The fix involves modifying
MmiHandlerUnRegister() to detect whether it is being called from
within the MmiManage() stack. If so, the removal of the MMI_HANDLER
is deferred until MmiManage() has finished executing.
Additionally, due to the possibility of recursive MmiManage() calls,
the unregistration and subsequent removal of the MMI_HANDLER are
ensured to occur only after the outermost MmiManage() invocation has
completed.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
StandaloneMmPkg/Core/Mmi.c | 161 +++++++++++++++++++++++++++++++------
1 file changed, 136 insertions(+), 25 deletions(-)
diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index 0de6fd17fc..e035245c87 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -34,11 +34,51 @@ typedef struct {
LIST_ENTRY Link; // Link on MMI_ENTRY.MmiHandlers
EFI_MM_HANDLER_ENTRY_POINT Handler; // The mm handler's entry point
MMI_ENTRY *MmiEntry;
+ BOOLEAN ToRemove; // To remove this MMI_HANDLER later
} MMI_HANDLER;
+//
+// mMmiManageCallingDepth is used to track the depth of recursive calls of MmiManage.
+//
+UINTN mMmiManageCallingDepth = 0;
+
LIST_ENTRY mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
LIST_ENTRY mMmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
+/**
+ Remove MmiHandler and free the memory it used.
+ If MmiEntry is empty, remove MmiEntry and free the memory it used.
+
+ @param MmiHandler Points to MMI handler.
+ @param MmiEntry Points to MMI Entry or NULL for root MMI handlers.
+
+ @retval TRUE MmiEntry is removed.
+ @retval FALSE MmiEntry is not removed.
+**/
+BOOLEAN
+RemoveMmiHandler (
+ IN MMI_HANDLER *MmiHandler,
+ IN MMI_ENTRY *MmiEntry
+ )
+{
+ ASSERT (MmiHandler->ToRemove);
+ RemoveEntryList (&MmiHandler->Link);
+ FreePool (MmiHandler);
+
+ //
+ // Remove the MMI_ENTRY if all handlers have been removed.
+ //
+ if (MmiEntry != NULL) {
+ if (IsListEmpty (&MmiEntry->MmiHandlers)) {
+ RemoveEntryList (&MmiEntry->AllEntries);
+ FreePool (MmiEntry);
+ return TRUE;
+ }
+ }
+
+ return FALSE;
+}
+
/**
Finds the MMI entry for the requested handler type.
@@ -126,13 +166,16 @@ MmiManage (
{
LIST_ENTRY *Link;
LIST_ENTRY *Head;
+ LIST_ENTRY *EntryLink;
MMI_ENTRY *MmiEntry;
MMI_HANDLER *MmiHandler;
- BOOLEAN SuccessReturn;
+ EFI_STATUS ReturnStatus;
+ BOOLEAN WillReturn;
EFI_STATUS Status;
- Status = EFI_NOT_FOUND;
- SuccessReturn = FALSE;
+ mMmiManageCallingDepth++;
+ Status = EFI_NOT_FOUND;
+ ReturnStatus = Status;
if (HandlerType == NULL) {
//
// Root MMI handler
@@ -171,7 +214,16 @@ MmiManage (
// no additional handlers will be processed and EFI_INTERRUPT_PENDING will be returned.
//
if (HandlerType != NULL) {
- return EFI_INTERRUPT_PENDING;
+ ReturnStatus = EFI_INTERRUPT_PENDING;
+ WillReturn = TRUE;
+ } else {
+ //
+ // If any other handler's result sets ReturnStatus as EFI_SUCCESS, the return status
+ // will be EFI_SUCCESS.
+ //
+ if (ReturnStatus != EFI_SUCCESS) {
+ ReturnStatus = Status;
+ }
}
break;
@@ -183,10 +235,10 @@ MmiManage (
// additional handlers will be processed.
//
if (HandlerType != NULL) {
- return EFI_SUCCESS;
+ WillReturn = TRUE;
}
- SuccessReturn = TRUE;
+ ReturnStatus = EFI_SUCCESS;
break;
case EFI_WARN_INTERRUPT_SOURCE_QUIESCED:
@@ -194,7 +246,7 @@ MmiManage (
// If at least one of the handlers returns EFI_WARN_INTERRUPT_SOURCE_QUIESCED
// then the function will return EFI_SUCCESS.
//
- SuccessReturn = TRUE;
+ ReturnStatus = EFI_SUCCESS;
break;
case EFI_WARN_INTERRUPT_SOURCE_PENDING:
@@ -202,6 +254,10 @@ MmiManage (
// If all the handlers returned EFI_WARN_INTERRUPT_SOURCE_PENDING
// then EFI_WARN_INTERRUPT_SOURCE_PENDING will be returned.
//
+ if (ReturnStatus != EFI_SUCCESS) {
+ ReturnStatus = Status;
+ }
+
break;
default:
@@ -211,13 +267,76 @@ MmiManage (
ASSERT_EFI_ERROR (Status);
break;
}
+
+ if (WillReturn) {
+ break;
+ }
}
- if (SuccessReturn) {
- Status = EFI_SUCCESS;
+ ASSERT (mMmiManageCallingDepth > 0);
+ mMmiManageCallingDepth--;
+
+ //
+ // MmiHandlerUnRegister() calls from MMI handlers are deferred till this point.
+ // Before returned from MmiManage, delete the MmiHandler which is
+ // marked as ToRemove.
+ // Note that MmiManage can be called recursively.
+ //
+ if (mMmiManageCallingDepth == 0) {
+ //
+ // Go through all MmiHandler in root Mmi handlers
+ //
+ for ( Link = GetFirstNode (&mRootMmiHandlerList)
+ ; !IsNull (&mRootMmiHandlerList, Link);
+ )
+ {
+ //
+ // MmiHandler might be removed in below, so cache the next link in Link
+ //
+ MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
+ Link = GetNextNode (&mRootMmiHandlerList, Link);
+ if (MmiHandler->ToRemove) {
+ //
+ // Remove MmiHandler if the ToRemove is set.
+ //
+ RemoveMmiHandler (MmiHandler, NULL);
+ }
+ }
+
+ //
+ // Go through all MmiHandler in non-root MMI handlers
+ //
+ for ( EntryLink = GetFirstNode (&mMmiEntryList)
+ ; !IsNull (&mMmiEntryList, EntryLink);
+ )
+ {
+ //
+ // MmiEntry might be removed in below, so cache the next link in EntryLink
+ //
+ MmiEntry = CR (EntryLink, MMI_ENTRY, AllEntries, MMI_ENTRY_SIGNATURE);
+ EntryLink = GetNextNode (&mMmiEntryList, EntryLink);
+ for ( Link = GetFirstNode (&MmiEntry->MmiHandlers)
+ ; !IsNull (&MmiEntry->MmiHandlers, Link);
+ )
+ {
+ //
+ // MmiHandler might be removed in below, so cache the next link in Link
+ //
+ MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
+ Link = GetNextNode (&MmiEntry->MmiHandlers, Link);
+ if (MmiHandler->ToRemove) {
+ //
+ // Remove MmiHandler if the ToRemove is set.
+ //
+ if (RemoveMmiHandler (MmiHandler, MmiEntry)) {
+ break;
+ }
+ }
+ }
+ }
}
- return Status;
+ return ReturnStatus;
}
/**
@@ -254,6 +373,7 @@ MmiHandlerRegister (
MmiHandler->Signature = MMI_HANDLER_SIGNATURE;
MmiHandler->Handler = Handler;
+ MmiHandler->ToRemove = FALSE;
if (HandlerType == NULL) {
//
@@ -309,26 +429,17 @@ MmiHandlerUnRegister (
return EFI_INVALID_PARAMETER;
}
- MmiEntry = MmiHandler->MmiEntry;
-
- RemoveEntryList (&MmiHandler->Link);
- FreePool (MmiHandler);
-
- if (MmiEntry == NULL) {
+ MmiHandler->ToRemove = TRUE;
+ if (mMmiManageCallingDepth > 0) {
//
- // This is root MMI handler
+ // This function is called from MmiManage()
+ // Do not delete or remove MmiHandler or MmiEntry now.
//
return EFI_SUCCESS;
}
- if (IsListEmpty (&MmiEntry->MmiHandlers)) {
- //
- // No handler registered for this interrupt now, remove the MMI_ENTRY
- //
- RemoveEntryList (&MmiEntry->AllEntries);
-
- FreePool (MmiEntry);
- }
+ MmiEntry = MmiHandler->MmiEntry;
+ RemoveMmiHandler (MmiHandler, MmiEntry);
return EFI_SUCCESS;
}
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117823): https://edk2.groups.io/g/devel/message/117823
Mute This Topic: https://groups.io/mt/105550122/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers Zhiguang Liu
@ 2024-04-16 3:52 ` Ni, Ray
0 siblings, 0 replies; 9+ messages in thread
From: Ni, Ray @ 2024-04-16 3:52 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io
Cc: Liming Gao, Wu, Jiaxin, Laszlo Ersek, Ard Biesheuvel,
Sami Mujawar
[-- Attachment #1: Type: text/plain, Size: 9277 bytes --]
Reviewed-by: Ray Ni <ray.ni@intel.com>
Thanks,
Ray
________________________________
From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Tuesday, April 16, 2024 10:41
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>
Subject: [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers
This patch fix a use-after-free issue where unregistering an
MMI handler could lead to the deletion of the MMI_HANDLER while it is
still in use by MmiManage(). The fix involves modifying
MmiHandlerUnRegister() to detect whether it is being called from
within the MmiManage() stack. If so, the removal of the MMI_HANDLER
is deferred until MmiManage() has finished executing.
Additionally, due to the possibility of recursive MmiManage() calls,
the unregistration and subsequent removal of the MMI_HANDLER are
ensured to occur only after the outermost MmiManage() invocation has
completed.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
StandaloneMmPkg/Core/Mmi.c | 161 +++++++++++++++++++++++++++++++------
1 file changed, 136 insertions(+), 25 deletions(-)
diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index 0de6fd17fc..e035245c87 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -34,11 +34,51 @@ typedef struct {
LIST_ENTRY Link; // Link on MMI_ENTRY.MmiHandlers
EFI_MM_HANDLER_ENTRY_POINT Handler; // The mm handler's entry point
MMI_ENTRY *MmiEntry;
+ BOOLEAN ToRemove; // To remove this MMI_HANDLER later
} MMI_HANDLER;
+//
+// mMmiManageCallingDepth is used to track the depth of recursive calls of MmiManage.
+//
+UINTN mMmiManageCallingDepth = 0;
+
LIST_ENTRY mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
LIST_ENTRY mMmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
+/**
+ Remove MmiHandler and free the memory it used.
+ If MmiEntry is empty, remove MmiEntry and free the memory it used.
+
+ @param MmiHandler Points to MMI handler.
+ @param MmiEntry Points to MMI Entry or NULL for root MMI handlers.
+
+ @retval TRUE MmiEntry is removed.
+ @retval FALSE MmiEntry is not removed.
+**/
+BOOLEAN
+RemoveMmiHandler (
+ IN MMI_HANDLER *MmiHandler,
+ IN MMI_ENTRY *MmiEntry
+ )
+{
+ ASSERT (MmiHandler->ToRemove);
+ RemoveEntryList (&MmiHandler->Link);
+ FreePool (MmiHandler);
+
+ //
+ // Remove the MMI_ENTRY if all handlers have been removed.
+ //
+ if (MmiEntry != NULL) {
+ if (IsListEmpty (&MmiEntry->MmiHandlers)) {
+ RemoveEntryList (&MmiEntry->AllEntries);
+ FreePool (MmiEntry);
+ return TRUE;
+ }
+ }
+
+ return FALSE;
+}
+
/**
Finds the MMI entry for the requested handler type.
@@ -126,13 +166,16 @@ MmiManage (
{
LIST_ENTRY *Link;
LIST_ENTRY *Head;
+ LIST_ENTRY *EntryLink;
MMI_ENTRY *MmiEntry;
MMI_HANDLER *MmiHandler;
- BOOLEAN SuccessReturn;
+ EFI_STATUS ReturnStatus;
+ BOOLEAN WillReturn;
EFI_STATUS Status;
- Status = EFI_NOT_FOUND;
- SuccessReturn = FALSE;
+ mMmiManageCallingDepth++;
+ Status = EFI_NOT_FOUND;
+ ReturnStatus = Status;
if (HandlerType == NULL) {
//
// Root MMI handler
@@ -171,7 +214,16 @@ MmiManage (
// no additional handlers will be processed and EFI_INTERRUPT_PENDING will be returned.
//
if (HandlerType != NULL) {
- return EFI_INTERRUPT_PENDING;
+ ReturnStatus = EFI_INTERRUPT_PENDING;
+ WillReturn = TRUE;
+ } else {
+ //
+ // If any other handler's result sets ReturnStatus as EFI_SUCCESS, the return status
+ // will be EFI_SUCCESS.
+ //
+ if (ReturnStatus != EFI_SUCCESS) {
+ ReturnStatus = Status;
+ }
}
break;
@@ -183,10 +235,10 @@ MmiManage (
// additional handlers will be processed.
//
if (HandlerType != NULL) {
- return EFI_SUCCESS;
+ WillReturn = TRUE;
}
- SuccessReturn = TRUE;
+ ReturnStatus = EFI_SUCCESS;
break;
case EFI_WARN_INTERRUPT_SOURCE_QUIESCED:
@@ -194,7 +246,7 @@ MmiManage (
// If at least one of the handlers returns EFI_WARN_INTERRUPT_SOURCE_QUIESCED
// then the function will return EFI_SUCCESS.
//
- SuccessReturn = TRUE;
+ ReturnStatus = EFI_SUCCESS;
break;
case EFI_WARN_INTERRUPT_SOURCE_PENDING:
@@ -202,6 +254,10 @@ MmiManage (
// If all the handlers returned EFI_WARN_INTERRUPT_SOURCE_PENDING
// then EFI_WARN_INTERRUPT_SOURCE_PENDING will be returned.
//
+ if (ReturnStatus != EFI_SUCCESS) {
+ ReturnStatus = Status;
+ }
+
break;
default:
@@ -211,13 +267,76 @@ MmiManage (
ASSERT_EFI_ERROR (Status);
break;
}
+
+ if (WillReturn) {
+ break;
+ }
}
- if (SuccessReturn) {
- Status = EFI_SUCCESS;
+ ASSERT (mMmiManageCallingDepth > 0);
+ mMmiManageCallingDepth--;
+
+ //
+ // MmiHandlerUnRegister() calls from MMI handlers are deferred till this point.
+ // Before returned from MmiManage, delete the MmiHandler which is
+ // marked as ToRemove.
+ // Note that MmiManage can be called recursively.
+ //
+ if (mMmiManageCallingDepth == 0) {
+ //
+ // Go through all MmiHandler in root Mmi handlers
+ //
+ for ( Link = GetFirstNode (&mRootMmiHandlerList)
+ ; !IsNull (&mRootMmiHandlerList, Link);
+ )
+ {
+ //
+ // MmiHandler might be removed in below, so cache the next link in Link
+ //
+ MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
+ Link = GetNextNode (&mRootMmiHandlerList, Link);
+ if (MmiHandler->ToRemove) {
+ //
+ // Remove MmiHandler if the ToRemove is set.
+ //
+ RemoveMmiHandler (MmiHandler, NULL);
+ }
+ }
+
+ //
+ // Go through all MmiHandler in non-root MMI handlers
+ //
+ for ( EntryLink = GetFirstNode (&mMmiEntryList)
+ ; !IsNull (&mMmiEntryList, EntryLink);
+ )
+ {
+ //
+ // MmiEntry might be removed in below, so cache the next link in EntryLink
+ //
+ MmiEntry = CR (EntryLink, MMI_ENTRY, AllEntries, MMI_ENTRY_SIGNATURE);
+ EntryLink = GetNextNode (&mMmiEntryList, EntryLink);
+ for ( Link = GetFirstNode (&MmiEntry->MmiHandlers)
+ ; !IsNull (&MmiEntry->MmiHandlers, Link);
+ )
+ {
+ //
+ // MmiHandler might be removed in below, so cache the next link in Link
+ //
+ MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
+ Link = GetNextNode (&MmiEntry->MmiHandlers, Link);
+ if (MmiHandler->ToRemove) {
+ //
+ // Remove MmiHandler if the ToRemove is set.
+ //
+ if (RemoveMmiHandler (MmiHandler, MmiEntry)) {
+ break;
+ }
+ }
+ }
+ }
}
- return Status;
+ return ReturnStatus;
}
/**
@@ -254,6 +373,7 @@ MmiHandlerRegister (
MmiHandler->Signature = MMI_HANDLER_SIGNATURE;
MmiHandler->Handler = Handler;
+ MmiHandler->ToRemove = FALSE;
if (HandlerType == NULL) {
//
@@ -309,26 +429,17 @@ MmiHandlerUnRegister (
return EFI_INVALID_PARAMETER;
}
- MmiEntry = MmiHandler->MmiEntry;
-
- RemoveEntryList (&MmiHandler->Link);
- FreePool (MmiHandler);
-
- if (MmiEntry == NULL) {
+ MmiHandler->ToRemove = TRUE;
+ if (mMmiManageCallingDepth > 0) {
//
- // This is root MMI handler
+ // This function is called from MmiManage()
+ // Do not delete or remove MmiHandler or MmiEntry now.
//
return EFI_SUCCESS;
}
- if (IsListEmpty (&MmiEntry->MmiHandlers)) {
- //
- // No handler registered for this interrupt now, remove the MMI_ENTRY
- //
- RemoveEntryList (&MmiEntry->AllEntries);
-
- FreePool (MmiEntry);
- }
+ MmiEntry = MmiHandler->MmiEntry;
+ RemoveMmiHandler (MmiHandler, MmiEntry);
return EFI_SUCCESS;
}
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117843): https://edk2.groups.io/g/devel/message/117843
Mute This Topic: https://groups.io/mt/105550122/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 17395 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers Zhiguang Liu
@ 2024-04-16 3:52 ` Ni, Ray
0 siblings, 0 replies; 9+ messages in thread
From: Ni, Ray @ 2024-04-16 3:52 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Liming Gao, Wu, Jiaxin, Laszlo Ersek
[-- Attachment #1: Type: text/plain, Size: 9674 bytes --]
Reviewed-by: Ray Ni <ray.ni@intel.com>
Thanks,
Ray
________________________________
From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Tuesday, April 16, 2024 10:41
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers
This patch fix a use-after-free issue where unregistering an
SMI handler could lead to the deletion of the SMI_HANDLER while it is
still in use by SmiManage(). The fix involves modifying
SmiHandlerUnRegister() to detect whether it is being called from
within the SmiManage() stack. If so, the removal of the SMI_HANDLER
is deferred until SmiManage() has finished executing.
Additionally, due to the possibility of recursive SmiManage() calls,
the unregistration and subsequent removal of the SMI_HANDLER are
ensured to occur only after the outermost SmiManage() invocation has
completed.
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 1 +
MdeModulePkg/Core/PiSmmCore/Smi.c | 164 ++++++++++++++++++++----
2 files changed, 139 insertions(+), 26 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
index b8a490a8c3..60073c78b4 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
@@ -93,6 +93,7 @@ typedef struct {
SMI_ENTRY *SmiEntry;
VOID *Context; // for profile
UINTN ContextSize; // for profile
+ BOOLEAN ToRemove; // To remove this SMI_HANDLER later
} SMI_HANDLER;
//
diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
index 2985f989c3..a84a1f48d3 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -8,6 +8,11 @@
#include "PiSmmCore.h"
+//
+// mSmiManageCallingDepth is used to track the depth of recursive calls of SmiManage.
+//
+UINTN mSmiManageCallingDepth = 0;
+
LIST_ENTRY mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
SMI_ENTRY mRootSmiEntry = {
@@ -79,6 +84,40 @@ SmmCoreFindSmiEntry (
return SmiEntry;
}
+/**
+ Remove SmiHandler and free the memory it used.
+ If SmiEntry is empty, remove SmiEntry and free the memory it used.
+
+ @param SmiHandler Points to SMI handler.
+ @param SmiEntry Points to SMI Entry or NULL for root SMI handlers.
+
+ @retval TRUE SmiEntry is removed.
+ @retval FALSE SmiEntry is not removed.
+**/
+BOOLEAN
+RemoveSmiHandler (
+ IN SMI_HANDLER *SmiHandler,
+ IN SMI_ENTRY *SmiEntry
+ )
+{
+ ASSERT (SmiHandler->ToRemove);
+ RemoveEntryList (&SmiHandler->Link);
+ FreePool (SmiHandler);
+
+ //
+ // Remove the SMI_ENTRY if all handlers have been removed.
+ //
+ if (SmiEntry != NULL) {
+ if (IsListEmpty (&SmiEntry->SmiHandlers)) {
+ RemoveEntryList (&SmiEntry->AllEntries);
+ FreePool (SmiEntry);
+ return TRUE;
+ }
+ }
+
+ return FALSE;
+}
+
/**
Manage SMI of a particular type.
@@ -104,15 +143,17 @@ SmiManage (
{
LIST_ENTRY *Link;
LIST_ENTRY *Head;
+ LIST_ENTRY *EntryLink;
SMI_ENTRY *SmiEntry;
SMI_HANDLER *SmiHandler;
- BOOLEAN SuccessReturn;
+ EFI_STATUS ReturnStatus;
+ BOOLEAN WillReturn;
EFI_STATUS Status;
PERF_FUNCTION_BEGIN ();
-
- Status = EFI_NOT_FOUND;
- SuccessReturn = FALSE;
+ mSmiManageCallingDepth++;
+ Status = EFI_NOT_FOUND;
+ ReturnStatus = Status;
if (HandlerType == NULL) {
//
// Root SMI handler
@@ -152,7 +193,16 @@ SmiManage (
//
if (HandlerType != NULL) {
PERF_FUNCTION_END ();
- return EFI_INTERRUPT_PENDING;
+ ReturnStatus = EFI_INTERRUPT_PENDING;
+ WillReturn = TRUE;
+ } else {
+ //
+ // If any other handler's result sets ReturnStatus as EFI_SUCCESS, the return status
+ // will be EFI_SUCCESS.
+ //
+ if (ReturnStatus != EFI_SUCCESS) {
+ ReturnStatus = Status;
+ }
}
break;
@@ -165,10 +215,10 @@ SmiManage (
//
if (HandlerType != NULL) {
PERF_FUNCTION_END ();
- return EFI_SUCCESS;
+ WillReturn = TRUE;
}
- SuccessReturn = TRUE;
+ ReturnStatus = EFI_SUCCESS;
break;
case EFI_WARN_INTERRUPT_SOURCE_QUIESCED:
@@ -176,7 +226,7 @@ SmiManage (
// If at least one of the handlers returns EFI_WARN_INTERRUPT_SOURCE_QUIESCED
// then the function will return EFI_SUCCESS.
//
- SuccessReturn = TRUE;
+ ReturnStatus = EFI_SUCCESS;
break;
case EFI_WARN_INTERRUPT_SOURCE_PENDING:
@@ -184,6 +234,10 @@ SmiManage (
// If all the handlers returned EFI_WARN_INTERRUPT_SOURCE_PENDING
// then EFI_WARN_INTERRUPT_SOURCE_PENDING will be returned.
//
+ if (ReturnStatus != EFI_SUCCESS) {
+ ReturnStatus = Status;
+ }
+
break;
default:
@@ -193,14 +247,74 @@ SmiManage (
ASSERT (FALSE);
break;
}
+
+ if (WillReturn) {
+ break;
+ }
}
- if (SuccessReturn) {
- Status = EFI_SUCCESS;
+ ASSERT (mSmiManageCallingDepth > 0);
+ mSmiManageCallingDepth--;
+
+ //
+ // SmiHandlerUnRegister() calls from SMI handlers are deferred till this point.
+ // Before returned from SmiManage, delete the SmiHandler which is
+ // marked as ToRemove.
+ // Note that SmiManage can be called recursively.
+ //
+ if (mSmiManageCallingDepth == 0) {
+ //
+ // Go through all SmiHandler in root SMI handlers
+ //
+ for ( Link = GetFirstNode (&mRootSmiEntry.SmiHandlers)
+ ; !IsNull (&mRootSmiEntry.SmiHandlers, Link);
+ )
+ {
+ //
+ // SmiHandler might be removed in below, so cache the next link in Link
+ //
+ SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
+ Link = GetNextNode (&mRootSmiEntry.SmiHandlers, Link);
+ if (SmiHandler->ToRemove) {
+ //
+ // Remove SmiHandler if the ToRemove is set.
+ //
+ RemoveSmiHandler (SmiHandler, NULL);
+ }
+ }
+
+ //
+ // Go through all SmiHandler in non-root SMI handlers
+ //
+ for ( EntryLink = GetFirstNode (&mSmiEntryList)
+ ; !IsNull (&mSmiEntryList, EntryLink);
+ )
+ {
+ //
+ // SmiEntry might be removed in below, so cache the next link in EntryLink
+ //
+ SmiEntry = CR (EntryLink, SMI_ENTRY, AllEntries, SMI_ENTRY_SIGNATURE);
+ EntryLink = GetNextNode (&mSmiEntryList, EntryLink);
+ for ( Link = GetFirstNode (&SmiEntry->SmiHandlers)
+ ; !IsNull (&SmiEntry->SmiHandlers, Link);
+ )
+ {
+ //
+ // SmiHandler might be removed in below, so cache the next link in Link
+ //
+ SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
+ Link = GetNextNode (&SmiEntry->SmiHandlers, Link);
+ if (SmiHandler->ToRemove) {
+ if (RemoveSmiHandler (SmiHandler, SmiEntry)) {
+ break;
+ }
+ }
+ }
+ }
}
PERF_FUNCTION_END ();
- return Status;
+ return ReturnStatus;
}
/**
@@ -238,6 +352,7 @@ SmiHandlerRegister (
SmiHandler->Signature = SMI_HANDLER_SIGNATURE;
SmiHandler->Handler = Handler;
SmiHandler->CallerAddr = (UINTN)RETURN_ADDRESS (0);
+ SmiHandler->ToRemove = FALSE;
if (HandlerType == NULL) {
//
@@ -318,30 +433,27 @@ SmiHandlerUnRegister (
}
}
- if ((EFI_HANDLE)SmiHandler != DispatchHandle) {
+ if (SmiHandler == NULL) {
return EFI_INVALID_PARAMETER;
}
- SmiEntry = SmiHandler->SmiEntry;
-
- RemoveEntryList (&SmiHandler->Link);
- FreePool (SmiHandler);
+ ASSERT ((EFI_HANDLE)SmiHandler == DispatchHandle);
+ SmiHandler->ToRemove = TRUE;
- if (SmiEntry == NULL) {
+ if (mSmiManageCallingDepth > 0) {
//
- // This is root SMI handler
+ // This function is called from SmiManage()
+ // Do not delete or remove SmiHandler or SmiEntry now.
+ // SmiManage will handle it later
//
return EFI_SUCCESS;
}
- if (IsListEmpty (&SmiEntry->SmiHandlers)) {
- //
- // No handler registered for this interrupt now, remove the SMI_ENTRY
- //
- RemoveEntryList (&SmiEntry->AllEntries);
-
- FreePool (SmiEntry);
- }
+ SmiEntry = SmiHandler->SmiEntry;
+ //
+ // For root SMI handler, use NULL for SmiEntry
+ //
+ RemoveSmiHandler (SmiHandler, (SmiEntry == &mRootSmiEntry) ? NULL : SmiEntry);
return EFI_SUCCESS;
}
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117844): https://edk2.groups.io/g/devel/message/117844
Mute This Topic: https://groups.io/mt/105550121/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 18282 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-04-16 3:52 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-16 2:41 [edk2-devel] [PATCH v4 0/6] Support to unregister SMI handler in SMI handlers Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 1/6] Revert 2ec8f0c6407f062441b205b900038933865c7b3c Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 2/6] Revert 049ff6c39c73edd3709c05bd0e46184320471358 Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 3/6] Revert 17b28722008eab745ce186b72cd325944cbe6bf0 Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 4/6] Revert ae1079b386a597108a8070652bf7cdaa4ec3dda3 Zhiguang Liu
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers Zhiguang Liu
2024-04-16 3:52 ` Ni, Ray
2024-04-16 2:41 ` [edk2-devel] [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers Zhiguang Liu
2024-04-16 3:52 ` Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox