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

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

[Ming Huang]

Changes since v1:
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     | 65 ++++++++++++++-----
 .../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, 80 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH edk2 v2 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 v2 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 v2 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     | 59 +++++++++++++++-----
 StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 +++++++
 StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
 3 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 5dfaf9d751..ba8639a26a 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,47 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {
 
 STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
 
+STATIC
+EFI_STATUS
+CheckBufferAddr (
+  IN UINTN CommBufferAddr
+  )
+{
+  UINTN      CommBufferSize;
+  EFI_STATUS Status;
+  UINT64     NsCommBufferEnd;
+  UINT64     SCommBufferEnd;
+  UINT64     CommBufferEnd;
+
+  Status = EFI_SUCCESS;
+  NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
+  SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
+
+  if ((CommBufferAddr >= mNsCommBuffer.PhysicalStart) &&
+      (CommBufferAddr < NsCommBufferEnd)) {
+    CommBufferEnd = NsCommBufferEnd;
+  } else if ((CommBufferAddr >= mSCommBuffer.PhysicalStart) &&
+             (CommBufferAddr <= SCommBufferEnd)) {
+    CommBufferEnd = SCommBufferEnd;
+  } else {
+    return EFI_ACCESS_DENIED;
+  }
+
+  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
+    Status = EFI_INVALID_PARAMETER;
+  }
+
+  // Find out the size of the buffer passed
+  CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
+    sizeof (EFI_MM_COMMUNICATE_HEADER);
+  // perform bounds check.
+  if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
+    Status = EFI_ACCESS_DENIED;
+  }
+
+  return Status;
+}
+
 /**
   The PI Standalone MM entry point for the TF-A CPU driver.
 
@@ -104,25 +146,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: [edk2-devel] [PATCH edk2 v2 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A

[Marvin Häuser]

Hey Ming,

Please check 
https://edk2.groups.io/g/devel/message/84973?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2CMarvin%2C20%2C2%2C0%2C86334815
While a few comments were cosmetical, there is an invalid memory access 
outlined too, which persists in this patch.

For the future, this should also check buffer alignment before casting 
(lets hope my corresponding patch passes review within this decade...).

Best regards,
Marvin

On 21.12.21 16:06, Ming Huang wrote:
> 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     | 59 +++++++++++++++-----
>   StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 +++++++
>   StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>   3 files changed, 68 insertions(+), 13 deletions(-)
>
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> index 5dfaf9d751..ba8639a26a 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,47 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {
>   
>   STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>   
> +STATIC
> +EFI_STATUS
> +CheckBufferAddr (
> +  IN UINTN CommBufferAddr
> +  )
> +{
> +  UINTN      CommBufferSize;
> +  EFI_STATUS Status;
> +  UINT64     NsCommBufferEnd;
> +  UINT64     SCommBufferEnd;
> +  UINT64     CommBufferEnd;
> +
> +  Status = EFI_SUCCESS;
> +  NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
> +  SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
> +
> +  if ((CommBufferAddr >= mNsCommBuffer.PhysicalStart) &&
> +      (CommBufferAddr < NsCommBufferEnd)) {
> +    CommBufferEnd = NsCommBufferEnd;
> +  } else if ((CommBufferAddr >= mSCommBuffer.PhysicalStart) &&
> +             (CommBufferAddr <= SCommBufferEnd)) {
> +    CommBufferEnd = SCommBufferEnd;
> +  } else {
> +    return EFI_ACCESS_DENIED;
> +  }
> +
> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
> +    Status = EFI_INVALID_PARAMETER;
> +  }
> +
> +  // Find out the size of the buffer passed
> +  CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
> +    sizeof (EFI_MM_COMMUNICATE_HEADER);
> +  // perform bounds check.
> +  if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
> +    Status = EFI_ACCESS_DENIED;
> +  }
> +
> +  return Status;
> +}
> +
>   /**
>     The PI Standalone MM entry point for the TF-A CPU driver.
>   
> @@ -104,25 +146,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 v2 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A

[Ming Huang]

Hi Marvin,

Apologize for missing your email. My thunderbird is odd that your email did't show in the thread.
Reply your comment in that email.

Thanks.

- Ming

在 12/22/21 9:01 PM, Marvin Häuser 写道:
> Hey Ming,
> 
> Please check https://edk2.groups.io/g/devel/message/84973?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2CMarvin%2C20%2C2%2C0%2C86334815
> While a few comments were cosmetical, there is an invalid memory access outlined too, which persists in this patch.
> 
> For the future, this should also check buffer alignment before casting (lets hope my corresponding patch passes review within this decade...).
> 
> Best regards,
> Marvin
> 
> On 21.12.21 16:06, Ming Huang wrote:
>> 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     | 59 +++++++++++++++-----
>>   StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 +++++++
>>   StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>   3 files changed, 68 insertions(+), 13 deletions(-)
>>
>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> index 5dfaf9d751..ba8639a26a 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,47 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {
>>     STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>   +STATIC
>> +EFI_STATUS
>> +CheckBufferAddr (
>> +  IN UINTN CommBufferAddr
>> +  )
>> +{
>> +  UINTN      CommBufferSize;
>> +  EFI_STATUS Status;
>> +  UINT64     NsCommBufferEnd;
>> +  UINT64     SCommBufferEnd;
>> +  UINT64     CommBufferEnd;
>> +
>> +  Status = EFI_SUCCESS;
>> +  NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
>> +  SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
>> +
>> +  if ((CommBufferAddr >= mNsCommBuffer.PhysicalStart) &&
>> +      (CommBufferAddr < NsCommBufferEnd)) {
>> +    CommBufferEnd = NsCommBufferEnd;
>> +  } else if ((CommBufferAddr >= mSCommBuffer.PhysicalStart) &&
>> +             (CommBufferAddr <= SCommBufferEnd)) {
>> +    CommBufferEnd = SCommBufferEnd;
>> +  } else {
>> +    return EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
>> +    Status = EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  // Find out the size of the buffer passed
>> +  CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
>> +    sizeof (EFI_MM_COMMUNICATE_HEADER);
>> +  // perform bounds check.
>> +  if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
>> +    Status = EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  return Status;
>> +}
>> +
>>   /**
>>     The PI Standalone MM entry point for the TF-A CPU driver.
>>   @@ -104,25 +146,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;
>>