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, 86 insertions, 58 deletions

diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c
index 47bc390f6f..754ab9a794 100644
--- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c
+++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c
@@ -14,7 +14,7 @@
   VariableServiceSetVariable(), VariableServiceQueryVariableInfo(), ReclaimForOS(), 

   SmmVariableGetStatistics() should also do validation based on its own knowledge.

 

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

+Copyright (c) 2010 - 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 

@@ -44,7 +44,9 @@ EFI_HANDLE                                           mSmmVariableHandle      = N
 EFI_HANDLE                                           mVariableHandle         = NULL;

 BOOLEAN                                              mAtRuntime              = FALSE;

 EFI_GUID                                             mZeroGuid               = {0, 0, 0, {0, 0, 0, 0, 0, 0, 0, 0}};

-  

+UINT8                                                *mVariableBufferPayload = NULL;

+UINTN                                                mVariableBufferPayloadSize;

+

 EFI_SMM_VARIABLE_PROTOCOL      gSmmVariable = {

   VariableServiceGetVariable,

   VariableServiceGetNextVariableName,

@@ -302,6 +304,8 @@ GetFvbCountAndBuffer (
   *NumberHandles = BufferSize / sizeof(EFI_HANDLE);

   if (EFI_ERROR(Status)) {

     *NumberHandles = 0;

+    FreePool (*Buffer);

+    *Buffer = NULL;

   }

 

   return Status;

@@ -338,6 +342,7 @@ SmmVariableGetStatistics (
   UINTN                                                NameLength;

   UINTN                                                StatisticsInfoSize;

   CHAR16                                               *InfoName;

+  EFI_GUID                                             VendorGuid;

  

   if (InfoEntry == NULL) {

     return EFI_INVALID_PARAMETER;

@@ -355,7 +360,9 @@ SmmVariableGetStatistics (
   }

   InfoName = (CHAR16 *)(InfoEntry + 1);

 

-  if (CompareGuid (&InfoEntry->VendorGuid, &mZeroGuid)) {

+  CopyGuid (&VendorGuid, &InfoEntry->VendorGuid);

+

+  if (CompareGuid (&VendorGuid, &mZeroGuid)) {

     //

     // Return the first variable info

     //

@@ -369,7 +376,7 @@ SmmVariableGetStatistics (
   // Get the next variable info

   //

   while (VariableInfo != NULL) {

-    if (CompareGuid (&VariableInfo->VendorGuid, &InfoEntry->VendorGuid)) {

+    if (CompareGuid (&VariableInfo->VendorGuid, &VendorGuid)) {

       NameLength = StrSize (VariableInfo->Name);

       if (NameLength == StrSize (InfoName)) {

         if (CompareMem (VariableInfo->Name, InfoName, NameLength) == 0) {

@@ -450,6 +457,7 @@ SmmVariableHandler (
   VARIABLE_INFO_ENTRY                              *VariableInfo;

   UINTN                                            InfoSize;

   UINTN                                            NameBufferSize;

+  UINTN                                            CommBufferPayloadSize;

 

   //

   // If input is invalid, stop processing this SMI

@@ -459,11 +467,17 @@ SmmVariableHandler (
   }

 

   if (*CommBufferSize < SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) {

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

+    return EFI_SUCCESS;

+  }

+  CommBufferPayloadSize = *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE;

+  if (CommBufferPayloadSize > mVariableBufferPayloadSize) {

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

     return EFI_SUCCESS;

   }

 

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

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

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

     return EFI_SUCCESS;

   }

   

@@ -471,7 +485,15 @@ SmmVariableHandler (
     

   switch (SmmVariableFunctionHeader->Function) {

     case SMM_VARIABLE_FUNCTION_GET_VARIABLE:

-      SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) SmmVariableFunctionHeader->Data;

+      if (CommBufferPayloadSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) {

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

+        return EFI_SUCCESS;

+      }

+      //

+      // Copy the input communicate buffer payload to pre-allocated SMM variable buffer payload.

+      //

+      CopyMem (mVariableBufferPayload, SmmVariableFunctionHeader->Data, CommBufferPayloadSize);

+      SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) mVariableBufferPayload;

       if (((UINTN)(~0) - SmmVariableHeader->DataSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) ||

          ((UINTN)(~0) - SmmVariableHeader->NameSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + SmmVariableHeader->DataSize)) {

         //

@@ -486,8 +508,8 @@ SmmVariableHandler (
       //

       // SMRAM range check already covered before

       //

-      if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) {

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

+      if (InfoSize > CommBufferPayloadSize) {

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

         Status = EFI_ACCESS_DENIED;

         goto EXIT;

       }

@@ -507,10 +529,19 @@ SmmVariableHandler (
                  &SmmVariableHeader->DataSize,

                  (UINT8 *)SmmVariableHeader->Name + SmmVariableHeader->NameSize

                  );

+      CopyMem (SmmVariableFunctionHeader->Data, mVariableBufferPayload, CommBufferPayloadSize);

       break;

       

     case SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME:

-      GetNextVariableName = (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) SmmVariableFunctionHeader->Data;

+      if (CommBufferPayloadSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {

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

+        return EFI_SUCCESS;

+      }

+      //

+      // Copy the input communicate buffer payload to pre-allocated SMM variable buffer payload.

+      //

+      CopyMem (mVariableBufferPayload, SmmVariableFunctionHeader->Data, CommBufferPayloadSize);

+      GetNextVariableName = (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) mVariableBufferPayload;

       if ((UINTN)(~0) - GetNextVariableName->NameSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {

         //

         // Prevent InfoSize overflow happen

@@ -523,13 +554,13 @@ SmmVariableHandler (
       //

       // SMRAM range check already covered before

       //

-      if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) {

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

+      if (InfoSize > CommBufferPayloadSize) {

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

         Status = EFI_ACCESS_DENIED;

         goto EXIT;

       }

 

-      NameBufferSize = *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE -  OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name);

+      NameBufferSize = CommBufferPayloadSize - OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name);

       if (NameBufferSize < sizeof (CHAR16) || GetNextVariableName->Name[NameBufferSize/sizeof (CHAR16) - 1] != L'\0') {

         //

         // Make sure input VariableName is A Null-terminated string.

@@ -543,10 +574,19 @@ SmmVariableHandler (
                  GetNextVariableName->Name,

                  &GetNextVariableName->Guid

                  );

+      CopyMem (SmmVariableFunctionHeader->Data, mVariableBufferPayload, CommBufferPayloadSize);

       break;

       

     case SMM_VARIABLE_FUNCTION_SET_VARIABLE:

-      SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) SmmVariableFunctionHeader->Data;

+      if (CommBufferPayloadSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) {

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

+        return EFI_SUCCESS;

+      }

+      //

+      // Copy the input communicate buffer payload to pre-allocated SMM variable buffer payload.

+      //

+      CopyMem (mVariableBufferPayload, SmmVariableFunctionHeader->Data, CommBufferPayloadSize);

+      SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) mVariableBufferPayload;

       if (((UINTN)(~0) - SmmVariableHeader->DataSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) ||

          ((UINTN)(~0) - SmmVariableHeader->NameSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + SmmVariableHeader->DataSize)) {

         //

@@ -562,8 +602,8 @@ SmmVariableHandler (
       // SMRAM range check already covered before

       // Data buffer should not contain SMM range

       //

-      if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) {

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

+      if (InfoSize > CommBufferPayloadSize) {

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

         Status = EFI_ACCESS_DENIED;

         goto EXIT;

       }

@@ -586,17 +626,11 @@ SmmVariableHandler (
       break;

       

     case SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO:

-      QueryVariableInfo = (SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO *) SmmVariableFunctionHeader->Data;

-      InfoSize = sizeof(SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO);

-

-      //

-      // SMRAM range check already covered before

-      //

-      if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) {

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

-        Status = EFI_ACCESS_DENIED;

-        goto EXIT;

+      if (CommBufferPayloadSize < sizeof (SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO)) {

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

+        return EFI_SUCCESS;

       }

+      QueryVariableInfo = (SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO *) SmmVariableFunctionHeader->Data;

   

       Status = VariableServiceQueryVariableInfo (

                  QueryVariableInfo->Attributes,

@@ -630,7 +664,7 @@ SmmVariableHandler (
       //

      

       if (InternalIsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBufferSize, sizeof(UINTN))) {

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

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

         Status = EFI_ACCESS_DENIED;

         goto EXIT;

       }  

@@ -786,6 +820,16 @@ VariableServiceInitialize (
 

   mSmramRangeCount = Size / sizeof (EFI_SMRAM_DESCRIPTOR);

 

+  mVariableBufferPayloadSize = MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize)) +

+                               OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - sizeof (VARIABLE_HEADER);

+

+  Status = gSmst->SmmAllocatePool (

+                    EfiRuntimeServicesData,

+                    mVariableBufferPayloadSize,

+                    (VOID **)&mVariableBufferPayload

+                    );

+  ASSERT_EFI_ERROR (Status);

+

   ///

   /// Register SMM variable SMI handler

   ///

diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c
index cdd407d66b..b7c7f4ffb7 100644
--- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c
+++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c
@@ -52,6 +52,7 @@ EFI_SMM_COMMUNICATION_PROTOCOL  *mSmmCommunication          = NULL;
 UINT8                           *mVariableBuffer            = NULL;

 UINT8                           *mVariableBufferPhysical    = NULL;

 UINTN                            mVariableBufferSize;

+UINTN                            mVariableBufferPayloadSize;

 EFI_LOCK                         mVariableServicesLock;

 

 /**

@@ -205,7 +206,6 @@ RuntimeServiceGetVariable (
   EFI_STATUS                                Status;

   UINTN                                     PayloadSize;

   SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE  *SmmVariableHeader;

-  UINTN                                     SmmCommBufPayloadSize;

   UINTN                                     TempDataSize;

   UINTN                                     VariableNameSize;

 

@@ -217,17 +217,13 @@ RuntimeServiceGetVariable (
     return EFI_INVALID_PARAMETER;

   }

 

-  //

-  // SMM Communication Buffer max payload size

-  //

-  SmmCommBufPayloadSize = mVariableBufferSize - (SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE);

   TempDataSize          = *DataSize;

   VariableNameSize      = StrSize (VariableName);

 

   //

   // If VariableName exceeds SMM payload limit. Return failure

   //

-  if (VariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) {

+  if (VariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) {

     return EFI_INVALID_PARAMETER;

   }

 

@@ -237,11 +233,11 @@ RuntimeServiceGetVariable (
   // Init the communicate buffer. The buffer data size is:

   // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.

   //

-  if (TempDataSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - VariableNameSize) {

+  if (TempDataSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - VariableNameSize) {

     //

     // If output data buffer exceed SMM payload limit. Trim output buffer to SMM payload size

     //

-    TempDataSize = SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - VariableNameSize;

+    TempDataSize = mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - VariableNameSize;

   }

   PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + VariableNameSize + TempDataSize;

 

@@ -316,7 +312,6 @@ RuntimeServiceGetNextVariableName (
   EFI_STATUS                                      Status;

   UINTN                                           PayloadSize;

   SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *SmmGetNextVariableName;

-  UINTN                                           SmmCommBufPayloadSize;

   UINTN                                           OutVariableNameSize;

   UINTN                                           InVariableNameSize;

 

@@ -324,17 +319,13 @@ RuntimeServiceGetNextVariableName (
     return EFI_INVALID_PARAMETER;

   }

 

-  //

-  // SMM Communication Buffer max payload size

-  //

-  SmmCommBufPayloadSize = mVariableBufferSize - (SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE);

   OutVariableNameSize   = *VariableNameSize;

   InVariableNameSize    = StrSize (VariableName);

 

   //

   // If input string exceeds SMM payload limit. Return failure

   //

-  if (InVariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {

+  if (InVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {

     return EFI_INVALID_PARAMETER;

   }

 

@@ -344,11 +335,11 @@ RuntimeServiceGetNextVariableName (
   // Init the communicate buffer. The buffer data size is:

   // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.

   //

-  if (OutVariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {

+  if (OutVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {

     //

     // If output buffer exceed SMM payload limit. Trim output buffer to SMM payload size

     //

-    OutVariableNameSize = SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name);

+    OutVariableNameSize = mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name);

   }

   //

   // Payload should be Guid + NameSize + MAX of Input & Output buffer

@@ -448,21 +439,13 @@ RuntimeServiceSetVariable (
     return EFI_INVALID_PARAMETER;

   }

 

-  if (DataSize >= mVariableBufferSize) {

-    //

-    // DataSize may be near MAX_ADDRESS incorrectly, this can cause the computed PayLoadSize to

-    // overflow to a small value and pass the check in InitCommunicateBuffer().

-    // To protect against this vulnerability, return EFI_INVALID_PARAMETER if DataSize is >= mVariableBufferSize.

-    // And there will be further check to ensure the total size is also not > mVariableBufferSize.

-    //

-    return EFI_INVALID_PARAMETER;

-  }

   VariableNameSize      = StrSize (VariableName);

 

-  if ((UINTN)(~0) - VariableNameSize < OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + DataSize) {

-    //

-    // Prevent PayloadSize overflow

-    //

+  //

+  // If VariableName or DataSize exceeds SMM payload limit. Return failure

+  //

+  if ((VariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) ||

+      (DataSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - VariableNameSize)){

     return EFI_INVALID_PARAMETER;

   }

 

@@ -672,10 +655,11 @@ SmmVariableReady (
   ASSERT_EFI_ERROR (Status);

   

   //

-  // Allocate memory for variable store.

+  // Allocate memory for variable communicate buffer.

   //

-  mVariableBufferSize  = SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE;

-  mVariableBufferSize += MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize));

+  mVariableBufferPayloadSize = MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize)) +

+                               OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - sizeof (VARIABLE_HEADER);

+  mVariableBufferSize  = SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + mVariableBufferPayloadSize;

   mVariableBuffer      = AllocateRuntimePool (mVariableBufferSize);

   ASSERT (mVariableBuffer != NULL);