* [RESEND PATCH 1/1] MdeModulePkg: Fix PiSmmCore integer over- and underflows.
@ 2022-10-04 14:27 Gerd Hoffmann
2022-10-10 2:02 ` 回复: [edk2-devel] " gaoliming
0 siblings, 1 reply; 2+ messages in thread
From: Gerd Hoffmann @ 2022-10-04 14:27 UTC (permalink / raw)
To: devel
Cc: Pawel Polawski, Liming Gao, Jian J Wang, Eric Dong, Ray Ni,
Oliver Steffen, Gerd Hoffmann
Prevents potential math over and underflows when comparing buffers
for SMM validity.
Original patch uploaded to bugzilla by Bret Barkelew <bret@corthon.com>.
I've adapted the patch to latest master, uncristified, dropped MU
comments.
Ref: CVE-2021-38578
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3387
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf | 1 +
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 +
MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 1 +
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 32 ++++++++++++++++-------
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 10 +++++--
5 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
index c8bfae3860fc..3df44b38f13c 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
@@ -60,6 +60,7 @@ [LibraryClasses]
PerformanceLib
HobLib
SmmMemLib
+ SafeIntLib
[Protocols]
gEfiDxeSmmReadyToLockProtocolGuid ## UNDEFINED # SmiHandlerRegister
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
index 6109d6b5449c..ddeb39cee266 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
@@ -46,6 +46,7 @@ [LibraryClasses]
DxeServicesLib
PcdLib
ReportStatusCodeLib
+ SafeIntLib
[Protocols]
gEfiSmmBase2ProtocolGuid ## PRODUCES
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
index 71422b9dfcdf..b8a490a8c3b5 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.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index 9e5c6cbe33dd..5ef8b31ee361 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.
@retval FALSE Buffer doesn't overlap.
**/
@@ -621,11 +622,21 @@ InternalIsBufferOverlapped (
IN UINTN Size2
)
{
+ UINTN End1;
+ UINTN End2;
+
+ // Prevents potential math over and underflows.
+ if (EFI_ERROR (SafeUintnAdd ((UINTN)Buff1, Size1, &End1)) ||
+ EFI_ERROR (SafeUintnAdd ((UINTN)Buff2, Size2, &End2)))
+ {
+ 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;
}
@@ -699,7 +710,10 @@ SmmEntryPoint (
(UINT8 *)gSmmCorePrivate,
sizeof (*gSmmCorePrivate)
);
- if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) || IsOverlapped) {
+ if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) ||
+ IsOverlapped ||
+ EFI_ERROR (SafeUintnSub (BufferSize, OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data), &BufferSize)))
+ {
//
// If CommunicationBuffer is not in valid address scope,
// or there is overlap between gSmmCorePrivate and CommunicationBuffer,
@@ -709,13 +723,13 @@ SmmEntryPoint (
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/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 4f00cebaf5ed..f08ca55e26b7 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -34,6 +34,7 @@
#include <Library/UefiRuntimeLib.h>
#include <Library/PcdLib.h>
#include <Library/ReportStatusCodeLib.h>
+#include <Library/SafeIntLib.h>
#include "PiSmmCorePrivateData.h"
@@ -1354,6 +1355,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.
**/
@@ -1366,8 +1368,12 @@ SmmIsSmramOverlap (
UINT64 RangeToCompareEnd;
UINT64 ReservedRangeToCompareEnd;
- RangeToCompareEnd = RangeToCompare->CpuStart + RangeToCompare->PhysicalSize;
- ReservedRangeToCompareEnd = ReservedRangeToCompare->SmramReservedStart + ReservedRangeToCompare->SmramReservedSize;
+ // Prevents potential math over and underflows.
+ if (EFI_ERROR (SafeUint64Add ((UINT64)RangeToCompare->CpuStart, RangeToCompare->PhysicalSize, &RangeToCompareEnd)) ||
+ EFI_ERROR (SafeUint64Add ((UINT64)ReservedRangeToCompare->SmramReservedStart, ReservedRangeToCompare->SmramReservedSize, &ReservedRangeToCompareEnd)))
+ {
+ return TRUE;
+ }
if ((RangeToCompare->CpuStart >= ReservedRangeToCompare->SmramReservedStart) &&
(RangeToCompare->CpuStart < ReservedRangeToCompareEnd))
--
2.37.3
^ permalink raw reply related [flat|nested] 2+ messages in thread
* 回复: [edk2-devel] [RESEND PATCH 1/1] MdeModulePkg: Fix PiSmmCore integer over- and underflows.
2022-10-04 14:27 [RESEND PATCH 1/1] MdeModulePkg: Fix PiSmmCore integer over- and underflows Gerd Hoffmann
@ 2022-10-10 2:02 ` gaoliming
0 siblings, 0 replies; 2+ messages in thread
From: gaoliming @ 2022-10-10 2:02 UTC (permalink / raw)
To: devel, kraxel
Cc: 'Pawel Polawski', 'Jian J Wang',
'Eric Dong', 'Ray Ni', 'Oliver Steffen'
The code change is good to me.
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Gerd
> Hoffmann
> 发送时间: 2022年10月4日 22:27
> 收件人: devel@edk2.groups.io
> 抄送: Pawel Polawski <ppolawsk@redhat.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Jian J Wang <jian.j.wang@intel.com>; Eric
> Dong <eric.dong@intel.com>; Ray Ni <ray.ni@intel.com>; Oliver Steffen
> <osteffen@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>
> 主题: [edk2-devel] [RESEND PATCH 1/1] MdeModulePkg: Fix PiSmmCore
> integer over- and underflows.
>
> Prevents potential math over and underflows when comparing buffers
> for SMM validity.
>
> Original patch uploaded to bugzilla by Bret Barkelew <bret@corthon.com>.
> I've adapted the patch to latest master, uncristified, dropped MU
> comments.
>
> Ref: CVE-2021-38578
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3387
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf | 1 +
> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 +
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 1 +
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 32
> ++++++++++++++++-------
> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 10 +++++--
> 5 files changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> index c8bfae3860fc..3df44b38f13c 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
> @@ -60,6 +60,7 @@ [LibraryClasses]
> PerformanceLib
> HobLib
> SmmMemLib
> + SafeIntLib
>
> [Protocols]
> gEfiDxeSmmReadyToLockProtocolGuid ## UNDEFINED #
> SmiHandlerRegister
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> index 6109d6b5449c..ddeb39cee266 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> @@ -46,6 +46,7 @@ [LibraryClasses]
> DxeServicesLib
> PcdLib
> ReportStatusCodeLib
> + SafeIntLib
>
> [Protocols]
> gEfiSmmBase2ProtocolGuid ## PRODUCES
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
> index 71422b9dfcdf..b8a490a8c3b5 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.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> index 9e5c6cbe33dd..5ef8b31ee361 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.
> @retval FALSE Buffer doesn't overlap.
>
> **/
> @@ -621,11 +622,21 @@ InternalIsBufferOverlapped (
> IN UINTN Size2
> )
> {
> + UINTN End1;
> + UINTN End2;
> +
> + // Prevents potential math over and underflows.
> + if (EFI_ERROR (SafeUintnAdd ((UINTN)Buff1, Size1, &End1)) ||
> + EFI_ERROR (SafeUintnAdd ((UINTN)Buff2, Size2, &End2)))
> + {
> + 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;
> }
>
> @@ -699,7 +710,10 @@ SmmEntryPoint (
> (UINT8 *)gSmmCorePrivate,
> sizeof (*gSmmCorePrivate)
> );
> - if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer,
> BufferSize) || IsOverlapped) {
> + if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer,
> BufferSize) ||
> + IsOverlapped ||
> + EFI_ERROR (SafeUintnSub (BufferSize, OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data), &BufferSize)))
> + {
> //
> // If CommunicationBuffer is not in valid address scope,
> // or there is overlap between gSmmCorePrivate and
> CommunicationBuffer,
> @@ -709,13 +723,13 @@ SmmEntryPoint (
> 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/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 4f00cebaf5ed..f08ca55e26b7 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -34,6 +34,7 @@
> #include <Library/UefiRuntimeLib.h>
> #include <Library/PcdLib.h>
> #include <Library/ReportStatusCodeLib.h>
> +#include <Library/SafeIntLib.h>
>
> #include "PiSmmCorePrivateData.h"
>
> @@ -1354,6 +1355,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.
>
> **/
> @@ -1366,8 +1368,12 @@ SmmIsSmramOverlap (
> UINT64 RangeToCompareEnd;
> UINT64 ReservedRangeToCompareEnd;
>
> - RangeToCompareEnd = RangeToCompare->CpuStart +
> RangeToCompare->PhysicalSize;
> - ReservedRangeToCompareEnd =
> ReservedRangeToCompare->SmramReservedStart +
> ReservedRangeToCompare->SmramReservedSize;
> + // Prevents potential math over and underflows.
> + if (EFI_ERROR (SafeUint64Add ((UINT64)RangeToCompare->CpuStart,
> RangeToCompare->PhysicalSize, &RangeToCompareEnd)) ||
> + EFI_ERROR (SafeUint64Add
> ((UINT64)ReservedRangeToCompare->SmramReservedStart,
> ReservedRangeToCompare->SmramReservedSize,
> &ReservedRangeToCompareEnd)))
> + {
> + return TRUE;
> + }
>
> if ((RangeToCompare->CpuStart >=
> ReservedRangeToCompare->SmramReservedStart) &&
> (RangeToCompare->CpuStart < ReservedRangeToCompareEnd))
> --
> 2.37.3
>
>
>
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-10-10 2:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-04 14:27 [RESEND PATCH 1/1] MdeModulePkg: Fix PiSmmCore integer over- and underflows Gerd Hoffmann
2022-10-10 2:02 ` 回复: [edk2-devel] " gaoliming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox