1. Fix TOCTOU issue in VariableSmm, FtwSmm, FpdtSmm, SmmCorePerformance SMM handler. For VariableSmm, pre-allocate a mVariableBufferPayload buffer with mVariableBufferPayloadSize(match with mVariableBufferPayloadSize in VariableSmmRuntimeDxe) to hold communicate buffer payload to avoid TOCTOU issue.

2. Add check to ensure CommBufferPayloadSize not exceed mVariableBufferPayloadSize or is enough to hold function structure in VariableSmm and FtwSmm.
3. Align FtwGetLastWrite() in FaultTolerantWriteSmmDxe.c to FtwGetLastWrite() in FaultTolerantWrite.c.

Signed-off-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@14325 6f19259b-4bc3-4df7-8a09-765794883524

2 files changed, 73 insertions, 31 deletions

diff --git a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
index 8b88ae41ee..2580d478a3 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c
@@ -369,6 +369,9 @@ SmmFaultTolerantWriteHandler (
   VOID                                             *PrivateData;

   EFI_HANDLE                                       SmmFvbHandle;

   UINTN                                            InfoSize;

+  UINTN                                            CommBufferPayloadSize;

+  UINTN                                            PrivateDataSize;

+  UINTN                                            Length;

 

 

   //

@@ -379,11 +382,13 @@ SmmFaultTolerantWriteHandler (
   }

 

   if (*CommBufferSize < SMM_FTW_COMMUNICATE_HEADER_SIZE) {

+    DEBUG ((EFI_D_ERROR, "SmmFtwHandler: SMM communication buffer size invalid!\n"));

     return EFI_SUCCESS;

   }

+  CommBufferPayloadSize = *CommBufferSize - SMM_FTW_COMMUNICATE_HEADER_SIZE;

 

   if (!InternalIsAddressValid ((UINTN)CommBuffer, *CommBufferSize)) {

-    DEBUG ((EFI_D_ERROR, "SMM communication buffer in SMRAM or overflow!\n"));

+    DEBUG ((EFI_D_ERROR, "SmmFtwHandler: SMM communication buffer in SMRAM or overflow!\n"));

     return EFI_SUCCESS;

   }

 

@@ -400,17 +405,11 @@ SmmFaultTolerantWriteHandler (
 

   switch (SmmFtwFunctionHeader->Function) {

     case FTW_FUNCTION_GET_MAX_BLOCK_SIZE:

-      SmmGetMaxBlockSizeHeader = (SMM_FTW_GET_MAX_BLOCK_SIZE_HEADER *) SmmFtwFunctionHeader->Data;

-      InfoSize = sizeof (SMM_FTW_GET_MAX_BLOCK_SIZE_HEADER);

-

-      //

-      // SMRAM range check already covered before

-      //

-      if (InfoSize > *CommBufferSize - SMM_FTW_COMMUNICATE_HEADER_SIZE) {

-        DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n"));

-        Status = EFI_ACCESS_DENIED;

-        break;

+      if (CommBufferPayloadSize < sizeof (SMM_FTW_GET_MAX_BLOCK_SIZE_HEADER)) {

+        DEBUG ((EFI_D_ERROR, "GetMaxBlockSize: SMM communication buffer size invalid!\n"));

+        return EFI_SUCCESS;

       }

+      SmmGetMaxBlockSizeHeader = (SMM_FTW_GET_MAX_BLOCK_SIZE_HEADER *) SmmFtwFunctionHeader->Data;

 

       Status = FtwGetMaxBlockSize (

                  &mFtwDevice->FtwInstance,

@@ -419,6 +418,10 @@ SmmFaultTolerantWriteHandler (
       break;

       

     case FTW_FUNCTION_ALLOCATE:

+      if (CommBufferPayloadSize < sizeof (SMM_FTW_ALLOCATE_HEADER)) {

+        DEBUG ((EFI_D_ERROR, "Allocate: SMM communication buffer size invalid!\n"));

+        return EFI_SUCCESS;

+      }

       SmmFtwAllocateHeader = (SMM_FTW_ALLOCATE_HEADER *) SmmFtwFunctionHeader->Data;

       Status = FtwAllocate (

                  &mFtwDevice->FtwInstance,

@@ -429,11 +432,36 @@ SmmFaultTolerantWriteHandler (
       break;

       

     case FTW_FUNCTION_WRITE:

+      if (CommBufferPayloadSize < OFFSET_OF (SMM_FTW_WRITE_HEADER, Data)) {

+        DEBUG ((EFI_D_ERROR, "Write: SMM communication buffer size invalid!\n"));

+        return EFI_SUCCESS;

+      }

       SmmFtwWriteHeader = (SMM_FTW_WRITE_HEADER *) SmmFtwFunctionHeader->Data;

-      if (SmmFtwWriteHeader->PrivateDataSize == 0) {

+      Length = SmmFtwWriteHeader->Length;

+      PrivateDataSize = SmmFtwWriteHeader->PrivateDataSize;

+      if (((UINTN)(~0) - Length < OFFSET_OF (SMM_FTW_WRITE_HEADER, Data)) ||

+        ((UINTN)(~0) - PrivateDataSize < OFFSET_OF (SMM_FTW_WRITE_HEADER, Data) + Length)) {

+        //

+        // Prevent InfoSize overflow

+        //

+        Status = EFI_ACCESS_DENIED;

+        break;

+      }

+      InfoSize = OFFSET_OF (SMM_FTW_WRITE_HEADER, Data) + Length + PrivateDataSize;

+

+      //

+      // SMRAM range check already covered before

+      //

+      if (InfoSize > CommBufferPayloadSize) {

+        DEBUG ((EFI_D_ERROR, "Write: Data size exceed communication buffer size limit!\n"));

+        Status = EFI_ACCESS_DENIED;

+        break;

+      }

+

+      if (PrivateDataSize == 0) {

         PrivateData = NULL;

       } else {

-        PrivateData = (VOID *)&SmmFtwWriteHeader->Data[SmmFtwWriteHeader->Length];

+        PrivateData = (VOID *)&SmmFtwWriteHeader->Data[Length];

       }

       Status = GetFvbByAddressAndAttribute (

                  SmmFtwWriteHeader->FvbBaseAddress, 

@@ -445,7 +473,7 @@ SmmFaultTolerantWriteHandler (
                    &mFtwDevice->FtwInstance,

                    SmmFtwWriteHeader->Lba,

                    SmmFtwWriteHeader->Offset,

-                   SmmFtwWriteHeader->Length,

+                   Length,

                    PrivateData,

                    SmmFvbHandle,

                    SmmFtwWriteHeader->Data

@@ -454,6 +482,10 @@ SmmFaultTolerantWriteHandler (
       break;

       

     case FTW_FUNCTION_RESTART:

+      if (CommBufferPayloadSize < sizeof (SMM_FTW_RESTART_HEADER)) {

+        DEBUG ((EFI_D_ERROR, "Restart: SMM communication buffer size invalid!\n"));

+        return EFI_SUCCESS;

+      }

       SmmFtwRestartHeader = (SMM_FTW_RESTART_HEADER *) SmmFtwFunctionHeader->Data;

       Status = GetFvbByAddressAndAttribute (

                  SmmFtwRestartHeader->FvbBaseAddress, 

@@ -470,20 +502,25 @@ SmmFaultTolerantWriteHandler (
       break;

       

     case FTW_FUNCTION_GET_LAST_WRITE:

+      if (CommBufferPayloadSize < OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data)) {

+        DEBUG ((EFI_D_ERROR, "GetLastWrite: SMM communication buffer size invalid!\n"));

+        return EFI_SUCCESS;

+      }

       SmmFtwGetLastWriteHeader = (SMM_FTW_GET_LAST_WRITE_HEADER *) SmmFtwFunctionHeader->Data;

-      if ((UINTN)(~0) - SmmFtwGetLastWriteHeader->PrivateDataSize < OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data)){

+      PrivateDataSize = SmmFtwGetLastWriteHeader->PrivateDataSize;

+      if ((UINTN)(~0) - PrivateDataSize < OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data)){

         //

         // Prevent InfoSize overflow

         //

         Status = EFI_ACCESS_DENIED;

         break;

       }

-      InfoSize = OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data) + SmmFtwGetLastWriteHeader->PrivateDataSize;

+      InfoSize = OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data) + PrivateDataSize;

 

       //

       // SMRAM range check already covered before

       //

-      if (InfoSize > *CommBufferSize - SMM_FTW_COMMUNICATE_HEADER_SIZE) {

+      if (InfoSize > CommBufferPayloadSize) {

         DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n"));

         Status = EFI_ACCESS_DENIED;

         break;

@@ -495,10 +532,11 @@ SmmFaultTolerantWriteHandler (
                  &SmmFtwGetLastWriteHeader->Lba,

                  &SmmFtwGetLastWriteHeader->Offset,

                  &SmmFtwGetLastWriteHeader->Length,

-                 &SmmFtwGetLastWriteHeader->PrivateDataSize,

+                 &PrivateDataSize,

                  (VOID *)SmmFtwGetLastWriteHeader->Data,

                  &SmmFtwGetLastWriteHeader->Complete

                  );

+      SmmFtwGetLastWriteHeader->PrivateDataSize = PrivateDataSize;

       break;

 

     default:

@@ -532,6 +570,7 @@ FvbNotificationEvent (
   EFI_STATUS                              Status;

   EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL   *FtwProtocol;

   EFI_HANDLE                              SmmFtwHandle;

+  EFI_HANDLE                              FtwHandle;

   

   //

   // Just return to avoid install SMM FaultTolerantWriteProtocol again

@@ -553,7 +592,7 @@ FvbNotificationEvent (
   if (EFI_ERROR(Status)) {

     return Status;

   }

-  

+

   //

   // Install protocol interface

   //

@@ -565,12 +604,18 @@ FvbNotificationEvent (
                     );

   ASSERT_EFI_ERROR (Status); 

 

+  ///

+  /// Register SMM FTW SMI handler

+  ///

+  Status = gSmst->SmiHandlerRegister (SmmFaultTolerantWriteHandler, &gEfiSmmFaultTolerantWriteProtocolGuid, &SmmFtwHandle);

+  ASSERT_EFI_ERROR (Status);

+

   //

   // Notify the Ftw wrapper driver SMM Ftw is ready

   //

-  SmmFtwHandle = NULL;

+  FtwHandle = NULL;

   Status = gBS->InstallProtocolInterface (

-                  &SmmFtwHandle,

+                  &FtwHandle,

                   &gEfiSmmFaultTolerantWriteProtocolGuid,

                   EFI_NATIVE_INTERFACE,

                   NULL

@@ -621,7 +666,6 @@ SmmFaultTolerantWriteInitialize (
   )

 {

   EFI_STATUS                              Status;

-  EFI_HANDLE                              FtwHandle;

   EFI_SMM_ACCESS2_PROTOCOL                *SmmAccess;

   UINTN                                   Size;

   VOID                                    *SmmEndOfDxeRegistration;

@@ -677,12 +721,6 @@ SmmFaultTolerantWriteInitialize (
   ASSERT_EFI_ERROR (Status);

 

   FvbNotificationEvent (NULL, NULL, NULL);

-

-  ///

-  /// Register SMM FTW SMI handler

-  ///

-  Status = gSmst->SmiHandlerRegister (SmmFaultTolerantWriteHandler, &gEfiSmmFaultTolerantWriteProtocolGuid, &FtwHandle);

-  ASSERT_EFI_ERROR (Status);

   

   return EFI_SUCCESS;

 }

diff --git a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmDxe.c b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmDxe.c
index 24b157df08..772d10dcd4 100644
--- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmDxe.c
+++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmDxe.c
@@ -3,7 +3,7 @@
   Implement the Fault Tolerant Write (FTW) protocol based on SMM FTW 

   module.

 

-Copyright (c) 2011, Intel Corporation. All rights reserved. <BR>

+Copyright (c) 2011 - 2013, Intel Corporation. All rights reserved. <BR>

 This program and the accompanying materials                          

 are licensed and made available under the terms and conditions of the BSD License         

 which accompanies this distribution.  The full text of the license may be found at        

@@ -463,13 +463,17 @@ FtwGetLastWrite (
   // Get data from SMM

   //

   *PrivateDataSize = SmmFtwGetLastWriteHeader->PrivateDataSize;

-  if (!EFI_ERROR (Status)) {

+  if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) {

     *Lba      = SmmFtwGetLastWriteHeader->Lba;

     *Offset   = SmmFtwGetLastWriteHeader->Offset; 

     *Length   = SmmFtwGetLastWriteHeader->Length;

     *Complete = SmmFtwGetLastWriteHeader->Complete;

     CopyGuid (CallerId, &SmmFtwGetLastWriteHeader->CallerId);

-    CopyMem (PrivateData, SmmFtwGetLastWriteHeader->Data, *PrivateDataSize);

+    if (Status == EFI_SUCCESS) {

+      CopyMem (PrivateData, SmmFtwGetLastWriteHeader->Data, *PrivateDataSize);

+    }

+  } else if (Status == EFI_NOT_FOUND) {

+    *Complete = SmmFtwGetLastWriteHeader->Complete;

   }

 

   FreePool (SmmCommunicateHeader);