Hi Marvin, Apologize for missing your email. My thunderbird is odd that your email did't show in the thread. Reply your comment in that email.
Thanks. - Ming 在 12/22/21 9:01 PM, Marvin Häuser 写道: > Hey Ming, > > Please check > https://edk2.groups.io/g/devel/message/84973?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2CMarvin%2C20%2C2%2C0%2C86334815 > While a few comments were cosmetical, there is an invalid memory access > outlined too, which persists in this patch. > > For the future, this should also check buffer alignment before casting (lets > hope my corresponding patch passes review within this decade...). > > Best regards, > Marvin > > On 21.12.21 16:06, Ming Huang wrote: >> There are two scene communicate with StandaloneMm(MM): >> 1 edk2 -> TF-A -> MM, communicate MM use non-secure buffer which >> specify by EFI_SECURE_PARTITION_BOOT_INFO.SpNsCommBufBase; >> 2 RAS scene: fiq -> TF-A -> MM, use secure buffer which >> specify by EFI_SECURE_PARTITION_BOOT_INFO.SpShareBufBase; >> >> For now, the second scene will failed because check buffer address. >> This patch add CheckBufferAddr() to support check address for secure >> buffer. >> >> Signed-off-by: Ming Huang <huangm...@linux.alibaba.com> >> --- >> StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 59 >> +++++++++++++++----- >> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 +++++++ >> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 + >> 3 files changed, 68 insertions(+), 13 deletions(-) >> >> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c >> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c >> index 5dfaf9d751..ba8639a26a 100644 >> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c >> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c >> @@ -50,6 +50,7 @@ EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext = >> NULL; >> // Descriptor with whereabouts of memory used for communication with the >> normal world >> EFI_MMRAM_DESCRIPTOR mNsCommBuffer; >> +EFI_MMRAM_DESCRIPTOR mSCommBuffer; >> MP_INFORMATION_HOB_DATA *mMpInformationHobData; >> @@ -60,6 +61,47 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = { >> STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL; >> +STATIC >> +EFI_STATUS >> +CheckBufferAddr ( >> + IN UINTN CommBufferAddr >> + ) >> +{ >> + UINTN CommBufferSize; >> + EFI_STATUS Status; >> + UINT64 NsCommBufferEnd; >> + UINT64 SCommBufferEnd; >> + UINT64 CommBufferEnd; >> + >> + Status = EFI_SUCCESS; >> + NsCommBufferEnd = mNsCommBuffer.PhysicalStart + >> mNsCommBuffer.PhysicalSize; >> + SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize; >> + >> + if ((CommBufferAddr >= mNsCommBuffer.PhysicalStart) && >> + (CommBufferAddr < NsCommBufferEnd)) { >> + CommBufferEnd = NsCommBufferEnd; >> + } else if ((CommBufferAddr >= mSCommBuffer.PhysicalStart) && >> + (CommBufferAddr <= SCommBufferEnd)) { >> + CommBufferEnd = SCommBufferEnd; >> + } else { >> + return EFI_ACCESS_DENIED; >> + } >> + >> + if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= >> CommBufferEnd) { >> + Status = EFI_INVALID_PARAMETER; >> + } >> + >> + // Find out the size of the buffer passed >> + CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) >> CommBufferAddr)->MessageLength + >> + sizeof (EFI_MM_COMMUNICATE_HEADER); >> + // perform bounds check. >> + if (CommBufferAddr + CommBufferSize >= CommBufferEnd) { >> + Status = EFI_ACCESS_DENIED; >> + } >> + >> + return Status; >> +} >> + >> /** >> The PI Standalone MM entry point for the TF-A CPU driver. >> @@ -104,25 +146,16 @@ PiMmStandaloneArmTfCpuDriverEntry ( >> return EFI_INVALID_PARAMETER; >> } >> - if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) { >> - return EFI_ACCESS_DENIED; >> - } >> - >> - if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= >> - (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) { >> - return EFI_INVALID_PARAMETER; >> + Status = CheckBufferAddr (NsCommBufferAddr); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status)); >> + return Status; >> } >> // Find out the size of the buffer passed >> NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) >> NsCommBufferAddr)->MessageLength + >> sizeof (EFI_MM_COMMUNICATE_HEADER); >> - // perform bounds check. >> - if (NsCommBufferAddr + NsCommBufferSize >= >> - mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) { >> - return EFI_ACCESS_DENIED; >> - } >> - >> GuidedEventContext = NULL; >> // Now that the secure world can see the normal world buffer, allocate >> // memory to copy the communication buffer to the secure world. >> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c >> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c >> index fd9c59b4da..96dad20dd1 100644 >> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c >> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c >> @@ -107,6 +107,7 @@ StandaloneMmCpuInitialize ( >> UINTN Index; >> UINTN ArraySize; >> VOID *HobStart; >> + EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob; >> ASSERT (SystemTable != NULL); >> mMmst = SystemTable; >> @@ -186,6 +187,26 @@ StandaloneMmCpuInitialize ( >> CopyMem (&mNsCommBuffer, NsCommBufMmramRange, >> sizeof(EFI_MMRAM_DESCRIPTOR)); >> DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", >> mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize)); >> + Status = GetGuidedHobData ( >> + HobStart, >> + &gEfiMmPeiMmramMemoryReserveGuid, >> + (VOID **) &MmramRangesHob >> + ); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "MmramRangesHob data extraction failed - 0x%x\n", >> Status)); >> + return Status; >> + } >> + >> + // >> + // As CreateHobListFromBootInfo(), the base and size of buffer shared with >> + // privileged Secure world software is in second one. >> + // >> + CopyMem ( >> + &mSCommBuffer, >> + &MmramRangesHob->Descriptor[0] + 1, >> + sizeof(EFI_MMRAM_DESCRIPTOR) >> + ); >> + >> // >> // Extract the MP information from the Hoblist >> // >> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h >> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h >> index 2c96439c15..2e03b20d85 100644 >> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h >> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h >> @@ -30,6 +30,7 @@ extern EFI_MM_CPU_PROTOCOL mMmCpuState; >> // >> extern EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext; >> extern EFI_MMRAM_DESCRIPTOR mNsCommBuffer; >> +extern EFI_MMRAM_DESCRIPTOR mSCommBuffer; >> extern MP_INFORMATION_HOB_DATA *mMpInformationHobData; >> extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig; >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#85199): https://edk2.groups.io/g/devel/message/85199 Mute This Topic: https://groups.io/mt/87878745/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
