From 32177f69c40f81a8cdb2033c422f3c76f57945e5 Mon Sep 17 00:00:00 2001 From: jyao1 Date: Mon, 23 Jul 2012 00:59:26 +0000 Subject: Add more security check for CommBuffer+CommBufferSize. signed off by: jiewen.yao@intel.com reviewed by: rui.sun@intel.com reviewed by: michael.d.kinney@intel.com git-svn-id: https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2@13545 6f19259b-4bc3-4df7-8a09-765794883524 --- .../Universal/LockBox/SmmLockBox/SmmLockBox.c | 105 ++++++++++++++++++++- .../Universal/LockBox/SmmLockBox/SmmLockBox.inf | 7 +- 2 files changed, 109 insertions(+), 3 deletions(-) diff --git a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c index 35862c6..b9c8190 100644 --- a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c +++ b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.c @@ -1,6 +1,14 @@ /** @file -Copyright (c) 2010 - 2011, Intel Corporation. All rights reserved.
+Copyright (c) 2010 - 2012, Intel Corporation. All rights reserved.
+ + 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. + + SmmLockBoxHandler(), SmmLockBoxRestore(), SmmLockBoxUpdate(), SmmLockBoxSave() + will receive untrusted input and do basic validation. This program and the accompanying materials are licensed and made available under the terms and conditions @@ -61,8 +69,39 @@ IsAddressInSmram ( } /** + This function check if the address refered by Buffer and Length is valid. + + @param Buffer the buffer address to be checked. + @param Length the buffer length to be checked. + + @retval TRUE this address is valid. + @retval FALSE this address is NOT valid. +**/ +BOOLEAN +IsAddressValid ( + IN UINTN Buffer, + IN UINTN Length + ) +{ + if (Buffer > (MAX_ADDRESS - Length)) { + // + // Overflow happen + // + return FALSE; + } + if (IsAddressInSmram ((EFI_PHYSICAL_ADDRESS)Buffer, (UINT64)Length)) { + return FALSE; + } + return TRUE; +} + +/** Dispatch function for SMM lock box save. + Caution: This function may receive untrusted input. + Restore buffer and length are external input, so this function will validate + it is in SMRAM. + @param LockBoxParameterSave parameter of lock box save **/ VOID @@ -82,6 +121,15 @@ SmmLockBoxSave ( } // + // Sanity check + // + if (!IsAddressValid ((UINTN)LockBoxParameterSave->Buffer, (UINTN)LockBoxParameterSave->Length)) { + DEBUG ((EFI_D_ERROR, "SmmLockBox Save address in SMRAM!\n")); + LockBoxParameterSave->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED; + return ; + } + + // // Save data // Status = SaveLockBox ( @@ -128,6 +176,10 @@ SmmLockBoxSetAttributes ( /** Dispatch function for SMM lock box update. + Caution: This function may receive untrusted input. + Restore buffer and length are external input, so this function will validate + it is in SMRAM. + @param LockBoxParameterUpdate parameter of lock box update **/ VOID @@ -147,6 +199,15 @@ SmmLockBoxUpdate ( } // + // Sanity check + // + if (!IsAddressValid ((UINTN)LockBoxParameterUpdate->Buffer, (UINTN)LockBoxParameterUpdate->Length)) { + DEBUG ((EFI_D_ERROR, "SmmLockBox Update address in SMRAM!\n")); + LockBoxParameterUpdate->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED; + return ; + } + + // // Update data // Status = UpdateLockBox ( @@ -162,6 +223,10 @@ SmmLockBoxUpdate ( /** Dispatch function for SMM lock box restore. + Caution: This function may receive untrusted input. + Restore buffer and length are external input, so this function will validate + it is in SMRAM. + @param LockBoxParameterRestore parameter of lock box restore **/ VOID @@ -174,7 +239,7 @@ SmmLockBoxRestore ( // // Sanity check // - if (IsAddressInSmram (LockBoxParameterRestore->Buffer, LockBoxParameterRestore->Length)) { + if (!IsAddressValid ((UINTN)LockBoxParameterRestore->Buffer, (UINTN)LockBoxParameterRestore->Length)) { DEBUG ((EFI_D_ERROR, "SmmLockBox Restore address in SMRAM!\n")); LockBoxParameterRestore->Header.ReturnStatus = (UINT64)EFI_ACCESS_DENIED; return ; @@ -220,6 +285,9 @@ SmmLockBoxRestoreAllInPlace ( /** Dispatch function for a Software SMI handler. + Caution: This function may receive untrusted input. + Communicate buffer and buffer size are external input, so this function will do basic validation. + @param DispatchHandle The unique handle assigned to this handler by SmiHandlerRegister(). @param Context Points to an optional handler context which was specified when the handler was registered. @@ -243,6 +311,18 @@ SmmLockBoxHandler ( DEBUG ((EFI_D_ERROR, "SmmLockBox SmmLockBoxHandler Enter\n")); + // + // Sanity check + // + if (*CommBufferSize < sizeof(EFI_SMM_LOCK_BOX_PARAMETER_HEADER)) { + DEBUG ((EFI_D_ERROR, "SmmLockBox Command Buffer Size invalid!\n")); + return EFI_SUCCESS; + } + if (!IsAddressValid ((UINTN)CommBuffer, *CommBufferSize)) { + DEBUG ((EFI_D_ERROR, "SmmLockBox Command Buffer in SMRAM!\n")); + return EFI_SUCCESS; + } + LockBoxParameterHeader = (EFI_SMM_LOCK_BOX_PARAMETER_HEADER *)((UINTN)CommBuffer); LockBoxParameterHeader->ReturnStatus = (UINT64)-1; @@ -253,21 +333,42 @@ SmmLockBoxHandler ( switch (LockBoxParameterHeader->Command) { case EFI_SMM_LOCK_BOX_COMMAND_SAVE: + if (*CommBufferSize < sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)) { + DEBUG ((EFI_D_ERROR, "SmmLockBox Command Buffer Size for SAVE invalid!\n")); + break; + } SmmLockBoxSave ((EFI_SMM_LOCK_BOX_PARAMETER_SAVE *)(UINTN)LockBoxParameterHeader); break; case EFI_SMM_LOCK_BOX_COMMAND_UPDATE: + if (*CommBufferSize < sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE)) { + DEBUG ((EFI_D_ERROR, "SmmLockBox Command Buffer Size for UPDATE invalid!\n")); + break; + } SmmLockBoxUpdate ((EFI_SMM_LOCK_BOX_PARAMETER_UPDATE *)(UINTN)LockBoxParameterHeader); break; case EFI_SMM_LOCK_BOX_COMMAND_RESTORE: + if (*CommBufferSize < sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE)) { + DEBUG ((EFI_D_ERROR, "SmmLockBox Command Buffer Size for RESTORE invalid!\n")); + break; + } SmmLockBoxRestore ((EFI_SMM_LOCK_BOX_PARAMETER_RESTORE *)(UINTN)LockBoxParameterHeader); break; case EFI_SMM_LOCK_BOX_COMMAND_SET_ATTRIBUTES: + if (*CommBufferSize < sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES)) { + DEBUG ((EFI_D_ERROR, "SmmLockBox Command Buffer Size for SET_ATTRIBUTES invalid!\n")); + break; + } SmmLockBoxSetAttributes ((EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES *)(UINTN)LockBoxParameterHeader); break; case EFI_SMM_LOCK_BOX_COMMAND_RESTORE_ALL_IN_PLACE: + if (*CommBufferSize < sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)) { + DEBUG ((EFI_D_ERROR, "SmmLockBox Command Buffer Size for RESTORE_ALL_IN_PLACE invalid!\n")); + break; + } SmmLockBoxRestoreAllInPlace ((EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE *)(UINTN)LockBoxParameterHeader); break; default: + DEBUG ((EFI_D_ERROR, "SmmLockBox Command invalid!\n")); break; } diff --git a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.inf b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.inf index 94ec412..559d39f 100644 --- a/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.inf +++ b/MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.inf @@ -1,7 +1,12 @@ ## @file # Component description file for LockBox SMM driver. # -# Copyright (c) 2010, Intel Corporation. All rights reserved.
+# Copyright (c) 2010 - 2012, Intel Corporation. All rights reserved.
+# +# 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. # # This program and the accompanying materials # are licensed and made available under the terms and conditions -- cgit v1.1