REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3387 Added use of SafeIntLib to validate values are not causing overflows or underflows in user controlled values when calculating buffer sizes. Signed-off-by: Miki Demeter Reviewed-by: Michael D Kinney Cc: Jian J Wang Cc: Liming Gao --- MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 41 ++++++++++++++++++----- MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 1 + MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf | 1 + MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 31 +++++++++++++---- MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 + 5 files changed, 60 insertions(+), 15 deletions(-) diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c index 9e5c6cbe33..875c7c0258 100644 --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c @@ -610,6 +610,7 @@ SmmEndOfS3ResumeHandler ( @param[in] Size2 Size of Buff2 @retval TRUE Buffers overlap in memory. + @retval TRUE Math error. Prevents potential math over and underflows. @retval FALSE Buffer doesn't overlap. **/ @@ -621,11 +622,24 @@ InternalIsBufferOverlapped ( IN UINTN Size2 ) { + UINTN End1; + UINTN End2; + BOOLEAN IsOverUnderflow1; + BOOLEAN IsOverUnderflow2; + + // Check for over or underflow + IsOverUnderflow1 = EFI_ERROR (SafeUintnAdd ((UINTN)Buff1, Size1, &End1)); + IsOverUnderflow2 = EFI_ERROR (SafeUintnAdd ((UINTN)Buff2, Size2, &End2)); + + if (IsOverUnderflow1 || IsOverUnderflow2) { + return TRUE; + } + // // If buff1's end is less than the start of buff2, then it's ok. // Also, if buff1's start is beyond buff2's end, then it's ok. // - if (((Buff1 + Size1) <= Buff2) || (Buff1 >= (Buff2 + Size2))) { + if ((End1 <= (UINTN)Buff2) || ((UINTN)Buff1 >= End2)) { return FALSE; } @@ -651,6 +665,7 @@ SmmEntryPoint ( EFI_SMM_COMMUNICATE_HEADER *CommunicateHeader; BOOLEAN InLegacyBoot; BOOLEAN IsOverlapped; + BOOLEAN IsOverUnderflow; VOID *CommunicationBuffer; UINTN BufferSize; @@ -699,23 +714,31 @@ SmmEntryPoint ( (UINT8 *)gSmmCorePrivate, sizeof (*gSmmCorePrivate) ); - if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) || IsOverlapped) { + // + // Check for over or underflows + // + IsOverUnderflow = EFI_ERROR (SafeUintnSub (BufferSize, OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data), &BufferSize)); + + if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) || + IsOverlapped || IsOverUnderflow) + { // // If CommunicationBuffer is not in valid address scope, // or there is overlap between gSmmCorePrivate and CommunicationBuffer, + // or there is over or underflow, // return EFI_INVALID_PARAMETER // gSmmCorePrivate->CommunicationBuffer = NULL; gSmmCorePrivate->ReturnStatus = EFI_ACCESS_DENIED; } else { CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommunicationBuffer; - BufferSize -= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data); - Status = SmiManage ( - &CommunicateHeader->HeaderGuid, - NULL, - CommunicateHeader->Data, - &BufferSize - ); + // BufferSize was updated by the SafeUintnSub() call above. + Status = SmiManage ( + &CommunicateHeader->HeaderGuid, + NULL, + CommunicateHeader->Data, + &BufferSize + ); // // Update CommunicationBuffer, BufferSize and ReturnStatus // Communicate service finished, reset the pointer to CommBuffer to NULL diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h index 71422b9dfc..b8a490a8c3 100644 --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h @@ -54,6 +54,7 @@ #include #include #include +#include #include "PiSmmCorePrivateData.h" #include "HeapGuard.h" diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf index c8bfae3860..3df44b38f1 100644 --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf @@ -60,6 +60,7 @@ PerformanceLib HobLib SmmMemLib + SafeIntLib [Protocols] gEfiDxeSmmReadyToLockProtocolGuid ## UNDEFINED # SmiHandlerRegister diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c index 4f00cebaf5..fbba868fd0 100644 --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c @@ -34,8 +34,8 @@ #include #include #include - #include "PiSmmCorePrivateData.h" +#include #define SMRAM_CAPABILITIES (EFI_MEMORY_WB | EFI_MEMORY_UC) @@ -1354,6 +1354,7 @@ SmmSplitSmramEntry ( @param[in] ReservedRangeToCompare Pointer to EFI_SMM_RESERVED_SMRAM_REGION to compare. @retval TRUE There is overlap. + @retval TRUE Math error. @retval FALSE There is no overlap. **/ @@ -1363,11 +1364,29 @@ SmmIsSmramOverlap ( IN EFI_SMM_RESERVED_SMRAM_REGION *ReservedRangeToCompare ) { - UINT64 RangeToCompareEnd; - UINT64 ReservedRangeToCompareEnd; - - RangeToCompareEnd = RangeToCompare->CpuStart + RangeToCompare->PhysicalSize; - ReservedRangeToCompareEnd = ReservedRangeToCompare->SmramReservedStart + ReservedRangeToCompare->SmramReservedSize; + UINT64 RangeToCompareEnd; + UINT64 ReservedRangeToCompareEnd; + BOOLEAN IsOverUnderflow1; + BOOLEAN IsOverUnderflow2; + + // Check for over or underflow. + IsOverUnderflow1 = EFI_ERROR ( + SafeUint64Add ( + (UINT64)RangeToCompare->CpuStart, + RangeToCompare->PhysicalSize, + &RangeToCompareEnd + ) + ); + IsOverUnderflow2 = EFI_ERROR ( + SafeUint64Add ( + (UINT64)ReservedRangeToCompare->SmramReservedStart, + ReservedRangeToCompare->SmramReservedSize, + &ReservedRangeToCompareEnd + ) + ); + if (IsOverUnderflow1 || IsOverUnderflow2) { + return TRUE; + } if ((RangeToCompare->CpuStart >= ReservedRangeToCompare->SmramReservedStart) && (RangeToCompare->CpuStart < ReservedRangeToCompareEnd)) diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf index 6109d6b544..ddeb39cee2 100644 --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf @@ -46,6 +46,7 @@ DxeServicesLib PcdLib ReportStatusCodeLib + SafeIntLib [Protocols] gEfiSmmBase2ProtocolGuid ## PRODUCES -- 2.21.0 --