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

[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

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

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

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

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

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

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

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

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

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

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