Re: [edk2-devel] [PATCH edk2 v3 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A

["Ming Huang" <huangming@linux.alibaba.com>]

Hi Sami,

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,
> 
> 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@linux.alibaba.com> 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@linux.alibaba.com>
>     > ---
>     >  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;
>     >  
> 
> 
> 
> 
> 
>