Re: [edk2-devel] [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers
Reviewed-by: Ray Ni
Thanks,
Ray
From: Liu, Zhiguang
Sent: Tuesday, April 16, 2024 10:41
To: devel@edk2.groups.io
Cc: Liu, Zhiguang ; Liming Gao
; Wu, Jiaxin ; Ni, Ray
; Laszlo Ersek
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
Cc: Jiaxin Wu
Cc: Ray Ni
Cc: Laszlo Ersek
Signed-off-by: Zhiguang Liu
---
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 TRUESmiEntry is removed.
+ @retval FALSE SmiEntry is not removed.
+**/
+BOOLEAN
+RemoveSmiHandler (
+ IN SMI_HANDLER *SmiHandler,
+ IN SMI_ENTRY*SmiEntry
+ )
+{
+ ASSERT (SmiHandler->ToRemove);
+ RemoveEntryList (>Link);
+ FreePool (SmiHandler);
+
+ //
+ // Remove the SMI_ENTRY if all handlers have been removed.
+ //
+ if (SmiEntry != NULL) {
+if (IsListEmpty (>SmiHandlers)) {
+ RemoveEntryList (>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
[edk2-devel] [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
Cc: Jiaxin Wu
Cc: Ray Ni
Cc: Laszlo Ersek
Signed-off-by: Zhiguang Liu
---
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 TRUESmiEntry is removed.
+ @retval FALSE SmiEntry is not removed.
+**/
+BOOLEAN
+RemoveSmiHandler (
+ IN SMI_HANDLER *SmiHandler,
+ IN SMI_ENTRY*SmiEntry
+ )
+{
+ ASSERT (SmiHandler->ToRemove);
+ RemoveEntryList (>Link);
+ FreePool (SmiHandler);
+
+ //
+ // Remove the SMI_ENTRY if all handlers have been removed.
+ //
+ if (SmiEntry != NULL) {
+if (IsListEmpty (>SmiHandlers)) {
+ RemoveEntryList (>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;
+
