[PATCH] target/arm: Get correct MMU index for other-security-state

[PATCH] target/arm: Get correct MMU index for other-security-state

[Peter Maydell]

In arm_v7m_mmu_idx_for_secstate() we get the 'priv' level to pass to
armv7m_mmu_idx_for_secstate_and_priv() by calling arm_current_el().
This is incorrect when the security state being queried is not the
current one, because arm_current_el() uses the current security state
to determine which of the banked CONTROL.nPRIV bits to look at.
The effect was that if (for instance) Secure state was in privileged
mode but Non-Secure was not then we would return the wrong MMU index.

The only places where we are using this function in a way that could
trigger this bug are for the stack loads during a v8M function-return
and for the instruction fetch of a v8M SG insn.

Fix the bug by expanding out the M-profile version of the
arm_current_el() logic inline so it can use the passed in secstate
rather than env->v7m.secure.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/m_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 036454234c7..aad01ea0127 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2719,7 +2719,8 @@ ARMMMUIdx arm_v7m_mmu_idx_for_secstate_and_priv(CPUARMState *env,
 /* Return the MMU index for a v7M CPU in the specified security state */
 ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env, bool secstate)
 {
-    bool priv = arm_current_el(env) != 0;
+    bool priv = arm_v7m_is_handler_mode(env) ||
+        !(env->v7m.control[secstate] & 1);
 
     return arm_v7m_mmu_idx_for_secstate_and_priv(env, secstate, priv);
 }
-- 
2.20.1

Re: [PATCH] target/arm: Get correct MMU index for other-security-state

[Peter Maydell]

Ping for code review, please ?

thanks
-- PMM

On Thu, 22 Oct 2020 at 17:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In arm_v7m_mmu_idx_for_secstate() we get the 'priv' level to pass to
> armv7m_mmu_idx_for_secstate_and_priv() by calling arm_current_el().
> This is incorrect when the security state being queried is not the
> current one, because arm_current_el() uses the current security state
> to determine which of the banked CONTROL.nPRIV bits to look at.
> The effect was that if (for instance) Secure state was in privileged
> mode but Non-Secure was not then we would return the wrong MMU index.
>
> The only places where we are using this function in a way that could
> trigger this bug are for the stack loads during a v8M function-return
> and for the instruction fetch of a v8M SG insn.
>
> Fix the bug by expanding out the M-profile version of the
> arm_current_el() logic inline so it can use the passed in secstate
> rather than env->v7m.secure.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/m_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
> index 036454234c7..aad01ea0127 100644
> --- a/target/arm/m_helper.c
> +++ b/target/arm/m_helper.c
> @@ -2719,7 +2719,8 @@ ARMMMUIdx arm_v7m_mmu_idx_for_secstate_and_priv(CPUARMState *env,
>  /* Return the MMU index for a v7M CPU in the specified security state */
>  ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env, bool secstate)
>  {
> -    bool priv = arm_current_el(env) != 0;
> +    bool priv = arm_v7m_is_handler_mode(env) ||
> +        !(env->v7m.control[secstate] & 1);
>
>      return arm_v7m_mmu_idx_for_secstate_and_priv(env, secstate, priv);
>  }
> --
> 2.20.1
>

Re: [PATCH] target/arm: Get correct MMU index for other-security-state

[Richard Henderson]

On 10/22/20 9:44 AM, Peter Maydell wrote:
> In arm_v7m_mmu_idx_for_secstate() we get the 'priv' level to pass to
> armv7m_mmu_idx_for_secstate_and_priv() by calling arm_current_el().
> This is incorrect when the security state being queried is not the
> current one, because arm_current_el() uses the current security state
> to determine which of the banked CONTROL.nPRIV bits to look at.
> The effect was that if (for instance) Secure state was in privileged
> mode but Non-Secure was not then we would return the wrong MMU index.
> 
> The only places where we are using this function in a way that could
> trigger this bug are for the stack loads during a v8M function-return
> and for the instruction fetch of a v8M SG insn.
> 
> Fix the bug by expanding out the M-profile version of the
> arm_current_el() logic inline so it can use the passed in secstate
> rather than env->v7m.secure.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/m_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~