[Patch] StandaloneMmPkg: Fixed communicating from TF-A failed issue


Ming Huang
 

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

For AArch64, when SPM enable in TF-A, TF-A may communicate to MM
with buffer address (PLAT_SPM_BUF_BASE). The address is different
from PcdMmBufferBase which use in edk2.
Checking address will let TF-A communicate failed to MM. So remove
below checking code:
if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
return EFI_ACCESS_DENIED;
}

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
index 63fbe26642..fe98d3181d 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
@@ -103,10 +103,6 @@ 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;
--
2.17.1


Ard Biesheuvel
 

On Tue, 8 Jun 2021 at 16:21, Ming Huang <huangming@linux.alibaba.com> wrote:

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

For AArch64, when SPM enable in TF-A, TF-A may communicate to MM
with buffer address (PLAT_SPM_BUF_BASE). The address is different
from PcdMmBufferBase which use in edk2.
Then why do we have PcdMmBufferBase?

Is it possible to set PcdMmBufferBase to the correct value?

Checking address will let TF-A communicate failed to MM. So remove
below checking code:
if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
return EFI_ACCESS_DENIED;
}

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
index 63fbe26642..fe98d3181d 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
@@ -103,10 +103,6 @@ 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;
--
2.17.1


Ming Huang
 

On 6/9/21 3:10 PM, Ard Biesheuvel wrote:
On Tue, 8 Jun 2021 at 16:21, Ming Huang <huangming@linux.alibaba.com> wrote:

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

For AArch64, when SPM enable in TF-A, TF-A may communicate to MM
with buffer address (PLAT_SPM_BUF_BASE). The address is different
from PcdMmBufferBase which use in edk2.
Then why do we have PcdMmBufferBase?
ArmPkg use this Pcd for the base address of non-secure communication buffer.


Is it possible to set PcdMmBufferBase to the correct value?
The secure communication may interrupt the non-secure communication. if we
use the same address (PcdMmBufferBase and PLAT_SPM_BUF_BASE), the date in
communication buffer may be corrupted.

Best Regards,
Ming


Checking address will let TF-A communicate failed to MM. So remove
below checking code:
if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
return EFI_ACCESS_DENIED;
}

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
index 63fbe26642..fe98d3181d 100644
--- a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++ b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
@@ -103,10 +103,6 @@ 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;
--
2.17.1


Omkar Anand Kulkarni
 

On 6/10/21 6:44 AM, Ming Huang via groups.io wrote:
On 6/9/21 3:10 PM, Ard Biesheuvel wrote:
On Tue, 8 Jun 2021 at 16:21, Ming Huang <huangming@linux.alibaba.com>
wrote:

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

For AArch64, when SPM enable in TF-A, TF-A may communicate to MM
with
buffer address (PLAT_SPM_BUF_BASE). The address is different from
PcdMmBufferBase which use in edk2.
Then why do we have PcdMmBufferBase?
ArmPkg use this Pcd for the base address of non-secure communication
buffer.


Is it possible to set PcdMmBufferBase to the correct value?
The secure communication may interrupt the non-secure communication. if
we use the same address (PcdMmBufferBase and PLAT_SPM_BUF_BASE), the
date in communication buffer may be corrupted.

Best Regards,
Ming
In case where an interrupt handler executing from EL3 makes a call into StandaloneMM, the handler in EL3 makes an spm call into StandaloneMM using PLAT_SPM_BUF_BASE buffer base address. This PLAT_SPM_BUF_BASE is a shared buffer between EL3 and S-EL0. This is where the following check fails and leads to spm call failure. So this change would help resolve this issue.

- Omkar



Checking address will let TF-A communicate failed to MM. So remove
below checking code:
if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
return EFI_ACCESS_DENIED;
}

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c |
4
----
1 file changed, 4 deletions(-)

diff --git
a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
index 63fbe26642..fe98d3181d 100644
---
a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++
b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
@@ -103,10 +103,6 @@ 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;
--
2.17.1



Ard Biesheuvel
 

On Wed, 16 Jun 2021 at 07:30, Omkar Kulkarni <Omkar.Kulkarni@arm.com> wrote:


On 6/10/21 6:44 AM, Ming Huang via groups.io wrote:
On 6/9/21 3:10 PM, Ard Biesheuvel wrote:
On Tue, 8 Jun 2021 at 16:21, Ming Huang <huangming@linux.alibaba.com>
wrote:

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

For AArch64, when SPM enable in TF-A, TF-A may communicate to MM
with
buffer address (PLAT_SPM_BUF_BASE). The address is different from
PcdMmBufferBase which use in edk2.
Then why do we have PcdMmBufferBase?
ArmPkg use this Pcd for the base address of non-secure communication
buffer.


Is it possible to set PcdMmBufferBase to the correct value?
The secure communication may interrupt the non-secure communication. if
we use the same address (PcdMmBufferBase and PLAT_SPM_BUF_BASE), the
date in communication buffer may be corrupted.

Best Regards,
Ming
In case where an interrupt handler executing from EL3 makes a call into StandaloneMM, the handler in EL3 makes an spm call into StandaloneMM using PLAT_SPM_BUF_BASE buffer base address. This PLAT_SPM_BUF_BASE is a shared buffer between EL3 and S-EL0. This is where the following check fails and leads to spm call failure. So this change would help resolve this issue.
But is it the right fix? Why would EDK2 even be aware of how EL3 and
S-EL0 communicate with each other, and where the buffer is located?




Checking address will let TF-A communicate failed to MM. So remove
below checking code:
if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
return EFI_ACCESS_DENIED;
}

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c |
4
----
1 file changed, 4 deletions(-)

diff --git
a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
index 63fbe26642..fe98d3181d 100644
---
a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++
b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
@@ -103,10 +103,6 @@ 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;
--
2.17.1



Ming Huang
 

On 6/16/21 10:10 PM, Ard Biesheuvel wrote:
On Wed, 16 Jun 2021 at 07:30, Omkar Kulkarni <Omkar.Kulkarni@arm.com> wrote:


On 6/10/21 6:44 AM, Ming Huang via groups.io wrote:
On 6/9/21 3:10 PM, Ard Biesheuvel wrote:
On Tue, 8 Jun 2021 at 16:21, Ming Huang <huangming@linux.alibaba.com>
wrote:

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

For AArch64, when SPM enable in TF-A, TF-A may communicate to MM
with
buffer address (PLAT_SPM_BUF_BASE). The address is different from
PcdMmBufferBase which use in edk2.
Then why do we have PcdMmBufferBase?
ArmPkg use this Pcd for the base address of non-secure communication
buffer.


Is it possible to set PcdMmBufferBase to the correct value?
The secure communication may interrupt the non-secure communication. if
we use the same address (PcdMmBufferBase and PLAT_SPM_BUF_BASE), the
date in communication buffer may be corrupted.

Best Regards,
Ming
In case where an interrupt handler executing from EL3 makes a call into StandaloneMM, the handler in EL3 makes an spm call into StandaloneMM using PLAT_SPM_BUF_BASE buffer base address. This PLAT_SPM_BUF_BASE is a shared buffer between EL3 and S-EL0. This is where the following check fails and leads to spm call failure. So this change would help resolve this issue.
But is it the right fix? Why would EDK2 even be aware of how EL3 and
S-EL0 communicate with each other, and where the buffer is located?
The root cause for this problem is that the StandaloneMmPkg and TF-A
are not full cooperative.
PLAT_SPM_BUF_BASE is not dynamic buffer just like PcdMmBufferBase.

Please refer PcdMmBufferBase comments in edk2-platforms/Platform/ARM/SgiPkg/SgiPlatform.dsc.inc:
#
# Set the base address and size of the buffer used
# for communication between the Normal world edk2
# with StandaloneMm image at S-EL0 through MM_COMMUNICATE.
# This buffer gets allocated in ATF and since we do not have
# a mechanism currently to communicate the base address and
# size of this buffer from ATF, hard-code it here
#
## MM Communicate
gArmTokenSpaceGuid.PcdMmBufferBase|0xFF600000
gArmTokenSpaceGuid.PcdMmBufferSize|0x10000


Best Regards,
Ming





Checking address will let TF-A communicate failed to MM. So remove
below checking code:
if (NsCommBufferAddr < mNsCommBuffer.PhysicalStart) {
return EFI_ACCESS_DENIED;
}

Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
---
StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c |
4
----
1 file changed, 4 deletions(-)

diff --git
a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
index 63fbe26642..fe98d3181d 100644
---
a/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
+++
b/StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c
@@ -103,10 +103,6 @@ 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;
--
2.17.1