The code change is good to me. Reviewed-by: Liming Gao <gaolim...@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 <ppola...@redhat.com>; Liming Gao > <gaolim...@byosoft.com.cn>; Jian J Wang <jian.j.w...@intel.com>; Eric > Dong <eric.d...@intel.com>; Ray Ni <ray...@intel.com>; Oliver Steffen > <ostef...@redhat.com>; Gerd Hoffmann <kra...@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 <b...@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 <kra...@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 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#94858): https://edk2.groups.io/g/devel/message/94858 Mute This Topic: https://groups.io/mt/94228599/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-