Re: [PATCH v1 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Leif,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Leif Lindholm <leif@nuviainc.com> 
Sent: 25 February 2021 11:38 AM
To: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: devel@edk2.groups.io; ardb+tianocore@kernel.org; Matteo Carlini <Matteo.Carlini@arm.com>; Ben Adderson <Ben.Adderson@arm.com>; nd <nd@arm.com>
Subject: Re: [PATCH v1 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

Hi Sami,

On Wed, Feb 24, 2021 at 19:37:56 +0000, Sami Mujawar wrote:
> 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, the error handling logic for the Get/Set Memory attributes
> introduced an issue wherein a status variable could be used without
> initialisation. This issue is reported by CLANG compiler 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 the
> uninitialised variable issue is fixed.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/1657_stmm_ffa_fix_unused_var_v1
> 
>  ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c | 92 ++++++++++----------
>  1 file changed, 45 insertions(+), 47 deletions(-)
> 
> diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
> index a30369af9c91fb8045dfec7a68e2bd072706d101..73b63ca396e5395bdf2112709b0aa2ab871a2a07 100644
> --- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
> +++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
> @@ -57,36 +57,35 @@ GetMemoryPermissions (
>      // for other Direct Request calls which are not atomic
>      // We therefore check only for Direct Response by the
>      // callee.
> -    if (GetMemoryPermissionsSvcArgs.Arg0 !=
> +    if (GetMemoryPermissionsSvcArgs.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 = GetMemoryPermissionsSvcArgs.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 (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 {
> +      // 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:
> +
> +        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 {
>      Ret = GetMemoryPermissionsSvcArgs.Arg0;
> @@ -150,35 +149,34 @@ RequestMemoryPermissionChange (
>      // for other Direct Request calls which are not atomic
>      // We therefore check only for Direct Response by the
>      // callee.
> -    if (ChangeMemoryPermissionsSvcArgs.Arg0 !=
> +    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;
> +    } 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 = 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;
> +      }

This patch applies the same change twice in the same file.
It looks to me like the switch statement should be in a static helper
function.
This would also improve readability of both host functions.

[SAMI] I will send an updated patch shortly.
[/SAMI]

/
    Leif

>      }
>    } else {
>      Ret = ChangeMemoryPermissionsSvcArgs.Arg0;
> -- 
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>