From 5e5bb2a9baefcd2f231696ea94576dab5565fbfb Mon Sep 17 00:00:00 2001 From: lzeng14 Date: Tue, 7 May 2013 05:38:32 +0000 Subject: [PATCH] 1. Fix TOCTOU issue in VariableSmm, FtwSmm, FpdtSmm, SmmCorePerformance SMM handler. For VariableSmm, pre-allocate a mVariableBufferPayload buffer with mVariableBufferPayloadSize(match with mVariableBufferPayloadSize in VariableSmmRuntimeDxe) to hold communicate buffer payload to avoid TOCTOU issue. 2. Add check to ensure CommBufferPayloadSize not exceed mVariableBufferPayloadSize or is enough to hold function structure in VariableSmm and FtwSmm. 3. Align FtwGetLastWrite() in FaultTolerantWriteSmmDxe.c to FtwGetLastWrite() in FaultTolerantWrite.c. Signed-off-by: Star Zeng Reviewed-by: Jiewen Yao git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@14325 6f19259b-4bc3-4df7-8a09-765794883524 --- .../SmmCorePerformanceLib.c | 44 +++++---- .../FirmwarePerformanceSmm.c | 14 ++- .../FaultTolerantWriteSmm.c | 94 ++++++++++++------ .../FaultTolerantWriteSmmDxe.c | 10 +- .../Universal/LockBox/SmmLockBox/SmmLockBox.c | 46 +++++---- .../Variable/RuntimeDxe/VariableSmm.c | 98 ++++++++++++++----- .../RuntimeDxe/VariableSmmRuntimeDxe.c | 48 +++------ .../RuntimeDxe/VariableSmm.c | 96 +++++++++++++----- .../RuntimeDxe/VariableSmmRuntimeDxe.c | 48 +++------ 9 files changed, 311 insertions(+), 187 deletions(-) diff --git a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c index 5755f2affc..2bfd62a2b9 100644 --- a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c +++ b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c @@ -540,6 +540,9 @@ SmmPerformanceHandlerEx ( SMM_PERF_COMMUNICATE_EX *SmmPerfCommData; GAUGE_DATA_ENTRY_EX *GaugeEntryExArray; UINTN DataSize; + GAUGE_DATA_ENTRY_EX *GaugeDataEx; + UINTN NumberOfEntries; + UINTN LogEntryKey; GaugeEntryExArray = NULL; @@ -555,7 +558,7 @@ SmmPerformanceHandlerEx ( } if (!IsAddressValid ((UINTN)CommBuffer, *CommBufferSize)) { - DEBUG ((EFI_D_ERROR, "SMM communcation data buffer in SMRAM or overflow!\n")); + DEBUG ((EFI_D_ERROR, "SmmPerformanceHandlerEx: SMM communcation data buffer in SMRAM or overflow!\n")); return EFI_SUCCESS; } @@ -568,8 +571,11 @@ SmmPerformanceHandlerEx ( break; case SMM_PERF_FUNCTION_GET_GAUGE_DATA : - if ( SmmPerfCommData->GaugeDataEx == NULL || SmmPerfCommData->NumberOfEntries == 0 || - (SmmPerfCommData->LogEntryKey + SmmPerfCommData->NumberOfEntries) > mGaugeData->NumberOfEntries) { + GaugeDataEx = SmmPerfCommData->GaugeDataEx; + NumberOfEntries = SmmPerfCommData->NumberOfEntries; + LogEntryKey = SmmPerfCommData->LogEntryKey; + if (GaugeDataEx == NULL || NumberOfEntries == 0 || LogEntryKey > mGaugeData->NumberOfEntries || + NumberOfEntries > mGaugeData->NumberOfEntries || (LogEntryKey + NumberOfEntries) > mGaugeData->NumberOfEntries) { Status = EFI_INVALID_PARAMETER; break; } @@ -577,17 +583,17 @@ SmmPerformanceHandlerEx ( // // Sanity check // - DataSize = SmmPerfCommData->NumberOfEntries * sizeof(GAUGE_DATA_ENTRY_EX); - if (!IsAddressValid ((UINTN)SmmPerfCommData->GaugeDataEx, DataSize)) { - DEBUG ((EFI_D_ERROR, "SMM Performance Data buffer in SMRAM or overflow!\n")); + DataSize = NumberOfEntries * sizeof(GAUGE_DATA_ENTRY_EX); + if (!IsAddressValid ((UINTN)GaugeDataEx, DataSize)) { + DEBUG ((EFI_D_ERROR, "SmmPerformanceHandlerEx: SMM Performance Data buffer in SMRAM or overflow!\n")); Status = EFI_ACCESS_DENIED; break; } GaugeEntryExArray = (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1); CopyMem( - (UINT8 *) (SmmPerfCommData->GaugeDataEx), - (UINT8 *) &GaugeEntryExArray[SmmPerfCommData->LogEntryKey], + (UINT8 *) GaugeDataEx, + (UINT8 *) &GaugeEntryExArray[LogEntryKey], DataSize ); Status = EFI_SUCCESS; @@ -640,6 +646,8 @@ SmmPerformanceHandler ( GAUGE_DATA_ENTRY_EX *GaugeEntryExArray; UINTN DataSize; UINTN Index; + GAUGE_DATA_ENTRY *GaugeData; + UINTN NumberOfEntries; UINTN LogEntryKey; GaugeEntryExArray = NULL; @@ -656,7 +664,7 @@ SmmPerformanceHandler ( } if (!IsAddressValid ((UINTN)CommBuffer, *CommBufferSize)) { - DEBUG ((EFI_D_ERROR, "SMM communcation data buffer in SMRAM or overflow!\n")); + DEBUG ((EFI_D_ERROR, "SmmPerformanceHandler: SMM communcation data buffer in SMRAM or overflow!\n")); return EFI_SUCCESS; } @@ -669,8 +677,11 @@ SmmPerformanceHandler ( break; case SMM_PERF_FUNCTION_GET_GAUGE_DATA : - if ( SmmPerfCommData->GaugeData == NULL || SmmPerfCommData->NumberOfEntries == 0 || - (SmmPerfCommData->LogEntryKey + SmmPerfCommData->NumberOfEntries) > mGaugeData->NumberOfEntries) { + GaugeData = SmmPerfCommData->GaugeData; + NumberOfEntries = SmmPerfCommData->NumberOfEntries; + LogEntryKey = SmmPerfCommData->LogEntryKey; + if (GaugeData == NULL || NumberOfEntries == 0 || LogEntryKey > mGaugeData->NumberOfEntries || + NumberOfEntries > mGaugeData->NumberOfEntries || (LogEntryKey + NumberOfEntries) > mGaugeData->NumberOfEntries) { Status = EFI_INVALID_PARAMETER; break; } @@ -678,19 +689,18 @@ SmmPerformanceHandler ( // // Sanity check // - DataSize = SmmPerfCommData->NumberOfEntries * sizeof(GAUGE_DATA_ENTRY); - if (!IsAddressValid ((UINTN)SmmPerfCommData->GaugeData, DataSize)) { - DEBUG ((EFI_D_ERROR, "SMM Performance Data buffer in SMRAM or overflow!\n")); + DataSize = NumberOfEntries * sizeof(GAUGE_DATA_ENTRY); + if (!IsAddressValid ((UINTN)GaugeData, DataSize)) { + DEBUG ((EFI_D_ERROR, "SmmPerformanceHandler: SMM Performance Data buffer in SMRAM or overflow!\n")); Status = EFI_ACCESS_DENIED; break; } GaugeEntryExArray = (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1); - LogEntryKey = SmmPerfCommData->LogEntryKey; - for (Index = 0; Index < SmmPerfCommData->NumberOfEntries; Index++) { + for (Index = 0; Index < NumberOfEntries; Index++) { CopyMem( - (UINT8 *) &(SmmPerfCommData->GaugeData[Index]), + (UINT8 *) &GaugeData[Index], (UINT8 *) &GaugeEntryExArray[LogEntryKey++], sizeof (GAUGE_DATA_ENTRY) ); diff --git a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.c b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.c index ebf81ca6f7..f3472e26f3 100644 --- a/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.c +++ b/MdeModulePkg/Universal/Acpi/FirmwarePerformanceDataTableSmm/FirmwarePerformanceSmm.c @@ -266,6 +266,8 @@ FpdtSmiHandler ( { EFI_STATUS Status; SMM_BOOT_RECORD_COMMUNICATE *SmmCommData; + UINTN BootRecordSize; + VOID *BootRecordData; // // If input is invalid, stop processing this SMI @@ -279,7 +281,7 @@ FpdtSmiHandler ( } if (!InternalIsAddressValid ((UINTN)CommBuffer, *CommBufferSize)) { - DEBUG ((EFI_D_ERROR, "SMM communication data buffer in SMRAM or overflow!\n")); + DEBUG ((EFI_D_ERROR, "FpdtSmiHandler: SMM communication data buffer in SMRAM or overflow!\n")); return EFI_SUCCESS; } @@ -293,7 +295,9 @@ FpdtSmiHandler ( break; case SMM_FPDT_FUNCTION_GET_BOOT_RECORD_DATA : - if (SmmCommData->BootRecordData == NULL || SmmCommData->BootRecordSize < mBootRecordSize) { + BootRecordData = SmmCommData->BootRecordData; + BootRecordSize = SmmCommData->BootRecordSize; + if (BootRecordData == NULL || BootRecordSize < mBootRecordSize) { Status = EFI_INVALID_PARAMETER; break; } @@ -302,14 +306,14 @@ FpdtSmiHandler ( // Sanity check // SmmCommData->BootRecordSize = mBootRecordSize; - if (!InternalIsAddressValid ((UINTN)SmmCommData->BootRecordData, mBootRecordSize)) { - DEBUG ((EFI_D_ERROR, "SMM Data buffer in SMRAM or overflow!\n")); + if (!InternalIsAddressValid ((UINTN)BootRecordData, mBootRecordSize)) { + DEBUG ((EFI_D_ERROR, "FpdtSmiHandler: SMM Data buffer in SMRAM or overflow!\n")); Status = EFI_ACCESS_DENIED; break; } CopyMem ( - (UINT8*)SmmCommData->BootRecordData, + (UINT8*)BootRecordData, mBootRecordBuffer, mBootRecordSize ); diff --git a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c index 8b88ae41ee..2580d478a3 100644 --- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c +++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c @@ -369,6 +369,9 @@ SmmFaultTolerantWriteHandler ( VOID *PrivateData; EFI_HANDLE SmmFvbHandle; UINTN InfoSize; + UINTN CommBufferPayloadSize; + UINTN PrivateDataSize; + UINTN Length; // @@ -379,11 +382,13 @@ SmmFaultTolerantWriteHandler ( } if (*CommBufferSize < SMM_FTW_COMMUNICATE_HEADER_SIZE) { + DEBUG ((EFI_D_ERROR, "SmmFtwHandler: SMM communication buffer size invalid!\n")); return EFI_SUCCESS; } + CommBufferPayloadSize = *CommBufferSize - SMM_FTW_COMMUNICATE_HEADER_SIZE; if (!InternalIsAddressValid ((UINTN)CommBuffer, *CommBufferSize)) { - DEBUG ((EFI_D_ERROR, "SMM communication buffer in SMRAM or overflow!\n")); + DEBUG ((EFI_D_ERROR, "SmmFtwHandler: SMM communication buffer in SMRAM or overflow!\n")); return EFI_SUCCESS; } @@ -400,17 +405,11 @@ SmmFaultTolerantWriteHandler ( switch (SmmFtwFunctionHeader->Function) { case FTW_FUNCTION_GET_MAX_BLOCK_SIZE: - SmmGetMaxBlockSizeHeader = (SMM_FTW_GET_MAX_BLOCK_SIZE_HEADER *) SmmFtwFunctionHeader->Data; - InfoSize = sizeof (SMM_FTW_GET_MAX_BLOCK_SIZE_HEADER); - - // - // SMRAM range check already covered before - // - if (InfoSize > *CommBufferSize - SMM_FTW_COMMUNICATE_HEADER_SIZE) { - DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); - Status = EFI_ACCESS_DENIED; - break; + if (CommBufferPayloadSize < sizeof (SMM_FTW_GET_MAX_BLOCK_SIZE_HEADER)) { + DEBUG ((EFI_D_ERROR, "GetMaxBlockSize: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; } + SmmGetMaxBlockSizeHeader = (SMM_FTW_GET_MAX_BLOCK_SIZE_HEADER *) SmmFtwFunctionHeader->Data; Status = FtwGetMaxBlockSize ( &mFtwDevice->FtwInstance, @@ -419,6 +418,10 @@ SmmFaultTolerantWriteHandler ( break; case FTW_FUNCTION_ALLOCATE: + if (CommBufferPayloadSize < sizeof (SMM_FTW_ALLOCATE_HEADER)) { + DEBUG ((EFI_D_ERROR, "Allocate: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } SmmFtwAllocateHeader = (SMM_FTW_ALLOCATE_HEADER *) SmmFtwFunctionHeader->Data; Status = FtwAllocate ( &mFtwDevice->FtwInstance, @@ -429,11 +432,36 @@ SmmFaultTolerantWriteHandler ( break; case FTW_FUNCTION_WRITE: + if (CommBufferPayloadSize < OFFSET_OF (SMM_FTW_WRITE_HEADER, Data)) { + DEBUG ((EFI_D_ERROR, "Write: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } SmmFtwWriteHeader = (SMM_FTW_WRITE_HEADER *) SmmFtwFunctionHeader->Data; - if (SmmFtwWriteHeader->PrivateDataSize == 0) { + Length = SmmFtwWriteHeader->Length; + PrivateDataSize = SmmFtwWriteHeader->PrivateDataSize; + if (((UINTN)(~0) - Length < OFFSET_OF (SMM_FTW_WRITE_HEADER, Data)) || + ((UINTN)(~0) - PrivateDataSize < OFFSET_OF (SMM_FTW_WRITE_HEADER, Data) + Length)) { + // + // Prevent InfoSize overflow + // + Status = EFI_ACCESS_DENIED; + break; + } + InfoSize = OFFSET_OF (SMM_FTW_WRITE_HEADER, Data) + Length + PrivateDataSize; + + // + // SMRAM range check already covered before + // + if (InfoSize > CommBufferPayloadSize) { + DEBUG ((EFI_D_ERROR, "Write: Data size exceed communication buffer size limit!\n")); + Status = EFI_ACCESS_DENIED; + break; + } + + if (PrivateDataSize == 0) { PrivateData = NULL; } else { - PrivateData = (VOID *)&SmmFtwWriteHeader->Data[SmmFtwWriteHeader->Length]; + PrivateData = (VOID *)&SmmFtwWriteHeader->Data[Length]; } Status = GetFvbByAddressAndAttribute ( SmmFtwWriteHeader->FvbBaseAddress, @@ -445,7 +473,7 @@ SmmFaultTolerantWriteHandler ( &mFtwDevice->FtwInstance, SmmFtwWriteHeader->Lba, SmmFtwWriteHeader->Offset, - SmmFtwWriteHeader->Length, + Length, PrivateData, SmmFvbHandle, SmmFtwWriteHeader->Data @@ -454,6 +482,10 @@ SmmFaultTolerantWriteHandler ( break; case FTW_FUNCTION_RESTART: + if (CommBufferPayloadSize < sizeof (SMM_FTW_RESTART_HEADER)) { + DEBUG ((EFI_D_ERROR, "Restart: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } SmmFtwRestartHeader = (SMM_FTW_RESTART_HEADER *) SmmFtwFunctionHeader->Data; Status = GetFvbByAddressAndAttribute ( SmmFtwRestartHeader->FvbBaseAddress, @@ -470,20 +502,25 @@ SmmFaultTolerantWriteHandler ( break; case FTW_FUNCTION_GET_LAST_WRITE: + if (CommBufferPayloadSize < OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data)) { + DEBUG ((EFI_D_ERROR, "GetLastWrite: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } SmmFtwGetLastWriteHeader = (SMM_FTW_GET_LAST_WRITE_HEADER *) SmmFtwFunctionHeader->Data; - if ((UINTN)(~0) - SmmFtwGetLastWriteHeader->PrivateDataSize < OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data)){ + PrivateDataSize = SmmFtwGetLastWriteHeader->PrivateDataSize; + if ((UINTN)(~0) - PrivateDataSize < OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data)){ // // Prevent InfoSize overflow // Status = EFI_ACCESS_DENIED; break; } - InfoSize = OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data) + SmmFtwGetLastWriteHeader->PrivateDataSize; + InfoSize = OFFSET_OF (SMM_FTW_GET_LAST_WRITE_HEADER, Data) + PrivateDataSize; // // SMRAM range check already covered before // - if (InfoSize > *CommBufferSize - SMM_FTW_COMMUNICATE_HEADER_SIZE) { + if (InfoSize > CommBufferPayloadSize) { DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); Status = EFI_ACCESS_DENIED; break; @@ -495,10 +532,11 @@ SmmFaultTolerantWriteHandler ( &SmmFtwGetLastWriteHeader->Lba, &SmmFtwGetLastWriteHeader->Offset, &SmmFtwGetLastWriteHeader->Length, - &SmmFtwGetLastWriteHeader->PrivateDataSize, + &PrivateDataSize, (VOID *)SmmFtwGetLastWriteHeader->Data, &SmmFtwGetLastWriteHeader->Complete ); + SmmFtwGetLastWriteHeader->PrivateDataSize = PrivateDataSize; break; default: @@ -532,6 +570,7 @@ FvbNotificationEvent ( EFI_STATUS Status; EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL *FtwProtocol; EFI_HANDLE SmmFtwHandle; + EFI_HANDLE FtwHandle; // // Just return to avoid install SMM FaultTolerantWriteProtocol again @@ -553,7 +592,7 @@ FvbNotificationEvent ( if (EFI_ERROR(Status)) { return Status; } - + // // Install protocol interface // @@ -565,12 +604,18 @@ FvbNotificationEvent ( ); ASSERT_EFI_ERROR (Status); + /// + /// Register SMM FTW SMI handler + /// + Status = gSmst->SmiHandlerRegister (SmmFaultTolerantWriteHandler, &gEfiSmmFaultTolerantWriteProtocolGuid, &SmmFtwHandle); + ASSERT_EFI_ERROR (Status); + // // Notify the Ftw wrapper driver SMM Ftw is ready // - SmmFtwHandle = NULL; + FtwHandle = NULL; Status = gBS->InstallProtocolInterface ( - &SmmFtwHandle, + &FtwHandle, &gEfiSmmFaultTolerantWriteProtocolGuid, EFI_NATIVE_INTERFACE, NULL @@ -621,7 +666,6 @@ SmmFaultTolerantWriteInitialize ( ) { EFI_STATUS Status; - EFI_HANDLE FtwHandle; EFI_SMM_ACCESS2_PROTOCOL *SmmAccess; UINTN Size; VOID *SmmEndOfDxeRegistration; @@ -677,12 +721,6 @@ SmmFaultTolerantWriteInitialize ( ASSERT_EFI_ERROR (Status); FvbNotificationEvent (NULL, NULL, NULL); - - /// - /// Register SMM FTW SMI handler - /// - Status = gSmst->SmiHandlerRegister (SmmFaultTolerantWriteHandler, &gEfiSmmFaultTolerantWriteProtocolGuid, &FtwHandle); - ASSERT_EFI_ERROR (Status); return EFI_SUCCESS; } diff --git a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmDxe.c b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmDxe.c index 24b157df08..772d10dcd4 100644 --- a/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmDxe.c +++ b/MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmDxe.c @@ -3,7 +3,7 @@ Implement the Fault Tolerant Write (FTW) protocol based on SMM FTW module. -Copyright (c) 2011, Intel Corporation. All rights reserved.
+Copyright (c) 2011 - 2013, 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 which accompanies this distribution. The full text of the license may be found at @@ -463,13 +463,17 @@ FtwGetLastWrite ( // Get data from SMM // *PrivateDataSize = SmmFtwGetLastWriteHeader->PrivateDataSize; - if (!EFI_ERROR (Status)) { + if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) { *Lba = SmmFtwGetLastWriteHeader->Lba; *Offset = SmmFtwGetLastWriteHeader->Offset; *Length = SmmFtwGetLastWriteHeader->Length; *Complete = SmmFtwGetLastWriteHeader->Complete; CopyGuid (CallerId, &SmmFtwGetLastWriteHeader->CallerId); - CopyMem (PrivateData, SmmFtwGetLastWriteHeader->Data, *PrivateDataSize); + if (Status == EFI_SUCCESS) { + CopyMem (PrivateData, SmmFtwGetLastWriteHeader->Data, *PrivateDataSize); + } + } else if (Status == EFI_NOT_FOUND) { + *Complete = SmmFtwGetLastWriteHeader->Complete; } FreePool (SmmCommunicateHeader); diff --git a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c index 7a0d6d29a5..4cb88106ee 100644 --- a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c +++ b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c @@ -111,6 +111,7 @@ SmmLockBoxSave ( ) { EFI_STATUS Status; + EFI_SMM_LOCK_BOX_PARAMETER_SAVE TempLockBoxParameterSave; // // Sanity check @@ -121,10 +122,12 @@ SmmLockBoxSave ( return ; } + CopyMem (&TempLockBoxParameterSave, LockBoxParameterSave, sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SAVE)); + // // Sanity check // - if (!IsAddressValid ((UINTN)LockBoxParameterSave->Buffer, (UINTN)LockBoxParameterSave->Length)) { + if (!IsAddressValid ((UINTN)TempLockBoxParameterSave.Buffer, (UINTN)TempLockBoxParameterSave.Length)) { DEBUG ((EFI_D_ERROR, "SmmLockBox Save address in SMRAM or buffer overflow!\n")); LockBoxParameterSave->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED; return ; @@ -134,9 +137,9 @@ SmmLockBoxSave ( // Save data // Status = SaveLockBox ( - &LockBoxParameterSave->Guid, - (VOID *)(UINTN)LockBoxParameterSave->Buffer, - (UINTN)LockBoxParameterSave->Length + &TempLockBoxParameterSave.Guid, + (VOID *)(UINTN)TempLockBoxParameterSave.Buffer, + (UINTN)TempLockBoxParameterSave.Length ); LockBoxParameterSave->Header.ReturnStatus = (UINT64)Status; return ; @@ -153,6 +156,7 @@ SmmLockBoxSetAttributes ( ) { EFI_STATUS Status; + EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES TempLockBoxParameterSetAttributes; // // Sanity check @@ -163,12 +167,14 @@ SmmLockBoxSetAttributes ( return ; } + CopyMem (&TempLockBoxParameterSetAttributes, LockBoxParameterSetAttributes, sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES)); + // // Update data // Status = SetLockBoxAttributes ( - &LockBoxParameterSetAttributes->Guid, - LockBoxParameterSetAttributes->Attributes + &TempLockBoxParameterSetAttributes.Guid, + TempLockBoxParameterSetAttributes.Attributes ); LockBoxParameterSetAttributes->Header.ReturnStatus = (UINT64)Status; return ; @@ -189,6 +195,7 @@ SmmLockBoxUpdate ( ) { EFI_STATUS Status; + EFI_SMM_LOCK_BOX_PARAMETER_UPDATE TempLockBoxParameterUpdate; // // Sanity check @@ -199,10 +206,12 @@ SmmLockBoxUpdate ( return ; } + CopyMem (&TempLockBoxParameterUpdate, LockBoxParameterUpdate, sizeof (EFI_SMM_LOCK_BOX_PARAMETER_UPDATE)); + // // Sanity check // - if (!IsAddressValid ((UINTN)LockBoxParameterUpdate->Buffer, (UINTN)LockBoxParameterUpdate->Length)) { + if (!IsAddressValid ((UINTN)TempLockBoxParameterUpdate.Buffer, (UINTN)TempLockBoxParameterUpdate.Length)) { DEBUG ((EFI_D_ERROR, "SmmLockBox Update address in SMRAM or buffer overflow!\n")); LockBoxParameterUpdate->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED; return ; @@ -212,10 +221,10 @@ SmmLockBoxUpdate ( // Update data // Status = UpdateLockBox ( - &LockBoxParameterUpdate->Guid, - (UINTN)LockBoxParameterUpdate->Offset, - (VOID *)(UINTN)LockBoxParameterUpdate->Buffer, - (UINTN)LockBoxParameterUpdate->Length + &TempLockBoxParameterUpdate.Guid, + (UINTN)TempLockBoxParameterUpdate.Offset, + (VOID *)(UINTN)TempLockBoxParameterUpdate.Buffer, + (UINTN)TempLockBoxParameterUpdate.Length ); LockBoxParameterUpdate->Header.ReturnStatus = (UINT64)Status; return ; @@ -236,11 +245,14 @@ SmmLockBoxRestore ( ) { EFI_STATUS Status; + EFI_SMM_LOCK_BOX_PARAMETER_RESTORE TempLockBoxParameterRestore; + + CopyMem (&TempLockBoxParameterRestore, LockBoxParameterRestore, sizeof (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE)); // // Sanity check // - if (!IsAddressValid ((UINTN)LockBoxParameterRestore->Buffer, (UINTN)LockBoxParameterRestore->Length)) { + if (!IsAddressValid ((UINTN)TempLockBoxParameterRestore.Buffer, (UINTN)TempLockBoxParameterRestore.Length)) { DEBUG ((EFI_D_ERROR, "SmmLockBox Restore address in SMRAM or buffer overflow!\n")); LockBoxParameterRestore->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED; return ; @@ -249,17 +261,17 @@ SmmLockBoxRestore ( // // Restore data // - if ((LockBoxParameterRestore->Length == 0) && (LockBoxParameterRestore->Buffer == 0)) { + if ((TempLockBoxParameterRestore.Length == 0) && (TempLockBoxParameterRestore.Buffer == 0)) { Status = RestoreLockBox ( - &LockBoxParameterRestore->Guid, + &TempLockBoxParameterRestore.Guid, NULL, NULL ); } else { Status = RestoreLockBox ( - &LockBoxParameterRestore->Guid, - (VOID *)(UINTN)LockBoxParameterRestore->Buffer, - (UINTN *)&LockBoxParameterRestore->Length + &TempLockBoxParameterRestore.Guid, + (VOID *)(UINTN)TempLockBoxParameterRestore.Buffer, + (UINTN *)&TempLockBoxParameterRestore.Length ); } LockBoxParameterRestore->Header.ReturnStatus = (UINT64)Status; diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c index f8e6bd5882..111a6cd411 100644 --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c @@ -15,7 +15,7 @@ VariableServiceSetVariable(), VariableServiceQueryVariableInfo(), ReclaimForOS(), SmmVariableGetStatistics() should also do validation based on its own knowledge. -Copyright (c) 2010 - 2012, Intel Corporation. All rights reserved.
+Copyright (c) 2010 - 2013, 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 which accompanies this distribution. The full text of the license may be found at @@ -44,7 +44,9 @@ EFI_HANDLE mSmmVariableHandle = N EFI_HANDLE mVariableHandle = NULL; BOOLEAN mAtRuntime = FALSE; EFI_GUID mZeroGuid = {0, 0, 0, {0, 0, 0, 0, 0, 0, 0, 0}}; - +UINT8 *mVariableBufferPayload = NULL; +UINTN mVariableBufferPayloadSize; + EFI_SMM_VARIABLE_PROTOCOL gSmmVariable = { VariableServiceGetVariable, VariableServiceGetNextVariableName, @@ -302,6 +304,8 @@ GetFvbCountAndBuffer ( *NumberHandles = BufferSize / sizeof(EFI_HANDLE); if (EFI_ERROR(Status)) { *NumberHandles = 0; + FreePool (*Buffer); + *Buffer = NULL; } return Status; @@ -337,7 +341,8 @@ SmmVariableGetStatistics ( UINTN NameLength; UINTN StatisticsInfoSize; CHAR16 *InfoName; - + EFI_GUID VendorGuid; + ASSERT (InfoEntry != NULL); VariableInfo = gVariableInfo; if (VariableInfo == NULL) { @@ -351,7 +356,9 @@ SmmVariableGetStatistics ( } InfoName = (CHAR16 *)(InfoEntry + 1); - if (CompareGuid (&InfoEntry->VendorGuid, &mZeroGuid)) { + CopyGuid (&VendorGuid, &InfoEntry->VendorGuid); + + if (CompareGuid (&VendorGuid, &mZeroGuid)) { // // Return the first variable info // @@ -365,7 +372,7 @@ SmmVariableGetStatistics ( // Get the next variable info // while (VariableInfo != NULL) { - if (CompareGuid (&VariableInfo->VendorGuid, &InfoEntry->VendorGuid)) { + if (CompareGuid (&VariableInfo->VendorGuid, &VendorGuid)) { NameLength = StrSize (VariableInfo->Name); if (NameLength == StrSize (InfoName)) { if (CompareMem (VariableInfo->Name, InfoName, NameLength) == 0) { @@ -445,6 +452,7 @@ SmmVariableHandler ( VARIABLE_INFO_ENTRY *VariableInfo; UINTN InfoSize; UINTN NameBufferSize; + UINTN CommBufferPayloadSize; // // If input is invalid, stop processing this SMI @@ -454,18 +462,32 @@ SmmVariableHandler ( } if (*CommBufferSize < SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { + DEBUG ((EFI_D_ERROR, "SmmVariableHandler: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } + CommBufferPayloadSize = *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE; + if (CommBufferPayloadSize > mVariableBufferPayloadSize) { + DEBUG ((EFI_D_ERROR, "SmmVariableHandler: SMM communication buffer payload size invalid!\n")); return EFI_SUCCESS; } if (!InternalIsAddressValid ((UINTN)CommBuffer, *CommBufferSize)) { - DEBUG ((EFI_D_ERROR, "SMM communication buffer in SMRAM or overflow!\n")); + DEBUG ((EFI_D_ERROR, "SmmVariableHandler: SMM communication buffer in SMRAM or overflow!\n")); return EFI_SUCCESS; } SmmVariableFunctionHeader = (SMM_VARIABLE_COMMUNICATE_HEADER *)CommBuffer; switch (SmmVariableFunctionHeader->Function) { case SMM_VARIABLE_FUNCTION_GET_VARIABLE: - SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) SmmVariableFunctionHeader->Data; + if (CommBufferPayloadSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { + DEBUG ((EFI_D_ERROR, "GetVariable: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } + // + // Copy the input communicate buffer payload to pre-allocated SMM variable buffer payload. + // + CopyMem (mVariableBufferPayload, SmmVariableFunctionHeader->Data, CommBufferPayloadSize); + SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) mVariableBufferPayload; 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)) { // @@ -480,8 +502,8 @@ SmmVariableHandler ( // // SMRAM range check already covered before // - if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { - DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); + if (InfoSize > CommBufferPayloadSize) { + DEBUG ((EFI_D_ERROR, "GetVariable: Data size exceed communication buffer size limit!\n")); Status = EFI_ACCESS_DENIED; goto EXIT; } @@ -501,10 +523,19 @@ SmmVariableHandler ( &SmmVariableHeader->DataSize, (UINT8 *)SmmVariableHeader->Name + SmmVariableHeader->NameSize ); + CopyMem (SmmVariableFunctionHeader->Data, mVariableBufferPayload, CommBufferPayloadSize); break; case SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME: - GetNextVariableName = (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) SmmVariableFunctionHeader->Data; + if (CommBufferPayloadSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { + DEBUG ((EFI_D_ERROR, "GetNextVariableName: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } + // + // Copy the input communicate buffer payload to pre-allocated SMM variable buffer payload. + // + CopyMem (mVariableBufferPayload, SmmVariableFunctionHeader->Data, CommBufferPayloadSize); + GetNextVariableName = (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) mVariableBufferPayload; if ((UINTN)(~0) - GetNextVariableName->NameSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { // // Prevent InfoSize overflow happen @@ -517,13 +548,13 @@ SmmVariableHandler ( // // SMRAM range check already covered before // - if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { - DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); + if (InfoSize > CommBufferPayloadSize) { + DEBUG ((EFI_D_ERROR, "GetNextVariableName: Data size exceed communication buffer size limit!\n")); Status = EFI_ACCESS_DENIED; goto EXIT; } - NameBufferSize = *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE - OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name); + NameBufferSize = CommBufferPayloadSize - 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. @@ -537,10 +568,19 @@ SmmVariableHandler ( GetNextVariableName->Name, &GetNextVariableName->Guid ); + CopyMem (SmmVariableFunctionHeader->Data, mVariableBufferPayload, CommBufferPayloadSize); break; case SMM_VARIABLE_FUNCTION_SET_VARIABLE: - SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) SmmVariableFunctionHeader->Data; + if (CommBufferPayloadSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { + DEBUG ((EFI_D_ERROR, "SetVariable: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } + // + // Copy the input communicate buffer payload to pre-allocated SMM variable buffer payload. + // + CopyMem (mVariableBufferPayload, SmmVariableFunctionHeader->Data, CommBufferPayloadSize); + SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) mVariableBufferPayload; 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)) { // @@ -556,8 +596,8 @@ SmmVariableHandler ( // SMRAM range check already covered before // Data buffer should not contain SMM range // - if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { - DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); + if (InfoSize > CommBufferPayloadSize) { + DEBUG ((EFI_D_ERROR, "SetVariable: Data size exceed communication buffer size limit!\n")); Status = EFI_ACCESS_DENIED; goto EXIT; } @@ -580,17 +620,11 @@ SmmVariableHandler ( break; case SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO: - QueryVariableInfo = (SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO *) SmmVariableFunctionHeader->Data; - InfoSize = sizeof(SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO); - - // - // SMRAM range check already covered before - // - if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { - DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); - Status = EFI_ACCESS_DENIED; - goto EXIT; + if (CommBufferPayloadSize < sizeof (SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO)) { + DEBUG ((EFI_D_ERROR, "QueryVariableInfo: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; } + QueryVariableInfo = (SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO *) SmmVariableFunctionHeader->Data; Status = VariableServiceQueryVariableInfo ( QueryVariableInfo->Attributes, @@ -624,7 +658,7 @@ SmmVariableHandler ( // if (InternalIsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBufferSize, sizeof(UINTN))) { - DEBUG ((EFI_D_ERROR, "SMM communication buffer in SMRAM!\n")); + DEBUG ((EFI_D_ERROR, "GetStatistics: SMM communication buffer in SMRAM!\n")); Status = EFI_ACCESS_DENIED; goto EXIT; } @@ -781,6 +815,16 @@ VariableServiceInitialize ( mSmramRangeCount = Size / sizeof (EFI_SMRAM_DESCRIPTOR); + mVariableBufferPayloadSize = MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize)) + + OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - sizeof (VARIABLE_HEADER); + + Status = gSmst->SmmAllocatePool ( + EfiRuntimeServicesData, + mVariableBufferPayloadSize, + (VOID **)&mVariableBufferPayload + ); + ASSERT_EFI_ERROR (Status); + /// /// Register SMM variable SMI handler /// diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c index e76600ee6a..eb67bae748 100644 --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c @@ -42,6 +42,7 @@ EFI_SMM_COMMUNICATION_PROTOCOL *mSmmCommunication = NULL; UINT8 *mVariableBuffer = NULL; UINT8 *mVariableBufferPhysical = NULL; UINTN mVariableBufferSize; +UINTN mVariableBufferPayloadSize; EFI_LOCK mVariableServicesLock; /** @@ -189,7 +190,6 @@ RuntimeServiceGetVariable ( EFI_STATUS Status; UINTN PayloadSize; SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *SmmVariableHeader; - UINTN SmmCommBufPayloadSize; UINTN TempDataSize; UINTN VariableNameSize; @@ -201,17 +201,13 @@ RuntimeServiceGetVariable ( return EFI_INVALID_PARAMETER; } - // - // SMM Communication Buffer max payload size - // - 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 (VariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { + if (VariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { return EFI_INVALID_PARAMETER; } @@ -221,11 +217,11 @@ 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) - VariableNameSize) { + if (TempDataSize > mVariableBufferPayloadSize - 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) - VariableNameSize; + TempDataSize = mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - VariableNameSize; } PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + VariableNameSize + TempDataSize; @@ -300,7 +296,6 @@ RuntimeServiceGetNextVariableName ( EFI_STATUS Status; UINTN PayloadSize; SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *SmmGetNextVariableName; - UINTN SmmCommBufPayloadSize; UINTN OutVariableNameSize; UINTN InVariableNameSize; @@ -308,17 +303,13 @@ RuntimeServiceGetNextVariableName ( return EFI_INVALID_PARAMETER; } - // - // 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 (InVariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { + if (InVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { return EFI_INVALID_PARAMETER; } @@ -328,11 +319,11 @@ RuntimeServiceGetNextVariableName ( // Init the communicate buffer. The buffer data size is: // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize. // - if (OutVariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { + if (OutVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { // // If output buffer exceed SMM payload limit. Trim output buffer to SMM payload size // - OutVariableNameSize = SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name); + OutVariableNameSize = mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name); } // // Payload should be Guid + NameSize + MAX of Input & Output buffer @@ -430,21 +421,13 @@ RuntimeServiceSetVariable ( return EFI_INVALID_PARAMETER; } - if (DataSize >= mVariableBufferSize) { - // - // DataSize may be near MAX_ADDRESS incorrectly, this can cause the computed PayLoadSize to - // overflow to a small value and pass the check in InitCommunicateBuffer(). - // To protect against this vulnerability, return EFI_INVALID_PARAMETER if DataSize is >= mVariableBufferSize. - // And there will be further check to ensure the total size is also not > mVariableBufferSize. - // - return EFI_INVALID_PARAMETER; - } VariableNameSize = StrSize (VariableName); - if ((UINTN)(~0) - VariableNameSize < OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + DataSize) { - // - // Prevent PayloadSize overflow - // + // + // If VariableName or DataSize exceeds SMM payload limit. Return failure + // + if ((VariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) || + (DataSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - VariableNameSize)){ return EFI_INVALID_PARAMETER; } @@ -654,10 +637,11 @@ SmmVariableReady ( ASSERT_EFI_ERROR (Status); // - // Allocate memory for variable store. + // Allocate memory for variable communicate buffer. // - mVariableBufferSize = SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE; - mVariableBufferSize += MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize)); + mVariableBufferPayloadSize = MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize)) + + OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - sizeof (VARIABLE_HEADER); + mVariableBufferSize = SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + mVariableBufferPayloadSize; mVariableBuffer = AllocateRuntimePool (mVariableBufferSize); ASSERT (mVariableBuffer != NULL); diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c index 47bc390f6f..754ab9a794 100644 --- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c +++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmm.c @@ -14,7 +14,7 @@ VariableServiceSetVariable(), VariableServiceQueryVariableInfo(), ReclaimForOS(), SmmVariableGetStatistics() should also do validation based on its own knowledge. -Copyright (c) 2010 - 2012, Intel Corporation. All rights reserved.
+Copyright (c) 2010 - 2013, 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 which accompanies this distribution. The full text of the license may be found at @@ -44,7 +44,9 @@ EFI_HANDLE mSmmVariableHandle = N EFI_HANDLE mVariableHandle = NULL; BOOLEAN mAtRuntime = FALSE; EFI_GUID mZeroGuid = {0, 0, 0, {0, 0, 0, 0, 0, 0, 0, 0}}; - +UINT8 *mVariableBufferPayload = NULL; +UINTN mVariableBufferPayloadSize; + EFI_SMM_VARIABLE_PROTOCOL gSmmVariable = { VariableServiceGetVariable, VariableServiceGetNextVariableName, @@ -302,6 +304,8 @@ GetFvbCountAndBuffer ( *NumberHandles = BufferSize / sizeof(EFI_HANDLE); if (EFI_ERROR(Status)) { *NumberHandles = 0; + FreePool (*Buffer); + *Buffer = NULL; } return Status; @@ -338,6 +342,7 @@ SmmVariableGetStatistics ( UINTN NameLength; UINTN StatisticsInfoSize; CHAR16 *InfoName; + EFI_GUID VendorGuid; if (InfoEntry == NULL) { return EFI_INVALID_PARAMETER; @@ -355,7 +360,9 @@ SmmVariableGetStatistics ( } InfoName = (CHAR16 *)(InfoEntry + 1); - if (CompareGuid (&InfoEntry->VendorGuid, &mZeroGuid)) { + CopyGuid (&VendorGuid, &InfoEntry->VendorGuid); + + if (CompareGuid (&VendorGuid, &mZeroGuid)) { // // Return the first variable info // @@ -369,7 +376,7 @@ SmmVariableGetStatistics ( // Get the next variable info // while (VariableInfo != NULL) { - if (CompareGuid (&VariableInfo->VendorGuid, &InfoEntry->VendorGuid)) { + if (CompareGuid (&VariableInfo->VendorGuid, &VendorGuid)) { NameLength = StrSize (VariableInfo->Name); if (NameLength == StrSize (InfoName)) { if (CompareMem (VariableInfo->Name, InfoName, NameLength) == 0) { @@ -450,6 +457,7 @@ SmmVariableHandler ( VARIABLE_INFO_ENTRY *VariableInfo; UINTN InfoSize; UINTN NameBufferSize; + UINTN CommBufferPayloadSize; // // If input is invalid, stop processing this SMI @@ -459,11 +467,17 @@ SmmVariableHandler ( } if (*CommBufferSize < SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { + DEBUG ((EFI_D_ERROR, "SmmVariableHandler: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } + CommBufferPayloadSize = *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE; + if (CommBufferPayloadSize > mVariableBufferPayloadSize) { + DEBUG ((EFI_D_ERROR, "SmmVariableHandler: SMM communication buffer payload size invalid!\n")); return EFI_SUCCESS; } if (!InternalIsAddressValid ((UINTN)CommBuffer, *CommBufferSize)) { - DEBUG ((EFI_D_ERROR, "SMM communication buffer in SMRAM or overflow!\n")); + DEBUG ((EFI_D_ERROR, "SmmVariableHandler: SMM communication buffer in SMRAM or overflow!\n")); return EFI_SUCCESS; } @@ -471,7 +485,15 @@ SmmVariableHandler ( switch (SmmVariableFunctionHeader->Function) { case SMM_VARIABLE_FUNCTION_GET_VARIABLE: - SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) SmmVariableFunctionHeader->Data; + if (CommBufferPayloadSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { + DEBUG ((EFI_D_ERROR, "GetVariable: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } + // + // Copy the input communicate buffer payload to pre-allocated SMM variable buffer payload. + // + CopyMem (mVariableBufferPayload, SmmVariableFunctionHeader->Data, CommBufferPayloadSize); + SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) mVariableBufferPayload; 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)) { // @@ -486,8 +508,8 @@ SmmVariableHandler ( // // SMRAM range check already covered before // - if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { - DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); + if (InfoSize > CommBufferPayloadSize) { + DEBUG ((EFI_D_ERROR, "GetVariable: Data size exceed communication buffer size limit!\n")); Status = EFI_ACCESS_DENIED; goto EXIT; } @@ -507,10 +529,19 @@ SmmVariableHandler ( &SmmVariableHeader->DataSize, (UINT8 *)SmmVariableHeader->Name + SmmVariableHeader->NameSize ); + CopyMem (SmmVariableFunctionHeader->Data, mVariableBufferPayload, CommBufferPayloadSize); break; case SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME: - GetNextVariableName = (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) SmmVariableFunctionHeader->Data; + if (CommBufferPayloadSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { + DEBUG ((EFI_D_ERROR, "GetNextVariableName: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } + // + // Copy the input communicate buffer payload to pre-allocated SMM variable buffer payload. + // + CopyMem (mVariableBufferPayload, SmmVariableFunctionHeader->Data, CommBufferPayloadSize); + GetNextVariableName = (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) mVariableBufferPayload; if ((UINTN)(~0) - GetNextVariableName->NameSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { // // Prevent InfoSize overflow happen @@ -523,13 +554,13 @@ SmmVariableHandler ( // // SMRAM range check already covered before // - if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { - DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); + if (InfoSize > CommBufferPayloadSize) { + DEBUG ((EFI_D_ERROR, "GetNextVariableName: Data size exceed communication buffer size limit!\n")); Status = EFI_ACCESS_DENIED; goto EXIT; } - NameBufferSize = *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE - OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name); + NameBufferSize = CommBufferPayloadSize - 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. @@ -543,10 +574,19 @@ SmmVariableHandler ( GetNextVariableName->Name, &GetNextVariableName->Guid ); + CopyMem (SmmVariableFunctionHeader->Data, mVariableBufferPayload, CommBufferPayloadSize); break; case SMM_VARIABLE_FUNCTION_SET_VARIABLE: - SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) SmmVariableFunctionHeader->Data; + if (CommBufferPayloadSize < OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { + DEBUG ((EFI_D_ERROR, "SetVariable: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; + } + // + // Copy the input communicate buffer payload to pre-allocated SMM variable buffer payload. + // + CopyMem (mVariableBufferPayload, SmmVariableFunctionHeader->Data, CommBufferPayloadSize); + SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) mVariableBufferPayload; 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)) { // @@ -562,8 +602,8 @@ SmmVariableHandler ( // SMRAM range check already covered before // Data buffer should not contain SMM range // - if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { - DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); + if (InfoSize > CommBufferPayloadSize) { + DEBUG ((EFI_D_ERROR, "SetVariable: Data size exceed communication buffer size limit!\n")); Status = EFI_ACCESS_DENIED; goto EXIT; } @@ -586,17 +626,11 @@ SmmVariableHandler ( break; case SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO: - QueryVariableInfo = (SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO *) SmmVariableFunctionHeader->Data; - InfoSize = sizeof(SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO); - - // - // SMRAM range check already covered before - // - if (InfoSize > *CommBufferSize - SMM_VARIABLE_COMMUNICATE_HEADER_SIZE) { - DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); - Status = EFI_ACCESS_DENIED; - goto EXIT; + if (CommBufferPayloadSize < sizeof (SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO)) { + DEBUG ((EFI_D_ERROR, "QueryVariableInfo: SMM communication buffer size invalid!\n")); + return EFI_SUCCESS; } + QueryVariableInfo = (SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO *) SmmVariableFunctionHeader->Data; Status = VariableServiceQueryVariableInfo ( QueryVariableInfo->Attributes, @@ -630,7 +664,7 @@ SmmVariableHandler ( // if (InternalIsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBufferSize, sizeof(UINTN))) { - DEBUG ((EFI_D_ERROR, "SMM communication buffer in SMRAM!\n")); + DEBUG ((EFI_D_ERROR, "GetStatistics: SMM communication buffer in SMRAM!\n")); Status = EFI_ACCESS_DENIED; goto EXIT; } @@ -786,6 +820,16 @@ VariableServiceInitialize ( mSmramRangeCount = Size / sizeof (EFI_SMRAM_DESCRIPTOR); + mVariableBufferPayloadSize = MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize)) + + OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - sizeof (VARIABLE_HEADER); + + Status = gSmst->SmmAllocatePool ( + EfiRuntimeServicesData, + mVariableBufferPayloadSize, + (VOID **)&mVariableBufferPayload + ); + ASSERT_EFI_ERROR (Status); + /// /// Register SMM variable SMI handler /// diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c index cdd407d66b..b7c7f4ffb7 100644 --- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c +++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/VariableSmmRuntimeDxe.c @@ -52,6 +52,7 @@ EFI_SMM_COMMUNICATION_PROTOCOL *mSmmCommunication = NULL; UINT8 *mVariableBuffer = NULL; UINT8 *mVariableBufferPhysical = NULL; UINTN mVariableBufferSize; +UINTN mVariableBufferPayloadSize; EFI_LOCK mVariableServicesLock; /** @@ -205,7 +206,6 @@ RuntimeServiceGetVariable ( EFI_STATUS Status; UINTN PayloadSize; SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *SmmVariableHeader; - UINTN SmmCommBufPayloadSize; UINTN TempDataSize; UINTN VariableNameSize; @@ -217,17 +217,13 @@ RuntimeServiceGetVariable ( return EFI_INVALID_PARAMETER; } - // - // SMM Communication Buffer max payload size - // - 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 (VariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { + if (VariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) { return EFI_INVALID_PARAMETER; } @@ -237,11 +233,11 @@ 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) - VariableNameSize) { + if (TempDataSize > mVariableBufferPayloadSize - 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) - VariableNameSize; + TempDataSize = mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - VariableNameSize; } PayloadSize = OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + VariableNameSize + TempDataSize; @@ -316,7 +312,6 @@ RuntimeServiceGetNextVariableName ( EFI_STATUS Status; UINTN PayloadSize; SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *SmmGetNextVariableName; - UINTN SmmCommBufPayloadSize; UINTN OutVariableNameSize; UINTN InVariableNameSize; @@ -324,17 +319,13 @@ RuntimeServiceGetNextVariableName ( return EFI_INVALID_PARAMETER; } - // - // 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 (InVariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { + if (InVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { return EFI_INVALID_PARAMETER; } @@ -344,11 +335,11 @@ RuntimeServiceGetNextVariableName ( // Init the communicate buffer. The buffer data size is: // SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize. // - if (OutVariableNameSize > SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { + if (OutVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)) { // // If output buffer exceed SMM payload limit. Trim output buffer to SMM payload size // - OutVariableNameSize = SmmCommBufPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name); + OutVariableNameSize = mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name); } // // Payload should be Guid + NameSize + MAX of Input & Output buffer @@ -448,21 +439,13 @@ RuntimeServiceSetVariable ( return EFI_INVALID_PARAMETER; } - if (DataSize >= mVariableBufferSize) { - // - // DataSize may be near MAX_ADDRESS incorrectly, this can cause the computed PayLoadSize to - // overflow to a small value and pass the check in InitCommunicateBuffer(). - // To protect against this vulnerability, return EFI_INVALID_PARAMETER if DataSize is >= mVariableBufferSize. - // And there will be further check to ensure the total size is also not > mVariableBufferSize. - // - return EFI_INVALID_PARAMETER; - } VariableNameSize = StrSize (VariableName); - if ((UINTN)(~0) - VariableNameSize < OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + DataSize) { - // - // Prevent PayloadSize overflow - // + // + // If VariableName or DataSize exceeds SMM payload limit. Return failure + // + if ((VariableNameSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name)) || + (DataSize > mVariableBufferPayloadSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - VariableNameSize)){ return EFI_INVALID_PARAMETER; } @@ -672,10 +655,11 @@ SmmVariableReady ( ASSERT_EFI_ERROR (Status); // - // Allocate memory for variable store. + // Allocate memory for variable communicate buffer. // - mVariableBufferSize = SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE; - mVariableBufferSize += MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize)); + mVariableBufferPayloadSize = MAX (PcdGet32 (PcdMaxVariableSize), PcdGet32 (PcdMaxHardwareErrorVariableSize)) + + OFFSET_OF (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) - sizeof (VARIABLE_HEADER); + mVariableBufferSize = SMM_COMMUNICATE_HEADER_SIZE + SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + mVariableBufferPayloadSize; mVariableBuffer = AllocateRuntimePool (mVariableBufferSize); ASSERT (mVariableBuffer != NULL); -- 2.39.2