Add more SMRAM range check to 3 SMI handler.

Signed-off-by: Chao Zhang <chao.b.zhang@intel.com>
Reviewed-by: Eric Jin <eric.jin@intel.com>

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

MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c

@@ -9,6 +9,13 @@
   This library is mainly used by SMM Core to start performance logging to ensure that
   SMM Performance and PerformanceEx Protocol are installed at the very beginning of SMM phase.
 
+ Caution: This module requires additional review when modified.
+ This driver will have external input - performance data and communicate buffer in SMM mode.
+ This external input must be validated carefully to avoid security issue like
+ buffer overflow, integer overflow.
+
+ SmmPerformanceHandlerEx(), SmmPerformanceHandler() will receive untrusted input and do basic validation.
+
 Copyright (c) 2011 - 2012, 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
@@ -474,6 +481,9 @@ IsAddressInSmram (
   Communication service SMI Handler entry.
 
   This SMI handler provides services for the performance wrapper driver.
+  
+   Caution: This function may receive untrusted input.
+   Communicate buffer and buffer size are external input, so this function will do basic validation.
 
   @param[in]     DispatchHandle  The unique handle assigned to this handler by SmiHandlerRegister().
   @param[in]     RegisterContext Points to an optional handler context which was specified when the
@@ -506,8 +516,22 @@ SmmPerformanceHandlerEx (
 
   GaugeEntryExArray = NULL;
 
-  ASSERT (CommBuffer != NULL);
+  //
+  // If input is invalid, stop processing this SMI
+  //
+  if (CommBuffer == NULL || CommBufferSize == NULL) {
+    return EFI_SUCCESS;
+  }
 
+  if(*CommBufferSize < sizeof (SMM_PERF_COMMUNICATE_EX)) {
+    return EFI_SUCCESS;
+  }
+
+  if (IsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBuffer, *CommBufferSize)) {
+    DEBUG ((EFI_D_ERROR, "SMM communcation data buffer is in SMRAM!\n"));
+    return EFI_SUCCESS;
+  }
+  
   SmmPerfCommData = (SMM_PERF_COMMUNICATE_EX *)CommBuffer;
 
   switch (SmmPerfCommData->Function) {
@@ -528,9 +552,9 @@ SmmPerformanceHandlerEx (
        //
        DataSize = SmmPerfCommData->NumberOfEntries * sizeof(GAUGE_DATA_ENTRY_EX);
        if (IsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)SmmPerfCommData->GaugeDataEx, DataSize)) {
-         DEBUG ((EFI_D_ERROR, "Smm Performance Data buffer is in SMRAM!\n"));
+         DEBUG ((EFI_D_ERROR, "SMM Performance Data buffer is in SMRAM!\n"));
          Status = EFI_ACCESS_DENIED;
-         break ;
+         break;
        }
 
        GaugeEntryExArray = (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1);
@@ -543,11 +567,12 @@ SmmPerformanceHandlerEx (
        break;
 
     default:
-       ASSERT (FALSE);
        Status = EFI_UNSUPPORTED;
   }
 
+
   SmmPerfCommData->ReturnStatus = Status;
+  
   return EFI_SUCCESS;
 }
 
@@ -556,6 +581,9 @@ SmmPerformanceHandlerEx (
 
   This SMI handler provides services for the performance wrapper driver.
 
+  Caution: This function may receive untrusted input.
+  Communicate buffer and buffer size are external input, so this function will do basic validation.
+
   @param[in]     DispatchHandle  The unique handle assigned to this handler by SmiHandlerRegister().
   @param[in]     RegisterContext Points to an optional handler context which was specified when the
                                  handler was registered.
@@ -586,10 +614,24 @@ SmmPerformanceHandler (
   UINTN                 DataSize;
   UINTN                 Index;
   UINTN                 LogEntryKey;
-
+  
   GaugeEntryExArray = NULL;
 
-  ASSERT (CommBuffer != NULL);
+  //
+  // If input is invalid, stop processing this SMI
+  //
+  if (CommBuffer == NULL || CommBufferSize == NULL) {
+    return EFI_SUCCESS;
+  }
+
+  if(*CommBufferSize < sizeof (SMM_PERF_COMMUNICATE)) {
+    return EFI_SUCCESS;
+  }
+
+  if (IsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBuffer, *CommBufferSize)) {
+    DEBUG ((EFI_D_ERROR, "SMM communcation data buffer is in SMRAM!\n"));
+    return EFI_SUCCESS;
+  }
 
   SmmPerfCommData = (SMM_PERF_COMMUNICATE *)CommBuffer;
 
@@ -611,9 +653,9 @@ SmmPerformanceHandler (
        //
        DataSize = SmmPerfCommData->NumberOfEntries * sizeof(GAUGE_DATA_ENTRY);
        if (IsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)SmmPerfCommData->GaugeData, DataSize)) {
-         DEBUG ((EFI_D_ERROR, "Smm Performance Data buffer is in SMRAM!\n"));
+         DEBUG ((EFI_D_ERROR, "SMM Performance Data buffer is in SMRAM!\n"));
          Status = EFI_ACCESS_DENIED;
-         break ;
+         break;
        }
 
        GaugeEntryExArray = (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1);
@@ -630,11 +672,12 @@ SmmPerformanceHandler (
        break;
 
     default:
-       ASSERT (FALSE);
        Status = EFI_UNSUPPORTED;
   }
 
+
   SmmPerfCommData->ReturnStatus = Status;
+  
   return EFI_SUCCESS;
 }
 

MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.c

@@ -4,6 +4,13 @@
   This module registers report status code listener to collect performance data
   for SMM driver boot records and S3 Suspend Performance Record.
 
+  Caution: This module requires additional review when modified.
+  This driver will have external input - communicate buffer in SMM mode.
+  This external input must be validated carefully to avoid security issue like
+  buffer overflow, integer overflow.
+
+  FpdtSmiHandler() will receive untrusted input and do basic validation.
+
   Copyright (c) 2011 - 2012, 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
@@ -204,6 +211,9 @@ InternalIsAddressInSmram (
 
   This SMI handler provides services for report SMM boot records. 
 
+  Caution: This function may receive untrusted input.
+  Communicate buffer and buffer size are external input, so this function will do basic validation.
+
   @param[in]     DispatchHandle  The unique handle assigned to this handler by SmiHandlerRegister().
   @param[in]     RegisterContext Points to an optional handler context which was specified when the
                                  handler was registered.
@@ -211,10 +221,14 @@ InternalIsAddressInSmram (
                                  be conveyed from a non-SMM environment into an SMM environment.
   @param[in, out] CommBufferSize The size of the CommBuffer.
 
-  @retval EFI_SUCCESS            The interrupt was handled and quiesced. No other handlers should still be called.
+  @retval EFI_SUCCESS                         The interrupt was handled and quiesced. No other handlers 
-  @retval EFI_INVALID_PARAMETER  The interrupt parameter is not valid. 
+                                              should still be called.
-  @retval EFI_ACCESS_DENIED      The interrupt buffer can't be written. 
+  @retval EFI_WARN_INTERRUPT_SOURCE_QUIESCED  The interrupt has been quiesced but other handlers should 
-  @retval EFI_UNSUPPORTED        The interrupt is not supported. 
+                                              still be called.
+  @retval EFI_WARN_INTERRUPT_SOURCE_PENDING   The interrupt is still pending and other handlers should still 
+                                              be called.
+  @retval EFI_INTERRUPT_PENDING               The interrupt could not be quiesced.
+
 **/
 EFI_STATUS
 EFIAPI
@@ -227,15 +241,27 @@ FpdtSmiHandler (
 {
   EFI_STATUS                   Status;
   SMM_BOOT_RECORD_COMMUNICATE  *SmmCommData;
-  
+
-  ASSERT (CommBuffer != NULL);
+  //
-  if (CommBuffer == NULL || *CommBufferSize < sizeof (SMM_BOOT_RECORD_COMMUNICATE)) {
+  // If input is invalid, stop processing this SMI
-    return EFI_INVALID_PARAMETER;
+  //
+  if (CommBuffer == NULL || CommBufferSize == NULL) {
+    return EFI_SUCCESS;
+  }
+
+  if(*CommBufferSize < sizeof (SMM_BOOT_RECORD_COMMUNICATE)) {
+    return EFI_SUCCESS;
+  }
+  
+  if (InternalIsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBuffer, *CommBufferSize)) {
+    DEBUG ((EFI_D_ERROR, "SMM communication data buffer is in SMRAM!\n"));
+    return EFI_SUCCESS;
   }
 
-  Status = EFI_SUCCESS;
   SmmCommData = (SMM_BOOT_RECORD_COMMUNICATE*)CommBuffer;
 
+  Status = EFI_SUCCESS;
+
   switch (SmmCommData->Function) {
     case SMM_FPDT_FUNCTION_GET_BOOT_RECORD_SIZE :
        SmmCommData->BootRecordSize = mBootRecordSize;
@@ -246,13 +272,13 @@ FpdtSmiHandler (
          Status = EFI_INVALID_PARAMETER;
          break;
        } 
-	   
+
        //
        // Sanity check
        //
        SmmCommData->BootRecordSize = mBootRecordSize;
        if (InternalIsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)SmmCommData->BootRecordData, mBootRecordSize)) {
-         DEBUG ((EFI_D_ERROR, "Smm Data buffer is in SMRAM!\n"));
+         DEBUG ((EFI_D_ERROR, "SMM Data buffer is in SMRAM!\n"));
          Status = EFI_ACCESS_DENIED;
          break;
        }
@@ -265,11 +291,11 @@ FpdtSmiHandler (
        break;
 
     default:
-       ASSERT (FALSE);
        Status = EFI_UNSUPPORTED;
   }
 
   SmmCommData->ReturnStatus = Status;
+  
   return EFI_SUCCESS;
 }