* [PATCH v1 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib
@ 2021-02-24 19:37 Sami Mujawar
2021-02-25 11:37 ` Leif Lindholm
0 siblings, 1 reply; 3+ messages in thread
From: Sami Mujawar @ 2021-02-24 19:37 UTC (permalink / raw)
To: devel; +Cc: Sami Mujawar, ardb+tianocore, leif, Matteo.Carlini, Ben.Adderson,
nd
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;
+ }
}
} else {
Ret = ChangeMemoryPermissionsSvcArgs.Arg0;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v1 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib
2021-02-24 19:37 [PATCH v1 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib Sami Mujawar
@ 2021-02-25 11:37 ` Leif Lindholm
2021-02-25 12:50 ` Sami Mujawar
0 siblings, 1 reply; 3+ messages in thread
From: Leif Lindholm @ 2021-02-25 11:37 UTC (permalink / raw)
To: Sami Mujawar; +Cc: devel, ardb+tianocore, Matteo.Carlini, Ben.Adderson, nd
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.
/
Leif
> }
> } else {
> Ret = ChangeMemoryPermissionsSvcArgs.Arg0;
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v1 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib
2021-02-25 11:37 ` Leif Lindholm
@ 2021-02-25 12:50 ` Sami Mujawar
0 siblings, 0 replies; 3+ messages in thread
From: Sami Mujawar @ 2021-02-25 12:50 UTC (permalink / raw)
To: Leif Lindholm
Cc: devel@edk2.groups.io, ardb+tianocore@kernel.org, Matteo Carlini,
Ben Adderson, nd
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)'
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-25 12:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-24 19:37 [PATCH v1 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib Sami Mujawar
2021-02-25 11:37 ` Leif Lindholm
2021-02-25 12:50 ` Sami Mujawar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox