1. Use the check IsAddressValid() to prevent SMM communication buffer overflow in SmmVariable, FtwSmm, FpdtSmm, SmmCorePerformance and SmmBaseHelper, and add check to prevent InfoSize overflows in SmmVariableHandler.

2. Refine the debug message.
3. Add check to make sure the input VariableName is A Null-terminated string.
4. Use local variable to hold StrSize (VariableName) to avoid duplicated StrSize calculation.

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

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

2 files changed, 110 insertions, 18 deletions

diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c
index 678cff3c7..47bc390f6 100644
--- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c
+++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c
@@ -93,6 +93,32 @@ InternalIsAddressInSmram (
   return FALSE;

 }

 

+/**

+  This function check if the address refered by Buffer and Length is valid.

+

+  @param Buffer  the buffer address to be checked.

+  @param Length  the buffer length to be checked.

+

+  @retval TRUE  this address is valid.

+  @retval FALSE this address is NOT valid.

+**/

+BOOLEAN

+InternalIsAddressValid (

+  IN UINTN                 Buffer,

+  IN UINTN                 Length

+  )

+{

+  if (Buffer > (MAX_ADDRESS - Length)) {

+    //

+    // Overflow happen

+    //

+    return FALSE;

+  }

+  if (InternalIsAddressInSmram ((EFI_PHYSICAL_ADDRESS)Buffer, (UINT64)Length)) {

+    return FALSE;

+  }

+  return TRUE;

+}

 

 /**

   Initializes a basic mutual exclusion lock.

@@ -423,6 +449,7 @@ SmmVariableHandler (
   SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO     *QueryVariableInfo;

   VARIABLE_INFO_ENTRY                              *VariableInfo;

   UINTN                                            InfoSize;

+  UINTN                                            NameBufferSize;

 

   //

   // If input is invalid, stop processing this SMI

@@ -435,8 +462,8 @@ SmmVariableHandler (
     return EFI_SUCCESS;

   }

 

-  if (InternalIsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBuffer, *CommBufferSize)) {

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

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

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

     return EFI_SUCCESS;

   }

   

@@ -445,6 +472,14 @@ SmmVariableHandler (
   switch (SmmVariableFunctionHeader->Function) {

     case SMM_VARIABLE_FUNCTION_GET_VARIABLE:

       SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) SmmVariableFunctionHeader->Data;

+      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)) {

+        //

+        // Prevent InfoSize overflow happen

+        //

+        Status = EFI_ACCESS_DENIED;

+        goto EXIT;

+      }

       InfoSize = OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) 

                  + SmmVariableHeader->DataSize + SmmVariableHeader->NameSize;

 

@@ -457,6 +492,14 @@ SmmVariableHandler (
         goto EXIT;

       }

 

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

+        //

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

+        //

+        Status = EFI_ACCESS_DENIED;

+        goto EXIT;

+      }

+

       Status = VariableServiceGetVariable (

                  SmmVariableHeader->Name,

                  &SmmVariableHeader->Guid,

@@ -468,6 +511,13 @@ SmmVariableHandler (
       

     case SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME:

       GetNextVariableName = (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) SmmVariableFunctionHeader->Data;

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

+        //

+        // Prevent InfoSize overflow happen

+        //

+        Status = EFI_ACCESS_DENIED;

+        goto EXIT;

+      }

       InfoSize = OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name) + GetNextVariableName->NameSize;

 

       //

@@ -479,6 +529,15 @@ SmmVariableHandler (
         goto EXIT;

       }

 

+      NameBufferSize = *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE -  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.

+        //

+        Status = EFI_ACCESS_DENIED;

+        goto EXIT;

+      }

+

       Status = VariableServiceGetNextVariableName (

                  &GetNextVariableName->NameSize,

                  GetNextVariableName->Name,

@@ -488,6 +547,14 @@ SmmVariableHandler (
       

     case SMM_VARIABLE_FUNCTION_SET_VARIABLE:

       SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) SmmVariableFunctionHeader->Data;

+      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)) {

+        //

+        // Prevent InfoSize overflow happen

+        //

+        Status = EFI_ACCESS_DENIED;

+        goto EXIT;

+      }

       InfoSize = OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)

                  + SmmVariableHeader->DataSize + SmmVariableHeader->NameSize;

 

@@ -501,6 +568,14 @@ SmmVariableHandler (
         goto EXIT;

       }

 

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

+        //

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

+        //

+        Status = EFI_ACCESS_DENIED;

+        goto EXIT;

+      }

+

       Status = VariableServiceSetVariable (

                  SmmVariableHeader->Name,

                  &SmmVariableHeader->Guid,

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

      

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

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

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

         Status = EFI_ACCESS_DENIED;

         goto EXIT;

       }  

diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c
index 5a02b77d5..cdd407d66 100644
--- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c
+++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c
@@ -207,6 +207,7 @@ RuntimeServiceGetVariable (
   SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE  *SmmVariableHeader;

   UINTN                                     SmmCommBufPayloadSize;

   UINTN                                     TempDataSize;

+  UINTN                                     VariableNameSize;

 

   if (VariableName == NULL || VendorGuid == NULL || DataSize == NULL) {

     return EFI_INVALID_PARAMETER;

@@ -221,11 +222,12 @@ RuntimeServiceGetVariable (
   //

   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 (StrSize (VariableName) > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) {

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

     return EFI_INVALID_PARAMETER;

   }

 

@@ -235,13 +237,13 @@ 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) - StrSize (VariableName)) {

+  if (TempDataSize > SmmCommBufPayloadSize - 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) - StrSize (VariableName);

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

   }

-  PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + TempDataSize;

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

 

   Status = InitCommunicateBuffer ((VOID **)&SmmVariableHeader, PayloadSize, SMM_VARIABLE_FUNCTION_GET_VARIABLE);

   if (EFI_ERROR (Status)) {

@@ -251,7 +253,7 @@ RuntimeServiceGetVariable (
 

   CopyGuid (&SmmVariableHeader->Guid, VendorGuid);

   SmmVariableHeader->DataSize   = TempDataSize;

-  SmmVariableHeader->NameSize   = StrSize (VariableName);

+  SmmVariableHeader->NameSize   = VariableNameSize;

   if (Attributes == NULL) {

     SmmVariableHeader->Attributes = 0;

   } else {

@@ -315,6 +317,8 @@ RuntimeServiceGetNextVariableName (
   UINTN                                           PayloadSize;

   SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *SmmGetNextVariableName;

   UINTN                                           SmmCommBufPayloadSize;

+  UINTN                                           OutVariableNameSize;

+  UINTN                                           InVariableNameSize;

 

   if (VariableNameSize == NULL || VariableName == NULL || VendorGuid == NULL) {

     return EFI_INVALID_PARAMETER;

@@ -324,11 +328,13 @@ RuntimeServiceGetNextVariableName (
   // 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 (StrSize (VariableName) > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {

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

     return EFI_INVALID_PARAMETER;

   }

 

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

   // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.

   //

-  if (*VariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) {

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

     //

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

     //

-    *VariableNameSize = SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name);

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

   }

   //

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

   //

-  PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name) + MAX (*VariableNameSize, StrSize (VariableName));

+  PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name) + MAX (OutVariableNameSize, InVariableNameSize);

 

   Status = InitCommunicateBuffer ((VOID **)&SmmGetNextVariableName, PayloadSize, SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME);

   if (EFI_ERROR (Status)) {

@@ -358,13 +364,16 @@ RuntimeServiceGetNextVariableName (
   //

   // SMM comm buffer->NameSize is buffer size for return string

   //

-  SmmGetNextVariableName->NameSize = *VariableNameSize;

+  SmmGetNextVariableName->NameSize = OutVariableNameSize;

 

   CopyGuid (&SmmGetNextVariableName->Guid, VendorGuid);

   //

   // Copy whole string

   //

-  CopyMem (SmmGetNextVariableName->Name, VariableName, StrSize (VariableName));

+  CopyMem (SmmGetNextVariableName->Name, VariableName, InVariableNameSize);

+  if (OutVariableNameSize > InVariableNameSize) {

+    ZeroMem ((UINT8 *) SmmGetNextVariableName->Name + InVariableNameSize, OutVariableNameSize - InVariableNameSize);

+  }

 

   //

   // Send data to SMM

@@ -374,7 +383,13 @@ RuntimeServiceGetNextVariableName (
   //

   // Get data from SMM.

   //

-  *VariableNameSize = SmmGetNextVariableName->NameSize;    

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

+    //

+    // SMM CommBuffer NameSize can be a trimed value

+    // Only update VariableNameSize when needed

+    //

+    *VariableNameSize = SmmGetNextVariableName->NameSize;

+  }

   if (EFI_ERROR (Status)) {

     goto Done;

   }

@@ -420,6 +435,7 @@ RuntimeServiceSetVariable (
   EFI_STATUS                                Status;

   UINTN                                     PayloadSize; 

   SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE  *SmmVariableHeader;

+  UINTN                                     VariableNameSize;

     

   //

   // Check input parameters.

@@ -441,8 +457,9 @@ RuntimeServiceSetVariable (
     //

     return EFI_INVALID_PARAMETER;

   }

+  VariableNameSize      = StrSize (VariableName);

 

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

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

     //

     // Prevent PayloadSize overflow

     //

@@ -455,7 +472,7 @@ RuntimeServiceSetVariable (
   // Init the communicate buffer. The buffer data size is:

   // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.

   //

-  PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + StrSize (VariableName) + DataSize;

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

   Status = InitCommunicateBuffer ((VOID **)&SmmVariableHeader, PayloadSize, SMM_VARIABLE_FUNCTION_SET_VARIABLE);

   if (EFI_ERROR (Status)) {

     goto Done;

@@ -464,7 +481,7 @@ RuntimeServiceSetVariable (
 

   CopyGuid ((EFI_GUID *) &SmmVariableHeader->Guid, VendorGuid);

   SmmVariableHeader->DataSize   = DataSize;

-  SmmVariableHeader->NameSize   = StrSize (VariableName);

+  SmmVariableHeader->NameSize   = VariableNameSize;

   SmmVariableHeader->Attributes = Attributes;

   CopyMem (SmmVariableHeader->Name, VariableName, SmmVariableHeader->NameSize);

   CopyMem ((UINT8 *) SmmVariableHeader->Name + SmmVariableHeader->NameSize, Data, DataSize);