Re: [PATCH v1 02/18] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.

[Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>]

My response inline.

-----Original Message-----
From: Achin Gupta
Sent: Wednesday, April 11, 2018 9:00 AM
To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Cc: edk2-devel@lists.01.org; michael.d.kinney@intel.com; liming.gao@intel.com; jiewen.yao@intel.com; leif.lindholm@linaro.org; ard.biesheuvel@linaro.org; nd <nd@arm.com>
Subject: Re: [PATCH v1 02/18] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.

Hi Supreeth,

CIL.

On Fri, Apr 06, 2018 at 03:42:07PM +0100, Supreeth Venkatesh wrote:
> PI v1.5 Specification Volume 4 defines Management Mode Core Interface
> and defines EFI_MM_COMMUNICATION_PROTOCOL. This protocol provides a
> means of communicating between drivers outside of MM and MMI handlers
> inside of MM.
>
> This patch implements the EFI_MM_COMMUNICATION_PROTOCOL DXE runtime
> driver for AARCH64 platforms. It uses SMCs allocated from the standard
> SMC range defined in DEN0060A_ARM_MM_Interface_Specification.pdf
> to communicate with the standalone MM environment in the secure world.
>
> This patch also adds the MM Communication driver (.inf) file to define
> entry point for this driver and other compile related information the
> driver needs.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> ---
>  .../Drivers/MmCommunicationDxe/MmCommunication.c   | 339 +++++++++++++++++++++
>  .../Drivers/MmCommunicationDxe/MmCommunication.inf |  50 +++
>  2 files changed, 389 insertions(+)
>  create mode 100644
> ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
>  create mode 100644
> ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
>
> diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> new file mode 100644
> index 0000000000..e801c1c601
> --- /dev/null
> +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> @@ -0,0 +1,339 @@
> +/** @file
> +
> +  Copyright (c) 2016-2017, ARM Limited. All rights reserved.
> +
> +  This program and the accompanying materials  are licensed and made
> + available under the terms and conditions of the BSD License  which
> + accompanies this distribution.  The full text of the license may be
> + found at  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Library/ArmLib.h>
> +#include <Library/ArmSmcLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DxeServicesTableLib.h> #include <Library/HobLib.h>
> +#include <Library/PcdLib.h> #include
> +<Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +
> +#include <Protocol/MmCommunication.h>
> +
> +#include <IndustryStandard/ArmStdSmc.h>
> +
> +//
> +// Address, Length of the pre-allocated buffer for communication with
> +the secure // world.
> +//
> +STATIC ARM_MEMORY_REGION_DESCRIPTOR  mNsCommBuffMemRegion;
> +
> +// Notification event when virtual address map is set.
> +STATIC EFI_EVENT  mSetVirtualAddressMapEvent;
> +
> +//
> +// Handle to install the MM Communication Protocol // STATIC
> +EFI_HANDLE  mMmCommunicateHandle;
> +
> +/**
> +  Communicates with a registered handler.
> +
> +  This function provides an interface to send and receive messages to
> + the  Standalone MM environment on behalf of UEFI services.  This
> + function is part  of the MM Communication Protocol that may be
> + called in physical mode prior to
> +  SetVirtualAddressMap() and in virtual mode after SetVirtualAddressMap().
> +
> +  @param[in]      This                The EFI_MM_COMMUNICATION_PROTOCOL
> +                                      instance.
> +  @param[in, out] CommBuffer          A pointer to the buffer to convey
> +                                      into MMRAM.
> +  @param[in, out] CommSize            The size of the data buffer being
> +                                      passed in. This is optional.
> +
> +  @retval EFI_SUCCESS                 The message was successfully posted.
> +  @retval EFI_INVALID_PARAMETER       The CommBuffer was NULL.
> +  @retval EFI_BAD_BUFFER_SIZE         The buffer size is incorrect for the MM
> +                                      implementation. If this error is
> +                                      returned, the MessageLength field in
> +                                      the CommBuffer header or the integer
> +                                      pointed by CommSize are updated to reflect
> +                                      the maximum payload size the
> +                                      implementation can accommodate.
> +  @retval EFI_ACCESS_DENIED           The CommunicateBuffer parameter
> +                                      or CommSize parameter, if not omitted,
> +                                      are in address range that cannot be
> +                                      accessed by the MM environment
> +**/ STATIC EFI_STATUS EFIAPI MmCommunicationCommunicate (
> +  IN CONST EFI_MM_COMMUNICATION_PROTOCOL  *This,
> +  IN OUT VOID                             *CommBuffer,
> +  IN OUT UINTN                            *CommSize OPTIONAL
> +  )
> +{
> +  EFI_MM_COMMUNICATE_HEADER   *CommunicateHeader;
> +  ARM_SMC_ARGS                CommunicateSmcArgs;
> +  EFI_STATUS                  Status;
> +  UINTN                       BufferSize;
> +
> +  CommunicateHeader = CommBuffer;
> +  Status = EFI_ACCESS_DENIED;
> +  BufferSize = 0;
> +
> +  ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS));
> +
> +  //
> +  // Check parameters
> +  //
> +  if (CommBuffer == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  // If the length of the CommBuffer is 0 then return the expected length.
> +  if (CommSize) {
> +    if (*CommSize == 0) {
> +      *CommSize = mNsCommBuffMemRegion.Length;
> +      return EFI_BAD_BUFFER_SIZE;
> +    }
> +    //
> +    // CommSize must hold HeaderGuid and MessageLength
> +    //
> +    if (*CommSize < sizeof (EFI_MM_COMMUNICATE_HEADER)) {
> +        return EFI_INVALID_PARAMETER;
> +    }

The check looks inadequate as it does not cater for the MessageLength specified in the EFI_MM_COMMUNICATE_HEADER i.e it will ensure there is at least a EFI_MM_COMMUNICATE_HEADER but will not ensure that the message is included as well.

Nit: It would be better if the parameters are checked before Status and CommunicateSmcArgs are updated above.
[Supreeth] As discussed, I will be relying on CommunicateHeader->MessageLength + Sizeof (Header) to calculate total size of communication payload as
CommBuffer is a Mandatory parameter as opposed to CommSize which is optional.
I have revamped the logic completely. Please check version 2.

> +    BufferSize = *CommSize;
> +  } else {
> +    BufferSize = CommunicateHeader->MessageLength +
> +                 sizeof (CommunicateHeader->HeaderGuid) +
> +                 sizeof (CommunicateHeader->MessageLength);
> +  }
> +
> +  //
> +  // If the buffer size is 0 or greater than what can be tolerated by
> + the MM  // environment then return the expected size.
> +  //
> +  if ((BufferSize == 0) ||
> +      (BufferSize > mNsCommBuffMemRegion.Length)) {
> +    CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length -
> +                                       sizeof (CommunicateHeader->HeaderGuid) -
> +                                       sizeof (CommunicateHeader->MessageLength);
> +    return EFI_BAD_BUFFER_SIZE;
> +  }

The 'if' condition works as expected to return the maximum buffer size if 'CommSize' is NULL. When 'CommSize' is used and the 'BufferSize' (derived from
'CommSize') is greater than what can be tolerated, then it is 'CommSize' that must be updated as per the PI spec. but the 'if'condition will still update the MessageLength.
[Supreeth] PI specification does not say MessageLength should not be updated either. I have revamped the logic completely. Please check version 2.
I plan to submit ECR w.r.t MessageLength.

> +
> +  // SMC Function ID
> +  CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64;
> +
> +  // Reserved for Future. Must be Zero.
> +  CommunicateSmcArgs.Arg1 = 0;
> +
> +  if (mNsCommBuffMemRegion.VirtualBase) {
> +    CopyMem ((VOID *)mNsCommBuffMemRegion.VirtualBase, CommBuffer,
> + BufferSize);  } else {
> +    return EFI_ACCESS_DENIED;
> +  }

VirtualBase was set to PhysicalBase in MmCommunicationInitialize(). Is there any reason why it would be unset here apart from a corruption of mNsCommBuffMemRegion structure? If not, then this is a panic condition. It is not the caller's fault and returning EFI_ACCESS_DENIED does not really help.
[Supreeth] This is not a critical event to affect to stop the working system in entirety. If there is a critical failure,
Secure side should anyway reset the system. The "if" condition was added as a result of someone else's feedback from earlier version of the patches.
I realize this is an extra check and I have removed this "if" condition.

> +
> +  // For the SMC64 version, this parameter is a 64-bit Physical
> + Address (PA)  // or Intermediate Physical Address (IPA).
> +  // For the SMC32 version, this parameter is a 32-bit PA or IPA.
> +  CommunicateSmcArgs.Arg2 = (UINTN)mNsCommBuffMemRegion.PhysicalBase;
> +
> +  // comm_size_address is a PA or an IPA that holds the size of the
> + // communication buffer being passed in. This parameter is optional
> + // and can be omitted by passing a zero.
> +  // ARM does not recommend using it since this might require the  //
> + implementation to create a separate memory mapping for the parameter.
> +  // ARM recommends storing the buffer size in the buffer itself.
> +  CommunicateSmcArgs.Arg3 = 0;
> +
> +  // Call the Standalone MM environment.
> +  ArmCallSmc (&CommunicateSmcArgs);
> +
> +  switch (CommunicateSmcArgs.Arg0) {
> +  case ARM_SMC_MM_RET_SUCCESS:
> +    // On exit, the size of data being returned is inferred from
> +    // CommSize or MessageLength + Header.
> +    CopyMem (CommBuffer,
> +             (const VOID *)mNsCommBuffMemRegion.VirtualBase,
> +             BufferSize);

Umm! I vaguely remember we had a chat about this but this does not seem right. The PI spec. does not help by being vague about how data should be copied out and its length reported. However, here is my take.

If CommSize is used, then it must be updated by the handler with the size of the data returned, zero otherwise. This is as far as the PI spec. goes in this regard.

Even though the spec. does not say this, if CommSize is not used then, MessageLength in the EFI_MM_COMMUNICATE_HEADER must be updated. This also means that the handler has to preserve the EFI_MM_COMMUNICATE_HEADER in the CommBuffer in the return path (again something the PI spec. does not say).

In the SMC ABI, CommSize maps to comm_size_address. We do not use this. So we must rely on the MessageLength on the way back.

Using BufferSize does not seem right to me. We are screwed if the input size is not equal to the output size. It is better to retrieve this information from the MessageLength as the comment says.
[Supreeth] As per our previous discussion, You had mentioned that "VariableDxe" that uses this driver assumes input size equal to output size. However, After looking into this further,
there are other drivers that consume this and for those drivers input size is not equal to output size.
When input size is not equal to output size, this may lead to overflow issues. PI specification does not provide any guidance regarding this. I plan to submit an ECR along the below lines.
   // Note: Very important to ensure that the consumer of this driver
    // has allocated CommBuffer sufficiently so that the return data
    // can be copied. Otherwise, this will lead to buffer overflow.
    // Assumption: CommBuffer = malloc (mNsCommBuffMemRegion.Length)
    // This guidance should be in the PI specification. TODO: ECR.

> +    Status = EFI_SUCCESS;
> +    break;
> +
> +  case ARM_SMC_MM_RET_NOT_SUPPORTED:

NOT_SUPPORTED means that MM_COMMUNICATE is not implemented which must not be the case if we are here. MmCommunicationInitialize() must call MM_VERSION to check this. In fact, the MM interface specification should be updated to remove NOT_SUPPORTED for MM_COMMUNICATE if MM_VERSION is implemented.

Can you panic() if this is returned?

[Supreeth] This is not a critical event to affect to stop the working system in entirety. If there is a critical failure, Secure side should anyway reset the system.
I have removed this case.

> +  case ARM_SMC_MM_RET_INVALID_PARAMS:
> +    Status = EFI_INVALID_PARAMETER;
> +    break;
> +
> +  case ARM_SMC_MM_RET_DENIED:
> +    Status = EFI_ACCESS_DENIED;
> +    break;
> +
> +  case ARM_SMC_MM_RET_NO_MEMORY:
> +    // Unexpected error since the CommSize was checked for zero length
> +    // prior to issuing the SMC
> +  default:
> +    Status = EFI_ACCESS_DENIED;
> +    ASSERT (0);
> +  }
> +
> +  return Status;
> +}
> +
> +//
> +// MM Communication Protocol instance //
> +EFI_MM_COMMUNICATION_PROTOCOL  mMmCommunication = {
> +  MmCommunicationCommunicate
> +};
> +
> +/**
> +  Notification callback on SetVirtualAddressMap event.
> +
> +  This function notifies the MM communication protocol interface on
> + SetVirtualAddressMap event and converts pointers used in this driver
> + from physical to virtual address.
> +
> +  @param  Event          SetVirtualAddressMap event.
> +  @param  Context        A context when the SetVirtualAddressMap triggered.
> +
> +  @retval EFI_SUCCESS    The function executed successfully.
> +  @retval Other          Some error occurred when executing this function.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +NotifySetVirtualAddressMap (
> +  IN EFI_EVENT  Event,
> +  IN VOID      *Context
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = gRT->ConvertPointer (EFI_OPTIONAL_PTR,
> +                                (VOID **)&mNsCommBuffMemRegion.VirtualBase
> +                               );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "NotifySetVirtualAddressMap():"
> +            " Unable to convert MM runtime pointer. Status:0x%r\n",
> + Status));  }
> +
> +}
> +
> +/**
> +  The Entry Point for MM Communication
> +
> +  This function installs the MM communication protocol interface and
> + finds out  what type of buffer management will be required prior to
> + invoking the  communication SMC.
> +
> +  @param  ImageHandle    The firmware allocated handle for the EFI image.
> +  @param  SystemTable    A pointer to the EFI System Table.
> +
> +  @retval EFI_SUCCESS    The entry point is executed successfully.
> +  @retval Other          Some error occurred when executing this entry point.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +MmCommunicationInitialize (
> +  IN EFI_HANDLE         ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS                 Status;
> +
> +  mNsCommBuffMemRegion.PhysicalBase = PcdGet64 (PcdMmBufferBase);  //
> + During boot , Virtual and Physical are same
> + mNsCommBuffMemRegion.VirtualBase =
> + mNsCommBuffMemRegion.PhysicalBase;
> +  mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize);
> +
> +  if (mNsCommBuffMemRegion.PhysicalBase == 0) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> +            "Invalid MM Buffer Base Address.\n"));
> +    goto ReturnErrorStatus;
> +  }
> +

We need to call MM_VERSION to check whether there is a StandaloneMm imp. in the Secure world or not.
[Supreeth] Agreed. Please see version 2.


> +  if (mNsCommBuffMemRegion.Length == 0) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> +            "Maximum Buffer Size is zero.\n"));
> +    goto ReturnErrorStatus;
> +  }
> +
> +  Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory,
> +                                mNsCommBuffMemRegion.PhysicalBase,
> +                                mNsCommBuffMemRegion.Length,
> +                                EFI_MEMORY_WB |
> +                                EFI_MEMORY_XP |
> +                                EFI_MEMORY_RUNTIME);  if (EFI_ERROR
> + (Status)) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> +            "Failed to add MM-NS Buffer Memory Space\n"));
> +    goto ReturnErrorStatus;
> +  }
> +
> +  Status = gDS->SetMemorySpaceAttributes(mNsCommBuffMemRegion.PhysicalBase,
> +                                         mNsCommBuffMemRegion.Length,
> +                                         EFI_MEMORY_WB |
> + EFI_MEMORY_XP);  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> +            "Failed to set MM-NS Buffer Memory attributes\n"));
> +    goto CleanAddedMemorySpace;
> +  }
> +
> +  Status = gBS->AllocatePages (AllocateAddress,
> +                               EfiRuntimeServicesData,
> +                               EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length),
> +                               &mNsCommBuffMemRegion.PhysicalBase);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
> +            "Failed to allocate MM-NS Buffer Memory Space\n"));
> +    goto CleanAddedMemorySpace;
> +  }
> +
> +  // Install the communication protocol  Status =
> + gBS->InstallProtocolInterface (&mMmCommunicateHandle,
> +                                          &gEfiMmCommunicationProtocolGuid,
> +                                          EFI_NATIVE_INTERFACE,
> +                                          &mMmCommunication);  if
> + (EFI_ERROR(Status)) {
> +    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: "
> +            "Failed to install MM communication protocol\n"));
> +    goto CleanAllocatedPages;
> +  }
> +
> +  // Register notification callback when  virtual address is
> + associated  // with the physical address.
> +  // Create a Set Virtual Address Map event.
> +  //
> +  Status = gBS->CreateEvent (EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE,  // Type
> +                             TPL_NOTIFY,                         // NotifyTpl
> +                             NotifySetVirtualAddressMap,         // NotifyFunction
> +                             NULL,                               // NotifyContext
> +                             &mSetVirtualAddressMapEvent         // Event
> +                            );
> +  if (Status == EFI_SUCCESS) {
> +    return Status;
> +  }
> +
> +  gBS->UninstallProtocolInterface(mMmCommunicateHandle,
> +                                  &gEfiMmCommunicationProtocolGuid,
> +                                  &mMmCommunication);
> +
> +CleanAllocatedPages:
> +  gBS->FreePages (mNsCommBuffMemRegion.PhysicalBase,
> +                  EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length));
> +
> +CleanAddedMemorySpace:
> +  gDS->RemoveMemorySpace (mNsCommBuffMemRegion.PhysicalBase,
> +                          mNsCommBuffMemRegion.Length);
> +
> +ReturnErrorStatus:
> +  return EFI_INVALID_PARAMETER;
> +}
> diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> new file mode 100644
> index 0000000000..344d55f333
> --- /dev/null
> +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
> @@ -0,0 +1,50 @@
> +#/** @file
> +#
> +#  DXE MM Communicate driver
> +#
> +#  Copyright (c) 2016 - 2017, ARM Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials #  are licensed and
> +made available under the terms and conditions of the BSD License #
> +which accompanies this distribution.  The full text of the license
> +may be found at #  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> +BASIS, #  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = ArmMmCommunication
> +  FILE_GUID                      = 09EE81D3-F15E-43F4-85B4-CB9873DA5D6B
> +  MODULE_TYPE                    = DXE_RUNTIME_DRIVER
> +  VERSION_STRING                 = 1.0
> +
> +  ENTRY_POINT                    = MmCommunicationInitialize
> +
> +[Sources.Common]
> +  MmCommunication.c

Since this is a AArch64 only driver. Should it not be under Sources.Common.AArch64?
[Supreeth] It is in ArmPkg,  so it implies it's a 32 bit or AArch64 driver, but as an added measure, I will move it under Sources. AArch64 and not Sources.Common.AArch64. (to be pedantic)
Cheers,
Achin

> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  ArmSmcLib
> +  BaseMemoryLib
> +  DebugLib
> +  DxeServicesTableLib
> +  HobLib
> +  UefiDriverEntryPoint
> +
> +[Protocols]
> +  gEfiMmCommunicationProtocolGuid              ## PRODUCES
> +
> +[Pcd.common]
> +  gArmTokenSpaceGuid.PcdMmBufferBase
> +  gArmTokenSpaceGuid.PcdMmBufferSize
> +
> +[Depex]
> +  AFTER gArmGicDxeFileGuid
> --
> 2.16.2
>
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.