* [PATCH] MdeModulePkg/DxeCore: Avoid accessing non-owned memory
@ 2017-07-05 10:46 Ruiyu Ni
2017-07-06 9:21 ` Zeng, Star
0 siblings, 1 reply; 2+ messages in thread
From: Ruiyu Ni @ 2017-07-05 10:46 UTC (permalink / raw)
To: edk2-devel; +Cc: Michael D Kinney, Liming Gao, Star Zeng, Hao A Wu
The patch fixes two kinds of issues in DxeCore that accesses memory
which might be freed or owned by other modules.
The existing bugs don't cause functionality issue.
1. CoreValidateHandle() checks whether the handle is valid by
validating its signature. The proper way is to check whether
the handle is in the handle database.
2. CoreDisconnectControllersUsingProtocolInterface() and
CoreOpenProtocol() de-reference Link pointer which is
already freed. The proper way is to not de-reference the pointer.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Core/Dxe/Hand/Handle.c | 54 ++++++++++++++++++-------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 59b89148c8..8e2f361805 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -72,15 +72,20 @@ CoreValidateHandle (
)
{
IHANDLE *Handle;
+ LIST_ENTRY *Link;
- Handle = (IHANDLE *)UserHandle;
- if (Handle == NULL) {
+ if (UserHandle == NULL) {
return EFI_INVALID_PARAMETER;
}
- if (Handle->Signature != EFI_HANDLE_SIGNATURE) {
- return EFI_INVALID_PARAMETER;
+
+ for (Link = gHandleList.ForwardLink; Link != &gHandleList; Link = Link->ForwardLink) {
+ Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
+ if (Handle == (IHANDLE *) UserHandle) {
+ return EFI_SUCCESS;
+ }
}
- return EFI_SUCCESS;
+
+ return EFI_INVALID_PARAMETER;
}
@@ -643,19 +648,16 @@ CoreDisconnectControllersUsingProtocolInterface (
//
do {
ItemFound = FALSE;
- for ( Link = Prot->OpenList.ForwardLink;
- (Link != &Prot->OpenList) && !ItemFound;
- Link = Link->ForwardLink ) {
+ for (Link = Prot->OpenList.ForwardLink; Link != &Prot->OpenList; Link = Link->ForwardLink) {
OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_SIGNATURE);
if ((OpenData->Attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) != 0) {
- ItemFound = TRUE;
CoreReleaseProtocolLock ();
Status = CoreDisconnectController (UserHandle, OpenData->AgentHandle, NULL);
CoreAcquireProtocolLock ();
- if (EFI_ERROR (Status)) {
- ItemFound = FALSE;
- break;
+ if (!EFI_ERROR (Status)) {
+ ItemFound = TRUE;
}
+ break;
}
}
} while (ItemFound);
@@ -664,21 +666,17 @@ CoreDisconnectControllersUsingProtocolInterface (
//
// Attempt to remove BY_HANDLE_PROTOOCL and GET_PROTOCOL and TEST_PROTOCOL Open List items
//
- do {
- ItemFound = FALSE;
- for ( Link = Prot->OpenList.ForwardLink;
- (Link != &Prot->OpenList) && !ItemFound;
- Link = Link->ForwardLink ) {
- OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_SIGNATURE);
- if ((OpenData->Attributes &
- (EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL | EFI_OPEN_PROTOCOL_GET_PROTOCOL | EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) != 0) {
- ItemFound = TRUE;
- RemoveEntryList (&OpenData->Link);
- Prot->OpenListCount--;
- CoreFreePool (OpenData);
- }
+ for (Link = Prot->OpenList.ForwardLink; Link != &Prot->OpenList;) {
+ OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_SIGNATURE);
+ if ((OpenData->Attributes &
+ (EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL | EFI_OPEN_PROTOCOL_GET_PROTOCOL | EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) != 0) {
+ Link = RemoveEntryList (&OpenData->Link);
+ Prot->OpenListCount--;
+ CoreFreePool (OpenData);
+ } else {
+ Link = Link->ForwardLink;
}
- } while (ItemFound);
+ }
}
//
@@ -1132,7 +1130,7 @@ CoreOpenProtocol (
if (ByDriver) {
do {
Disconnect = FALSE;
- for ( Link = Prot->OpenList.ForwardLink; (Link != &Prot->OpenList) && (!Disconnect); Link = Link->ForwardLink) {
+ for (Link = Prot->OpenList.ForwardLink; Link != &Prot->OpenList; Link = Link->ForwardLink) {
OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_SIGNATURE);
if ((OpenData->Attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) != 0) {
Disconnect = TRUE;
@@ -1142,6 +1140,8 @@ CoreOpenProtocol (
if (EFI_ERROR (Status)) {
Status = EFI_ACCESS_DENIED;
goto Done;
+ } else {
+ break;
}
}
}
--
2.12.2.windows.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] MdeModulePkg/DxeCore: Avoid accessing non-owned memory
2017-07-05 10:46 [PATCH] MdeModulePkg/DxeCore: Avoid accessing non-owned memory Ruiyu Ni
@ 2017-07-06 9:21 ` Zeng, Star
0 siblings, 0 replies; 2+ messages in thread
From: Zeng, Star @ 2017-07-06 9:21 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org
Cc: Kinney, Michael D, Gao, Liming, Wu, Hao A, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: Ni, Ruiyu
Sent: Wednesday, July 5, 2017 6:46 PM
To: edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
Subject: [PATCH] MdeModulePkg/DxeCore: Avoid accessing non-owned memory
The patch fixes two kinds of issues in DxeCore that accesses memory which might be freed or owned by other modules.
The existing bugs don't cause functionality issue.
1. CoreValidateHandle() checks whether the handle is valid by
validating its signature. The proper way is to check whether
the handle is in the handle database.
2. CoreDisconnectControllersUsingProtocolInterface() and
CoreOpenProtocol() de-reference Link pointer which is
already freed. The proper way is to not de-reference the pointer.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
---
MdeModulePkg/Core/Dxe/Hand/Handle.c | 54 ++++++++++++++++++-------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 59b89148c8..8e2f361805 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -72,15 +72,20 @@ CoreValidateHandle (
)
{
IHANDLE *Handle;
+ LIST_ENTRY *Link;
- Handle = (IHANDLE *)UserHandle;
- if (Handle == NULL) {
+ if (UserHandle == NULL) {
return EFI_INVALID_PARAMETER;
}
- if (Handle->Signature != EFI_HANDLE_SIGNATURE) {
- return EFI_INVALID_PARAMETER;
+
+ for (Link = gHandleList.ForwardLink; Link != &gHandleList; Link = Link->ForwardLink) {
+ Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
+ if (Handle == (IHANDLE *) UserHandle) {
+ return EFI_SUCCESS;
+ }
}
- return EFI_SUCCESS;
+
+ return EFI_INVALID_PARAMETER;
}
@@ -643,19 +648,16 @@ CoreDisconnectControllersUsingProtocolInterface (
//
do {
ItemFound = FALSE;
- for ( Link = Prot->OpenList.ForwardLink;
- (Link != &Prot->OpenList) && !ItemFound;
- Link = Link->ForwardLink ) {
+ for (Link = Prot->OpenList.ForwardLink; Link != &Prot->OpenList;
+ Link = Link->ForwardLink) {
OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_SIGNATURE);
if ((OpenData->Attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) != 0) {
- ItemFound = TRUE;
CoreReleaseProtocolLock ();
Status = CoreDisconnectController (UserHandle, OpenData->AgentHandle, NULL);
CoreAcquireProtocolLock ();
- if (EFI_ERROR (Status)) {
- ItemFound = FALSE;
- break;
+ if (!EFI_ERROR (Status)) {
+ ItemFound = TRUE;
}
+ break;
}
}
} while (ItemFound);
@@ -664,21 +666,17 @@ CoreDisconnectControllersUsingProtocolInterface (
//
// Attempt to remove BY_HANDLE_PROTOOCL and GET_PROTOCOL and TEST_PROTOCOL Open List items
//
- do {
- ItemFound = FALSE;
- for ( Link = Prot->OpenList.ForwardLink;
- (Link != &Prot->OpenList) && !ItemFound;
- Link = Link->ForwardLink ) {
- OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_SIGNATURE);
- if ((OpenData->Attributes &
- (EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL | EFI_OPEN_PROTOCOL_GET_PROTOCOL | EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) != 0) {
- ItemFound = TRUE;
- RemoveEntryList (&OpenData->Link);
- Prot->OpenListCount--;
- CoreFreePool (OpenData);
- }
+ for (Link = Prot->OpenList.ForwardLink; Link != &Prot->OpenList;) {
+ OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_SIGNATURE);
+ if ((OpenData->Attributes &
+ (EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL | EFI_OPEN_PROTOCOL_GET_PROTOCOL | EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) != 0) {
+ Link = RemoveEntryList (&OpenData->Link);
+ Prot->OpenListCount--;
+ CoreFreePool (OpenData);
+ } else {
+ Link = Link->ForwardLink;
}
- } while (ItemFound);
+ }
}
//
@@ -1132,7 +1130,7 @@ CoreOpenProtocol (
if (ByDriver) {
do {
Disconnect = FALSE;
- for ( Link = Prot->OpenList.ForwardLink; (Link != &Prot->OpenList) && (!Disconnect); Link = Link->ForwardLink) {
+ for (Link = Prot->OpenList.ForwardLink; Link !=
+ &Prot->OpenList; Link = Link->ForwardLink) {
OpenData = CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_SIGNATURE);
if ((OpenData->Attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) != 0) {
Disconnect = TRUE;
@@ -1142,6 +1140,8 @@ CoreOpenProtocol (
if (EFI_ERROR (Status)) {
Status = EFI_ACCESS_DENIED;
goto Done;
+ } else {
+ break;
}
}
}
--
2.12.2.windows.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-07-06 9:19 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-05 10:46 [PATCH] MdeModulePkg/DxeCore: Avoid accessing non-owned memory Ruiyu Ni
2017-07-06 9:21 ` Zeng, Star
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox