Fix memory overflow & VariableSize check issue for SetVariable append write.

Signed-off-by: Chao Zhang <chao.b.zhang@intel.com>
Reviewed-by  : Fu Siyuan  <siyuan.fu@intel.com>
Reviewed-by  : Dong Guo   <guo.dong@intel.com>

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

3 files changed, 73 insertions, 30 deletions

diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/AuthService.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/AuthService.c
index 6f8808a75..440ede914 100644
--- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/AuthService.c
+++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/AuthService.c
@@ -192,7 +192,7 @@ AutenticatedVariableServiceInitialize (
   //

   // Reserved runtime buffer for "Append" operation in virtual mode.

   //

-  mStorageArea  = AllocateRuntimePool (PcdGet32 (PcdMaxVariableSize));

+  mStorageArea  = AllocateRuntimePool (MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize)));

   if (mStorageArea == NULL) {

     return EFI_OUT_OF_RESOURCES;

   }

@@ -1316,20 +1316,24 @@ ProcessVariable (
   will be appended to the original EFI_SIGNATURE_LIST, duplicate EFI_SIGNATURE_DATA

   will be ignored.

 

-  @param[in, out]  Data            Pointer to original EFI_SIGNATURE_LIST.

-  @param[in]       DataSize        Size of Data buffer.

-  @param[in]       NewData         Pointer to new EFI_SIGNATURE_LIST to be appended.

-  @param[in]       NewDataSize     Size of NewData buffer.

+  @param[in, out]  Data              Pointer to original EFI_SIGNATURE_LIST.

+  @param[in]       DataSize          Size of Data buffer.

+  @param[in]       FreeBufSize       Size of free data buffer 

+  @param[in]       NewData           Pointer to new EFI_SIGNATURE_LIST to be appended.

+  @param[in]       NewDataSize       Size of NewData buffer.

+  @param[out]      MergedBufSize     Size of the merged buffer

 

-  @return Size of the merged buffer.

+  @return EFI_BUFFER_TOO_SMALL if input Data buffer overflowed

 

 **/

-UINTN

+EFI_STATUS

 AppendSignatureList (

   IN  OUT VOID            *Data,

   IN  UINTN               DataSize,

+  IN  UINTN               FreeBufSize,

   IN  VOID                *NewData,

-  IN  UINTN               NewDataSize

+  IN  UINTN               NewDataSize,

+  OUT UINTN               *MergedBufSize

   )

 {

   EFI_SIGNATURE_LIST  *CertList;

@@ -1388,15 +1392,25 @@ AppendSignatureList (
         // New EFI_SIGNATURE_DATA, append it.

         //

         if (CopiedCount == 0) {

+          if (FreeBufSize < sizeof (EFI_SIGNATURE_LIST) + NewCertList->SignatureHeaderSize) {

+            return EFI_BUFFER_TOO_SMALL;

+          }

+

           //

           // Copy EFI_SIGNATURE_LIST header for only once.

           //

+

           CopyMem (Tail, NewCertList, sizeof (EFI_SIGNATURE_LIST) + NewCertList->SignatureHeaderSize);

           Tail = Tail + sizeof (EFI_SIGNATURE_LIST) + NewCertList->SignatureHeaderSize;

+          FreeBufSize -= sizeof (EFI_SIGNATURE_LIST) + NewCertList->SignatureHeaderSize;

         }

 

+        if (FreeBufSize < NewCertList->SignatureSize) {

+          return EFI_BUFFER_TOO_SMALL;

+        }

         CopyMem (Tail, NewCert, NewCertList->SignatureSize);

         Tail += NewCertList->SignatureSize;

+        FreeBufSize -= NewCertList->SignatureSize;

         CopiedCount++;

       }

 

@@ -1416,7 +1430,8 @@ AppendSignatureList (
     NewCertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) NewCertList + NewCertList->SignatureListSize);

   }

 

-  return (Tail - (UINT8 *) Data);

+  *MergedBufSize = (Tail - (UINT8 *) Data);

+  return EFI_SUCCESS;

 }

 

 /**

diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/AuthService.h b/SecurityPkg/VariableAuthenticated/RuntimeDxe/AuthService.h
index 510030d70..1fd687b03 100644
--- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/AuthService.h
+++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/AuthService.h
@@ -2,7 +2,7 @@
   The internal header file includes the common header files, defines

   internal structure and functions used by AuthService module.

 

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

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

@@ -249,20 +249,24 @@ ProcessVarWithKek (
   will be appended to the original EFI_SIGNATURE_LIST, duplicate EFI_SIGNATURE_DATA

   will be ignored.

 

-  @param[in, out]  Data            Pointer to original EFI_SIGNATURE_LIST.

-  @param[in]       DataSize        Size of Data buffer.

-  @param[in]       NewData         Pointer to new EFI_SIGNATURE_LIST to be appended.

-  @param[in]       NewDataSize     Size of NewData buffer.

+  @param[in, out]  Data             Pointer to original EFI_SIGNATURE_LIST.

+  @param[in]       DataSize         Size of Data buffer.

+  @param[in]       FreeBufSize      Size of free data buffer 

+  @param[in]       NewData          Pointer to new EFI_SIGNATURE_LIST to be appended.

+  @param[in]       NewDataSize      Size of NewData buffer.

+  @param[out]      MergedBufSize    Size of the merged buffer

 

-  @return Size of the merged buffer.

+  @return EFI_BUFFER_TOO_SMALL if input Data buffer overflowed

 

 **/

-UINTN

+EFI_STATUS

 AppendSignatureList (

   IN  OUT VOID            *Data,

   IN  UINTN               DataSize,

+  IN  UINTN               FreeBufSize,

   IN  VOID                *NewData,

-  IN  UINTN               NewDataSize

+  IN  UINTN               NewDataSize,

+  OUT UINTN               *MergedBufSize

   );

 

 /**

diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c
index ebe04b50f..f10d8f7d8 100644
--- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c
+++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c
@@ -1649,7 +1649,7 @@ UpdateVariable (
   EFI_STATUS                          Status;

   VARIABLE_HEADER                     *NextVariable;

   UINTN                               ScratchSize;

-  UINTN                               ScratchDataSize;

+  UINTN                               MaxDataSize;

   UINTN                               NonVolatileVarableStoreSize;

   UINTN                               VarNameOffset;

   UINTN                               VarDataOffset;

@@ -1664,7 +1664,6 @@ UpdateVariable (
   UINTN                               CacheOffset;

   UINTN                               BufSize;

   UINTN                               DataOffset;

-  UINTN                               RevBufSize;

 

   if (mVariableModuleGlobal->FvbInstance == NULL) {

     //

@@ -1713,7 +1712,7 @@ UpdateVariable (
   //

   NextVariable = GetEndPointer ((VARIABLE_STORE_HEADER *) ((UINTN) mVariableModuleGlobal->VariableGlobal.VolatileVariableBase));

   ScratchSize = MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize));

-  ScratchDataSize = ScratchSize - sizeof (VARIABLE_HEADER) - StrSize (VariableName) - GET_PAD_SIZE (StrSize (VariableName));

+

 

   if (Variable->CurrPtr != NULL) {

     //

@@ -1827,14 +1826,36 @@ UpdateVariable (
         DataOffset = sizeof (VARIABLE_HEADER) + Variable->CurrPtr->NameSize + GET_PAD_SIZE (Variable->CurrPtr->NameSize);

         CopyMem (mStorageArea, (UINT8*)((UINTN) Variable->CurrPtr + DataOffset), Variable->CurrPtr->DataSize);

 

+        //

+        // Set Max Common Variable Data Size as default MaxDataSize 

+        //

+        MaxDataSize = PcdGet32 (PcdMaxVariableSize) - sizeof (VARIABLE_HEADER) - StrSize (VariableName) - GET_PAD_SIZE (StrSize (VariableName));

+

+

         if ((CompareGuid (VendorGuid, &gEfiImageSecurityDatabaseGuid) &&

             ((StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE) == 0) || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE1) == 0))) ||

             (CompareGuid (VendorGuid, &gEfiGlobalVariableGuid) && (StrCmp (VariableName, EFI_KEY_EXCHANGE_KEY_NAME) == 0))) {

+

           //

           // For variables with formatted as EFI_SIGNATURE_LIST, the driver shall not perform an append of

           // EFI_SIGNATURE_DATA values that are already part of the existing variable value.

           //

-          BufSize = AppendSignatureList (mStorageArea, Variable->CurrPtr->DataSize, Data, DataSize);

+          Status = AppendSignatureList (

+                     mStorageArea, 

+                     Variable->CurrPtr->DataSize, 

+                     MaxDataSize - Variable->CurrPtr->DataSize,

+                     Data, 

+                     DataSize, 

+                     &BufSize

+                     );

+          if (Status == EFI_BUFFER_TOO_SMALL) {

+            //

+            // Signture List is too long, Failed to Append

+            //

+            Status = EFI_INVALID_PARAMETER;

+            goto Done;

+          }

+

           if (BufSize == Variable->CurrPtr->DataSize) {

             if ((TimeStamp == NULL) || CompareTimeStamp (TimeStamp, &Variable->CurrPtr->TimeStamp)) {

               //

@@ -1849,20 +1870,23 @@ UpdateVariable (
         } else {

           //

           // For other Variables, append the new data to the end of previous data.

+          // Max Harware error record variable data size is different from common variable

           //

+          if ((Attributes & EFI_VARIABLE_HARDWARE_ERROR_RECORD) == EFI_VARIABLE_HARDWARE_ERROR_RECORD) {

+            MaxDataSize = PcdGet32 (PcdMaxHardwareErrorVariableSize) - sizeof (VARIABLE_HEADER) - StrSize (VariableName) - GET_PAD_SIZE (StrSize (VariableName));

+          }

+

+          if (Variable->CurrPtr->DataSize + DataSize > MaxDataSize) {

+            //

+            // Exsiting data + Appended data exceed maximum variable size limitation 

+            //

+            Status = EFI_INVALID_PARAMETER;

+            goto Done;

+          }

           CopyMem ((UINT8*)((UINTN) mStorageArea + Variable->CurrPtr->DataSize), Data, DataSize);

           BufSize = Variable->CurrPtr->DataSize + DataSize;

         }

 

-        RevBufSize = MIN (PcdGet32 (PcdMaxVariableSize), ScratchDataSize);

-        if (BufSize > RevBufSize) {

-          //

-          // If variable size (previous + current) is bigger than reserved buffer in runtime,

-          // return EFI_OUT_OF_RESOURCES.

-          //

-          return EFI_OUT_OF_RESOURCES;

-        }

-

         //

         // Override Data and DataSize which are used for combined data area including previous and new data.

         //