5 files changed, 60 insertions(+), 15 deletions(-)
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 <miki.demeter@intel.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
---
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 <Library/PerformanceLib.h>
#include <Library/HobLib.h>
#include <Library/SmmMemLib.h>
+#include <Library/SafeIntLib.h>
#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 <Library/UefiRuntimeLib.h>
#include <Library/PcdLib.h>
#include <Library/ReportStatusCodeLib.h>
-
#include "PiSmmCorePrivateData.h"
+#include <Library/SafeIntLib.h>
#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
--
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#95779): https://edk2.groups.io/g/devel/message/95779
Mute This Topic: https://groups.io/mt/94697674/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
发件人: Demeter, Miki <miki.demeter@intel.com>
发送时间: 2022年11月1日 6:32
收件人: devel@edk2.groups.io
抄送: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
<gaoliming@byosoft.com.cn>; Wang, Jian J <jian.j.wang@intel.com>
主题: [Patch v4] MdeModulePkg/PiSmmCoreSmmEntryPoint
underflow(CVE-2021-38578)
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 <miki.demeter@intel.com
<mailto:miki.demeter@intel.com> >
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com <mailto:michael.d.
kinney@intel.com> >
Cc: Jian J Wang <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com> >
Cc: Liming Gao <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn> >
---
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 <Library/PerformanceLib.h>
#include <Library/HobLib.h>
#include <Library/SmmMemLib.h>
+#include <Library/SafeIntLib.h>
#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 <Library/UefiRuntimeLib.h>
#include <Library/PcdLib.h>
#include <Library/ReportStatusCodeLib.h>
-
#include "PiSmmCorePrivateData.h"
+#include <Library/SafeIntLib.h>
#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
--
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#95786): https://edk2.groups.io/g/devel/message/95786
Mute This Topic: https://groups.io/mt/94700349/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.