From 2445a70e62e5dd9678bfd29bed15e22343871803 Mon Sep 17 00:00:00 2001 From: czhang46 Date: Fri, 13 Jul 2012 05:15:06 +0000 Subject: [PATCH] Add SMRAM range check to variable SMM SMI handler. Signed-off-by: Chao Zhang Reviewed-by: Fu, Siyuan git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@13530 6f19259b-4bc3-4df7-8a09-765794883524 --- .../Variable/RuntimeDxe/VariableSmm.c | 168 ++++++++++++++++-- .../Variable/RuntimeDxe/VariableSmm.inf | 20 ++- 2 files changed, 166 insertions(+), 22 deletions(-) diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c index 8d949c3965..d4403a2530 100644 --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c @@ -4,11 +4,22 @@ implements an SMI handler to communicate with the DXE runtime driver to provide variable services. -Copyright (c) 2010 - 2011, 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 -http://opensource.org/licenses/bsd-license.php + Caution: This module requires additional review when modified. + This driver will have external input - variable data and communicate buffer in SMM mode. + This external input must be validated carefully to avoid security issue like + buffer overflow, integer overflow. + + SmmVariableHandler() will receive untrusted input and do basic validation. + + Each sub function VariableServiceGetVariable(), VariableServiceGetNextVariableName(), + VariableServiceSetVariable(), VariableServiceQueryVariableInfo(), ReclaimForOS(), + SmmVariableGetStatistics() should also do validation based on its own knowledge. + +Copyright (c) 2010 - 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 +which accompanies this distribution. The full text of the license may be found at +http://opensource.org/licenses/bsd-license.php THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. @@ -17,12 +28,17 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #include #include #include +#include + #include #include #include #include "Variable.h" +EFI_SMRAM_DESCRIPTOR *mSmramRanges; +UINTN mSmramRangeCount; + extern VARIABLE_INFO_ENTRY *gVariableInfo; EFI_HANDLE mSmmVariableHandle = NULL; EFI_HANDLE mVariableHandle = NULL; @@ -50,6 +66,34 @@ AtRuntime ( return mAtRuntime; } +/** + This function check if the address is in SMRAM. + + @param Buffer the buffer address to be checked. + @param Length the buffer length to be checked. + + @retval TRUE this address is in SMRAM. + @retval FALSE this address is NOT in SMRAM. +**/ +BOOLEAN +InternalIsAddressInSmram ( + IN EFI_PHYSICAL_ADDRESS Buffer, + IN UINT64 Length + ) +{ + UINTN Index; + + for (Index = 0; Index < mSmramRangeCount; Index ++) { + if (((Buffer >= mSmramRanges[Index].CpuStart) && (Buffer < mSmramRanges[Index].CpuStart + mSmramRanges[Index].PhysicalSize)) || + ((mSmramRanges[Index].CpuStart >= Buffer) && (mSmramRanges[Index].CpuStart < Buffer + Length))) { + return TRUE; + } + } + + return FALSE; +} + + /** Initializes a basic mutual exclusion lock. @@ -241,12 +285,15 @@ GetFvbCountAndBuffer ( /** Get the variable statistics information from the information buffer pointed by gVariableInfo. - @param[in, out] InfoEntry A pointer to the buffer of variable information entry. - On input, point to the variable information returned last time. if - InfoEntry->VendorGuid is zero, return the first information. - On output, point to the next variable information. - @param[in, out] InfoSize On input, the size of the variable information buffer. - On output, the returned variable information size. + Caution: This function may be invoked at SMM runtime. + InfoEntry and InfoSize are external input. Care must be taken to make sure not security issue at runtime. + + @param[in, out] InfoEntry A pointer to the buffer of variable information entry. + On input, point to the variable information returned last time. if + InfoEntry->VendorGuid is zero, return the first information. + On output, point to the next variable information. + @param[in, out] InfoSize On input, the size of the variable information buffer. + On output, the returned variable information size. @retval EFI_SUCCESS The variable information is found and returned successfully. @retval EFI_UNSUPPORTED No variable inoformation exists in variable driver. The @@ -334,6 +381,12 @@ SmmVariableGetStatistics ( This SMI handler provides services for the variable wrapper driver. + Caution: This function may receive untrusted input. + This variable data and communicate buffer are external input, so this function will do basic validation. + Each sub function VariableServiceGetVariable(), VariableServiceGetNextVariableName(), + VariableServiceSetVariable(), VariableServiceQueryVariableInfo(), ReclaimForOS(), + SmmVariableGetStatistics() should also do validation based on its own knowledge. + @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. @@ -366,12 +419,38 @@ SmmVariableHandler ( VARIABLE_INFO_ENTRY *VariableInfo; UINTN InfoSize; - ASSERT (CommBuffer != NULL); + // + // If input is invalid, stop processing this SMI + // + if (CommBuffer == NULL || CommBufferSize == NULL) { + return EFI_SUCCESS; + } + + if (*CommBufferSize < sizeof(SMM_VARIABLE_COMMUNICATE_HEADER) - 1) { + return EFI_SUCCESS; + } + + if (InternalIsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBuffer, *CommBufferSize)) { + DEBUG ((EFI_D_ERROR, "SMM communication buffer size is in SMRAM!\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; + SmmVariableHeader = (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) SmmVariableFunctionHeader->Data; + InfoSize = OFFSET_OF(SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE, Name) + + SmmVariableHeader->DataSize + SmmVariableHeader->NameSize; + + // + // SMRAM range check already covered before + // + if (InfoSize > *CommBufferSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_HEADER, Data)) { + DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); + Status = EFI_ACCESS_DENIED; + goto EXIT; + } + Status = VariableServiceGetVariable ( SmmVariableHeader->Name, &SmmVariableHeader->Guid, @@ -383,6 +462,17 @@ SmmVariableHandler ( case SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME: GetNextVariableName = (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) SmmVariableFunctionHeader->Data; + InfoSize = OFFSET_OF(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name) + GetNextVariableName->NameSize; + + // + // SMRAM range check already covered before + // + if (InfoSize > *CommBufferSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_HEADER, Data)) { + DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); + Status = EFI_ACCESS_DENIED; + goto EXIT; + } + Status = VariableServiceGetNextVariableName ( &GetNextVariableName->NameSize, GetNextVariableName->Name, @@ -403,6 +493,17 @@ SmmVariableHandler ( 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 - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_HEADER, Data)) { + DEBUG ((EFI_D_ERROR, "Data size exceed communication buffer size limit!\n")); + Status = EFI_ACCESS_DENIED; + goto EXIT; + } + Status = VariableServiceQueryVariableInfo ( QueryVariableInfo->Attributes, &QueryVariableInfo->MaximumVariableStorageSize, @@ -424,15 +525,28 @@ SmmVariableHandler ( case SMM_VARIABLE_FUNCTION_GET_STATISTICS: VariableInfo = (VARIABLE_INFO_ENTRY *) SmmVariableFunctionHeader->Data; InfoSize = *CommBufferSize - OFFSET_OF (SMM_VARIABLE_COMMUNICATE_HEADER, Data); + + // + // Do not need to check SmmVariableFunctionHeader->Data in SMRAM here. + // It is covered by previous CommBuffer check + // + + if (InternalIsAddressInSmram ((EFI_PHYSICAL_ADDRESS)(UINTN)CommBufferSize, sizeof(UINTN))) { + DEBUG ((EFI_D_ERROR, "SMM communication buffer size is in SMRAM!\n")); + Status = EFI_ACCESS_DENIED; + goto EXIT; + } + Status = SmmVariableGetStatistics (VariableInfo, &InfoSize); *CommBufferSize = InfoSize + OFFSET_OF (SMM_VARIABLE_COMMUNICATE_HEADER, Data); break; default: - ASSERT (FALSE); Status = EFI_UNSUPPORTED; } +EXIT: + SmmVariableFunctionHeader->ReturnStatus = Status; return EFI_SUCCESS; @@ -532,7 +646,9 @@ VariableServiceInitialize ( EFI_STATUS Status; EFI_HANDLE VariableHandle; VOID *SmmFtwRegistration; - + EFI_SMM_ACCESS2_PROTOCOL *SmmAccess; + UINTN Size; + // // Variable initialize. // @@ -551,6 +667,28 @@ VariableServiceInitialize ( ); ASSERT_EFI_ERROR (Status); + // + // Get SMRAM information + // + Status = gBS->LocateProtocol (&gEfiSmmAccess2ProtocolGuid, NULL, (VOID **)&SmmAccess); + ASSERT_EFI_ERROR (Status); + + Size = 0; + Status = SmmAccess->GetCapabilities (SmmAccess, &Size, NULL); + ASSERT (Status == EFI_BUFFER_TOO_SMALL); + + Status = gSmst->SmmAllocatePool ( + EfiRuntimeServicesData, + Size, + (VOID **)&mSmramRanges + ); + ASSERT_EFI_ERROR (Status); + + Status = SmmAccess->GetCapabilities (SmmAccess, &Size, mSmramRanges); + ASSERT_EFI_ERROR (Status); + + mSmramRangeCount = Size / sizeof (EFI_SMRAM_DESCRIPTOR); + /// /// Register SMM variable SMI handler /// diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf index da0a0e9ff8..11009c299c 100644 --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf @@ -8,14 +8,19 @@ # This module should be used with SMM Runtime DXE module together. The # SMM Runtime DXE module would install variable arch protocol and variable # write arch protocol based on SMM variable module. -# Copyright (c) 2010 - 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 -# which accompanies this distribution. The full text of the license may be found at -# http://opensource.org/licenses/bsd-license.php -# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, -# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +# Caution: This module requires additional review when modified. +# This driver will have external input - variable data and communicate buffer in SMM mode. +# This external input must be validated carefully to avoid security issue like +# buffer overflow, integer overflow. +# +# Copyright (c) 2010 - 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 +# which accompanies this distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. # # ## @@ -62,6 +67,7 @@ gEfiSmmFirmwareVolumeBlockProtocolGuid ## SOMETIMES_CONSUMES gEfiSmmVariableProtocolGuid ## ALWAYS_PRODUCES gEfiSmmFaultTolerantWriteProtocolGuid ## SOMETIMES_CONSUMES + gEfiSmmAccess2ProtocolGuid ## ALWAYS_CONSUMES [Guids] gEfiVariableGuid ## PRODUCES ## Configuration Table Guid -- 2.39.2