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

["Sami Mujawar" <sami.mujawar@arm.com>]

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;
    >