1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
|
From cab1f02565d3b29081dd21afb074f35fdb4e1fd6 Mon Sep 17 00:00:00 2001
From: Miki Demeter <miki.demeter@intel.com>
Date: Thu, 27 Oct 2022 16:20:54 -0700
Subject: [PATCH] MdeModulePkg/PiSmmCore:SmmEntryPoint underflow(CVE-2021-38578)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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>
Reviewed-by: 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
@@ -609,6 +609,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.
**/
@@ -620,11 +621,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.
**/
@@ -1353,11 +1354,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.27.0
|
