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

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

[Ming Huang]

Fix issues in StandaloneMmPkg for supporting communicate from
TF-A with secure buffer.

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     | 76 +++++++++++++++----
 .../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, 91 insertions(+), 25 deletions(-)

-- 
2.17.1

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

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {
 
 STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
 
+STATIC
+EFI_STATUS
+CheckBufferAddr (
+  IN UINTN CommBufferAddr
+  )
+{
+  UINTN      CommBufferSize;
+  EFI_STATUS Status;
+
+  Status =  EFI_SUCCESS;
+  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
+    Status = EFI_ACCESS_DENIED;
+  }
+
+  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
+      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
+    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 >=
+      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
+    Status =  EFI_ACCESS_DENIED;
+  }
+
+  if (!EFI_ERROR (Status)) {
+    return EFI_SUCCESS;
+  }
+
+  Status =  EFI_SUCCESS;
+  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
+    Status = EFI_ACCESS_DENIED;
+  }
+
+  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
+      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
+    Status =  EFI_INVALID_PARAMETER;
+  }
+
+  // perform bounds check.
+  if (CommBufferAddr + CommBufferSize >=
+      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
+    Status =  EFI_ACCESS_DENIED;
+  }
+
+  return Status;
+}
+
 /**
   The PI Standalone MM entry point for the TF-A CPU driver.
 
@@ -104,25 +157,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 v1 3/3] StandaloneMmPkg: Fix check buffer address failed issue from TF-A

[Omkar Anand Kulkarni]

Hi Ming,

Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
Few comments mentioned inline.

- Omkar


 On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
> ++++++++++++++++----
>  StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
> ++++++
> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>  3 files changed, 79 insertions(+), 13 deletions(-)
>
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
> {
>
>  STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>
> +STATIC
> +EFI_STATUS
> +CheckBufferAddr (
> +  IN UINTN CommBufferAddr
> +  )
> +{
> +  UINTN      CommBufferSize;
> +  EFI_STATUS Status;
> +
> +  Status =  EFI_SUCCESS;
> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
> +    Status = EFI_ACCESS_DENIED;
> +  }
> +
> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
> +    Status =  EFI_INVALID_PARAMETER;

Single space after "Status = "

- Omkar


> +  }
> +
> +  // 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 >=
> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
> +    Status =  EFI_ACCESS_DENIED;

Single space after "Status = "

- Omkar

> +  }
> +
> +  if (!EFI_ERROR (Status)) {


In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?

- Omkar


> +    return EFI_SUCCESS;
> +  }
> +
> +  Status =  EFI_SUCCESS;
> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
> +    Status = EFI_ACCESS_DENIED;
> +  }
> +
> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
> +    Status =  EFI_INVALID_PARAMETER;
> +  }
> +
> +  // perform bounds check.
> +  if (CommBufferAddr + CommBufferSize >=
> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
> +    Status =  EFI_ACCESS_DENIED;
> +  }
> +
> +  return Status;
> +}
> +


CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.

STATIC
EFI_STATUS
CheckBufferAddr (
  IN UINTN CommBufferAddr
  )
{
  UINTN                CommBufferSize;
  EFI_STATUS           Status;
  EFI_MMRAM_DESCRIPTOR CommBuffer;

  if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
      CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
    CommBuffer = mSCommBuffer;
  } else {
    CommBuffer = mNsCommBuffer;
  }

  if (CommBufferAddr < CommBuffer.PhysicalStart) {
    Status = EFI_ACCESS_DENIED;
  }

  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
      (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
    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 >=
      CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
    Status = EFI_ACCESS_DENIED;
  }

  return Status;
}

- Omkar


>  /**
>    The PI Standalone MM entry point for the TF-A CPU driver.
>
> @@ -104,25 +157,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,

Can this be changed to
&MmramRangesHob->Descriptor[1],

- Omkar

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

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

[Ard Biesheuvel]

(+ Masahisa, Masami)


On Fri, 15 Oct 2021 at 11:07, Ming Huang <huangming@linux.alibaba.com> wrote:
>
> 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>

Could someone please check how this change of interpretation affects
standalone MM running on SynQuacer?


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

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

[Ming Huang]

On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
> Hi Ming,
> 
> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
> Few comments mentioned inline.
> 
> - Omkar
> 
> 
>  On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>> ++++++++++++++++----
>>  StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>> ++++++
>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>  3 files changed, 79 insertions(+), 13 deletions(-)
>>
>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>> {
>>
>>  STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>
>> +STATIC
>> +EFI_STATUS
>> +CheckBufferAddr (
>> +  IN UINTN CommBufferAddr
>> +  )
>> +{
>> +  UINTN      CommBufferSize;
>> +  EFI_STATUS Status;
>> +
>> +  Status =  EFI_SUCCESS;
>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>> +    Status = EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>> +    Status =  EFI_INVALID_PARAMETER;
> 
> Single space after "Status = "

Modify it in v2.

> 
> - Omkar
> 
> 
>> +  }
>> +
>> +  // 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 >=
>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>> +    Status =  EFI_ACCESS_DENIED;
> 
> Single space after "Status = "

Modify it in v2.

> 
> - Omkar
> 
>> +  }
>> +
>> +  if (!EFI_ERROR (Status)) {
> 
> 
> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
> 
> - Omkar
> 
> 
>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  Status =  EFI_SUCCESS;
>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>> +    Status = EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>> +    Status =  EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  // perform bounds check.
>> +  if (CommBufferAddr + CommBufferSize >=
>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>> +    Status =  EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  return Status;
>> +}
>> +
> 
> 
> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.

Your example is a good idea to solve this case. I may modify it like below in v2:

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

- Ming

> 
> STATIC
> EFI_STATUS
> CheckBufferAddr (
>   IN UINTN CommBufferAddr
>   )
> {
>   UINTN                CommBufferSize;
>   EFI_STATUS           Status;
>   EFI_MMRAM_DESCRIPTOR CommBuffer;
> 
>   if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>       CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>     CommBuffer = mSCommBuffer;
>   } else {
>     CommBuffer = mNsCommBuffer;
>   }
> 
>   if (CommBufferAddr < CommBuffer.PhysicalStart) {
>     Status = EFI_ACCESS_DENIED;
>   }
> 
>   if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>       (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>     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 >=
>       CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>     Status = EFI_ACCESS_DENIED;
>   }
> 
>   return Status;
> }
> 
> - Omkar
> 
> 
>>  /**
>>    The PI Standalone MM entry point for the TF-A CPU driver.
>>
>> @@ -104,25 +157,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,
> 
> Can this be changed to
> &MmramRangesHob->Descriptor[1],
> 
> - Omkar
> 
>> +    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
>>
>>
>>
>>
>>
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 
> 
> 
> 

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

[Masahisa Kojima]

Hi Ard,

On Wed, 15 Dec 2021 at 17:45, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> (+ Masahisa, Masami)
>
>
> On Fri, 15 Oct 2021 at 11:07, Ming Huang <huangming@linux.alibaba.com> wrote:
> >
> > 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>
>
> Could someone please check how this change of interpretation affects
> standalone MM running on SynQuacer?

In conclusion, this patch does not affect the standalone MM implementation
on SynQuacer.

In my check, this buffer is used for data sharing between EL3 and Secure-EL1/0.
Currently this buffer is only used to pass the struct spm_mm_boot_info_t
from EL3 to StMM during Stmm initialization, I could not find other use cases.

TF-A maps the buffer only 64KB, not multiplied by the number of cores.
https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/socionext/synquacer/include/platform_def.h#n98
So the following definition is something strange and leads to
misunderstanding, but there is no problem.
   #define PLAT_SPM_BUF_BASE (BL32_LIMIT - 32 * PLAT_SPM_BUF_SIZE)
Arm platform statically allocates 1MB buffer in TF-A,
so I think it is better to follow the arm platform definition.


This is different topic, but assertion occurs in DEBUG build if StMM
and SECURE_BOOT_ENABLED=TRUE.

===
Loading driver at 0x000FABB0000 EntryPoint=0x000FABC447C CapsuleRuntimeDxe.efi
add-symbol-file
/home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe/DEBUG/SecureBootConfigDxe.dll
0xFA6BA000
Loading driver at 0x000FA6B9000 EntryPoint=0x000FA6C9610 SecureBootConfigDxe.efi
add-symbol-file
/home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/BdsDxe/BdsDxe/DEBUG/BdsDxe.dll
0xFA69D000
Loading driver at 0x000FA69C000 EntryPoint=0x000FA6A7E98 BdsDxe.efi
DxeCapsuleLibFmp: Failed to lock variable
39B68C46-F7FB-441B-B6EC-16B0F69821F3 CapsuleMax.  Status = Invalid
Parameter

ASSERT_EFI_ERROR (Status = Invalid Parameter)
ASSERT [BdsDxe]
/home/ubuntu/src/sbsa-qemu/edk2/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleReportLib.c(133):
!(((INTN)(RETURN_STATUS)(Status)) < 0)
===

Probably it is due to the missing support of VariablePolicy, StMM
outputs the following log output.
===
CommBuffer - 0xFC85CC90, CommBufferSize - 0x52
!!! DEPRECATED INTERFACE !!! VariableLockRequestToLock() will go away soon!
!!! DEPRECATED INTERFACE !!! Please move to use Variable Policy!
!!! DEPRECATED INTERFACE !!! Variable:
8BE4DF61-93CA-11D2-AA0D-00E098032B8C PlatformRecovery0000
===

That's all for now.

Thanks,
Masahisa Kojima

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

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

[Marvin Häuser]

Hey all,

> On 15. Dec 2021, at 16:02, Ming Huang <huangming@linux.alibaba.com> wrote:
> 
> 
> 
>> On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
>> Hi Ming,
>> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
>> Few comments mentioned inline.
>> - Omkar
>>> On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>>> ++++++++++++++++----
>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>>> ++++++
>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>> 3 files changed, 79 insertions(+), 13 deletions(-)
>>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>>> {
>>> STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>> +STATIC
>>> +EFI_STATUS
>>> +CheckBufferAddr (
>>> +  IN UINTN CommBufferAddr
>>> +  )
>>> +{
>>> +  UINTN      CommBufferSize;
>>> +  EFI_STATUS Status;
>>> +
>>> +  Status =  EFI_SUCCESS;
>>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>> +    Status = EFI_ACCESS_DENIED;
>>> +  }
>>> +
>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>> +    Status =  EFI_INVALID_PARAMETER;
>> Single space after "Status = "
> 
> Modify it in v2.
> 
>> - Omkar
>>> +  }
>>> +
>>> +  // 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 >=
>>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>>> +    Status =  EFI_ACCESS_DENIED;
>> Single space after "Status = "
> 
> Modify it in v2.
> 
>> - Omkar
>>> +  }
>>> +
>>> +  if (!EFI_ERROR (Status)) {
>> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
>> - Omkar
>>> +    return EFI_SUCCESS;
>>> +  }
>>> +
>>> +  Status =  EFI_SUCCESS;
>>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>>> +    Status = EFI_ACCESS_DENIED;
>>> +  }
>>> +
>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>>> +    Status =  EFI_INVALID_PARAMETER;
>>> +  }
>>> +
>>> +  // perform bounds check.
>>> +  if (CommBufferAddr + CommBufferSize >=
>>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>>> +    Status =  EFI_ACCESS_DENIED;
>>> +  }
>>> +
>>> +  return Status;
>>> +}
>>> +
>> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
>> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.
> 
> Your example is a good idea to solve this case. I may modify it like below in v2:
> 
> 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)) {

I find it odd the check here (lesser-equals) is inconsistent with the check above (lesser). It’d be caught below anyway, but I’d change this to lesser to keep the return codes consistent.

> CommBufferEnd = SCommBufferEnd;
> } else {
> return EFI_ACCESS_DENIED;
> }
> 
> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {

Why is greater-equals used here? MessageLength == 0 is not filtered below, so this looks odd to be honest, as this is only the theoretical maximum buffer end.

How do you know this cannot wraparound? I actually don’t think we do. We do know it holds that CommBufferAddr <= CommBufferEnd though, so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would give you that for free, if we assume the UINT64 variables above are actually bounded by UINTN, which seems reasonable - could ASSERT.

Alternatively, you could not store the maximum buffer end but the maximum buffer size, so the additions of the buffer start would just vanish. This might be more readable too I think.

> Status =  EFI_INVALID_PARAMETER;

Why is there no return here? This can proceed when the buffer cannot fit this header, and yet below the header is dereferenced.

> }
> 
> // Find out the size of the buffer passed
> CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
> sizeof (EFI_MM_COMMUNICATE_HEADER);

Same wraparound concern, same suggestion for solving it.

> // perform bounds check.
> if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
> Status =  EFI_ACCESS_DENIED;

It’s obviously not bad here, but for consistency’s sake, to mitigate bugs introduced by future changes, and readability, I’d return here and just return EFI_SUCCESS below, removing the code requirement of keeping Status consistent with the check results.

Finally, I really believe this kind of function should be abstracted in a way that it can be consumed by all places that accept any sort of communication buffer. Buffer validity checking is too critical than to duplicate it in every consumer.

Thanks!

Best regards,
Marvin

> }
> 
> return Status;
> }
> 
> - Ming
> 
>> STATIC
>> EFI_STATUS
>> CheckBufferAddr (
>> IN UINTN CommBufferAddr
>> )
>> {
>> UINTN                CommBufferSize;
>> EFI_STATUS           Status;
>> EFI_MMRAM_DESCRIPTOR CommBuffer;
>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>> CommBuffer = mSCommBuffer;
>> } else {
>> CommBuffer = mNsCommBuffer;
>> }
>> if (CommBufferAddr < CommBuffer.PhysicalStart) {
>> Status = EFI_ACCESS_DENIED;
>> }
>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>> 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 >=
>> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>> Status = EFI_ACCESS_DENIED;
>> }
>> return Status;
>> }
>> - Omkar
>>> /**
>>> The PI Standalone MM entry point for the TF-A CPU driver.
>>> @@ -104,25 +157,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,
>> Can this be changed to
>> &MmramRangesHob->Descriptor[1],
>> - Omkar
>>> +    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
>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 
> 
> 

Re: [edk2-devel] [PATCH edk2 v1 1/3] StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field

[Jeff Fan]

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

You may try to enlarger stmm_ns_comm_buf_size in Optee to check if it could solve ASSERT issue.



fanjianfeng@byosoft.com.cn
 
From: Masahisa Kojima
Date: 2021-12-16 16:07
To: Ard Biesheuvel
CC: Ming Huang; Masami Hiramatsu; edk2-devel-groups-io; Sami Mujawar; Ard Biesheuvel; Jiewen Yao; Supreeth Venkatesh; ming.huang-
Subject: Re: [edk2-devel] [PATCH edk2 v1 1/3] StandaloneMmPkg: Fix issue about SpPcpuSharedBufSize field
Hi Ard,
 
On Wed, 15 Dec 2021 at 17:45, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> (+ Masahisa, Masami)
>
>
> On Fri, 15 Oct 2021 at 11:07, Ming Huang <huangming@linux.alibaba.com> wrote:
> >
> > 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>
>
> Could someone please check how this change of interpretation affects
> standalone MM running on SynQuacer?
 
In conclusion, this patch does not affect the standalone MM implementation
on SynQuacer.
 
In my check, this buffer is used for data sharing between EL3 and Secure-EL1/0.
Currently this buffer is only used to pass the struct spm_mm_boot_info_t
from EL3 to StMM during Stmm initialization, I could not find other use cases.
 
TF-A maps the buffer only 64KB, not multiplied by the number of cores.
https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/socionext/synquacer/include/platform_def.h#n98
So the following definition is something strange and leads to
misunderstanding, but there is no problem.
   #define PLAT_SPM_BUF_BASE (BL32_LIMIT - 32 * PLAT_SPM_BUF_SIZE)
Arm platform statically allocates 1MB buffer in TF-A,
so I think it is better to follow the arm platform definition.
 
 
This is different topic, but assertion occurs in DEBUG build if StMM
and SECURE_BOOT_ENABLED=TRUE.
 
===
Loading driver at 0x000FABB0000 EntryPoint=0x000FABC447C CapsuleRuntimeDxe.efi
add-symbol-file
/home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe/DEBUG/SecureBootConfigDxe.dll
0xFA6BA000
Loading driver at 0x000FA6B9000 EntryPoint=0x000FA6C9610 SecureBootConfigDxe.efi
add-symbol-file
/home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/BdsDxe/BdsDxe/DEBUG/BdsDxe.dll
0xFA69D000
Loading driver at 0x000FA69C000 EntryPoint=0x000FA6A7E98 BdsDxe.efi
DxeCapsuleLibFmp: Failed to lock variable
39B68C46-F7FB-441B-B6EC-16B0F69821F3 CapsuleMax.  Status = Invalid
Parameter
 
ASSERT_EFI_ERROR (Status = Invalid Parameter)
ASSERT [BdsDxe]
/home/ubuntu/src/sbsa-qemu/edk2/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleReportLib.c(133):
!(((INTN)(RETURN_STATUS)(Status)) < 0)
===
 
Probably it is due to the missing support of VariablePolicy, StMM
outputs the following log output.
===
CommBuffer - 0xFC85CC90, CommBufferSize - 0x52
!!! DEPRECATED INTERFACE !!! VariableLockRequestToLock() will go away soon!
!!! DEPRECATED INTERFACE !!! Please move to use Variable Policy!
!!! DEPRECATED INTERFACE !!! Variable:
8BE4DF61-93CA-11D2-AA0D-00E098032B8C PlatformRecovery0000
===
 
That's all for now.
 
Thanks,
Masahisa Kojima
 
>
>
> > ---
> >  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
> >
 
 

 
 
 

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

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

[Ard Biesheuvel]

On Thu, 16 Dec 2021 at 09:07, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Ard,
>
> On Wed, 15 Dec 2021 at 17:45, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > (+ Masahisa, Masami)
> >
> >
> > On Fri, 15 Oct 2021 at 11:07, Ming Huang <huangming@linux.alibaba.com> wrote:
> > >
> > > 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>
> >
> > Could someone please check how this change of interpretation affects
> > standalone MM running on SynQuacer?
>
> In conclusion, this patch does not affect the standalone MM implementation
> on SynQuacer.
>
> In my check, this buffer is used for data sharing between EL3 and Secure-EL1/0.
> Currently this buffer is only used to pass the struct spm_mm_boot_info_t
> from EL3 to StMM during Stmm initialization, I could not find other use cases.
>
> TF-A maps the buffer only 64KB, not multiplied by the number of cores.
> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/socionext/synquacer/include/platform_def.h#n98
> So the following definition is something strange and leads to
> misunderstanding, but there is no problem.
>    #define PLAT_SPM_BUF_BASE (BL32_LIMIT - 32 * PLAT_SPM_BUF_SIZE)
> Arm platform statically allocates 1MB buffer in TF-A,
> so I think it is better to follow the arm platform definition.
>
>

Thanks for confirming

> This is different topic, but assertion occurs in DEBUG build if StMM
> and SECURE_BOOT_ENABLED=TRUE.
>
> ===
> Loading driver at 0x000FABB0000 EntryPoint=0x000FABC447C CapsuleRuntimeDxe.efi
> add-symbol-file
> /home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe/DEBUG/SecureBootConfigDxe.dll
> 0xFA6BA000
> Loading driver at 0x000FA6B9000 EntryPoint=0x000FA6C9610 SecureBootConfigDxe.efi
> add-symbol-file
> /home/ubuntu/src/sbsa-qemu/Build/DeveloperBox/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/BdsDxe/BdsDxe/DEBUG/BdsDxe.dll
> 0xFA69D000
> Loading driver at 0x000FA69C000 EntryPoint=0x000FA6A7E98 BdsDxe.efi
> DxeCapsuleLibFmp: Failed to lock variable
> 39B68C46-F7FB-441B-B6EC-16B0F69821F3 CapsuleMax.  Status = Invalid
> Parameter
>
> ASSERT_EFI_ERROR (Status = Invalid Parameter)
> ASSERT [BdsDxe]
> /home/ubuntu/src/sbsa-qemu/edk2/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleReportLib.c(133):
> !(((INTN)(RETURN_STATUS)(Status)) < 0)
> ===
>
> Probably it is due to the missing support of VariablePolicy, StMM
> outputs the following log output.
> ===
> CommBuffer - 0xFC85CC90, CommBufferSize - 0x52
> !!! DEPRECATED INTERFACE !!! VariableLockRequestToLock() will go away soon!
> !!! DEPRECATED INTERFACE !!! Please move to use Variable Policy!
> !!! DEPRECATED INTERFACE !!! Variable:
> 8BE4DF61-93CA-11D2-AA0D-00E098032B8C PlatformRecovery0000
> ===
>
> That's all for now.
>
> Thanks,
> Masahisa Kojima
>
> >
> >
> > > ---
> > >  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
> > >

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

[Ming Huang]

在 12/9/21 1:46 AM, Omkar Anand Kulkarni 写道:
> Hi Ming,
> 
> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
> Few comments mentioned inline.
> 
> - Omkar
> 
> 
>  On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>> ++++++++++++++++----
>>  StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>> ++++++
>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>  3 files changed, 79 insertions(+), 13 deletions(-)
>>
>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>> {
>>
>>  STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>
>> +STATIC
>> +EFI_STATUS
>> +CheckBufferAddr (
>> +  IN UINTN CommBufferAddr
>> +  )
>> +{
>> +  UINTN      CommBufferSize;
>> +  EFI_STATUS Status;
>> +
>> +  Status =  EFI_SUCCESS;
>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>> +    Status = EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>> +    Status =  EFI_INVALID_PARAMETER;
> 
> Single space after "Status = "
> 
> - Omkar
> 
> 
>> +  }
>> +
>> +  // 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 >=
>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>> +    Status =  EFI_ACCESS_DENIED;
> 
> Single space after "Status = "
> 
> - Omkar
> 
>> +  }
>> +
>> +  if (!EFI_ERROR (Status)) {
> 
> 
> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
> 
> - Omkar
> 
> 
>> +    return EFI_SUCCESS;
>> +  }
>> +
>> +  Status =  EFI_SUCCESS;
>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>> +    Status = EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>> +    Status =  EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  // perform bounds check.
>> +  if (CommBufferAddr + CommBufferSize >=
>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>> +    Status =  EFI_ACCESS_DENIED;
>> +  }
>> +
>> +  return Status;
>> +}
>> +
> 
> 
> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.
> 
> STATIC
> EFI_STATUS
> CheckBufferAddr (
>   IN UINTN CommBufferAddr
>   )
> {
>   UINTN                CommBufferSize;
>   EFI_STATUS           Status;
>   EFI_MMRAM_DESCRIPTOR CommBuffer;
> 
>   if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>       CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>     CommBuffer = mSCommBuffer;
>   } else {
>     CommBuffer = mNsCommBuffer;
>   }
> 
>   if (CommBufferAddr < CommBuffer.PhysicalStart) {
>     Status = EFI_ACCESS_DENIED;
>   }
> 
>   if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>       (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>     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 >=
>       CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>     Status = EFI_ACCESS_DENIED;
>   }
> 
>   return Status;
> }
> 
> - Omkar
> 
> 
>>  /**
>>    The PI Standalone MM entry point for the TF-A CPU driver.
>>
>> @@ -104,25 +157,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,
> 
> Can this be changed to
> &MmramRangesHob->Descriptor[1],

I missed this comment at last email.
The struct define:
EFI_MMRAM_DESCRIPTOR  Descriptor[1];

I guess some static code analysis tool may report out of array range, if modify to 
&MmramRangesHob->Descriptor[1].

- Ming

> 
> - Omkar
> 
>> +    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
>>
>>
>>
>>
>>
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 
> 
> 
> 

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

[Ming Huang]

在 12/16/21 5:15 PM, Marvin Häuser 写道:
> Hey all,
> 
>> On 15. Dec 2021, at 16:02, Ming Huang <huangming@linux.alibaba.com> wrote:
>>
>> 
>>
>>> On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
>>> Hi Ming,
>>> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
>>> Few comments mentioned inline.
>>> - Omkar
>>>> On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>>>> ++++++++++++++++----
>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>>>> ++++++
>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>>> 3 files changed, 79 insertions(+), 13 deletions(-)
>>>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>>>> {
>>>> STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>>> +STATIC
>>>> +EFI_STATUS
>>>> +CheckBufferAddr (
>>>> +  IN UINTN CommBufferAddr
>>>> +  )
>>>> +{
>>>> +  UINTN      CommBufferSize;
>>>> +  EFI_STATUS Status;
>>>> +
>>>> +  Status =  EFI_SUCCESS;
>>>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>> +    Status = EFI_ACCESS_DENIED;
>>>> +  }
>>>> +
>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>> +    Status =  EFI_INVALID_PARAMETER;
>>> Single space after "Status = "
>>
>> Modify it in v2.
>>
>>> - Omkar
>>>> +  }
>>>> +
>>>> +  // 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 >=
>>>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>>>> +    Status =  EFI_ACCESS_DENIED;
>>> Single space after "Status = "
>>
>> Modify it in v2.
>>
>>> - Omkar
>>>> +  }
>>>> +
>>>> +  if (!EFI_ERROR (Status)) {
>>> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
>>> - Omkar
>>>> +    return EFI_SUCCESS;
>>>> +  }
>>>> +
>>>> +  Status =  EFI_SUCCESS;
>>>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>>>> +    Status = EFI_ACCESS_DENIED;
>>>> +  }
>>>> +
>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>> +  }
>>>> +
>>>> +  // perform bounds check.
>>>> +  if (CommBufferAddr + CommBufferSize >=
>>>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>>>> +    Status =  EFI_ACCESS_DENIED;
>>>> +  }
>>>> +
>>>> +  return Status;
>>>> +}
>>>> +
>>> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
>>> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.
>>
>> Your example is a good idea to solve this case. I may modify it like below in v2:
>>
>> 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)) {
> 
> I find it odd the check here (lesser-equals) is inconsistent with the check above (lesser). It’d be caught below anyway, but I’d change this to lesser to keep the return codes consistent.

Should be lesser, modify it in v3.

> 
>> CommBufferEnd = SCommBufferEnd;
>> } else {
>> return EFI_ACCESS_DENIED;
>> }
>>
>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
> 
> Why is greater-equals used here? MessageLength == 0 is not filtered below, so this looks odd to be honest, as this is only the theoretical maximum buffer end.
> 
> How do you know this cannot wraparound? I actually don’t think we do. We do know it holds that CommBufferAddr <= CommBufferEnd though, so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would give you that for free, if we assume the UINT64 variables above are actually bounded by UINTN, which seems reasonable - could ASSERT.

In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd)
is the same with: CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER)

> 
> Alternatively, you could not store the maximum buffer end but the maximum buffer size, so the additions of the buffer start would just vanish. This might be more readable too I think.

As CommBufferAddr may be not at the begin of communicate buffer,
so check size with the maximum buffer size is not enough.

> 
>> Status =  EFI_INVALID_PARAMETER;
> 
> Why is there no return here? This can proceed when the buffer cannot fit this header, and yet below the header is dereferenced.

Modify it in v3.

> 
>> }
>>
>> // Find out the size of the buffer passed
>> CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
>> sizeof (EFI_MM_COMMUNICATE_HEADER);
> 
> Same wraparound concern, same suggestion for solving it.
> 
>> // perform bounds check.
>> if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
>> Status =  EFI_ACCESS_DENIED;
> 
> It’s obviously not bad here, but for consistency’s sake, to mitigate bugs introduced by future changes, and readability, I’d return here and just return EFI_SUCCESS below, removing the code requirement of keeping Status consistent with the check results.

Modify it in v3.

- Ming

> 
> Finally, I really believe this kind of function should be abstracted in a way that it can be consumed by all places that accept any sort of communication buffer. Buffer validity checking is too critical than to duplicate it in every consumer.
> 
> Thanks!
> 
> Best regards,
> Marvin
> 
>> }
>>
>> return Status;
>> }
>>
>> - Ming
>>
>>> STATIC
>>> EFI_STATUS
>>> CheckBufferAddr (
>>> IN UINTN CommBufferAddr
>>> )
>>> {
>>> UINTN                CommBufferSize;
>>> EFI_STATUS           Status;
>>> EFI_MMRAM_DESCRIPTOR CommBuffer;
>>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>> CommBuffer = mSCommBuffer;
>>> } else {
>>> CommBuffer = mNsCommBuffer;
>>> }
>>> if (CommBufferAddr < CommBuffer.PhysicalStart) {
>>> Status = EFI_ACCESS_DENIED;
>>> }
>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>>> 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 >=
>>> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>>> Status = EFI_ACCESS_DENIED;
>>> }
>>> return Status;
>>> }
>>> - Omkar
>>>> /**
>>>> The PI Standalone MM entry point for the TF-A CPU driver.
>>>> @@ -104,25 +157,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,
>>> Can this be changed to
>>> &MmramRangesHob->Descriptor[1],
>>> - Omkar
>>>> +    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
>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>
>>
>> 

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

[Marvin Häuser]

On 23.12.21 11:46, Ming Huang wrote:
>
> 在 12/16/21 5:15 PM, Marvin Häuser 写道:
>> Hey all,
>>
>>> On 15. Dec 2021, at 16:02, Ming Huang <huangming@linux.alibaba.com> wrote:
>>>
>>> 
>>>
>>>> On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
>>>> Hi Ming,
>>>> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
>>>> Few comments mentioned inline.
>>>> - Omkar
>>>>> On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>>>>> ++++++++++++++++----
>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>>>>> ++++++
>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>>>> 3 files changed, 79 insertions(+), 13 deletions(-)
>>>>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>>>>> {
>>>>> STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>>>> +STATIC
>>>>> +EFI_STATUS
>>>>> +CheckBufferAddr (
>>>>> +  IN UINTN CommBufferAddr
>>>>> +  )
>>>>> +{
>>>>> +  UINTN      CommBufferSize;
>>>>> +  EFI_STATUS Status;
>>>>> +
>>>>> +  Status =  EFI_SUCCESS;
>>>>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>> +  }
>>>>> +
>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>> Single space after "Status = "
>>> Modify it in v2.
>>>
>>>> - Omkar
>>>>> +  }
>>>>> +
>>>>> +  // 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 >=
>>>>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>> Single space after "Status = "
>>> Modify it in v2.
>>>
>>>> - Omkar
>>>>> +  }
>>>>> +
>>>>> +  if (!EFI_ERROR (Status)) {
>>>> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
>>>> - Omkar
>>>>> +    return EFI_SUCCESS;
>>>>> +  }
>>>>> +
>>>>> +  Status =  EFI_SUCCESS;
>>>>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>> +  }
>>>>> +
>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>> +  }
>>>>> +
>>>>> +  // perform bounds check.
>>>>> +  if (CommBufferAddr + CommBufferSize >=
>>>>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>> +  }
>>>>> +
>>>>> +  return Status;
>>>>> +}
>>>>> +
>>>> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
>>>> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.
>>> Your example is a good idea to solve this case. I may modify it like below in v2:
>>>
>>> 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)) {
>> I find it odd the check here (lesser-equals) is inconsistent with the check above (lesser). It’d be caught below anyway, but I’d change this to lesser to keep the return codes consistent.
> Should be lesser, modify it in v3.
>
>>> CommBufferEnd = SCommBufferEnd;
>>> } else {
>>> return EFI_ACCESS_DENIED;
>>> }
>>>
>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
>> Why is greater-equals used here? MessageLength == 0 is not filtered below, so this looks odd to be honest, as this is only the theoretical maximum buffer end.
>>
>> How do you know this cannot wraparound? I actually don’t think we do. We do know it holds that CommBufferAddr <= CommBufferEnd though, so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would give you that for free, if we assume the UINT64 variables above are actually bounded by UINTN, which seems reasonable - could ASSERT.
> In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd)
> is the same with: CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER)

Okay, assume:
CommBufferEnd = MAX_UINTN
CommBufferAddr = MAX_UINTN - 1 (MAX_UINTN - 1 < MAX_UINTN, so the check 
above would pass)
sizeof (EFI_MM_COMMUNICATE_HEADER) = 16 (should be accurate for 64-bit 
architectures)

Then (assume implicit mod 2^N on both sides due to bounded integers!):
(CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) 
<=> ((MAX_UINTN - 1) + 16) >= MAX_UINTN <=> MAX_UINTN + 15 >= MAX_UINTN 
<=>(wraparound!!) 14 >= MAX_UINTN <=> FALSE

And (assume implicit mod 2^N on both sides due to bounded integers!):
CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) <=> 
MAX_UINTN - (MAX_UINTN - 1) < 16 <=> 1 < 16 <=> TRUE

Due to wraparound semantics and the knowledge derived by the if-checks 
above they are by no means the same. The left term of the first equation 
can wrap around (or it cannot, but then we need some concrete proof that 
it cannot), and the left term of the second equation obviously cannot 
(directly follows from the if statements before).

>> Alternatively, you could not store the maximum buffer end but the maximum buffer size, so the additions of the buffer start would just vanish. This might be more readable too I think.
> As CommBufferAddr may be not at the begin of communicate buffer,
> so check size with the maximum buffer size is not enough.
>
>>> Status =  EFI_INVALID_PARAMETER;
>> Why is there no return here? This can proceed when the buffer cannot fit this header, and yet below the header is dereferenced.
> Modify it in v3.
>
>>> }
>>>
>>> // Find out the size of the buffer passed
>>> CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
>>> sizeof (EFI_MM_COMMUNICATE_HEADER);
>> Same wraparound concern, same suggestion for solving it.
>>
>>> // perform bounds check.
>>> if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
>>> Status =  EFI_ACCESS_DENIED;
>> It’s obviously not bad here, but for consistency’s sake, to mitigate bugs introduced by future changes, and readability, I’d return here and just return EFI_SUCCESS below, removing the code requirement of keeping Status consistent with the check results.
> Modify it in v3.

Thanks for the modifications.

Best regards,
Marvin

>
> - Ming
>
>> Finally, I really believe this kind of function should be abstracted in a way that it can be consumed by all places that accept any sort of communication buffer. Buffer validity checking is too critical than to duplicate it in every consumer.
>>
>> Thanks!
>>
>> Best regards,
>> Marvin
>>
>>> }
>>>
>>> return Status;
>>> }
>>>
>>> - Ming
>>>
>>>> STATIC
>>>> EFI_STATUS
>>>> CheckBufferAddr (
>>>> IN UINTN CommBufferAddr
>>>> )
>>>> {
>>>> UINTN                CommBufferSize;
>>>> EFI_STATUS           Status;
>>>> EFI_MMRAM_DESCRIPTOR CommBuffer;
>>>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>>>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>> CommBuffer = mSCommBuffer;
>>>> } else {
>>>> CommBuffer = mNsCommBuffer;
>>>> }
>>>> if (CommBufferAddr < CommBuffer.PhysicalStart) {
>>>> Status = EFI_ACCESS_DENIED;
>>>> }
>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>>>> 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 >=
>>>> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>>>> Status = EFI_ACCESS_DENIED;
>>>> }
>>>> return Status;
>>>> }
>>>> - Omkar
>>>>> /**
>>>>> The PI Standalone MM entry point for the TF-A CPU driver.
>>>>> @@ -104,25 +157,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,
>>>> Can this be changed to
>>>> &MmramRangesHob->Descriptor[1],
>>>> - Omkar
>>>>> +    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
>>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>>
>>> 

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

[Ming Huang]

在 12/23/21 7:05 PM, Marvin Häuser 写道:
> On 23.12.21 11:46, Ming Huang wrote:
>>
>> 在 12/16/21 5:15 PM, Marvin Häuser 写道:
>>> Hey all,
>>>
>>>> On 15. Dec 2021, at 16:02, Ming Huang <huangming@linux.alibaba.com> wrote:
>>>>
>>>> 
>>>>
>>>>> On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
>>>>> Hi Ming,
>>>>> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
>>>>> Few comments mentioned inline.
>>>>> - Omkar
>>>>>> On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>>>>>> ++++++++++++++++----
>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>>>>>> ++++++
>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>>>>> 3 files changed, 79 insertions(+), 13 deletions(-)
>>>>>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>>>>>> {
>>>>>> STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>>>>> +STATIC
>>>>>> +EFI_STATUS
>>>>>> +CheckBufferAddr (
>>>>>> +  IN UINTN CommBufferAddr
>>>>>> +  )
>>>>>> +{
>>>>>> +  UINTN      CommBufferSize;
>>>>>> +  EFI_STATUS Status;
>>>>>> +
>>>>>> +  Status =  EFI_SUCCESS;
>>>>>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>> +  }
>>>>>> +
>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>> Single space after "Status = "
>>>> Modify it in v2.
>>>>
>>>>> - Omkar
>>>>>> +  }
>>>>>> +
>>>>>> +  // 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 >=
>>>>>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>> Single space after "Status = "
>>>> Modify it in v2.
>>>>
>>>>> - Omkar
>>>>>> +  }
>>>>>> +
>>>>>> +  if (!EFI_ERROR (Status)) {
>>>>> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
>>>>> - Omkar
>>>>>> +    return EFI_SUCCESS;
>>>>>> +  }
>>>>>> +
>>>>>> +  Status =  EFI_SUCCESS;
>>>>>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>> +  }
>>>>>> +
>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>>> +  }
>>>>>> +
>>>>>> +  // perform bounds check.
>>>>>> +  if (CommBufferAddr + CommBufferSize >=
>>>>>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>>> +  }
>>>>>> +
>>>>>> +  return Status;
>>>>>> +}
>>>>>> +
>>>>> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
>>>>> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.
>>>> Your example is a good idea to solve this case. I may modify it like below in v2:
>>>>
>>>> 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)) {
>>> I find it odd the check here (lesser-equals) is inconsistent with the check above (lesser). It’d be caught below anyway, but I’d change this to lesser to keep the return codes consistent.
>> Should be lesser, modify it in v3.
>>
>>>> CommBufferEnd = SCommBufferEnd;
>>>> } else {
>>>> return EFI_ACCESS_DENIED;
>>>> }
>>>>
>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
>>> Why is greater-equals used here? MessageLength == 0 is not filtered below, so this looks odd to be honest, as this is only the theoretical maximum buffer end.
>>>
>>> How do you know this cannot wraparound? I actually don’t think we do. We do know it holds that CommBufferAddr <= CommBufferEnd though, so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would give you that for free, if we assume the UINT64 variables above are actually bounded by UINTN, which seems reasonable - could ASSERT.
>> In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd)
>> is the same with: CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER)
> 
> Okay, assume:
> CommBufferEnd = MAX_UINTN
> CommBufferAddr = MAX_UINTN - 1 (MAX_UINTN - 1 < MAX_UINTN, so the check above would pass)
> sizeof (EFI_MM_COMMUNICATE_HEADER) = 16 (should be accurate for 64-bit architectures)
> 
> Then (assume implicit mod 2^N on both sides due to bounded integers!):
> (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) <=> ((MAX_UINTN - 1) + 16) >= MAX_UINTN <=> MAX_UINTN + 15 >= MAX_UINTN <=>(wraparound!!) 14 >= MAX_UINTN <=> FALSE
> 
> And (assume implicit mod 2^N on both sides due to bounded integers!):
> CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) <=> MAX_UINTN - (MAX_UINTN - 1) < 16 <=> 1 < 16 <=> TRUE
> 
> Due to wraparound semantics and the knowledge derived by the if-checks above they are by no means the same. The left term of the first equation can wrap around (or it cannot, but then we need some concrete proof that it cannot), and the left term of the second equation obviously cannot (directly follows from the if statements before).

I got it. Thanks for your proper comments.
I may modify it like below in v3:
----------------------------------------
STATIC
EFI_STATUS
CheckBufferAddr (
  IN UINTN BufferAddr
  )
{
  UINTN  BufferSize;
  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_INVALID_PARAMETER;
  }

  // Find out the size of the buffer passed
  BufferSize = ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength +
    sizeof (EFI_MM_COMMUNICATE_HEADER);
  // perform bounds check.
  if ((CommBufferEnd - BufferAddr) < BufferSize) {
    return EFI_ACCESS_DENIED;
  }

  return EFI_SUCCESS;
}
----------------------------------------

- Ming

> 
>>> Alternatively, you could not store the maximum buffer end but the maximum buffer size, so the additions of the buffer start would just vanish. This might be more readable too I think.
>> As CommBufferAddr may be not at the begin of communicate buffer,
>> so check size with the maximum buffer size is not enough.
>>
>>>> Status =  EFI_INVALID_PARAMETER;
>>> Why is there no return here? This can proceed when the buffer cannot fit this header, and yet below the header is dereferenced.
>> Modify it in v3.
>>
>>>> }
>>>>
>>>> // Find out the size of the buffer passed
>>>> CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
>>>> sizeof (EFI_MM_COMMUNICATE_HEADER);
>>> Same wraparound concern, same suggestion for solving it.
>>>
>>>> // perform bounds check.
>>>> if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
>>>> Status =  EFI_ACCESS_DENIED;
>>> It’s obviously not bad here, but for consistency’s sake, to mitigate bugs introduced by future changes, and readability, I’d return here and just return EFI_SUCCESS below, removing the code requirement of keeping Status consistent with the check results.
>> Modify it in v3.
> 
> Thanks for the modifications.
> 
> Best regards,
> Marvin
> 
>>
>> - Ming
>>
>>> Finally, I really believe this kind of function should be abstracted in a way that it can be consumed by all places that accept any sort of communication buffer. Buffer validity checking is too critical than to duplicate it in every consumer.
>>>
>>> Thanks!
>>>
>>> Best regards,
>>> Marvin
>>>
>>>> }
>>>>
>>>> return Status;
>>>> }
>>>>
>>>> - Ming
>>>>
>>>>> STATIC
>>>>> EFI_STATUS
>>>>> CheckBufferAddr (
>>>>> IN UINTN CommBufferAddr
>>>>> )
>>>>> {
>>>>> UINTN                CommBufferSize;
>>>>> EFI_STATUS           Status;
>>>>> EFI_MMRAM_DESCRIPTOR CommBuffer;
>>>>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>>>>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>> CommBuffer = mSCommBuffer;
>>>>> } else {
>>>>> CommBuffer = mNsCommBuffer;
>>>>> }
>>>>> if (CommBufferAddr < CommBuffer.PhysicalStart) {
>>>>> Status = EFI_ACCESS_DENIED;
>>>>> }
>>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>>>>> 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 >=
>>>>> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>>>>> Status = EFI_ACCESS_DENIED;
>>>>> }
>>>>> return Status;
>>>>> }
>>>>> - Omkar
>>>>>> /**
>>>>>> The PI Standalone MM entry point for the TF-A CPU driver.
>>>>>> @@ -104,25 +157,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,
>>>>> Can this be changed to
>>>>> &MmramRangesHob->Descriptor[1],
>>>>> - Omkar
>>>>>> +    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
>>>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>>>
>>>> 

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

[Marvin Häuser]

On 24.12.21 02:18, Ming Huang wrote:
>
> 在 12/23/21 7:05 PM, Marvin Häuser 写道:
>> On 23.12.21 11:46, Ming Huang wrote:
>>> 在 12/16/21 5:15 PM, Marvin Häuser 写道:
>>>> Hey all,
>>>>
>>>>> On 15. Dec 2021, at 16:02, Ming Huang <huangming@linux.alibaba.com> wrote:
>>>>>
>>>>> 
>>>>>
>>>>>> On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
>>>>>> Hi Ming,
>>>>>> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
>>>>>> Few comments mentioned inline.
>>>>>> - Omkar
>>>>>>> On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>>>>>>> ++++++++++++++++----
>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>>>>>>> ++++++
>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>>>>>> 3 files changed, 79 insertions(+), 13 deletions(-)
>>>>>>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>>>>>>> {
>>>>>>> STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>>>>>> +STATIC
>>>>>>> +EFI_STATUS
>>>>>>> +CheckBufferAddr (
>>>>>>> +  IN UINTN CommBufferAddr
>>>>>>> +  )
>>>>>>> +{
>>>>>>> +  UINTN      CommBufferSize;
>>>>>>> +  EFI_STATUS Status;
>>>>>>> +
>>>>>>> +  Status =  EFI_SUCCESS;
>>>>>>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>>> +  }
>>>>>>> +
>>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>>> Single space after "Status = "
>>>>> Modify it in v2.
>>>>>
>>>>>> - Omkar
>>>>>>> +  }
>>>>>>> +
>>>>>>> +  // 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 >=
>>>>>>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>>> Single space after "Status = "
>>>>> Modify it in v2.
>>>>>
>>>>>> - Omkar
>>>>>>> +  }
>>>>>>> +
>>>>>>> +  if (!EFI_ERROR (Status)) {
>>>>>> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
>>>>>> - Omkar
>>>>>>> +    return EFI_SUCCESS;
>>>>>>> +  }
>>>>>>> +
>>>>>>> +  Status =  EFI_SUCCESS;
>>>>>>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>>> +  }
>>>>>>> +
>>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>>>> +  }
>>>>>>> +
>>>>>>> +  // perform bounds check.
>>>>>>> +  if (CommBufferAddr + CommBufferSize >=
>>>>>>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>>>> +  }
>>>>>>> +
>>>>>>> +  return Status;
>>>>>>> +}
>>>>>>> +
>>>>>> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
>>>>>> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.
>>>>> Your example is a good idea to solve this case. I may modify it like below in v2:
>>>>>
>>>>> 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)) {
>>>> I find it odd the check here (lesser-equals) is inconsistent with the check above (lesser). It’d be caught below anyway, but I’d change this to lesser to keep the return codes consistent.
>>> Should be lesser, modify it in v3.
>>>
>>>>> CommBufferEnd = SCommBufferEnd;
>>>>> } else {
>>>>> return EFI_ACCESS_DENIED;
>>>>> }
>>>>>
>>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
>>>> Why is greater-equals used here? MessageLength == 0 is not filtered below, so this looks odd to be honest, as this is only the theoretical maximum buffer end.
>>>>
>>>> How do you know this cannot wraparound? I actually don’t think we do. We do know it holds that CommBufferAddr <= CommBufferEnd though, so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would give you that for free, if we assume the UINT64 variables above are actually bounded by UINTN, which seems reasonable - could ASSERT.
>>> In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd)
>>> is the same with: CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER)
>> Okay, assume:
>> CommBufferEnd = MAX_UINTN
>> CommBufferAddr = MAX_UINTN - 1 (MAX_UINTN - 1 < MAX_UINTN, so the check above would pass)
>> sizeof (EFI_MM_COMMUNICATE_HEADER) = 16 (should be accurate for 64-bit architectures)
>>
>> Then (assume implicit mod 2^N on both sides due to bounded integers!):
>> (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) <=> ((MAX_UINTN - 1) + 16) >= MAX_UINTN <=> MAX_UINTN + 15 >= MAX_UINTN <=>(wraparound!!) 14 >= MAX_UINTN <=> FALSE
>>
>> And (assume implicit mod 2^N on both sides due to bounded integers!):
>> CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) <=> MAX_UINTN - (MAX_UINTN - 1) < 16 <=> 1 < 16 <=> TRUE
>>
>> Due to wraparound semantics and the knowledge derived by the if-checks above they are by no means the same. The left term of the first equation can wrap around (or it cannot, but then we need some concrete proof that it cannot), and the left term of the second equation obviously cannot (directly follows from the if statements before).
> I got it. Thanks for your proper comments.
> I may modify it like below in v3:
> ----------------------------------------
> STATIC
> EFI_STATUS
> CheckBufferAddr (
>    IN UINTN BufferAddr
>    )
> {
>    UINTN  BufferSize;
>    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_INVALID_PARAMETER;

Just another cosmetic thing I just noticed, why is this Invalid 
Parameter? The check above yields Access Denied when the buffer start is 
out of a trusted range, OK. The check below yields Access Denied when 
the buffer data extends beyond the trusted range, OK. What makes this 
check different from the other ones that it gets a different return 
code? I'm not sure on the policy of function documentation (i.e. whether 
one is needed), but what would the difference be in their descriptions?

>    }
>
>    // Find out the size of the buffer passed
>    BufferSize = ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength +
>      sizeof (EFI_MM_COMMUNICATE_HEADER);

Same issue as above, can also be solved by rewriting as subtraction. 
Because you now know that (CommBufferEnd - BufferAddr) >= sizeof 
(EFI_MM_COMMUNICATE_HEADER).

Best regards,
Marvin

>    // perform bounds check.
>    if ((CommBufferEnd - BufferAddr) < BufferSize) {
>      return EFI_ACCESS_DENIED;
>    }
>
>    return EFI_SUCCESS;
> }
> ----------------------------------------
>
> - Ming
>
>>>> Alternatively, you could not store the maximum buffer end but the maximum buffer size, so the additions of the buffer start would just vanish. This might be more readable too I think.
>>> As CommBufferAddr may be not at the begin of communicate buffer,
>>> so check size with the maximum buffer size is not enough.
>>>
>>>>> Status =  EFI_INVALID_PARAMETER;
>>>> Why is there no return here? This can proceed when the buffer cannot fit this header, and yet below the header is dereferenced.
>>> Modify it in v3.
>>>
>>>>> }
>>>>>
>>>>> // Find out the size of the buffer passed
>>>>> CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
>>>>> sizeof (EFI_MM_COMMUNICATE_HEADER);
>>>> Same wraparound concern, same suggestion for solving it.
>>>>
>>>>> // perform bounds check.
>>>>> if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
>>>>> Status =  EFI_ACCESS_DENIED;
>>>> It’s obviously not bad here, but for consistency’s sake, to mitigate bugs introduced by future changes, and readability, I’d return here and just return EFI_SUCCESS below, removing the code requirement of keeping Status consistent with the check results.
>>> Modify it in v3.
>> Thanks for the modifications.
>>
>> Best regards,
>> Marvin
>>
>>> - Ming
>>>
>>>> Finally, I really believe this kind of function should be abstracted in a way that it can be consumed by all places that accept any sort of communication buffer. Buffer validity checking is too critical than to duplicate it in every consumer.
>>>>
>>>> Thanks!
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>>> }
>>>>>
>>>>> return Status;
>>>>> }
>>>>>
>>>>> - Ming
>>>>>
>>>>>> STATIC
>>>>>> EFI_STATUS
>>>>>> CheckBufferAddr (
>>>>>> IN UINTN CommBufferAddr
>>>>>> )
>>>>>> {
>>>>>> UINTN                CommBufferSize;
>>>>>> EFI_STATUS           Status;
>>>>>> EFI_MMRAM_DESCRIPTOR CommBuffer;
>>>>>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>>>>>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>> CommBuffer = mSCommBuffer;
>>>>>> } else {
>>>>>> CommBuffer = mNsCommBuffer;
>>>>>> }
>>>>>> if (CommBufferAddr < CommBuffer.PhysicalStart) {
>>>>>> Status = EFI_ACCESS_DENIED;
>>>>>> }
>>>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>>>>>> 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 >=
>>>>>> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>>>>>> Status = EFI_ACCESS_DENIED;
>>>>>> }
>>>>>> return Status;
>>>>>> }
>>>>>> - Omkar
>>>>>>> /**
>>>>>>> The PI Standalone MM entry point for the TF-A CPU driver.
>>>>>>> @@ -104,25 +157,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,
>>>>>> Can this be changed to
>>>>>> &MmramRangesHob->Descriptor[1],
>>>>>> - Omkar
>>>>>>> +    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
>>>>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>>>> 

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

[Ming Huang]

在 12/24/21 9:52 PM, Marvin Häuser 写道:
> On 24.12.21 02:18, Ming Huang wrote:
>>
>> 在 12/23/21 7:05 PM, Marvin Häuser 写道:
>>> On 23.12.21 11:46, Ming Huang wrote:
>>>> 在 12/16/21 5:15 PM, Marvin Häuser 写道:
>>>>> Hey all,
>>>>>
>>>>>> On 15. Dec 2021, at 16:02, Ming Huang <huangming@linux.alibaba.com> wrote:
>>>>>>
>>>>>> 
>>>>>>
>>>>>>> On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
>>>>>>> Hi Ming,
>>>>>>> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
>>>>>>> Few comments mentioned inline.
>>>>>>> - Omkar
>>>>>>>> On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>>>>>>>> ++++++++++++++++----
>>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>>>>>>>> ++++++
>>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>>>>>>> 3 files changed, 79 insertions(+), 13 deletions(-)
>>>>>>>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>>>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>>>>>>>> {
>>>>>>>> STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>>>>>>> +STATIC
>>>>>>>> +EFI_STATUS
>>>>>>>> +CheckBufferAddr (
>>>>>>>> +  IN UINTN CommBufferAddr
>>>>>>>> +  )
>>>>>>>> +{
>>>>>>>> +  UINTN      CommBufferSize;
>>>>>>>> +  EFI_STATUS Status;
>>>>>>>> +
>>>>>>>> +  Status =  EFI_SUCCESS;
>>>>>>>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>>>> +  }
>>>>>>>> +
>>>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>>>> Single space after "Status = "
>>>>>> Modify it in v2.
>>>>>>
>>>>>>> - Omkar
>>>>>>>> +  }
>>>>>>>> +
>>>>>>>> +  // 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 >=
>>>>>>>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>>>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>>>> Single space after "Status = "
>>>>>> Modify it in v2.
>>>>>>
>>>>>>> - Omkar
>>>>>>>> +  }
>>>>>>>> +
>>>>>>>> +  if (!EFI_ERROR (Status)) {
>>>>>>> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
>>>>>>> - Omkar
>>>>>>>> +    return EFI_SUCCESS;
>>>>>>>> +  }
>>>>>>>> +
>>>>>>>> +  Status =  EFI_SUCCESS;
>>>>>>>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>>>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>>>> +  }
>>>>>>>> +
>>>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>>>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>>>>> +  }
>>>>>>>> +
>>>>>>>> +  // perform bounds check.
>>>>>>>> +  if (CommBufferAddr + CommBufferSize >=
>>>>>>>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>>>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>>>>> +  }
>>>>>>>> +
>>>>>>>> +  return Status;
>>>>>>>> +}
>>>>>>>> +
>>>>>>> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
>>>>>>> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.
>>>>>> Your example is a good idea to solve this case. I may modify it like below in v2:
>>>>>>
>>>>>> 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)) {
>>>>> I find it odd the check here (lesser-equals) is inconsistent with the check above (lesser). It’d be caught below anyway, but I’d change this to lesser to keep the return codes consistent.
>>>> Should be lesser, modify it in v3.
>>>>
>>>>>> CommBufferEnd = SCommBufferEnd;
>>>>>> } else {
>>>>>> return EFI_ACCESS_DENIED;
>>>>>> }
>>>>>>
>>>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
>>>>> Why is greater-equals used here? MessageLength == 0 is not filtered below, so this looks odd to be honest, as this is only the theoretical maximum buffer end.
>>>>>
>>>>> How do you know this cannot wraparound? I actually don’t think we do. We do know it holds that CommBufferAddr <= CommBufferEnd though, so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would give you that for free, if we assume the UINT64 variables above are actually bounded by UINTN, which seems reasonable - could ASSERT.
>>>> In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd)
>>>> is the same with: CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER)
>>> Okay, assume:
>>> CommBufferEnd = MAX_UINTN
>>> CommBufferAddr = MAX_UINTN - 1 (MAX_UINTN - 1 < MAX_UINTN, so the check above would pass)
>>> sizeof (EFI_MM_COMMUNICATE_HEADER) = 16 (should be accurate for 64-bit architectures)
>>>
>>> Then (assume implicit mod 2^N on both sides due to bounded integers!):
>>> (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) <=> ((MAX_UINTN - 1) + 16) >= MAX_UINTN <=> MAX_UINTN + 15 >= MAX_UINTN <=>(wraparound!!) 14 >= MAX_UINTN <=> FALSE
>>>
>>> And (assume implicit mod 2^N on both sides due to bounded integers!):
>>> CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) <=> MAX_UINTN - (MAX_UINTN - 1) < 16 <=> 1 < 16 <=> TRUE
>>>
>>> Due to wraparound semantics and the knowledge derived by the if-checks above they are by no means the same. The left term of the first equation can wrap around (or it cannot, but then we need some concrete proof that it cannot), and the left term of the second equation obviously cannot (directly follows from the if statements before).
>> I got it. Thanks for your proper comments.
>> I may modify it like below in v3:
>> ----------------------------------------
>> STATIC
>> EFI_STATUS
>> CheckBufferAddr (
>>    IN UINTN BufferAddr
>>    )
>> {
>>    UINTN  BufferSize;
>>    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_INVALID_PARAMETER;
> 
> Just another cosmetic thing I just noticed, why is this Invalid Parameter? The check above yields Access Denied when the buffer start is out of a trusted range, OK. The check below yields Access Denied when the buffer data extends beyond the trusted range, OK. What makes this check different from the other ones that it gets a different return code? I'm not sure on the policy of function documentation (i.e. whether one is needed), but what would the difference be in their descriptions?

Okay, EFI_ACCESS_DENIED should be return. Modify it in v3.

> 
>>    }
>>
>>    // Find out the size of the buffer passed
>>    BufferSize = ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength +
>>      sizeof (EFI_MM_COMMUNICATE_HEADER);
> 
> Same issue as above, can also be solved by rewriting as subtraction. Because you now know that (CommBufferEnd - BufferAddr) >= sizeof (EFI_MM_COMMUNICATE_HEADER).

Sorry, I haven't understood what you mean. Could you rewrite CheckBufferAddr() as a sample?

Thank you very much.
Merry Christmas!

- Ming

> 
> Best regards,
> Marvin
> 
>>    // perform bounds check.
>>    if ((CommBufferEnd - BufferAddr) < BufferSize) {
>>      return EFI_ACCESS_DENIED;
>>    }
>>
>>    return EFI_SUCCESS;
>> }
>> ----------------------------------------
>>
>> - Ming
>>
>>>>> Alternatively, you could not store the maximum buffer end but the maximum buffer size, so the additions of the buffer start would just vanish. This might be more readable too I think.
>>>> As CommBufferAddr may be not at the begin of communicate buffer,
>>>> so check size with the maximum buffer size is not enough.
>>>>
>>>>>> Status =  EFI_INVALID_PARAMETER;
>>>>> Why is there no return here? This can proceed when the buffer cannot fit this header, and yet below the header is dereferenced.
>>>> Modify it in v3.
>>>>
>>>>>> }
>>>>>>
>>>>>> // Find out the size of the buffer passed
>>>>>> CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
>>>>>> sizeof (EFI_MM_COMMUNICATE_HEADER);
>>>>> Same wraparound concern, same suggestion for solving it.
>>>>>
>>>>>> // perform bounds check.
>>>>>> if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
>>>>>> Status =  EFI_ACCESS_DENIED;
>>>>> It’s obviously not bad here, but for consistency’s sake, to mitigate bugs introduced by future changes, and readability, I’d return here and just return EFI_SUCCESS below, removing the code requirement of keeping Status consistent with the check results.
>>>> Modify it in v3.
>>> Thanks for the modifications.
>>>
>>> Best regards,
>>> Marvin
>>>
>>>> - Ming
>>>>
>>>>> Finally, I really believe this kind of function should be abstracted in a way that it can be consumed by all places that accept any sort of communication buffer. Buffer validity checking is too critical than to duplicate it in every consumer.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Best regards,
>>>>> Marvin
>>>>>
>>>>>> }
>>>>>>
>>>>>> return Status;
>>>>>> }
>>>>>>
>>>>>> - Ming
>>>>>>
>>>>>>> STATIC
>>>>>>> EFI_STATUS
>>>>>>> CheckBufferAddr (
>>>>>>> IN UINTN CommBufferAddr
>>>>>>> )
>>>>>>> {
>>>>>>> UINTN                CommBufferSize;
>>>>>>> EFI_STATUS           Status;
>>>>>>> EFI_MMRAM_DESCRIPTOR CommBuffer;
>>>>>>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>>>>>>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>>> CommBuffer = mSCommBuffer;
>>>>>>> } else {
>>>>>>> CommBuffer = mNsCommBuffer;
>>>>>>> }
>>>>>>> if (CommBufferAddr < CommBuffer.PhysicalStart) {
>>>>>>> Status = EFI_ACCESS_DENIED;
>>>>>>> }
>>>>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>>>>>>> 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 >=
>>>>>>> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>>>>>>> Status = EFI_ACCESS_DENIED;
>>>>>>> }
>>>>>>> return Status;
>>>>>>> }
>>>>>>> - Omkar
>>>>>>>> /**
>>>>>>>> The PI Standalone MM entry point for the TF-A CPU driver.
>>>>>>>> @@ -104,25 +157,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,
>>>>>>> Can this be changed to
>>>>>>> &MmramRangesHob->Descriptor[1],
>>>>>>> - Omkar
>>>>>>>> +    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
>>>>>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>>>>> 

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

[Marvin Häuser]

On 25.12.21 03:09, Ming Huang wrote:
>
> 在 12/24/21 9:52 PM, Marvin Häuser 写道:
>> On 24.12.21 02:18, Ming Huang wrote:
>>> 在 12/23/21 7:05 PM, Marvin Häuser 写道:
>>>> On 23.12.21 11:46, Ming Huang wrote:
>>>>> 在 12/16/21 5:15 PM, Marvin Häuser 写道:
>>>>>> Hey all,
>>>>>>
>>>>>>> On 15. Dec 2021, at 16:02, Ming Huang <huangming@linux.alibaba.com> wrote:
>>>>>>>
>>>>>>> 
>>>>>>>
>>>>>>>> On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
>>>>>>>> Hi Ming,
>>>>>>>> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
>>>>>>>> Few comments mentioned inline.
>>>>>>>> - Omkar
>>>>>>>>> On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>>>>>>>>> ++++++++++++++++----
>>>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>>>>>>>>> ++++++
>>>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>>>>>>>> 3 files changed, 79 insertions(+), 13 deletions(-)
>>>>>>>>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>>>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>>>>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>>>>>>>>> {
>>>>>>>>> STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>>>>>>>> +STATIC
>>>>>>>>> +EFI_STATUS
>>>>>>>>> +CheckBufferAddr (
>>>>>>>>> +  IN UINTN CommBufferAddr
>>>>>>>>> +  )
>>>>>>>>> +{
>>>>>>>>> +  UINTN      CommBufferSize;
>>>>>>>>> +  EFI_STATUS Status;
>>>>>>>>> +
>>>>>>>>> +  Status =  EFI_SUCCESS;
>>>>>>>>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>>>>> +  }
>>>>>>>>> +
>>>>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>>>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>>>>> Single space after "Status = "
>>>>>>> Modify it in v2.
>>>>>>>
>>>>>>>> - Omkar
>>>>>>>>> +  }
>>>>>>>>> +
>>>>>>>>> +  // 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 >=
>>>>>>>>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>>>>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>>>>> Single space after "Status = "
>>>>>>> Modify it in v2.
>>>>>>>
>>>>>>>> - Omkar
>>>>>>>>> +  }
>>>>>>>>> +
>>>>>>>>> +  if (!EFI_ERROR (Status)) {
>>>>>>>> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
>>>>>>>> - Omkar
>>>>>>>>> +    return EFI_SUCCESS;
>>>>>>>>> +  }
>>>>>>>>> +
>>>>>>>>> +  Status =  EFI_SUCCESS;
>>>>>>>>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>>>>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>>>>> +  }
>>>>>>>>> +
>>>>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>>>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>>>>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>>>>>> +  }
>>>>>>>>> +
>>>>>>>>> +  // perform bounds check.
>>>>>>>>> +  if (CommBufferAddr + CommBufferSize >=
>>>>>>>>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>>>>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>>>>>> +  }
>>>>>>>>> +
>>>>>>>>> +  return Status;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
>>>>>>>> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.
>>>>>>> Your example is a good idea to solve this case. I may modify it like below in v2:
>>>>>>>
>>>>>>> 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)) {
>>>>>> I find it odd the check here (lesser-equals) is inconsistent with the check above (lesser). It’d be caught below anyway, but I’d change this to lesser to keep the return codes consistent.
>>>>> Should be lesser, modify it in v3.
>>>>>
>>>>>>> CommBufferEnd = SCommBufferEnd;
>>>>>>> } else {
>>>>>>> return EFI_ACCESS_DENIED;
>>>>>>> }
>>>>>>>
>>>>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
>>>>>> Why is greater-equals used here? MessageLength == 0 is not filtered below, so this looks odd to be honest, as this is only the theoretical maximum buffer end.
>>>>>>
>>>>>> How do you know this cannot wraparound? I actually don’t think we do. We do know it holds that CommBufferAddr <= CommBufferEnd though, so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would give you that for free, if we assume the UINT64 variables above are actually bounded by UINTN, which seems reasonable - could ASSERT.
>>>>> In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd)
>>>>> is the same with: CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER)
>>>> Okay, assume:
>>>> CommBufferEnd = MAX_UINTN
>>>> CommBufferAddr = MAX_UINTN - 1 (MAX_UINTN - 1 < MAX_UINTN, so the check above would pass)
>>>> sizeof (EFI_MM_COMMUNICATE_HEADER) = 16 (should be accurate for 64-bit architectures)
>>>>
>>>> Then (assume implicit mod 2^N on both sides due to bounded integers!):
>>>> (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) <=> ((MAX_UINTN - 1) + 16) >= MAX_UINTN <=> MAX_UINTN + 15 >= MAX_UINTN <=>(wraparound!!) 14 >= MAX_UINTN <=> FALSE
>>>>
>>>> And (assume implicit mod 2^N on both sides due to bounded integers!):
>>>> CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) <=> MAX_UINTN - (MAX_UINTN - 1) < 16 <=> 1 < 16 <=> TRUE
>>>>
>>>> Due to wraparound semantics and the knowledge derived by the if-checks above they are by no means the same. The left term of the first equation can wrap around (or it cannot, but then we need some concrete proof that it cannot), and the left term of the second equation obviously cannot (directly follows from the if statements before).
>>> I got it. Thanks for your proper comments.
>>> I may modify it like below in v3:
>>> ----------------------------------------
>>> STATIC
>>> EFI_STATUS
>>> CheckBufferAddr (
>>>     IN UINTN BufferAddr
>>>     )
>>> {
>>>     UINTN  BufferSize;
>>>     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_INVALID_PARAMETER;
>> Just another cosmetic thing I just noticed, why is this Invalid Parameter? The check above yields Access Denied when the buffer start is out of a trusted range, OK. The check below yields Access Denied when the buffer data extends beyond the trusted range, OK. What makes this check different from the other ones that it gets a different return code? I'm not sure on the policy of function documentation (i.e. whether one is needed), but what would the difference be in their descriptions?
> Okay, EFI_ACCESS_DENIED should be return. Modify it in v3.
>
>>>     }
>>>
>>>     // Find out the size of the buffer passed
>>>     BufferSize = ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength +
>>>       sizeof (EFI_MM_COMMUNICATE_HEADER);
>> Same issue as above, can also be solved by rewriting as subtraction. Because you now know that (CommBufferEnd - BufferAddr) >= sizeof (EFI_MM_COMMUNICATE_HEADER).
> Sorry, I haven't understood what you mean. Could you rewrite CheckBufferAddr() as a sample?

CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER) < ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength

Best regards,
Marvin


>
> Thank you very much.
> Merry Christmas!
>
> - Ming
>
>> Best regards,
>> Marvin
>>
>>>     // perform bounds check.
>>>     if ((CommBufferEnd - BufferAddr) < BufferSize) {
>>>       return EFI_ACCESS_DENIED;
>>>     }
>>>
>>>     return EFI_SUCCESS;
>>> }
>>> ----------------------------------------
>>>
>>> - Ming
>>>
>>>>>> Alternatively, you could not store the maximum buffer end but the maximum buffer size, so the additions of the buffer start would just vanish. This might be more readable too I think.
>>>>> As CommBufferAddr may be not at the begin of communicate buffer,
>>>>> so check size with the maximum buffer size is not enough.
>>>>>
>>>>>>> Status =  EFI_INVALID_PARAMETER;
>>>>>> Why is there no return here? This can proceed when the buffer cannot fit this header, and yet below the header is dereferenced.
>>>>> Modify it in v3.
>>>>>
>>>>>>> }
>>>>>>>
>>>>>>> // Find out the size of the buffer passed
>>>>>>> CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
>>>>>>> sizeof (EFI_MM_COMMUNICATE_HEADER);
>>>>>> Same wraparound concern, same suggestion for solving it.
>>>>>>
>>>>>>> // perform bounds check.
>>>>>>> if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
>>>>>>> Status =  EFI_ACCESS_DENIED;
>>>>>> It’s obviously not bad here, but for consistency’s sake, to mitigate bugs introduced by future changes, and readability, I’d return here and just return EFI_SUCCESS below, removing the code requirement of keeping Status consistent with the check results.
>>>>> Modify it in v3.
>>>> Thanks for the modifications.
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>>> - Ming
>>>>>
>>>>>> Finally, I really believe this kind of function should be abstracted in a way that it can be consumed by all places that accept any sort of communication buffer. Buffer validity checking is too critical than to duplicate it in every consumer.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> Best regards,
>>>>>> Marvin
>>>>>>
>>>>>>> }
>>>>>>>
>>>>>>> return Status;
>>>>>>> }
>>>>>>>
>>>>>>> - Ming
>>>>>>>
>>>>>>>> STATIC
>>>>>>>> EFI_STATUS
>>>>>>>> CheckBufferAddr (
>>>>>>>> IN UINTN CommBufferAddr
>>>>>>>> )
>>>>>>>> {
>>>>>>>> UINTN                CommBufferSize;
>>>>>>>> EFI_STATUS           Status;
>>>>>>>> EFI_MMRAM_DESCRIPTOR CommBuffer;
>>>>>>>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>>>>>>>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>>>> CommBuffer = mSCommBuffer;
>>>>>>>> } else {
>>>>>>>> CommBuffer = mNsCommBuffer;
>>>>>>>> }
>>>>>>>> if (CommBufferAddr < CommBuffer.PhysicalStart) {
>>>>>>>> Status = EFI_ACCESS_DENIED;
>>>>>>>> }
>>>>>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>>> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>>>>>>>> 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 >=
>>>>>>>> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>>>>>>>> Status = EFI_ACCESS_DENIED;
>>>>>>>> }
>>>>>>>> return Status;
>>>>>>>> }
>>>>>>>> - Omkar
>>>>>>>>> /**
>>>>>>>>> The PI Standalone MM entry point for the TF-A CPU driver.
>>>>>>>>> @@ -104,25 +157,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,
>>>>>>>> Can this be changed to
>>>>>>>> &MmramRangesHob->Descriptor[1],
>>>>>>>> - Omkar
>>>>>>>>> +    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
>>>>>>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>>>>>> 

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

[Ming Huang]

在 12/30/21 8:27 PM, Marvin Häuser 写道:
> 
> 
> On 25.12.21 03:09, Ming Huang wrote:
>>
>> 在 12/24/21 9:52 PM, Marvin Häuser 写道:
>>> On 24.12.21 02:18, Ming Huang wrote:
>>>> 在 12/23/21 7:05 PM, Marvin Häuser 写道:
>>>>> On 23.12.21 11:46, Ming Huang wrote:
>>>>>> 在 12/16/21 5:15 PM, Marvin Häuser 写道:
>>>>>>> Hey all,
>>>>>>>
>>>>>>>> On 15. Dec 2021, at 16:02, Ming Huang <huangming@linux.alibaba.com> wrote:
>>>>>>>>
>>>>>>>> 
>>>>>>>>
>>>>>>>>> On 12/9/21 1:46 AM, Omkar Anand Kulkarni wrote:
>>>>>>>>> Hi Ming,
>>>>>>>>> Thanks for this patch. This patch helps to resolve Standalone MM issue while exercising RAS use case.
>>>>>>>>> Few comments mentioned inline.
>>>>>>>>> - Omkar
>>>>>>>>>> On 10/15/21 2:39 PM, Ming Huang via groups.io 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     | 70
>>>>>>>>>> ++++++++++++++++----
>>>>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21
>>>>>>>>>> ++++++
>>>>>>>>>> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h |  1 +
>>>>>>>>>> 3 files changed, 79 insertions(+), 13 deletions(-)
>>>>>>>>>> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>>>>>> b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
>>>>>>>>>> index 5dfaf9d751..63fab1bd78 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,58 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig =
>>>>>>>>>> {
>>>>>>>>>> STATIC EFI_MM_ENTRY_POINT     mMmEntryPoint = NULL;
>>>>>>>>>> +STATIC
>>>>>>>>>> +EFI_STATUS
>>>>>>>>>> +CheckBufferAddr (
>>>>>>>>>> +  IN UINTN CommBufferAddr
>>>>>>>>>> +  )
>>>>>>>>>> +{
>>>>>>>>>> +  UINTN      CommBufferSize;
>>>>>>>>>> +  EFI_STATUS Status;
>>>>>>>>>> +
>>>>>>>>>> +  Status =  EFI_SUCCESS;
>>>>>>>>>> +  if (CommBufferAddr < mNsCommBuffer.PhysicalStart) {
>>>>>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>>>>>> +  }
>>>>>>>>>> +
>>>>>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>>>>> +      (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>>>>>> Single space after "Status = "
>>>>>>>> Modify it in v2.
>>>>>>>>
>>>>>>>>> - Omkar
>>>>>>>>>> +  }
>>>>>>>>>> +
>>>>>>>>>> +  // 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 >=
>>>>>>>>>> +      mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
>>>>>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>>>>>> Single space after "Status = "
>>>>>>>> Modify it in v2.
>>>>>>>>
>>>>>>>>> - Omkar
>>>>>>>>>> +  }
>>>>>>>>>> +
>>>>>>>>>> +  if (!EFI_ERROR (Status)) {
>>>>>>>>> In case of error this function call will not return from here. It will execute the code below comparing the MM Communicate buffer address with the Secure buffer address, which may cause wrong return type being returned. Can you check this, please?
>>>>>>>>> - Omkar
>>>>>>>>>> +    return EFI_SUCCESS;
>>>>>>>>>> +  }
>>>>>>>>>> +
>>>>>>>>>> +  Status =  EFI_SUCCESS;
>>>>>>>>>> +  if (CommBufferAddr < mSCommBuffer.PhysicalStart) {
>>>>>>>>>> +    Status = EFI_ACCESS_DENIED;
>>>>>>>>>> +  }
>>>>>>>>>> +
>>>>>>>>>> +  if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>>>>> +      (mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize)) {
>>>>>>>>>> +    Status =  EFI_INVALID_PARAMETER;
>>>>>>>>>> +  }
>>>>>>>>>> +
>>>>>>>>>> +  // perform bounds check.
>>>>>>>>>> +  if (CommBufferAddr + CommBufferSize >=
>>>>>>>>>> +      mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize) {
>>>>>>>>>> +    Status =  EFI_ACCESS_DENIED;
>>>>>>>>>> +  }
>>>>>>>>>> +
>>>>>>>>>> +  return Status;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>> CheckBufferAddr() function performs validity and overflow checks on the Communication buffers. These checks are same for both the non-secure
>>>>>>>>> MM communicate buffer and secure buffer shared between EL3 and S-EL0. Can this code be combined ( example below)? This will help mitigate the above mentioned return type issue as well.
>>>>>>>> Your example is a good idea to solve this case. I may modify it like below in v2:
>>>>>>>>
>>>>>>>> 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)) {
>>>>>>> I find it odd the check here (lesser-equals) is inconsistent with the check above (lesser). It’d be caught below anyway, but I’d change this to lesser to keep the return codes consistent.
>>>>>> Should be lesser, modify it in v3.
>>>>>>
>>>>>>>> CommBufferEnd = SCommBufferEnd;
>>>>>>>> } else {
>>>>>>>> return EFI_ACCESS_DENIED;
>>>>>>>> }
>>>>>>>>
>>>>>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) {
>>>>>>> Why is greater-equals used here? MessageLength == 0 is not filtered below, so this looks odd to be honest, as this is only the theoretical maximum buffer end.
>>>>>>>
>>>>>>> How do you know this cannot wraparound? I actually don’t think we do. We do know it holds that CommBufferAddr <= CommBufferEnd though, so checking CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) would give you that for free, if we assume the UINT64 variables above are actually bounded by UINTN, which seems reasonable - could ASSERT.
>>>>>> In my mind, (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd)
>>>>>> is the same with: CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER)
>>>>> Okay, assume:
>>>>> CommBufferEnd = MAX_UINTN
>>>>> CommBufferAddr = MAX_UINTN - 1 (MAX_UINTN - 1 < MAX_UINTN, so the check above would pass)
>>>>> sizeof (EFI_MM_COMMUNICATE_HEADER) = 16 (should be accurate for 64-bit architectures)
>>>>>
>>>>> Then (assume implicit mod 2^N on both sides due to bounded integers!):
>>>>> (CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >= CommBufferEnd) <=> ((MAX_UINTN - 1) + 16) >= MAX_UINTN <=> MAX_UINTN + 15 >= MAX_UINTN <=>(wraparound!!) 14 >= MAX_UINTN <=> FALSE
>>>>>
>>>>> And (assume implicit mod 2^N on both sides due to bounded integers!):
>>>>> CommBufferEnd - CommBufferAddr < sizeof (EFI_MM_COMMUNICATE_HEADER) <=> MAX_UINTN - (MAX_UINTN - 1) < 16 <=> 1 < 16 <=> TRUE
>>>>>
>>>>> Due to wraparound semantics and the knowledge derived by the if-checks above they are by no means the same. The left term of the first equation can wrap around (or it cannot, but then we need some concrete proof that it cannot), and the left term of the second equation obviously cannot (directly follows from the if statements before).
>>>> I got it. Thanks for your proper comments.
>>>> I may modify it like below in v3:
>>>> ----------------------------------------
>>>> STATIC
>>>> EFI_STATUS
>>>> CheckBufferAddr (
>>>>     IN UINTN BufferAddr
>>>>     )
>>>> {
>>>>     UINTN  BufferSize;
>>>>     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_INVALID_PARAMETER;
>>> Just another cosmetic thing I just noticed, why is this Invalid Parameter? The check above yields Access Denied when the buffer start is out of a trusted range, OK. The check below yields Access Denied when the buffer data extends beyond the trusted range, OK. What makes this check different from the other ones that it gets a different return code? I'm not sure on the policy of function documentation (i.e. whether one is needed), but what would the difference be in their descriptions?
>> Okay, EFI_ACCESS_DENIED should be return. Modify it in v3.
>>
>>>>     }
>>>>
>>>>     // Find out the size of the buffer passed
>>>>     BufferSize = ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength +
>>>>       sizeof (EFI_MM_COMMUNICATE_HEADER);
>>> Same issue as above, can also be solved by rewriting as subtraction. Because you now know that (CommBufferEnd - BufferAddr) >= sizeof (EFI_MM_COMMUNICATE_HEADER).
>> Sorry, I haven't understood what you mean. Could you rewrite CheckBufferAddr() as a sample?
> 
> CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER) < ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength

Modify it in v3.
Thanks for your help.

- Ming

> 
> Best regards,
> Marvin
> 
> 
>>
>> Thank you very much.
>> Merry Christmas!
>>
>> - Ming
>>
>>> Best regards,
>>> Marvin
>>>
>>>>     // perform bounds check.
>>>>     if ((CommBufferEnd - BufferAddr) < BufferSize) {
>>>>       return EFI_ACCESS_DENIED;
>>>>     }
>>>>
>>>>     return EFI_SUCCESS;
>>>> }
>>>> ----------------------------------------
>>>>
>>>> - Ming
>>>>
>>>>>>> Alternatively, you could not store the maximum buffer end but the maximum buffer size, so the additions of the buffer start would just vanish. This might be more readable too I think.
>>>>>> As CommBufferAddr may be not at the begin of communicate buffer,
>>>>>> so check size with the maximum buffer size is not enough.
>>>>>>
>>>>>>>> Status =  EFI_INVALID_PARAMETER;
>>>>>>> Why is there no return here? This can proceed when the buffer cannot fit this header, and yet below the header is dereferenced.
>>>>>> Modify it in v3.
>>>>>>
>>>>>>>> }
>>>>>>>>
>>>>>>>> // Find out the size of the buffer passed
>>>>>>>> CommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) CommBufferAddr)->MessageLength +
>>>>>>>> sizeof (EFI_MM_COMMUNICATE_HEADER);
>>>>>>> Same wraparound concern, same suggestion for solving it.
>>>>>>>
>>>>>>>> // perform bounds check.
>>>>>>>> if (CommBufferAddr + CommBufferSize >= CommBufferEnd) {
>>>>>>>> Status =  EFI_ACCESS_DENIED;
>>>>>>> It’s obviously not bad here, but for consistency’s sake, to mitigate bugs introduced by future changes, and readability, I’d return here and just return EFI_SUCCESS below, removing the code requirement of keeping Status consistent with the check results.
>>>>>> Modify it in v3.
>>>>> Thanks for the modifications.
>>>>>
>>>>> Best regards,
>>>>> Marvin
>>>>>
>>>>>> - Ming
>>>>>>
>>>>>>> Finally, I really believe this kind of function should be abstracted in a way that it can be consumed by all places that accept any sort of communication buffer. Buffer validity checking is too critical than to duplicate it in every consumer.
>>>>>>>
>>>>>>> Thanks!
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Marvin
>>>>>>>
>>>>>>>> }
>>>>>>>>
>>>>>>>> return Status;
>>>>>>>> }
>>>>>>>>
>>>>>>>> - Ming
>>>>>>>>
>>>>>>>>> STATIC
>>>>>>>>> EFI_STATUS
>>>>>>>>> CheckBufferAddr (
>>>>>>>>> IN UINTN CommBufferAddr
>>>>>>>>> )
>>>>>>>>> {
>>>>>>>>> UINTN                CommBufferSize;
>>>>>>>>> EFI_STATUS           Status;
>>>>>>>>> EFI_MMRAM_DESCRIPTOR CommBuffer;
>>>>>>>>> if (CommBufferAddr < mNsCommBuffer.PhysicalStart ||
>>>>>>>>> CommBufferAddr > (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
>>>>>>>>> CommBuffer = mSCommBuffer;
>>>>>>>>> } else {
>>>>>>>>> CommBuffer = mNsCommBuffer;
>>>>>>>>> }
>>>>>>>>> if (CommBufferAddr < CommBuffer.PhysicalStart) {
>>>>>>>>> Status = EFI_ACCESS_DENIED;
>>>>>>>>> }
>>>>>>>>> if ((CommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
>>>>>>>>> (CommBuffer.PhysicalStart + CommBuffer.PhysicalSize)) {
>>>>>>>>> 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 >=
>>>>>>>>> CommBuffer.PhysicalStart + CommBuffer.PhysicalSize) {
>>>>>>>>> Status = EFI_ACCESS_DENIED;
>>>>>>>>> }
>>>>>>>>> return Status;
>>>>>>>>> }
>>>>>>>>> - Omkar
>>>>>>>>>> /**
>>>>>>>>>> The PI Standalone MM entry point for the TF-A CPU driver.
>>>>>>>>>> @@ -104,25 +157,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,
>>>>>>>>> Can this be changed to
>>>>>>>>> &MmramRangesHob->Descriptor[1],
>>>>>>>>> - Omkar
>>>>>>>>>> +    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
>>>>>>>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>>>>>>>
> 
> 
> 
> 
>