* [PATCH] MdeModulePkg/PiSmmCore: Cache CommunicationBuffer info before using it
@ 2016-11-18 7:01 Jeff Fan
2016-11-22 6:25 ` Yao, Jiewen
0 siblings, 1 reply; 2+ messages in thread
From: Jeff Fan @ 2016-11-18 7:01 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Feng Tian, Michael D Kinney
gSmmCorePrivate->CommunicationBuffer and gSmmCorePrivate->BufferSize locate at
runtime memory region. That means they could be modified by non-SMM code during
runtime.
We should cache them into SMM local variables before we verify them. After
verification, we should use the cached ones directly instead of the ones in
gSmmCorePrivate.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>
---
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index b877a33..de8db65 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -432,6 +432,8 @@ SmmEntryPoint (
EFI_SMM_COMMUNICATE_HEADER *CommunicateHeader;
BOOLEAN InLegacyBoot;
BOOLEAN IsOverlapped;
+ VOID *CommunicationBuffer;
+ UINTN BufferSize;
PERF_START (NULL, "SMM", NULL, 0) ;
@@ -463,17 +465,19 @@ SmmEntryPoint (
// Check to see if this is a Synchronous SMI sent through the SMM Communication
// Protocol or an Asynchronous SMI
//
- if (gSmmCorePrivate->CommunicationBuffer != NULL) {
+ CommunicationBuffer = gSmmCorePrivate->CommunicationBuffer;
+ BufferSize = gSmmCorePrivate->BufferSize;
+ if (CommunicationBuffer != NULL) {
//
// Synchronous SMI for SMM Core or request from Communicate protocol
//
IsOverlapped = InternalIsBufferOverlapped (
- (UINT8 *) gSmmCorePrivate->CommunicationBuffer,
- gSmmCorePrivate->BufferSize,
+ (UINT8 *) CommunicationBuffer,
+ BufferSize,
(UINT8 *) gSmmCorePrivate,
sizeof (*gSmmCorePrivate)
);
- if (!SmmIsBufferOutsideSmmValid ((UINTN)gSmmCorePrivate->CommunicationBuffer, gSmmCorePrivate->BufferSize) || IsOverlapped) {
+ if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) || IsOverlapped) {
//
// If CommunicationBuffer is not in valid address scope,
// or there is overlap between gSmmCorePrivate and CommunicationBuffer,
@@ -482,19 +486,19 @@ SmmEntryPoint (
gSmmCorePrivate->CommunicationBuffer = NULL;
gSmmCorePrivate->ReturnStatus = EFI_INVALID_PARAMETER;
} else {
- CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)gSmmCorePrivate->CommunicationBuffer;
- gSmmCorePrivate->BufferSize -= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data);
+ CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommunicationBuffer;
+ BufferSize -= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data);
Status = SmiManage (
&CommunicateHeader->HeaderGuid,
NULL,
CommunicateHeader->Data,
- &gSmmCorePrivate->BufferSize
+ &BufferSize
);
//
// Update CommunicationBuffer, BufferSize and ReturnStatus
// Communicate service finished, reset the pointer to CommBuffer to NULL
//
- gSmmCorePrivate->BufferSize += OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data);
+ gSmmCorePrivate->BufferSize = BufferSize + OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data);
gSmmCorePrivate->CommunicationBuffer = NULL;
gSmmCorePrivate->ReturnStatus = (Status == EFI_SUCCESS) ? EFI_SUCCESS : EFI_NOT_FOUND;
}
--
2.9.3.windows.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] MdeModulePkg/PiSmmCore: Cache CommunicationBuffer info before using it
2016-11-18 7:01 [PATCH] MdeModulePkg/PiSmmCore: Cache CommunicationBuffer info before using it Jeff Fan
@ 2016-11-22 6:25 ` Yao, Jiewen
0 siblings, 0 replies; 2+ messages in thread
From: Yao, Jiewen @ 2016-11-22 6:25 UTC (permalink / raw)
To: Fan, Jeff, edk2-devel@lists.01.org; +Cc: Tian, Feng, Kinney, Michael D
Reviewed-by: Jiewen.yao@intel.com
> -----Original Message-----
> From: Fan, Jeff
> Sent: Friday, November 18, 2016 3:02 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Tian, Feng <feng.tian@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH] MdeModulePkg/PiSmmCore: Cache CommunicationBuffer
> info before using it
>
> gSmmCorePrivate->CommunicationBuffer and
> gSmmCorePrivate->BufferSize locate at
> runtime memory region. That means they could be modified by non-SMM
> code during
> runtime.
>
> We should cache them into SMM local variables before we verify them. After
> verification, we should use the cached ones directly instead of the ones in
> gSmmCorePrivate.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> index b877a33..de8db65 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> @@ -432,6 +432,8 @@ SmmEntryPoint (
> EFI_SMM_COMMUNICATE_HEADER *CommunicateHeader;
> BOOLEAN InLegacyBoot;
> BOOLEAN IsOverlapped;
> + VOID *CommunicationBuffer;
> + UINTN BufferSize;
>
> PERF_START (NULL, "SMM", NULL, 0) ;
>
> @@ -463,17 +465,19 @@ SmmEntryPoint (
> // Check to see if this is a Synchronous SMI sent through the SMM
> Communication
> // Protocol or an Asynchronous SMI
> //
> - if (gSmmCorePrivate->CommunicationBuffer != NULL) {
> + CommunicationBuffer = gSmmCorePrivate->CommunicationBuffer;
> + BufferSize = gSmmCorePrivate->BufferSize;
> + if (CommunicationBuffer != NULL) {
> //
> // Synchronous SMI for SMM Core or request from Communicate
> protocol
> //
> IsOverlapped = InternalIsBufferOverlapped (
> - (UINT8 *)
> gSmmCorePrivate->CommunicationBuffer,
> - gSmmCorePrivate->BufferSize,
> + (UINT8 *) CommunicationBuffer,
> + BufferSize,
> (UINT8 *) gSmmCorePrivate,
> sizeof (*gSmmCorePrivate)
> );
> - if (!SmmIsBufferOutsideSmmValid
> ((UINTN)gSmmCorePrivate->CommunicationBuffer,
> gSmmCorePrivate->BufferSize) || IsOverlapped) {
> + if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer,
> BufferSize) || IsOverlapped) {
> //
> // If CommunicationBuffer is not in valid address scope,
> // or there is overlap between gSmmCorePrivate and
> CommunicationBuffer,
> @@ -482,19 +486,19 @@ SmmEntryPoint (
> gSmmCorePrivate->CommunicationBuffer = NULL;
> gSmmCorePrivate->ReturnStatus = EFI_INVALID_PARAMETER;
> } else {
> - CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER
> *)gSmmCorePrivate->CommunicationBuffer;
> - gSmmCorePrivate->BufferSize -= OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data);
> + CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER
> *)CommunicationBuffer;
> + BufferSize -= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER,
> Data);
> Status = SmiManage (
> &CommunicateHeader->HeaderGuid,
> NULL,
> CommunicateHeader->Data,
> - &gSmmCorePrivate->BufferSize
> + &BufferSize
> );
> //
> // Update CommunicationBuffer, BufferSize and ReturnStatus
> // Communicate service finished, reset the pointer to
> CommBuffer to NULL
> //
> - gSmmCorePrivate->BufferSize += OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data);
> + gSmmCorePrivate->BufferSize = BufferSize + OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data);
> gSmmCorePrivate->CommunicationBuffer = NULL;
> gSmmCorePrivate->ReturnStatus = (Status == EFI_SUCCESS) ?
> EFI_SUCCESS : EFI_NOT_FOUND;
> }
> --
> 2.9.3.windows.2
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-11-22 6:25 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-18 7:01 [PATCH] MdeModulePkg/PiSmmCore: Cache CommunicationBuffer info before using it Jeff Fan
2016-11-22 6:25 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox