diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c index 398386c8f72..0a28fedba73 100644 --- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c @@ -19,6 +19,7 @@ #include #include #include +#include // MU_CHANGE: Add SafeIntLib for safe integer operations #include #include @@ -213,6 +214,7 @@ MmCommunicationCommon ( EFI_MM_COMMUNICATE_HEADER *CommunicateHeader; EFI_MM_COMMUNICATE_HEADER_V3 *CommunicateHeaderV3; UINTN BufferSize; + UINTN InputBufferSize; // MU_CHANGE UINTN *MessageSize; UINTN HeaderSize; EFI_STATUS Status; @@ -243,15 +245,41 @@ MmCommunicationCommon ( BufferSize = CommunicateHeaderV3->BufferSize; MessageSize = &CommunicateHeaderV3->MessageSize; HeaderSize = sizeof (EFI_MM_COMMUNICATE_HEADER_V3); + + // MU_CHANGE Starts: Add a check to ensure that the buffer size in the header is sane + if (BufferSize < HeaderSize) { + return EFI_INVALID_PARAMETER; + } + + if (BufferSize - HeaderSize < *MessageSize) { + return EFI_INVALID_PARAMETER; + } + + // MU_CHANGE Ends } else { - BufferSize = CommunicateHeader->MessageLength + - sizeof (CommunicateHeader->HeaderGuid) + - sizeof (CommunicateHeader->MessageLength); + // MU_CHANGE: Use SafeIntLib for safe arithmetic operations + // BufferSize = CommunicateHeader->MessageLength + + // sizeof (CommunicateHeader->HeaderGuid) + + // sizeof (CommunicateHeader->MessageLength); + Status = SafeUintnAdd ( + CommunicateHeader->MessageLength, + OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data), + &BufferSize + ); + if (EFI_ERROR (Status)) { + return EFI_INVALID_PARAMETER; + } + MessageSize = &CommunicateHeader->MessageLength; - HeaderSize = sizeof (CommunicateHeader->HeaderGuid) + - sizeof (CommunicateHeader->MessageLength); + // MU_CHANGE: Avoid direct calculation of HeaderSize + // HeaderSize = sizeof (CommunicateHeader->HeaderGuid) + + // sizeof (CommunicateHeader->MessageLength); + HeaderSize = OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data); } + // MU_CHANGE: Record the input buffer size for later comparison with the returned buffer size + InputBufferSize = BufferSize; + // If CommSize is not omitted, perform size inspection before proceeding. if (CommSize != NULL) { // This case can be used by the consumer of this driver to find out the @@ -269,6 +297,9 @@ MmCommunicationCommon ( if (*CommSize < BufferSize) { Status = EFI_INVALID_PARAMETER; } + + // MU_CHANGE: Record the updated input buffer size for later comparison with the returned buffer size + InputBufferSize = *CommSize; } // @@ -297,7 +328,8 @@ MmCommunicationCommon ( } if (!EFI_ERROR (Status)) { - ZeroMem (CommBufferVirtual, BufferSize); + // MU_CHANGE: Do not clear the input buffer as we will copy the returned data to the caller's buffer below + // ZeroMem (CommBufferVirtual, BufferSize); // On successful return, the size of data being returned is inferred from // MessageLength + Header. CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)mNsCommBuffMemRegion.VirtualBase; @@ -320,12 +352,22 @@ MmCommunicationCommon ( CommunicateHeaderV3 = (EFI_MM_COMMUNICATE_HEADER_V3 *)CommunicateHeader; BufferSize = CommunicateHeaderV3->BufferSize; } else { - BufferSize = CommunicateHeader->MessageLength + - sizeof (CommunicateHeader->HeaderGuid) + - sizeof (CommunicateHeader->MessageLength); + // MU_CHANGE: Use SafeIntLib for safe arithmetic operations + // BufferSize = CommunicateHeader->MessageLength + + // sizeof (CommunicateHeader->HeaderGuid) + + // sizeof (CommunicateHeader->MessageLength); + Status = SafeUintnAdd ( + CommunicateHeader->MessageLength, + OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data), + &BufferSize + ); + if (EFI_ERROR (Status)) { + return EFI_INVALID_PARAMETER; + } } - if (BufferSize > mNsCommBuffMemRegion.Length) { + // MU_CHANGE: Add a check to ensure that the returned buffer size does not exceed the caller supplied buffer size + if (BufferSize > InputBufferSize) { // Something bad has happened, we should have landed in ARM_SMC_MM_RET_NO_MEMORY Status = EFI_BAD_BUFFER_SIZE; DEBUG (( @@ -333,7 +375,7 @@ MmCommunicationCommon ( "%a Returned buffer exceeds communication buffer limit. Has: 0x%llx vs. max: 0x%llx!\n", __func__, BufferSize, - (UINTN)mNsCommBuffMemRegion.Length + InputBufferSize )); } else { CopyMem ( diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf index c9d37d89505..706c0ea4f1b 100644 --- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf @@ -39,6 +39,7 @@ DxeServicesTableLib HobLib UefiDriverEntryPoint + SafeIntLib # MU_CHANGE [Protocols] gEfiDxeMmReadyToLockProtocolGuid ## UNDEFINED # SmiHandlerRegister diff --git a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c index dac94ddd57c..c79f0faf89a 100644 --- a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c +++ b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c @@ -22,6 +22,7 @@ #include #include #include +#include // MU_CHANGE: Use SafeIntLib for safe arithmetic operations // // Partition ID if FF-A support is enabled @@ -342,6 +343,7 @@ MmCommunicationPeimCommon ( EFI_MM_COMMUNICATE_HEADER_V3 *CommunicateHeaderV3; EFI_STATUS Status; UINTN BufferSize; + UINTN InputBufferSize; // MU_CHANGE: Use SafeIntLib for safe arithmetic operations UINTN HeaderSize; // @@ -362,7 +364,31 @@ MmCommunicationPeimCommon ( // This is a v3 header CommunicateHeaderV3 = (EFI_MM_COMMUNICATE_HEADER_V3 *)(UINTN)CommBuffer; HeaderSize = sizeof (EFI_MM_COMMUNICATE_HEADER_V3); - BufferSize = CommunicateHeaderV3->BufferSize; + + // MU_CHANGE Starts: Add a check to ensure that the buffer size in the header is sane + InputBufferSize = CommunicateHeaderV3->BufferSize; + + if (InputBufferSize < HeaderSize) { + DEBUG (( + DEBUG_ERROR, + "%a Invalid BufferSize value 0x%llx!\n", + __func__, + InputBufferSize + )); + return EFI_INVALID_PARAMETER; + } + + if (InputBufferSize - HeaderSize < CommunicateHeaderV3->MessageSize) { + DEBUG (( + DEBUG_ERROR, + "%a Invalid BufferSize value 0x%llx!\n", + __func__, + InputBufferSize + )); + return EFI_INVALID_PARAMETER; + } + + // MU_CHANGE Ends } else { // This is a v1 header, do some checks if (CommSize == NULL) { @@ -390,35 +416,67 @@ MmCommunicationPeimCommon ( return EFI_BAD_BUFFER_SIZE; } - HeaderSize = sizeof (CommunicateHeader->HeaderGuid) + - sizeof (CommunicateHeader->MessageLength); + // MU_CHANGE Starts: Avoid unsafe arithmetic operations + // HeaderSize = sizeof (CommunicateHeader->HeaderGuid) + + // sizeof (CommunicateHeader->MessageLength); + HeaderSize = OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data); + // MU_CHANGE Ends // CommBuffer is a mandatory parameter. Hence, Rely on // MessageLength + Header to ascertain the // total size of the communication payload rather than // rely on optional CommSize parameter - BufferSize = CommunicateHeader->MessageLength + - sizeof (CommunicateHeader->HeaderGuid) + - sizeof (CommunicateHeader->MessageLength); + // MU_CHANGE: Use SafeIntLib for safe arithmetic operations + // BufferSize = CommunicateHeader->MessageLength + + // sizeof (CommunicateHeader->HeaderGuid) + + // sizeof (CommunicateHeader->MessageLength); + Status = SafeUintnAdd (CommunicateHeader->MessageLength, OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data), &InputBufferSize); + if (EFI_ERROR (Status)) { + DEBUG (( + DEBUG_ERROR, + "%a Overflow occurred while calculating input BufferSize!\n", + __func__ + )); + return Status; + } + + // MU_CHANGE Ends + // // If CommSize is supplied it must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER); // - if (*CommSize != BufferSize) { + // MU_CHANGE: Record the input buffer size for later comparison with the returned buffer size + if (*CommSize != InputBufferSize) { DEBUG (( DEBUG_ERROR, "%a Unexpected CommSize value, has: 0x%llx vs. expected: 0x%llx!\n", __func__, *CommSize, - BufferSize + InputBufferSize )); return EFI_INVALID_PARAMETER; } } + // MU_CHANGE Starts: Add a check to ensure that the buffer size does not exceed the allocated MM Communication Buffer size + if (InputBufferSize > (UINTN)PcdGet64 (PcdMmBufferSize)) { + DEBUG (( + DEBUG_ERROR, + "%a Input buffer exceeds communication buffer limit. Has: 0x%llx vs. max: 0x%llx!\n", + __func__, + InputBufferSize, + (UINTN)PcdGet64 (PcdMmBufferSize) + )); + return EFI_BAD_BUFFER_SIZE; + } + + // MU_CHANGE Ends + // Now we know that the size is something we can handle, copy it over to the designated comm buffer. CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)(UINTN)(PcdGet64 (PcdMmBufferBase)); - CopyMem (CommunicateHeader, CommBuffer, BufferSize); + // MU_CHANGE: Record the input buffer size for later comparison with the returned buffer size + CopyMem (CommunicateHeader, CommBuffer, InputBufferSize); if (IsFfaSupported ()) { Status = SendFfaMmCommunicate (); } else { @@ -447,20 +505,33 @@ MmCommunicationPeimCommon ( CommunicateHeaderV3 = (EFI_MM_COMMUNICATE_HEADER_V3 *)CommunicateHeader; BufferSize = CommunicateHeaderV3->BufferSize; } else { - BufferSize = CommunicateHeader->MessageLength + - sizeof (CommunicateHeader->HeaderGuid) + - sizeof (CommunicateHeader->MessageLength); + // MU_CHANGE: Use SafeIntLib for safe arithmetic operations + // BufferSize = CommunicateHeader->MessageLength + + // sizeof (CommunicateHeader->HeaderGuid) + + // sizeof (CommunicateHeader->MessageLength); + Status = SafeUintnAdd (CommunicateHeader->MessageLength, OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data), &BufferSize); + if (EFI_ERROR (Status)) { + DEBUG (( + DEBUG_ERROR, + "%a Overflow occurred while calculating returned BufferSize!\n", + __func__ + )); + return Status; + } + + // MU_CHANGE Ends } - if (BufferSize > (UINTN)PcdGet64 (PcdMmBufferSize)) { + // MU_CHANGE: Add a check to ensure that the returned buffer size does not exceed the caller supplied buffer size + if (InputBufferSize < BufferSize) { // Something bad has happened, we should have landed in ARM_SMC_MM_RET_NO_MEMORY Status = EFI_BAD_BUFFER_SIZE; DEBUG (( DEBUG_ERROR, - "%a Returned buffer exceeds communication buffer limit. Has: 0x%llx vs. max: 0x%llx!\n", + "%a Returned buffer size is larger than input buffer size. Input: 0x%llx vs. returned: 0x%llx!\n", __func__, - BufferSize, - (UINTN)PcdGet64 (PcdMmBufferSize) + InputBufferSize, + BufferSize )); } else { CopyMem (CommBuffer, CommunicateHeader, BufferSize); diff --git a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf index 7a422a7396d..d3e2c864884 100644 --- a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf +++ b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.inf @@ -29,6 +29,7 @@ PeimEntryPoint PeiServicesLib HobLib + SafeIntLib # MU_CHANGE [Pcd] gArmTokenSpaceGuid.PcdMmBufferBase diff --git a/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.c b/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.c index f49de8cfc79..41b974c84c1 100644 --- a/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.c +++ b/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.c @@ -504,6 +504,11 @@ ValidateMmCommBufferAddr ( &gEfiMmCommunicateHeaderV3Guid )) { + // MU_CHANGE: Now this is supposed to be a V3 header, so we need to check the size of the buffer in the header as well + if (CommBufferRange < sizeof (EFI_MM_COMMUNICATE_HEADER_V3)) { + return EFI_ACCESS_DENIED; + } + CommBufferHeaderV3 = (EFI_MM_COMMUNICATE_HEADER_V3 *)CommBufferAddr; Status = SafeUint64Add ( CommBufferHeaderV3->MessageSize, @@ -513,9 +518,24 @@ ValidateMmCommBufferAddr ( if (EFI_ERROR (Status)) { return EFI_ACCESS_DENIED; } + + // MU_CHANGE Starts: Make sure the buffer size in the header is also sane + if (BufferSize > CommBufferHeaderV3->BufferSize) { + return EFI_ACCESS_DENIED; + } + + BufferSize = CommBufferHeaderV3->BufferSize; + // MU_CHANGE Ends } else { - BufferSize = ((EFI_MM_COMMUNICATE_HEADER *)CommBufferAddr)->MessageLength + - OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data); + // MU_CHANGE: Use SafeIntLib for safe arithmetic operations + Status = SafeUint64Add ( + ((EFI_MM_COMMUNICATE_HEADER *)CommBufferAddr)->MessageLength, + OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data), + &BufferSize + ); + if (EFI_ERROR (Status)) { + return EFI_ACCESS_DENIED; + } } Status = SafeUint64Add ( diff --git a/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.inf b/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.inf index d46281de712..1a9dadb6416 100644 --- a/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.inf +++ b/ArmPkg/Library/ArmStandaloneMmCoreEntryPoint/ArmStandaloneMmCoreEntryPoint.inf @@ -50,6 +50,7 @@ ArmFfaLib StackCheckLib HobLib + SafeIntLib # MU_CHANGE [Guids] gMpInformationHobGuid diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c b/StandaloneMmPkg/Core/StandaloneMmCore.c index 2345f9a2322..6de5471f2f6 100644 --- a/StandaloneMmPkg/Core/StandaloneMmCore.c +++ b/StandaloneMmPkg/Core/StandaloneMmCore.c @@ -511,6 +511,8 @@ MmEntryPoint ( EFI_GUID *CommGuid; UINTN CommGuidOffset; UINTN CommHeaderSize; + EFI_STATUS SafeIntStatus; // MU_CHANGE: Use SafeIntLib for safe arithmetic operations + UINTN MaxBufferSize; // MU_CHANGE: Use SafeIntLib for safe arithmetic operations DEBUG ((DEBUG_INFO, "MmEntryPoint ...\n")); @@ -560,10 +562,34 @@ MmEntryPoint ( LegacyCommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)(UINTN)mMmCommunicationBuffer->PhysicalStart; CommGuidOffset = OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, HeaderGuid); CommHeaderSize = OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data); - BufferSize = OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data) + LegacyCommunicateHeader->MessageLength; + // MU_CHANGE: Use SafeIntLib for safe arithmetic operations + SafeIntStatus = SafeUintnAdd ( + OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data), + LegacyCommunicateHeader->MessageLength, + &BufferSize + ); + if (EFI_ERROR (SafeIntStatus)) { + DEBUG ((DEBUG_ERROR, "Failed to calculate buffer size: %r\n", SafeIntStatus)); + ASSERT_EFI_ERROR (SafeIntStatus); + return; + } + } + + // MU_CHANGE: Ensure the buffer size does not exceed the allocated MM Communication Buffer size + SafeIntStatus = SafeUintnMult ( + EFI_PAGE_SIZE, + mMmCommunicationBuffer->NumberOfPages, + &MaxBufferSize + ); + if (EFI_ERROR (SafeIntStatus)) { + DEBUG ((DEBUG_ERROR, "Failed to convert number of pages to bytes: %r\n", SafeIntStatus)); + ASSERT_EFI_ERROR (SafeIntStatus); + return; } - if (BufferSize <= EFI_PAGES_TO_SIZE (mMmCommunicationBuffer->NumberOfPages)) { + // MU_CHANGE: The check for buffer size is done using SafeIntLib above, so the explicit check here is commented out. + // if (BufferSize <= EFI_PAGES_TO_SIZE (mMmCommunicationBuffer->NumberOfPages)) { + if (BufferSize <= MaxBufferSize) { // // Shadow the data from MM Communication Buffer to internal buffer // @@ -574,19 +600,43 @@ MmEntryPoint ( ); ZeroMem ( (UINT8 *)mInternalCommBufferCopy + BufferSize, - EFI_PAGES_TO_SIZE (mMmCommunicationBuffer->NumberOfPages) - BufferSize + MaxBufferSize - BufferSize // MU_CHANGE: Use MaxBufferSize instead of EFI_PAGES_TO_SIZE ); - BufferSize -= CommHeaderSize; - Status = MmiManage ( - (EFI_GUID *)((UINT8 *)mInternalCommBufferCopy + CommGuidOffset), - NULL, - (UINT8 *)mInternalCommBufferCopy + CommHeaderSize, - &BufferSize - ); + // MU_CHANGE: Ensure the buffer size does not exceed the allocated MM Communication Buffer size + // BufferSize -= CommHeaderSize; + SafeIntStatus = SafeUintnSub ( + BufferSize, + CommHeaderSize, + &BufferSize + ); + if (EFI_ERROR (SafeIntStatus)) { + DEBUG ((DEBUG_ERROR, "Failed to subtract header from buffer size: %r\n", SafeIntStatus)); + ASSERT_EFI_ERROR (SafeIntStatus); + return; + } + + Status = MmiManage ( + (EFI_GUID *)((UINT8 *)mInternalCommBufferCopy + CommGuidOffset), + NULL, + (UINT8 *)mInternalCommBufferCopy + CommHeaderSize, + &BufferSize + ); + + // MU_CHANGE: Use SafeIntLib to safely add the communication header size back to the buffer size if needed + // BufferSize = BufferSize + CommHeaderSize; + SafeIntStatus = SafeUintnAdd ( + BufferSize, + CommHeaderSize, + &BufferSize + ); + if (EFI_ERROR (SafeIntStatus)) { + DEBUG ((DEBUG_ERROR, "Failed to calculate total buffer size: %r\n", SafeIntStatus)); + ASSERT_EFI_ERROR (SafeIntStatus); + return; + } - BufferSize = BufferSize + CommHeaderSize; - if (BufferSize <= EFI_PAGES_TO_SIZE (mMmCommunicationBuffer->NumberOfPages)) { + if (BufferSize <= MaxBufferSize) { // // Copy the data back to MM Communication Buffer // diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.h b/StandaloneMmPkg/Core/StandaloneMmCore.h index da123c530c9..8aa76384df7 100644 --- a/StandaloneMmPkg/Core/StandaloneMmCore.h +++ b/StandaloneMmPkg/Core/StandaloneMmCore.h @@ -48,6 +48,7 @@ #include #include #include +#include // MU_CHANGE: Add SafeIntLib for safe integer operations #include "StandaloneMmCorePrivateData.h" diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf index 1cad6a39426..2640e617637 100644 --- a/StandaloneMmPkg/Core/StandaloneMmCore.inf +++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf @@ -54,6 +54,7 @@ HobPrintLib ImagePropertiesRecordLib PerformanceLib + SafeIntLib # MU_CHANGE: Add SafeIntLib for safe integer operations [Protocols] gEfiDxeMmReadyToLockProtocolGuid ## UNDEFINED # SmiHandlerRegister diff --git a/StandaloneMmPkg/Drivers/MmCommunicationDxe/MmCommunicationDxe.c b/StandaloneMmPkg/Drivers/MmCommunicationDxe/MmCommunicationDxe.c index fec27801ea9..e113a1648bb 100644 --- a/StandaloneMmPkg/Drivers/MmCommunicationDxe/MmCommunicationDxe.c +++ b/StandaloneMmPkg/Drivers/MmCommunicationDxe/MmCommunicationDxe.c @@ -157,6 +157,11 @@ ProcessCommunicationBuffer ( return EFI_UNSUPPORTED; } + // MU_CHANGE: Make sure the returned buffer size does not exceed the caller supplied buffer size + if (CommonBufferStatus->ReturnBufferSize > BufferSize) { + return EFI_BAD_BUFFER_SIZE; + } + // // Copy the returned data to the non-mmram buffer (CommBuffer) // diff --git a/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.c b/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.c index 537ce79257d..e71fc42981c 100644 --- a/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.c +++ b/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.c @@ -60,6 +60,7 @@ Communicate ( EFI_HOB_GUID_TYPE *GuidHob; MM_COMM_BUFFER *MmCommBuffer; MM_COMM_BUFFER_STATUS *MmCommBufferStatus; + UINTN MaxBufferSize; // MU_CHANGE: Use SafeIntLib for safe arithmetic operations DEBUG ((DEBUG_INFO, "StandaloneMmIpl Communicate Enter\n")); @@ -91,11 +92,24 @@ Communicate ( } } - if (TempCommSize > EFI_PAGES_TO_SIZE (MmCommBuffer->NumberOfPages)) { - DEBUG ((DEBUG_ERROR, "Communicate buffer size (%d) is over MAX (%d) size!", TempCommSize, EFI_PAGES_TO_SIZE (MmCommBuffer->NumberOfPages))); + // MU_CHANGE: Use SafeIntLib for safe arithmetic operations + // if (TempCommSize > EFI_PAGES_TO_SIZE (MmCommBuffer->NumberOfPages)) { + // DEBUG ((DEBUG_ERROR, "Communicate buffer size (%d) is over MAX (%d) size!", TempCommSize, EFI_PAGES_TO_SIZE (MmCommBuffer->NumberOfPages))); + // return EFI_INVALID_PARAMETER; + // } + Status = SafeUintnMult (MmCommBuffer->NumberOfPages, EFI_PAGE_SIZE, &MaxBufferSize); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Overflow occurred while calculating MaxBufferSize!\n")); + return EFI_INVALID_PARAMETER; + } + + if (TempCommSize > MaxBufferSize) { + DEBUG ((DEBUG_ERROR, "Communicate buffer size (%d) is over MAX (%d) size!", TempCommSize, MaxBufferSize)); return EFI_INVALID_PARAMETER; } + // MU_CHANGE Ends + CopyMem ((VOID *)(UINTN)MmCommBuffer->PhysicalStart, CommBuffer, TempCommSize); MmCommBufferStatus->IsCommBufferValid = TRUE; @@ -118,7 +132,19 @@ Communicate ( // // Return status from software SMI // - *CommSize = (UINTN)MmCommBufferStatus->ReturnBufferSize; + // MU_CHANGE: Ensure the buffer size does not exceed the caller supplied buffer size + Status = SafeUint64ToUintn (MmCommBufferStatus->ReturnBufferSize, CommSize); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Overflow occurred while converting ReturnBufferSize to CommSize!\n")); + return EFI_BAD_BUFFER_SIZE; + } + + // MU_CHANGE: Ensure the buffer size does not exceed the caller supplied buffer size + if (*CommSize > TempCommSize) { + DEBUG ((DEBUG_ERROR, "Returned buffer size is larger than the Communication Buffer, TempCommSize: 0x%llx, ReturnBufferSize: 0x%llx\n", TempCommSize, *CommSize)); + ASSERT (*CommSize <= TempCommSize); + return EFI_BAD_BUFFER_SIZE; + } // // Copy the returned data to the non-mmram buffer (CommBuffer) @@ -156,12 +182,13 @@ Communicate3 ( EFI_STATUS Status; EFI_PEI_MM_CONTROL_PPI *MmControl; UINT8 SmiCommand; - UINTN Size; - UINTN TempCommSize; + UINT64 Size; + UINT64 TempCommSize; EFI_HOB_GUID_TYPE *GuidHob; MM_COMM_BUFFER *MmCommBuffer; MM_COMM_BUFFER_STATUS *MmCommBufferStatus; EFI_MM_COMMUNICATE_HEADER_V3 *CommunicateHeader; + UINT64 MaxBufferSize; // MU_CHANGE: Use SafeIntLib for safe arithmetic operations DEBUG ((DEBUG_INFO, "StandaloneMmIpl Communicate Enter\n")); @@ -203,11 +230,24 @@ Communicate3 ( } } - if (TempCommSize > EFI_PAGES_TO_SIZE (MmCommBuffer->NumberOfPages)) { - DEBUG ((DEBUG_ERROR, "Communicate buffer size (%d) is over MAX (%d) size!", TempCommSize, EFI_PAGES_TO_SIZE (MmCommBuffer->NumberOfPages))); + // MU_CHANGE Starts: Use SafeIntLib for safe arithmetic operations + // if (TempCommSize > EFI_PAGES_TO_SIZE (MmCommBuffer->NumberOfPages)) { + // DEBUG ((DEBUG_ERROR, "Communicate buffer size (%d) is over MAX (%d) size!", TempCommSize, EFI_PAGES_TO_SIZE (MmCommBuffer->NumberOfPages))); + // return EFI_INVALID_PARAMETER; + // } + Status = SafeUint64Mult (MmCommBuffer->NumberOfPages, EFI_PAGE_SIZE, &MaxBufferSize); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Overflow occurred while calculating MaxBufferSize!\n")); + return EFI_INVALID_PARAMETER; + } + + if (TempCommSize > MaxBufferSize) { + DEBUG ((DEBUG_ERROR, "Communicate buffer size (%d) is over MAX (%d) size!", TempCommSize, MaxBufferSize)); return EFI_INVALID_PARAMETER; } + // MU_CHANGE Ends + CopyMem ((VOID *)(UINTN)MmCommBuffer->PhysicalStart, CommBuffer, TempCommSize); MmCommBufferStatus->IsCommBufferValid = TRUE; @@ -230,7 +270,14 @@ Communicate3 ( // // Return status from software SMI // - TempCommSize = (UINTN)MmCommBufferStatus->ReturnBufferSize; + // MU_CHANGE: Ensure the buffer size does not exceed the caller supplied buffer size + if (MmCommBufferStatus->ReturnBufferSize > TempCommSize) { + DEBUG ((DEBUG_ERROR, "Returned buffer size is larger than the Communication Buffer, TempCommSize: 0x%llx, ReturnBufferSize: 0x%llx\n", TempCommSize, MmCommBufferStatus->ReturnBufferSize)); + ASSERT (MmCommBufferStatus->ReturnBufferSize <= TempCommSize); + return EFI_BAD_BUFFER_SIZE; + } + + TempCommSize = MmCommBufferStatus->ReturnBufferSize; // // Copy the returned data to the non-mmram buffer (CommBuffer) diff --git a/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.h b/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.h index 430e7c578b3..65440b8839a 100644 --- a/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.h +++ b/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.h @@ -28,6 +28,7 @@ #include #include #include +#include // MU_CHANGE: Add SafeIntLib for safe integer operations /** Communicates with a registered handler. diff --git a/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.inf b/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.inf index c06899e8ae6..1d55a8d564a 100644 --- a/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.inf +++ b/StandaloneMmPkg/Drivers/StandaloneMmIplPei/StandaloneMmIplPei.inf @@ -44,6 +44,7 @@ PeCoffLib CacheMaintenanceLib MmPlatformHobProducerLib + SafeIntLib # MU_CHANGE [Guids] gMmCommBufferHobGuid