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


Ming Huang
 

There are two scene communicate with StandaloneMm(MM):
1 edk2 -> TF-A -> MM, communicate MM use non-secure buffer which
specify by EFI_SECURE_PARTITION_BOOT_INFO.SpNsCommBufBase;
2 RAS scene: fiq -> TF-A -> MM, use secure buffer which
specify by EFI_SECURE_PARTITION_BOOT_INFO.SpShareBufBase;

For now, the second scene will failed because check buffer address.
This patch add CheckBufferAddr() to support check address for secure
buffer.

Signed-off-by: Ming Huang <huangming@...>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 54 +++++++++++++++-----
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 ++++++++
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 +
3 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 5dfaf9d751..d0ba415671 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
@@ -50,6 +50,7 @@ EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext = NULL;

// Descriptor with whereabouts of memory used for communication with the normal world
EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
+EFI_MMRAM_DESCRIPTOR mSCommBuffer;

MP_INFORMATION_HOB_DATA *mMpInformationHobData;

@@ -60,6 +61,42 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {

STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL;

+STATIC
+EFI_STATUS
+CheckBufferAddr (
+ IN UINTN BufferAddr
+ )
+{
+ UINT64 NsCommBufferEnd;
+ UINT64 SCommBufferEnd;
+ UINT64 CommBufferEnd;
+
+ NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
+ SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
+
+ if ((BufferAddr >= mNsCommBuffer.PhysicalStart) &&
+ (BufferAddr < NsCommBufferEnd)) {
+ CommBufferEnd = NsCommBufferEnd;
+ } else if ((BufferAddr >= mSCommBuffer.PhysicalStart) &&
+ (BufferAddr < SCommBufferEnd)) {
+ CommBufferEnd = SCommBufferEnd;
+ } else {
+ return EFI_ACCESS_DENIED;
+ }
+
+ if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNICATE_HEADER)) {
+ return EFI_ACCESS_DENIED;
+ }
+
+ // perform bounds check.
+ if ((CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER)) <
+ ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength) {
+ return EFI_ACCESS_DENIED;
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
The PI Standalone MM entry point for the TF-A CPU driver.

@@ -104,25 +141,16 @@ PiMmStandaloneArmTfCpuDriverEntry (
return EFI_INVALID_PARAMETER;
}

- if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
- return EFI_ACCESS_DENIED;
- }
-
- if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
- (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
- return EFI_INVALID_PARAMETER;
+ Status = CheckBufferAddr (NsCommBufferAddr);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status));
+ return Status;
}

// Find out the size of the buffer passed
NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) NsCommBufferAddr)->MessageLength +
sizeof (EFI_MM_COMMUNICATE_HEADER);

- // perform bounds check.
- if (NsCommBufferAddr + NsCommBufferSize >=
- mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
- return EFI_ACCESS_DENIED;
- }
-
GuidedEventContext = NULL;
// Now that the secure world can see the normal world buffer, allocate
// memory to copy the communication buffer to the secure world.
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
index fd9c59b4da..96dad20dd1 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
@@ -107,6 +107,7 @@ StandaloneMmCpuInitialize (
UINTN Index;
UINTN ArraySize;
VOID *HobStart;
+ EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob;

ASSERT (SystemTable != NULL);
mMmst = SystemTable;
@@ -186,6 +187,26 @@ StandaloneMmCpuInitialize (
CopyMem (&mNsCommBuffer, NsCommBufMmramRange, sizeof(EFI_MMRAM_DESCRIPTOR));
DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));

+ Status = GetGuidedHobData (
+ HobStart,
+ &gEfiMmPeiMmramMemoryReserveGuid,
+ (VOID **) &MmramRangesHob
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "MmramRangesHob data extraction failed - 0x%x\n", Status));
+ return Status;
+ }
+
+ //
+ // As CreateHobListFromBootInfo(), the base and size of buffer shared with
+ // privileged Secure world software is in second one.
+ //
+ CopyMem (
+ &mSCommBuffer,
+ &MmramRangesHob->Descriptor[0] + 1,
+ sizeof(EFI_MMRAM_DESCRIPTOR)
+ );
+
//
// Extract the MP information from the Hoblist
//
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
index 2c96439c15..2e03b20d85 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
@@ -30,6 +30,7 @@ extern EFI_MM_CPU_PROTOCOL mMmCpuState;
//
extern EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext;
extern EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
+extern EFI_MMRAM_DESCRIPTOR mSCommBuffer;
extern MP_INFORMATION_HOB_DATA *mMpInformationHobData;
extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig;

--
2.17.1


Ming Huang
 

Hi,

Any comment about this series?

在 12/31/21 7:06 PM, Ming Huang 写道:

There are two scene communicate with StandaloneMm(MM):
1 edk2 -> TF-A -> MM, communicate MM use non-secure buffer which
specify by EFI_SECURE_PARTITION_BOOT_INFO.SpNsCommBufBase;
2 RAS scene: fiq -> TF-A -> MM, use secure buffer which
specify by EFI_SECURE_PARTITION_BOOT_INFO.SpShareBufBase;

For now, the second scene will failed because check buffer address.
This patch add CheckBufferAddr() to support check address for secure
buffer.

Signed-off-by: Ming Huang <huangming@...>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 54 +++++++++++++++-----
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 ++++++++
StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 +
3 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
index 5dfaf9d751..d0ba415671 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
@@ -50,6 +50,7 @@ EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext = NULL;

// Descriptor with whereabouts of memory used for communication with the normal world
EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
+EFI_MMRAM_DESCRIPTOR mSCommBuffer;

MP_INFORMATION_HOB_DATA *mMpInformationHobData;

@@ -60,6 +61,42 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {

STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL;

+STATIC
+EFI_STATUS
+CheckBufferAddr (
+ IN UINTN BufferAddr
+ )
+{
+ UINT64 NsCommBufferEnd;
+ UINT64 SCommBufferEnd;
+ UINT64 CommBufferEnd;
+
+ NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
+ SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
+
+ if ((BufferAddr >= mNsCommBuffer.PhysicalStart) &&
+ (BufferAddr < NsCommBufferEnd)) {
+ CommBufferEnd = NsCommBufferEnd;
+ } else if ((BufferAddr >= mSCommBuffer.PhysicalStart) &&
+ (BufferAddr < SCommBufferEnd)) {
+ CommBufferEnd = SCommBufferEnd;
+ } else {
+ return EFI_ACCESS_DENIED;
+ }
+
+ if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNICATE_HEADER)) {
+ return EFI_ACCESS_DENIED;
+ }
+
+ // perform bounds check.
+ if ((CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER)) <
+ ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength) {
+ return EFI_ACCESS_DENIED;
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
The PI Standalone MM entry point for the TF-A CPU driver.

@@ -104,25 +141,16 @@ PiMmStandaloneArmTfCpuDriverEntry (
return EFI_INVALID_PARAMETER;
}

- if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
- return EFI_ACCESS_DENIED;
- }
-
- if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
- (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
- return EFI_INVALID_PARAMETER;
+ Status = CheckBufferAddr (NsCommBufferAddr);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status));
+ return Status;
}

// Find out the size of the buffer passed
NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) NsCommBufferAddr)->MessageLength +
sizeof (EFI_MM_COMMUNICATE_HEADER);

- // perform bounds check.
- if (NsCommBufferAddr + NsCommBufferSize >=
- mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
- return EFI_ACCESS_DENIED;
- }
-
GuidedEventContext = NULL;
// Now that the secure world can see the normal world buffer, allocate
// memory to copy the communication buffer to the secure world.
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
index fd9c59b4da..96dad20dd1 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
@@ -107,6 +107,7 @@ StandaloneMmCpuInitialize (
UINTN Index;
UINTN ArraySize;
VOID *HobStart;
+ EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob;

ASSERT (SystemTable != NULL);
mMmst = SystemTable;
@@ -186,6 +187,26 @@ StandaloneMmCpuInitialize (
CopyMem (&mNsCommBuffer, NsCommBufMmramRange, sizeof(EFI_MMRAM_DESCRIPTOR));
DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));

+ Status = GetGuidedHobData (
+ HobStart,
+ &gEfiMmPeiMmramMemoryReserveGuid,
+ (VOID **) &MmramRangesHob
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "MmramRangesHob data extraction failed - 0x%x\n", Status));
+ return Status;
+ }
+
+ //
+ // As CreateHobListFromBootInfo(), the base and size of buffer shared with
+ // privileged Secure world software is in second one.
+ //
+ CopyMem (
+ &mSCommBuffer,
+ &MmramRangesHob->Descriptor[0] + 1,
+ sizeof(EFI_MMRAM_DESCRIPTOR)
+ );
+
//
// Extract the MP information from the Hoblist
//
diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
index 2c96439c15..2e03b20d85 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
@@ -30,6 +30,7 @@ extern EFI_MM_CPU_PROTOCOL mMmCpuState;
//
extern EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext;
extern EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
+extern EFI_MMRAM_DESCRIPTOR mSCommBuffer;
extern MP_INFORMATION_HOB_DATA *mMpInformationHobData;
extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig;


Sami Mujawar
 

Hi Ming,

I am not sure if this is an issue at my end, but I cannot apply this patch series, can you check, please?
Also, is it possible to share these patches on a Github branch.

Regards,

Sami Mujawar

On 30/03/2022, 10:37, "Ming Huang" <huangming@...> wrote:

Hi,

Any comment about this series?

在 12/31/21 7:06 PM, Ming Huang 写道:

There are two scene communicate with StandaloneMm(MM):
> 1 edk2 -> TF-A -> MM, communicate MM use non-secure buffer which
> specify by EFI_SECURE_PARTITION_BOOT_INFO.SpNsCommBufBase;
> 2 RAS scene: fiq -> TF-A -> MM, use secure buffer which
> specify by EFI_SECURE_PARTITION_BOOT_INFO.SpShareBufBase;
>
> For now, the second scene will failed because check buffer address.
> This patch add CheckBufferAddr() to support check address for secure
> buffer.
>
> Signed-off-by: Ming Huang <huangming@...>
> ---
> StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 54 +++++++++++++++-----
> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 ++++++++
> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 +
> 3 files changed, 63 insertions(+), 13 deletions(-)
>
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> index 5dfaf9d751..d0ba415671 100644
> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> @@ -50,6 +50,7 @@ EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext = NULL;
>
> // Descriptor with whereabouts of memory used for communication with the normal world
> EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
> +EFI_MMRAM_DESCRIPTOR mSCommBuffer;
>
> MP_INFORMATION_HOB_DATA *mMpInformationHobData;
>
> @@ -60,6 +61,42 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {
>
> STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL;
>
> +STATIC
> +EFI_STATUS
> +CheckBufferAddr (
> + IN UINTN BufferAddr
> + )
> +{
> + UINT64 NsCommBufferEnd;
> + UINT64 SCommBufferEnd;
> + UINT64 CommBufferEnd;
> +
> + NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
> + SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
> +
> + if ((BufferAddr >= mNsCommBuffer.PhysicalStart) &&
> + (BufferAddr < NsCommBufferEnd)) {
> + CommBufferEnd = NsCommBufferEnd;
> + } else if ((BufferAddr >= mSCommBuffer.PhysicalStart) &&
> + (BufferAddr < SCommBufferEnd)) {
> + CommBufferEnd = SCommBufferEnd;
> + } else {
> + return EFI_ACCESS_DENIED;
> + }
> +
> + if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNICATE_HEADER)) {
> + return EFI_ACCESS_DENIED;
> + }
> +
> + // perform bounds check.
> + if ((CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER)) <
> + ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength) {
> + return EFI_ACCESS_DENIED;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> The PI Standalone MM entry point for the TF-A CPU driver.
>
> @@ -104,25 +141,16 @@ PiMmStandaloneArmTfCpuDriverEntry (
> return EFI_INVALID_PARAMETER;
> }
>
> - if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
> - return EFI_ACCESS_DENIED;
> - }
> -
> - if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
> - (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
> - return EFI_INVALID_PARAMETER;
> + Status = CheckBufferAddr (NsCommBufferAddr);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status));
> + return Status;
> }
>
> // Find out the size of the buffer passed
> NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) NsCommBufferAddr)->MessageLength +
> sizeof (EFI_MM_COMMUNICATE_HEADER);
>
> - // perform bounds check.
> - if (NsCommBufferAddr + NsCommBufferSize >=
> - mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
> - return EFI_ACCESS_DENIED;
> - }
> -
> GuidedEventContext = NULL;
> // Now that the secure world can see the normal world buffer, allocate
> // memory to copy the communication buffer to the secure world.
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
> index fd9c59b4da..96dad20dd1 100644
> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
> @@ -107,6 +107,7 @@ StandaloneMmCpuInitialize (
> UINTN Index;
> UINTN ArraySize;
> VOID *HobStart;
> + EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob;
>
> ASSERT (SystemTable != NULL);
> mMmst = SystemTable;
> @@ -186,6 +187,26 @@ StandaloneMmCpuInitialize (
> CopyMem (&mNsCommBuffer, NsCommBufMmramRange, sizeof(EFI_MMRAM_DESCRIPTOR));
> DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));
>
> + Status = GetGuidedHobData (
> + HobStart,
> + &gEfiMmPeiMmramMemoryReserveGuid,
> + (VOID **) &MmramRangesHob
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "MmramRangesHob data extraction failed - 0x%x\n", Status));
> + return Status;
> + }
> +
> + //
> + // As CreateHobListFromBootInfo(), the base and size of buffer shared with
> + // privileged Secure world software is in second one.
> + //
> + CopyMem (
> + &mSCommBuffer,
> + &MmramRangesHob->Descriptor[0] + 1,
> + sizeof(EFI_MMRAM_DESCRIPTOR)
> + );
> +
> //
> // Extract the MP information from the Hoblist
> //
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
> index 2c96439c15..2e03b20d85 100644
> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
> @@ -30,6 +30,7 @@ extern EFI_MM_CPU_PROTOCOL mMmCpuState;
> //
> extern EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext;
> extern EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
> +extern EFI_MMRAM_DESCRIPTOR mSCommBuffer;
> extern MP_INFORMATION_HOB_DATA *mMpInformationHobData;
> extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig;
>


Ming Huang
 

Hi Sami,

I am sure this is an issue for RAS case using Standalone MM.

Please refer Omkar's comments for v1 (12/9/21, 1:46 AM):
-----------------------------------------------------------------
Hi Ming,

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

- Omkar
-----------------------------------------------------------------

Regards,

Ming Huang


在 4/6/22 5:03 PM, Sami Mujawar 写道:

Hi Ming,

I am not sure if this is an issue at my end, but I cannot apply this patch series, can you check, please?
Also, is it possible to share these patches on a Github branch.

Regards,

Sami Mujawar

On 30/03/2022, 10:37, "Ming Huang" <huangming@...> wrote:

Hi,

Any comment about this series?

在 12/31/21 7:06 PM, Ming Huang 写道:
> There are two scene communicate with StandaloneMm(MM):
> 1 edk2 -> TF-A -> MM, communicate MM use non-secure buffer which
> specify by EFI_SECURE_PARTITION_BOOT_INFO.SpNsCommBufBase;
> 2 RAS scene: fiq -> TF-A -> MM, use secure buffer which
> specify by EFI_SECURE_PARTITION_BOOT_INFO.SpShareBufBase;
>
> For now, the second scene will failed because check buffer address.
> This patch add CheckBufferAddr() to support check address for secure
> buffer.
>
> Signed-off-by: Ming Huang <huangming@...>
> ---
> StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 54 +++++++++++++++-----
> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 ++++++++
> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 +
> 3 files changed, 63 insertions(+), 13 deletions(-)
>
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> index 5dfaf9d751..d0ba415671 100644
> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> @@ -50,6 +50,7 @@ EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext = NULL;
>
> // Descriptor with whereabouts of memory used for communication with the normal world
> EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
> +EFI_MMRAM_DESCRIPTOR mSCommBuffer;
>
> MP_INFORMATION_HOB_DATA *mMpInformationHobData;
>
> @@ -60,6 +61,42 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {
>
> STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL;
>
> +STATIC
> +EFI_STATUS
> +CheckBufferAddr (
> + IN UINTN BufferAddr
> + )
> +{
> + UINT64 NsCommBufferEnd;
> + UINT64 SCommBufferEnd;
> + UINT64 CommBufferEnd;
> +
> + NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
> + SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
> +
> + if ((BufferAddr >= mNsCommBuffer.PhysicalStart) &&
> + (BufferAddr < NsCommBufferEnd)) {
> + CommBufferEnd = NsCommBufferEnd;
> + } else if ((BufferAddr >= mSCommBuffer.PhysicalStart) &&
> + (BufferAddr < SCommBufferEnd)) {
> + CommBufferEnd = SCommBufferEnd;
> + } else {
> + return EFI_ACCESS_DENIED;
> + }
> +
> + if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNICATE_HEADER)) {
> + return EFI_ACCESS_DENIED;
> + }
> +
> + // perform bounds check.
> + if ((CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER)) <
> + ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength) {
> + return EFI_ACCESS_DENIED;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> The PI Standalone MM entry point for the TF-A CPU driver.
>
> @@ -104,25 +141,16 @@ PiMmStandaloneArmTfCpuDriverEntry (
> return EFI_INVALID_PARAMETER;
> }
>
> - if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
> - return EFI_ACCESS_DENIED;
> - }
> -
> - if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
> - (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
> - return EFI_INVALID_PARAMETER;
> + Status = CheckBufferAddr (NsCommBufferAddr);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status));
> + return Status;
> }
>
> // Find out the size of the buffer passed
> NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) NsCommBufferAddr)->MessageLength +
> sizeof (EFI_MM_COMMUNICATE_HEADER);
>
> - // perform bounds check.
> - if (NsCommBufferAddr + NsCommBufferSize >=
> - mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
> - return EFI_ACCESS_DENIED;
> - }
> -
> GuidedEventContext = NULL;
> // Now that the secure world can see the normal world buffer, allocate
> // memory to copy the communication buffer to the secure world.
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
> index fd9c59b4da..96dad20dd1 100644
> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
> @@ -107,6 +107,7 @@ StandaloneMmCpuInitialize (
> UINTN Index;
> UINTN ArraySize;
> VOID *HobStart;
> + EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob;
>
> ASSERT (SystemTable != NULL);
> mMmst = SystemTable;
> @@ -186,6 +187,26 @@ StandaloneMmCpuInitialize (
> CopyMem (&mNsCommBuffer, NsCommBufMmramRange, sizeof(EFI_MMRAM_DESCRIPTOR));
> DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));
>
> + Status = GetGuidedHobData (
> + HobStart,
> + &gEfiMmPeiMmramMemoryReserveGuid,
> + (VOID **) &MmramRangesHob
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "MmramRangesHob data extraction failed - 0x%x\n", Status));
> + return Status;
> + }
> +
> + //
> + // As CreateHobListFromBootInfo(), the base and size of buffer shared with
> + // privileged Secure world software is in second one.
> + //
> + CopyMem (
> + &mSCommBuffer,
> + &MmramRangesHob->Descriptor[0] + 1,
> + sizeof(EFI_MMRAM_DESCRIPTOR)
> + );
> +
> //
> // Extract the MP information from the Hoblist
> //
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
> index 2c96439c15..2e03b20d85 100644
> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
> @@ -30,6 +30,7 @@ extern EFI_MM_CPU_PROTOCOL mMmCpuState;
> //
> extern EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext;
> extern EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
> +extern EFI_MMRAM_DESCRIPTOR mSCommBuffer;
> extern MP_INFORMATION_HOB_DATA *mMpInformationHobData;
> extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig;
>






Ming Huang
 

Hi Sami,

Sorry for the late reply.

https://github.com/waip23/edk2/tree/upstread-fix-mm-issue
branch: upstread-fix-mm-issue

Regards,

Ming Huang

在 4/6/22 5:03 PM, Sami Mujawar 写道:

Hi Ming,

I am not sure if this is an issue at my end, but I cannot apply this patch series, can you check, please?
Also, is it possible to share these patches on a Github branch.

Regards,

Sami Mujawar

On 30/03/2022, 10:37, "Ming Huang" <huangming@...> wrote:

Hi,

Any comment about this series?

在 12/31/21 7:06 PM, Ming Huang 写道:
> There are two scene communicate with StandaloneMm(MM):
> 1 edk2 -> TF-A -> MM, communicate MM use non-secure buffer which
> specify by EFI_SECURE_PARTITION_BOOT_INFO.SpNsCommBufBase;
> 2 RAS scene: fiq -> TF-A -> MM, use secure buffer which
> specify by EFI_SECURE_PARTITION_BOOT_INFO.SpShareBufBase;
>
> For now, the second scene will failed because check buffer address.
> This patch add CheckBufferAddr() to support check address for secure
> buffer.
>
> Signed-off-by: Ming Huang <huangming@...>
> ---
> StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c | 54 +++++++++++++++-----
> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c | 21 ++++++++
> StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h | 1 +
> 3 files changed, 63 insertions(+), 13 deletions(-)
>
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> index 5dfaf9d751..d0ba415671 100644
> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/EventHandle.c
> @@ -50,6 +50,7 @@ EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext = NULL;
>
> // Descriptor with whereabouts of memory used for communication with the normal world
> EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
> +EFI_MMRAM_DESCRIPTOR mSCommBuffer;
>
> MP_INFORMATION_HOB_DATA *mMpInformationHobData;
>
> @@ -60,6 +61,42 @@ EFI_MM_CONFIGURATION_PROTOCOL mMmConfig = {
>
> STATIC EFI_MM_ENTRY_POINT mMmEntryPoint = NULL;
>
> +STATIC
> +EFI_STATUS
> +CheckBufferAddr (
> + IN UINTN BufferAddr
> + )
> +{
> + UINT64 NsCommBufferEnd;
> + UINT64 SCommBufferEnd;
> + UINT64 CommBufferEnd;
> +
> + NsCommBufferEnd = mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize;
> + SCommBufferEnd = mSCommBuffer.PhysicalStart + mSCommBuffer.PhysicalSize;
> +
> + if ((BufferAddr >= mNsCommBuffer.PhysicalStart) &&
> + (BufferAddr < NsCommBufferEnd)) {
> + CommBufferEnd = NsCommBufferEnd;
> + } else if ((BufferAddr >= mSCommBuffer.PhysicalStart) &&
> + (BufferAddr < SCommBufferEnd)) {
> + CommBufferEnd = SCommBufferEnd;
> + } else {
> + return EFI_ACCESS_DENIED;
> + }
> +
> + if ((CommBufferEnd - BufferAddr) < sizeof (EFI_MM_COMMUNICATE_HEADER)) {
> + return EFI_ACCESS_DENIED;
> + }
> +
> + // perform bounds check.
> + if ((CommBufferEnd - BufferAddr - sizeof (EFI_MM_COMMUNICATE_HEADER)) <
> + ((EFI_MM_COMMUNICATE_HEADER *) BufferAddr)->MessageLength) {
> + return EFI_ACCESS_DENIED;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> The PI Standalone MM entry point for the TF-A CPU driver.
>
> @@ -104,25 +141,16 @@ PiMmStandaloneArmTfCpuDriverEntry (
> return EFI_INVALID_PARAMETER;
> }
>
> - if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
> - return EFI_ACCESS_DENIED;
> - }
> -
> - if ((NsCommBufferAddr + sizeof (EFI_MM_COMMUNICATE_HEADER)) >=
> - (mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize)) {
> - return EFI_INVALID_PARAMETER;
> + Status = CheckBufferAddr (NsCommBufferAddr);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Check Buffer failed: %r\n", Status));
> + return Status;
> }
>
> // Find out the size of the buffer passed
> NsCommBufferSize = ((EFI_MM_COMMUNICATE_HEADER *) NsCommBufferAddr)->MessageLength +
> sizeof (EFI_MM_COMMUNICATE_HEADER);
>
> - // perform bounds check.
> - if (NsCommBufferAddr + NsCommBufferSize >=
> - mNsCommBuffer.PhysicalStart + mNsCommBuffer.PhysicalSize) {
> - return EFI_ACCESS_DENIED;
> - }
> -
> GuidedEventContext = NULL;
> // Now that the secure world can see the normal world buffer, allocate
> // memory to copy the communication buffer to the secure world.
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
> index fd9c59b4da..96dad20dd1 100644
> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.c
> @@ -107,6 +107,7 @@ StandaloneMmCpuInitialize (
> UINTN Index;
> UINTN ArraySize;
> VOID *HobStart;
> + EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob;
>
> ASSERT (SystemTable != NULL);
> mMmst = SystemTable;
> @@ -186,6 +187,26 @@ StandaloneMmCpuInitialize (
> CopyMem (&mNsCommBuffer, NsCommBufMmramRange, sizeof(EFI_MMRAM_DESCRIPTOR));
> DEBUG ((DEBUG_INFO, "mNsCommBuffer: 0x%016lx - 0x%lx\n", mNsCommBuffer.CpuStart, mNsCommBuffer.PhysicalSize));
>
> + Status = GetGuidedHobData (
> + HobStart,
> + &gEfiMmPeiMmramMemoryReserveGuid,
> + (VOID **) &MmramRangesHob
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "MmramRangesHob data extraction failed - 0x%x\n", Status));
> + return Status;
> + }
> +
> + //
> + // As CreateHobListFromBootInfo(), the base and size of buffer shared with
> + // privileged Secure world software is in second one.
> + //
> + CopyMem (
> + &mSCommBuffer,
> + &MmramRangesHob->Descriptor[0] + 1,
> + sizeof(EFI_MMRAM_DESCRIPTOR)
> + );
> +
> //
> // Extract the MP information from the Hoblist
> //
> diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
> index 2c96439c15..2e03b20d85 100644
> --- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
> +++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/StandaloneMmCpu.h
> @@ -30,6 +30,7 @@ extern EFI_MM_CPU_PROTOCOL mMmCpuState;
> //
> extern EFI_MM_COMMUNICATE_HEADER **PerCpuGuidedEventContext;
> extern EFI_MMRAM_DESCRIPTOR mNsCommBuffer;
> +extern EFI_MMRAM_DESCRIPTOR mSCommBuffer;
> extern MP_INFORMATION_HOB_DATA *mMpInformationHobData;
> extern EFI_MM_CONFIGURATION_PROTOCOL mMmConfig;
>






Sami Mujawar
 

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

Thanks,

Sami Mujawar