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

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

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