ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

The following patches added support for StandaloneMM using FF-A:
9da5ee116a28 ArmPkg: Allow FF-A calls to set memory region's attributes
0e43e02b9bd8 ArmPkg: Allow FF-A calls to get memory region's attributes

However, in the error handling logic for the Get/Set Memory attributes,
the CLANG compiler reports that a status variable could be used without
initialisation. This issue is a false positive and is not seen with GCC.

The Get/Set Memory attributes operation is atomic and therefore an
FFA_INTERRUPT or FFA_SUCCESS response is not expected in response
to FFA_MSG_SEND_DIRECT_REQ. So the remaining cases that could occur
are:
 - the target sends FFA_MSG_SEND_DIRECT_RESP with a success or
   failure code.
 or
 - FFA_MSG_SEND_DIRECT_REQ transmission failure.

Therefore,
 - reorder the error handling conditions such that it prevents the
   uninitialised variable issue being flagged by CLANG.
 - move the repetitive code to a static helper function and add
   documentation at the appropriate places.
 - fix error handling in functions that invoke GetMemoryPermissions().

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org>

1 files changed, 179 insertions, 144 deletions

diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index a30369af9c..5f453d18e4 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -1,10 +1,15 @@
 /** @file

-*  File managing the MMU for ARMv8 architecture in S-EL0

-*

-*  Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.<BR>

-*

-*  SPDX-License-Identifier: BSD-2-Clause-Patent

-*

+  File managing the MMU for ARMv8 architecture in S-EL0

+

+  Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.<BR>

+  SPDX-License-Identifier: BSD-2-Clause-Patent

+

+  @par Reference(s):

+  - [1] SPM based on the MM interface.

+        (https://trustedfirmware-a.readthedocs.io/en/latest/components/

+         secure-partition-manager-mm.html)

+  - [2] Arm Firmware Framework for Armv8-A, DEN0077A, version 1.0

+        (https://developer.arm.com/documentation/den0077/a)

 **/

 

 #include <Uefi.h>

@@ -19,37 +24,37 @@
 #include <Library/DebugLib.h>

 #include <Library/PcdLib.h>

 

+/** Send memory permission request to target.

+

+  @param [in, out]  SvcArgs     Pointer to SVC arguments to send. On

+                                return it contains the response parameters.

+  @param [out]      RetVal      Pointer to return the response value.

+

+  @retval EFI_SUCCESS           Request successfull.

+  @retval EFI_INVALID_PARAMETER A parameter is invalid.

+  @retval EFI_NOT_READY         Callee is busy or not in a state to handle

+                                this request.

+  @retval EFI_UNSUPPORTED       This function is not implemented by the

+                                callee.

+  @retval EFI_ABORTED           Message target ran into an unexpected error

+                                and has aborted.

+  @retval EFI_ACCESS_DENIED     Access denied.

+  @retval EFI_OUT_OF_RESOURCES  Out of memory to perform operation.

+**/

 STATIC

 EFI_STATUS

-GetMemoryPermissions (

-  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,

-  OUT UINT32                    *MemoryAttributes

+SendMemoryPermissionRequest (

+  IN OUT  ARM_SVC_ARGS *SvcArgs,

+     OUT  INT32        *RetVal

   )

 {

-  INT32         Ret;

-  ARM_SVC_ARGS  GetMemoryPermissionsSvcArgs;

-  BOOLEAN       FfaEnabled;

-

-  ZeroMem (&GetMemoryPermissionsSvcArgs, sizeof (ARM_SVC_ARGS));

-

-  FfaEnabled = FeaturePcdGet (PcdFfaEnable);

-  if (FfaEnabled) {

-    GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;

-    GetMemoryPermissionsSvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;

-    GetMemoryPermissionsSvcArgs.Arg2 = 0;

-    GetMemoryPermissionsSvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;

-    GetMemoryPermissionsSvcArgs.Arg4 = BaseAddress;

-  } else {

-    GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;

-    GetMemoryPermissionsSvcArgs.Arg1 = BaseAddress;

-    GetMemoryPermissionsSvcArgs.Arg2 = 0;

-    GetMemoryPermissionsSvcArgs.Arg3 = 0;

+  if ((SvcArgs == NULL) || (RetVal == NULL)) {

+    return EFI_INVALID_PARAMETER;

   }

 

-  *MemoryAttributes = 0;

-  ArmCallSvc (&GetMemoryPermissionsSvcArgs);

-  if (FfaEnabled) {

-    // Getting memory attributes is an atomic call, with

+  ArmCallSvc (SvcArgs);

+  if (FeaturePcdGet (PcdFfaEnable)) {

+    // Get/Set memory attributes is an atomic call, with

     // StandaloneMm at S-EL0 being the caller and the SPM

     // core being the callee. Thus there won't be a

     // FFA_INTERRUPT or FFA_SUCCESS response to the Direct

@@ -57,148 +62,178 @@ GetMemoryPermissions (
     // for other Direct Request calls which are not atomic

     // We therefore check only for Direct Response by the

     // callee.

-    if (GetMemoryPermissionsSvcArgs.Arg0 !=

-        ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {

+    if (SvcArgs->Arg0 == ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {

+      // A Direct Response means FF-A success

+      // Now check the payload for errors

+      // The callee sends back the return value

+      // in Arg3

+      *RetVal = SvcArgs->Arg3;

+    } else {

       // If Arg0 is not a Direct Response, that means we

       // have an FF-A error. We need to check Arg2 for the

       // FF-A error code.

-      Ret = GetMemoryPermissionsSvcArgs.Arg2;

-      switch (Ret) {

-      case ARM_FFA_SPM_RET_INVALID_PARAMETERS:

+      // See [2], Table 10.8: FFA_ERROR encoding.

+      *RetVal = SvcArgs->Arg2;

+      switch (*RetVal) {

+        case ARM_FFA_SPM_RET_INVALID_PARAMETERS:

+          return EFI_INVALID_PARAMETER;

 

-        return EFI_INVALID_PARAMETER;

+        case ARM_FFA_SPM_RET_DENIED:

+          return EFI_ACCESS_DENIED;

 

-      case ARM_FFA_SPM_RET_DENIED:

-        return EFI_NOT_READY;

+        case ARM_FFA_SPM_RET_NOT_SUPPORTED:

+          return EFI_UNSUPPORTED;

 

-      case ARM_FFA_SPM_RET_NOT_SUPPORTED:

-        return EFI_UNSUPPORTED;

+        case ARM_FFA_SPM_RET_BUSY:

+          return EFI_NOT_READY;

 

-      case ARM_FFA_SPM_RET_BUSY:

-        return EFI_NOT_READY;

+        case ARM_FFA_SPM_RET_ABORTED:

+          return EFI_ABORTED;

 

-      case ARM_FFA_SPM_RET_ABORTED:

-        return EFI_ABORTED;

+        default:

+          // Undefined error code received.

+          ASSERT (0);

+          return EFI_INVALID_PARAMETER;

       }

-    } else if (GetMemoryPermissionsSvcArgs.Arg0 ==

-               ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {

-      // A Direct Response means FF-A success

-      // Now check the payload for errors

-      // The callee sends back the return value

-      // in Arg3

-      Ret = GetMemoryPermissionsSvcArgs.Arg3;

     }

   } else {

-    Ret = GetMemoryPermissionsSvcArgs.Arg0;

+    *RetVal = SvcArgs->Arg0;

   }

 

-  if (Ret & BIT31) {

+  // Check error response from Callee.

+  if (*RetVal & BIT31) {

     // Bit 31 set means there is an error retured

-    switch (Ret) {

-    case ARM_SVC_SPM_RET_INVALID_PARAMS:

-      return EFI_INVALID_PARAMETER;

+    // See [1], Section 13.5.5.1 MM_SP_MEMORY_ATTRIBUTES_GET_AARCH64 and

+    // Section 13.5.5.2 MM_SP_MEMORY_ATTRIBUTES_SET_AARCH64.

+    switch (*RetVal) {

+      case ARM_SVC_SPM_RET_NOT_SUPPORTED:

+        return EFI_UNSUPPORTED;

+

+      case ARM_SVC_SPM_RET_INVALID_PARAMS:

+        return EFI_INVALID_PARAMETER;

+

+      case ARM_SVC_SPM_RET_DENIED:

+        return EFI_ACCESS_DENIED;

+

+      case ARM_SVC_SPM_RET_NO_MEMORY:

+        return EFI_OUT_OF_RESOURCES;

 

-    case ARM_SVC_SPM_RET_NOT_SUPPORTED:

-      return EFI_UNSUPPORTED;

+      default:

+        // Undefined error code received.

+        ASSERT (0);

+        return EFI_INVALID_PARAMETER;

     }

-  } else {

-    *MemoryAttributes = Ret;

   }

 

   return EFI_SUCCESS;

 }

 

+/** Request the permission attributes of a memory region from S-EL0.

+

+  @param [in]   BaseAddress          Base address for the memory region.

+  @param [out]  MemoryAttributes     Pointer to return the memory attributes.

+

+  @retval EFI_SUCCESS             Request successfull.

+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.

+  @retval EFI_NOT_READY           Callee is busy or not in a state to handle

+                                  this request.

+  @retval EFI_UNSUPPORTED         This function is not implemented by the

+                                  callee.

+  @retval EFI_ABORTED             Message target ran into an unexpected error

+                                  and has aborted.

+  @retval EFI_ACCESS_DENIED       Access denied.

+  @retval EFI_OUT_OF_RESOURCES    Out of memory to perform operation.

+**/

 STATIC

 EFI_STATUS

-RequestMemoryPermissionChange (

+GetMemoryPermissions (

   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,

-  IN  UINT64                    Length,

-  IN  UINTN                     Permissions

+  OUT UINT32                    *MemoryAttributes

   )

 {

+  EFI_STATUS    Status;

   INT32         Ret;

-  BOOLEAN       FfaEnabled;

-  ARM_SVC_ARGS  ChangeMemoryPermissionsSvcArgs;

-

-  ZeroMem (&ChangeMemoryPermissionsSvcArgs, sizeof (ARM_SVC_ARGS));

+  ARM_SVC_ARGS  SvcArgs;

 

-  FfaEnabled = FeaturePcdGet (PcdFfaEnable);

-

-  if (FfaEnabled) {

-    ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;

-    ChangeMemoryPermissionsSvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;

-    ChangeMemoryPermissionsSvcArgs.Arg2 = 0;

-    ChangeMemoryPermissionsSvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;

-    ChangeMemoryPermissionsSvcArgs.Arg4 = BaseAddress;

-    ChangeMemoryPermissionsSvcArgs.Arg5 = EFI_SIZE_TO_PAGES (Length);

-    ChangeMemoryPermissionsSvcArgs.Arg6 = Permissions;

-  } else {

-    ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;

-    ChangeMemoryPermissionsSvcArgs.Arg1 = BaseAddress;

-    ChangeMemoryPermissionsSvcArgs.Arg2 = EFI_SIZE_TO_PAGES (Length);

-    ChangeMemoryPermissionsSvcArgs.Arg3 = Permissions;

+  if (MemoryAttributes == NULL) {

+    return EFI_INVALID_PARAMETER;

   }

 

-  ArmCallSvc (&ChangeMemoryPermissionsSvcArgs);

-

-  if (FfaEnabled) {

-    // Setting memory attributes is an atomic call, with

-    // StandaloneMm at S-EL0 being the caller and the SPM

-    // core being the callee. Thus there won't be a

-    // FFA_INTERRUPT or FFA_SUCCESS response to the Direct

-    // Request sent above. This will have to be considered

-    // for other Direct Request calls which are not atomic

-    // We therefore check only for Direct Response by the

-    // callee.

-    if (ChangeMemoryPermissionsSvcArgs.Arg0 !=

-        ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {

-      // If Arg0 is not a Direct Response, that means we

-      // have an FF-A error. We need to check Arg2 for the

-      // FF-A error code.

-      Ret = ChangeMemoryPermissionsSvcArgs.Arg2;

-      switch (Ret) {

-      case ARM_FFA_SPM_RET_INVALID_PARAMETERS:

-        return EFI_INVALID_PARAMETER;

-

-      case ARM_FFA_SPM_RET_DENIED:

-        return EFI_NOT_READY;

-

-      case ARM_FFA_SPM_RET_NOT_SUPPORTED:

-        return EFI_UNSUPPORTED;

-

-      case ARM_FFA_SPM_RET_BUSY:

-        return EFI_NOT_READY;

-

-      case ARM_FFA_SPM_RET_ABORTED:

-        return EFI_ABORTED;

-      }

-    } else if (ChangeMemoryPermissionsSvcArgs.Arg0 ==

-               ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {

-      // A Direct Response means FF-A success

-      // Now check the payload for errors

-      // The callee sends back the return value

-      // in Arg3

-      Ret = ChangeMemoryPermissionsSvcArgs.Arg3;

-    }

+  // Prepare the message parameters.

+  // See [1], Section 13.5.5.1 MM_SP_MEMORY_ATTRIBUTES_GET_AARCH64.

+  ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS));

+  if (FeaturePcdGet (PcdFfaEnable)) {

+    // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.

+    SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;

+    SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;

+    SvcArgs.Arg2 = 0;

+    SvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;

+    SvcArgs.Arg4 = BaseAddress;

   } else {

-    Ret = ChangeMemoryPermissionsSvcArgs.Arg0;

+    SvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;

+    SvcArgs.Arg1 = BaseAddress;

+    SvcArgs.Arg2 = 0;

+    SvcArgs.Arg3 = 0;

   }

 

-  switch (Ret) {

-  case ARM_SVC_SPM_RET_NOT_SUPPORTED:

-    return EFI_UNSUPPORTED;

-

-  case ARM_SVC_SPM_RET_INVALID_PARAMS:

-    return EFI_INVALID_PARAMETER;

+  Status = SendMemoryPermissionRequest (&SvcArgs, &Ret);

+  if (EFI_ERROR (Status)) {

+    *MemoryAttributes = 0;

+    return Status;

+  }

 

-  case ARM_SVC_SPM_RET_DENIED:

-    return EFI_ACCESS_DENIED;

+  *MemoryAttributes = Ret;

+  return Status;

+}

 

-  case ARM_SVC_SPM_RET_NO_MEMORY:

-    return EFI_BAD_BUFFER_SIZE;

+/** Set the permission attributes of a memory region from S-EL0.

+

+  @param [in]  BaseAddress     Base address for the memory region.

+  @param [in]  Length          Length of the memory region.

+  @param [in]  Permissions     Memory access controls attributes.

+

+  @retval EFI_SUCCESS             Request successfull.

+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.

+  @retval EFI_NOT_READY           Callee is busy or not in a state to handle

+                                  this request.

+  @retval EFI_UNSUPPORTED         This function is not implemented by the

+                                  callee.

+  @retval EFI_ABORTED             Message target ran into an unexpected error

+                                  and has aborted.

+  @retval EFI_ACCESS_DENIED       Access denied.

+  @retval EFI_OUT_OF_RESOURCES    Out of memory to perform operation.

+**/

+STATIC

+EFI_STATUS

+RequestMemoryPermissionChange (

+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,

+  IN  UINT64                    Length,

+  IN  UINT32                    Permissions

+  )

+{

+  INT32         Ret;

+  ARM_SVC_ARGS  SvcArgs;

+

+  // Prepare the message parameters.

+  // See [1], Section 13.5.5.2 MM_SP_MEMORY_ATTRIBUTES_SET_AARCH64.

+  ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS));

+  if (FeaturePcdGet (PcdFfaEnable)) {

+    // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.

+    SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;

+    SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;

+    SvcArgs.Arg2 = 0;

+    SvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;

+    SvcArgs.Arg4 = BaseAddress;

+    SvcArgs.Arg5 = EFI_SIZE_TO_PAGES (Length);

+    SvcArgs.Arg6 = Permissions;

+  } else {

+    SvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;

+    SvcArgs.Arg1 = BaseAddress;

+    SvcArgs.Arg2 = EFI_SIZE_TO_PAGES (Length);

+    SvcArgs.Arg3 = Permissions;

   }

 

-  return EFI_SUCCESS;

+  return SendMemoryPermissionRequest (&SvcArgs, &Ret);

 }

 

 EFI_STATUS

@@ -212,7 +247,7 @@ ArmSetMemoryRegionNoExec (
   UINT32 CodePermission;

 

   Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);

-  if (Status != EFI_INVALID_PARAMETER) {

+  if (!EFI_ERROR (Status)) {

     CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;

     return RequestMemoryPermissionChange (

              BaseAddress,

@@ -220,7 +255,7 @@ ArmSetMemoryRegionNoExec (
              MemoryAttributes | CodePermission

              );

   }

-  return EFI_INVALID_PARAMETER;

+  return Status;

 }

 

 EFI_STATUS

@@ -234,7 +269,7 @@ ArmClearMemoryRegionNoExec (
   UINT32 CodePermission;

 

   Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);

-  if (Status != EFI_INVALID_PARAMETER) {

+  if (!EFI_ERROR (Status)) {

     CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;

     return RequestMemoryPermissionChange (

              BaseAddress,

@@ -242,7 +277,7 @@ ArmClearMemoryRegionNoExec (
              MemoryAttributes & ~CodePermission

              );

   }

-  return EFI_INVALID_PARAMETER;

+  return Status;

 }

 

 EFI_STATUS

@@ -256,7 +291,7 @@ ArmSetMemoryRegionReadOnly (
   UINT32 DataPermission;

 

   Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);

-  if (Status != EFI_INVALID_PARAMETER) {

+  if (!EFI_ERROR (Status)) {

     DataPermission = SET_MEM_ATTR_DATA_PERM_RO << SET_MEM_ATTR_DATA_PERM_SHIFT;

     return RequestMemoryPermissionChange (

              BaseAddress,

@@ -264,7 +299,7 @@ ArmSetMemoryRegionReadOnly (
              MemoryAttributes | DataPermission

              );

   }

-  return EFI_INVALID_PARAMETER;

+  return Status;

 }

 

 EFI_STATUS

@@ -278,7 +313,7 @@ ArmClearMemoryRegionReadOnly (
   UINT32 PermissionRequest;

 

   Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);

-  if (Status != EFI_INVALID_PARAMETER) {

+  if (!EFI_ERROR (Status)) {

     PermissionRequest = SET_MEM_ATTR_MAKE_PERM_REQUEST (SET_MEM_ATTR_DATA_PERM_RW,

                                                         MemoryAttributes);

     return RequestMemoryPermissionChange (

@@ -287,5 +322,5 @@ ArmClearMemoryRegionReadOnly (
              PermissionRequest

              );

   }

-  return EFI_INVALID_PARAMETER;

+  return Status;

 }