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

2 files changed, 89 insertions, 20 deletions

diff --git a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c
index 214d044bcb..314d46d03a 100644
--- a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c
+++ b/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;

 }

 

diff --git a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.c b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.c
index 771c7f95d9..a3f8159b98 100644
--- a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.c
+++ b/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_INVALID_PARAMETER  The interrupt parameter is not valid. 

-  @retval EFI_ACCESS_DENIED      The interrupt buffer can't be written. 

-  @retval EFI_UNSUPPORTED        The interrupt is not supported. 

+  @retval EFI_SUCCESS                         The interrupt was handled and quiesced. No other handlers 

+                                              should still be called.

+  @retval EFI_WARN_INTERRUPT_SOURCE_QUIESCED  The interrupt has been quiesced but other handlers should 

+                                              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;

+

+  //

+  // If input is invalid, stop processing this SMI

+  //

+  if (CommBuffer == NULL || CommBufferSize == NULL) {

+    return EFI_SUCCESS;

+  }

+

+  if(*CommBufferSize < sizeof (SMM_BOOT_RECORD_COMMUNICATE)) {

+    return EFI_SUCCESS;

+  }

   

-  ASSERT (CommBuffer != NULL);

-  if (CommBuffer == NULL || *CommBufferSize < sizeof (SMM_BOOT_RECORD_COMMUNICATE)) {

-    return EFI_INVALID_PARAMETER;

+  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;

 }