From ccd2f6b0c6bade1a04bf91beea5c9bf55cb2f83a Mon Sep 17 00:00:00 2001 From: czhang46 Date: Wed, 27 Jun 2012 05:08:49 +0000 Subject: Add more SMRAM range check to 3 SMI handler. Signed-off-by: Chao Zhang Reviewed-by: Eric Jin git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@13477 6f19259b-4bc3-4df7-8a09-765794883524 --- .../SmmCorePerformanceLib/SmmCorePerformanceLib.c | 61 ++++++++++++++++++---- .../FirmwarePerformanceSmm.c | 48 +++++++++++++---- 2 files changed, 89 insertions(+), 20 deletions(-) (limited to 'MdeModulePkg') 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.
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.
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; } -- cgit v1.2.3