From 31eaefd4df78d58ad4087a13f6fc7607b266d04e Mon Sep 17 00:00:00 2001 From: Sami Mujawar Date: Thu, 25 Feb 2021 17:11:10 +0000 Subject: 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 Tested-by: Sughosh Ganu Reviewed-by: Sughosh Ganu --- .../AArch64/ArmMmuStandaloneMmLib.c | 323 ++++++++++++--------- 1 file changed, 179 insertions(+), 144 deletions(-) diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c index a30369a..5f453d1 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.
-* -* 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.
+ 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 @@ -19,37 +24,37 @@ #include #include +/** 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; } -- cgit v1.1