[edk2] [Patch] MdeModulePkg: Fix potential integer overflow issue

Zeng, Star

2015-07-09 10:15:26 UTC

Reviewed-by: Star Zeng <***@intel.com>

-----Original Message-----
From: Ni, Ruiyu
Sent: Thursday, July 9, 2015 5:29 PM
To: edk2-***@lists.sourceforge.net
Cc: Ni, Ruiyu; Zeng, Star
Subject: [Patch] MdeModulePkg: Fix potential integer overflow issue

In certain rare circumstance, the data passed from outside of SMM may be invalid resulting the integer overflow. The issue are found by code review.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <***@intel.com>
Cc: Star Zeng <***@intel.com>
---
.../SmmCorePerformanceLib/SmmCorePerformanceLib.c | 32 ++++++++++++----------
.../Library/SmmLockBoxLib/SmmLockBoxSmmLib.c | 3 +-
2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c
index f28b657..e59cc28 100644
--- a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c
+++ b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c
@@ -482,7 +482,8 @@ SmmPerformanceHandlerEx (
EFI_STATUS Status;
SMM_PERF_COMMUNICATE_EX *SmmPerfCommData;
GAUGE_DATA_ENTRY_EX *GaugeEntryExArray;
- UINTN DataSize;
+ UINT64 DataSize;
+ UINTN Index;
GAUGE_DATA_ENTRY_EX *GaugeDataEx;
UINTN NumberOfEntries;
UINTN LogEntryKey;
@@ -521,7 +522,7 @@ SmmPerformanceHandlerEx (
NumberOfEntries = SmmPerfCommData->NumberOfEntries;
LogEntryKey = SmmPerfCommData->LogEntryKey;
if (GaugeDataEx == NULL || NumberOfEntries == 0 || LogEntryKey > mGaugeData->NumberOfEntries ||
- NumberOfEntries > mGaugeData->NumberOfEntries || (LogEntryKey + NumberOfEntries) > mGaugeData->NumberOfEntries) {
+ NumberOfEntries > mGaugeData->NumberOfEntries || LogEntryKey
+ > (mGaugeData->NumberOfEntries - NumberOfEntries)) {
Status = EFI_INVALID_PARAMETER;
break;
}
@@ -529,19 +530,22 @@ SmmPerformanceHandlerEx (
//
// Sanity check
//
- DataSize = NumberOfEntries * sizeof(GAUGE_DATA_ENTRY_EX);
- if (!SmmIsBufferOutsideSmmValid ((UINTN)GaugeDataEx, DataSize)) {
+ DataSize = MultU64x32 (NumberOfEntries, sizeof(GAUGE_DATA_ENTRY_EX));
+ if (!SmmIsBufferOutsideSmmValid ((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 *) GaugeDataEx,
- (UINT8 *) &GaugeEntryExArray[LogEntryKey],
- DataSize
- );
+
+ for (Index = 0; Index < NumberOfEntries; Index++) {
+ CopyMem (
+ (UINT8 *) &GaugeDataEx[Index],
+ (UINT8 *) &GaugeEntryExArray[LogEntryKey++],
+ sizeof (GAUGE_DATA_ENTRY_EX)
+ );
+ }
Status = EFI_SUCCESS;
break;

@@ -590,7 +594,7 @@ SmmPerformanceHandler (
EFI_STATUS Status;
SMM_PERF_COMMUNICATE *SmmPerfCommData;
GAUGE_DATA_ENTRY_EX *GaugeEntryExArray;
- UINTN DataSize;
+ UINT64 DataSize;
UINTN Index;
GAUGE_DATA_ENTRY *GaugeData;
UINTN NumberOfEntries;
@@ -630,7 +634,7 @@ SmmPerformanceHandler (
NumberOfEntries = SmmPerfCommData->NumberOfEntries;
LogEntryKey = SmmPerfCommData->LogEntryKey;
if (GaugeData == NULL || NumberOfEntries == 0 || LogEntryKey > mGaugeData->NumberOfEntries ||
- NumberOfEntries > mGaugeData->NumberOfEntries || (LogEntryKey + NumberOfEntries) > mGaugeData->NumberOfEntries) {
+ NumberOfEntries > mGaugeData->NumberOfEntries || LogEntryKey
+ > (mGaugeData->NumberOfEntries - NumberOfEntries)) {
Status = EFI_INVALID_PARAMETER;
break;
}
@@ -638,8 +642,8 @@ SmmPerformanceHandler (
//
// Sanity check
//
- DataSize = NumberOfEntries * sizeof(GAUGE_DATA_ENTRY);
- if (!SmmIsBufferOutsideSmmValid ((UINTN)GaugeData, DataSize)) {
+ DataSize = MultU64x32 (NumberOfEntries, sizeof(GAUGE_DATA_ENTRY));
+ if (!SmmIsBufferOutsideSmmValid ((UINTN) GaugeData, DataSize)) {
DEBUG ((EFI_D_ERROR, "SmmPerformanceHandler: SMM Performance Data buffer in SMRAM or overflow!\n"));
Status = EFI_ACCESS_DENIED;
break;
@@ -648,7 +652,7 @@ SmmPerformanceHandler (
GaugeEntryExArray = (GAUGE_DATA_ENTRY_EX *) (mGaugeData + 1);

for (Index = 0; Index < NumberOfEntries; Index++) {
- CopyMem(
+ CopyMem (
(UINT8 *) &GaugeData[Index],
(UINT8 *) &GaugeEntryExArray[LogEntryKey++],
sizeof (GAUGE_DATA_ENTRY)
diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c
index a789daf..23a786d 100644
--- a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c
+++ b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.c
@@ -1,6 +1,6 @@
/** @file

-Copyright (c) 2010 - 2014, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>

This program and the accompanying materials are licensed and made available under the terms and conditions @@ -394,6 +394,7 @@ UpdateLockBox (
DEBUG ((EFI_D_INFO, "SmmLockBoxSmmLib UpdateLockBox - Exit (%r)\n", EFI_BUFFER_TOO_SMALL));
return EFI_BUFFER_TOO_SMALL;
}
+ ASSERT ((UINTN)LockBox->SmramBuffer <= (MAX_ADDRESS - Offset));
CopyMem ((VOID *)((UINTN)LockBox->SmramBuffer + Offset), Buffer, Length);

//
--
1.9.5.msysgit.1