Date
1 - 6 of 6
[PATCH edk2 v3 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A
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 <huangming@...> --- StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 54 +++++++++++++++----- StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 ++++++++ StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 + 3 files changed, 63 insertions(+), 13 deletions(-) diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c index 5dfaf9d751..d0ba415671 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,42 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = { STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL; +STATIC +EFI_STATUS +CheckBufferAddr ( + IN UINTN BufferAddr + ) +{ + UINT64 NsCommBufferEnd; + UINT64 SCommBufferEnd; + UINT64 CommBufferEnd; + + NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize; + SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize; + + if ((BufferAddr >= mNsCommBuffer.PhysicalStart) && + (BufferAddr < NsCommBufferEnd)) { + CommBufferEnd = NsCommBufferEnd; + } else if ((BufferAddr >= mSCommBuffer.PhysicalStart) && + (BufferAddr < SCommBufferEnd)) { + CommBufferEnd = SCommBufferEnd; + } else { + return EFI_ACCESS_DENIED; + } + + if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNICATE_HEADER)) { + return EFI_ACCESS_DENIED; + } + + // perform bounds check. + if ((CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER)) < + ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength) { + return EFI_ACCESS_DENIED; + } + + return EFI_SUCCESS; +} + /** The PI Standalone MM entry point for the TF-A CPU driver. @@ -104,25 +141,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; -- 2.17.1
|
|
Hi,
toggle quoted messageShow quoted text
Any comment about this series? 在 12/31/21 7:06 PM, Ming Huang 写道:
There are two scene communicate with StandaloneMm(MM):
|
|
Sami Mujawar
Hi Ming,
toggle quoted messageShow quoted text
I am not sure if this is an issue at my end, but I cannot apply this patch series, can you check, please? Also, is it possible to share these patches on a Github branch. Regards, Sami Mujawar On 30/03/2022, 10:37, "Ming Huang" <huangming@...> wrote: Hi, Any comment about this series? 在 12/31/21 7:06 PM, Ming Huang 写道:
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 <huangming@...> > --- > StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 54 +++++++++++++++----- > StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 ++++++++ > StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 + > 3 files changed, 63 insertions(+), 13 deletions(-) > > diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c > index 5dfaf9d751..d0ba415671 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,42 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = { > > STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL; > > +STATIC > +EFI_STATUS > +CheckBufferAddr ( > + IN UINTN BufferAddr > + ) > +{ > + UINT64 NsCommBufferEnd; > + UINT64 SCommBufferEnd; > + UINT64 CommBufferEnd; > + > + NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize; > + SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize; > + > + if ((BufferAddr >= mNsCommBuffer.PhysicalStart) && > + (BufferAddr < NsCommBufferEnd)) { > + CommBufferEnd = NsCommBufferEnd; > + } else if ((BufferAddr >= mSCommBuffer.PhysicalStart) && > + (BufferAddr < SCommBufferEnd)) { > + CommBufferEnd = SCommBufferEnd; > + } else { > + return EFI_ACCESS_DENIED; > + } > + > + if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNICATE_HEADER)) { > + return EFI_ACCESS_DENIED; > + } > + > + // perform bounds check. > + if ((CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER)) < > + ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength) { > + return EFI_ACCESS_DENIED; > + } > + > + return EFI_SUCCESS; > +} > + > /** > The PI Standalone MM entry point for the TF-A CPU driver. > > @@ -104,25 +141,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; >
|
|
Hi Sami,
toggle quoted messageShow quoted text
I am sure this is an issue for RAS case using Standalone MM. Please refer Omkar's comments for v1 (12/9/21, 1:46 AM): ----------------------------------------------------------------- Hi Ming, Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case. Few comments mentioned inline. - Omkar ----------------------------------------------------------------- Regards, Ming Huang 在 4/6/22 5:03 PM, Sami Mujawar 写道:
Hi Ming,
|
|
Hi Sami,
toggle quoted messageShow quoted text
Sorry for the late reply. https://github.com/waip23/edk2/tree/upstread-fix-mm-issue branch: upstread-fix-mm-issue Regards, Ming Huang 在 4/6/22 5:03 PM, Sami Mujawar 写道:
Hi Ming,
|
|
Sami Mujawar
Fixed issues reported by CI and merged as f193b945eac5..5496c763aadd
Thanks, Sami Mujawar
|
|