[PATCH edk2 v3 0/3] Fix several issues in StanaloneMmPkg

[PATCH edk2 v3 0/3] Fix several issues in StanaloneMmPkg

[Ming Huang]

Changes since v2:
Modify CheckBufferAddr() function.

Ming Huang (3):
  StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field
  StandaloneMmPkg: Replace DEBUG_INFO with DEBUG_ERROR
  StandaloneMmPkg: Fix check buffer address failed issue from TF-A

 .../Drivers/StandaloneMmCpu/EventHandle.c     | 60 ++++++++++++++-----
 .../Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 33 ++++++++--
 .../Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
 .../Library/Arm/StandaloneMmCoreEntryPoint.h  |  2 +-
 .../Arm/CreateHobList.c                       |  2 +-
 .../Arm/StandaloneMmCoreEntryPoint.c          |  2 +-
 6 files changed, 75 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH edk2 v3 1/3] StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field

[Ming Huang]

TF-A: TrustedFirmware-A
SPM: Secure Partition Manager(MM)

In TF-A, the name of this field is sp_shared_buf_size. This field is
the size of range for transmit data from TF-A to standaloneMM when
SPM enable.

SpPcpuSharedBufSize is pass from TF-A while StandaloneMM initialize.
So, SpPcpuSharedBufSize should be rename to SpSharedBufSize and this field
should no multiply by PayloadBootInfo->NumCpus;

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
 StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h                    | 2 +-
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c              | 2 +-
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
index c44f7066c6..f1683ecb61 100644
--- a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
+++ b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
@@ -41,7 +41,7 @@ typedef struct {
   UINT64                        SpPcpuStackSize;
   UINT64                        SpHeapSize;
   UINT64                        SpNsCommBufSize;
-  UINT64                        SpPcpuSharedBufSize;
+  UINT64                        SpSharedBufSize;
   UINT32                        NumSpMemRegions;
   UINT32                        NumCpus;
   EFI_SECURE_PARTITION_CPU_INFO *CpuInfo;
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
index 85f8194687..93773c9fe8 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
@@ -173,7 +173,7 @@ CreateHobListFromBootInfo (
   // Base and size of buffer shared with privileged Secure world software
   MmramRanges[1].PhysicalStart = PayloadBootInfo->SpSharedBufBase;
   MmramRanges[1].CpuStart      = PayloadBootInfo->SpSharedBufBase;
-  MmramRanges[1].PhysicalSize  = PayloadBootInfo->SpPcpuSharedBufSize * PayloadBootInfo->NumCpus;
+  MmramRanges[1].PhysicalSize  = PayloadBootInfo->SpSharedBufSize;
   MmramRanges[1].RegionState   = EFI_CACHEABLE | EFI_ALLOCATED;
 
   // Base and size of buffer used for synchronous communication with Normal
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
index 49cf51a789..5db7019dda 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
@@ -87,7 +87,7 @@ GetAndPrintBootinformation (
   DEBUG ((DEBUG_INFO, "SpPcpuStackSize - 0x%x\n", PayloadBootInfo->SpPcpuStackSize));
   DEBUG ((DEBUG_INFO, "SpHeapSize      - 0x%x\n", PayloadBootInfo->SpHeapSize));
   DEBUG ((DEBUG_INFO, "SpNsCommBufSize - 0x%x\n", PayloadBootInfo->SpNsCommBufSize));
-  DEBUG ((DEBUG_INFO, "SpPcpuSharedBufSize - 0x%x\n", PayloadBootInfo->SpPcpuSharedBufSize));
+  DEBUG ((DEBUG_INFO, "SpSharedBufSize - 0x%x\n", PayloadBootInfo->SpSharedBufSize));
 
   DEBUG ((DEBUG_INFO, "NumCpus         - 0x%x\n", PayloadBootInfo->NumCpus));
   DEBUG ((DEBUG_INFO, "CpuInfo         - 0x%p\n", PayloadBootInfo->CpuInfo));
-- 
2.17.1

[PATCH edk2 v3 2/3] StandaloneMmPkg: Replace DEBUG_INFO with DEBUG_ERROR

[Ming Huang]

DEBUG_ERROR should be used in error branch.

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
 StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c     |  6 +++---
 StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 12 ++++++------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 165d696f99..5dfaf9d751 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
@@ -95,7 +95,7 @@ PiMmStandaloneArmTfCpuDriverEntry (
   //
   if ((ARM_SMC_ID_MM_COMMUNICATE != EventId) &&
       (ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ != EventId)) {
-    DEBUG ((DEBUG_INFO, "UnRecognized Event - 0x%x\n", EventId));
+    DEBUG ((DEBUG_ERROR, "UnRecognized Event - 0x%x\n", EventId));
     return EFI_INVALID_PARAMETER;
   }
 
@@ -133,7 +133,7 @@ PiMmStandaloneArmTfCpuDriverEntry (
                     );
 
   if (Status != EFI_SUCCESS) {
-    DEBUG ((DEBUG_INFO, "Mem alloc failed - 0x%x\n", EventId));
+    DEBUG ((DEBUG_ERROR, "Mem alloc failed - 0x%x\n", EventId));
     return EFI_OUT_OF_RESOURCES;
   }
 
@@ -156,7 +156,7 @@ PiMmStandaloneArmTfCpuDriverEntry (
   mMmst->CpuSaveState = NULL;
 
   if (mMmEntryPoint == NULL) {
-    DEBUG ((DEBUG_INFO, "Mm Entry point Not Found\n"));
+    DEBUG ((DEBUG_ERROR, "Mm Entry point Not Found\n"));
     return EFI_UNSUPPORTED;
   }
 
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
index 10097f792f..fd9c59b4da 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
@@ -143,7 +143,7 @@ StandaloneMmCpuInitialize (
 
   // Bail out if the Hoblist could not be found
   if (Index >= mMmst->NumberOfTableEntries) {
-    DEBUG ((DEBUG_INFO, "Hoblist not found - 0x%x\n", Index));
+    DEBUG ((DEBUG_ERROR, "Hoblist not found - 0x%x\n", Index));
     return EFI_OUT_OF_RESOURCES;
   }
 
@@ -158,7 +158,7 @@ StandaloneMmCpuInitialize (
              (VOID **) &CpuDriverEntryPointDesc
              );
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_INFO, "ArmTfCpuDriverEpDesc HOB data extraction failed - 0x%x\n", Status));
+    DEBUG ((DEBUG_ERROR, "ArmTfCpuDriverEpDesc HOB data extraction failed - 0x%x\n", Status));
     return Status;
   }
 
@@ -176,7 +176,7 @@ StandaloneMmCpuInitialize (
              (VOID **) &NsCommBufMmramRange
              );
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_INFO, "NsCommBufMmramRange HOB data extraction failed - 0x%x\n", Status));
+    DEBUG ((DEBUG_ERROR, "NsCommBufMmramRange HOB data extraction failed - 0x%x\n", Status));
     return Status;
   }
 
@@ -195,7 +195,7 @@ StandaloneMmCpuInitialize (
              (VOID **) &MpInformationHobData
              );
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_INFO, "MpInformationHob extraction failed - 0x%x\n", Status));
+    DEBUG ((DEBUG_ERROR, "MpInformationHob extraction failed - 0x%x\n", Status));
     return Status;
   }
 
@@ -213,7 +213,7 @@ StandaloneMmCpuInitialize (
                     (VOID **) &mMpInformationHobData
                     );
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_INFO, "mMpInformationHobData mem alloc failed - 0x%x\n", Status));
+    DEBUG ((DEBUG_ERROR, "mMpInformationHobData mem alloc failed - 0x%x\n", Status));
     return Status;
   }
 
@@ -243,7 +243,7 @@ StandaloneMmCpuInitialize (
                     (VOID **) &PerCpuGuidedEventContext
                     );
   if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_INFO, "PerCpuGuidedEventContext mem alloc failed - 0x%x\n", Status));
+    DEBUG ((DEBUG_ERROR, "PerCpuGuidedEventContext mem alloc failed - 0x%x\n", Status));
     return Status;
   }
   return Status;
-- 
2.17.1

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

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

Re: [PATCH edk2 v3 0/3] Fix several issues in StanaloneMmPkg

[Ming Huang]

Hi,

Any comment about this series ?

在 12/31/21 7:06 PM, Ming Huang 写道:
> Changes since v2:
> Modify CheckBufferAddr() function.
> 
> Ming Huang (3):
>   StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field
>   StandaloneMmPkg: Replace DEBUG_INFO with DEBUG_ERROR
>   StandaloneMmPkg: Fix check buffer address failed issue from TF-A
> 
>  .../Drivers/StandaloneMmCpu/EventHandle.c     | 60 ++++++++++++++-----
>  .../Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 33 ++++++++--
>  .../Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>  .../Library/Arm/StandaloneMmCoreEntryPoint.h  |  2 +-
>  .../Arm/CreateHobList.c                       |  2 +-
>  .../Arm/StandaloneMmCoreEntryPoint.c          |  2 +-
>  6 files changed, 75 insertions(+), 25 deletions(-)
> 

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

[Ming Huang]

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

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

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

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

[Ming Huang]

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

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

[Ming Huang]

Hi Sami,

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

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

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 97 bytes --]

Fixed issues reported by CI and merged as f193b945eac5..5496c763aadd

Thanks,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 113 bytes --]