[PATCH 0/3] Bug fixes related to secure 2 stage translation

[PATCH 0/3] Bug fixes related to secure 2 stage translation

[Idan Horowitz]

Idan Horowitz (3):
  target/arm: Check VSTCR.SW when assigning the stage 2 output PA space
  target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk
  target/arm: Determine final stage 2 output PA space based on original
    IPA

 target/arm/helper.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space

[Idan Horowitz]

As per the AArch64.SS2OutputPASpace() psuedo-code in the ARMv8 ARM when the
PA space of the IPA is non secure, the output PA space is secure if and only
if all of the bits VTCR.<NSW, NSA>, VSTCR.<SW, SA> are not set.

Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 812ca591f4..d0265b760f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12697,7 +12697,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
                 } else {
                     attrs->secure =
                         !((env->cp15.vtcr_el2.raw_tcr & (VTCR_NSA | VTCR_NSW))
-                        || (env->cp15.vstcr_el2.raw_tcr & VSTCR_SA));
+                        || (env->cp15.vstcr_el2.raw_tcr & (VSTCR_SA | VSTCR_SW)));
                 }
             }
             return 0;
-- 
2.35.1

Re: [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space

[Rémi Denis-Courmont]

Le sunnuntaina 27. maaliskuuta 2022, 12.34.26 EEST Idan Horowitz a écrit :
> As per the AArch64.SS2OutputPASpace() psuedo-code in the ARMv8 ARM when the
> PA space of the IPA is non secure, the output PA space is secure if and only
> if all of the bits VTCR.<NSW, NSA>, VSTCR.<SW, SA> are not set.
> 
> Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 812ca591f4..d0265b760f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -12697,7 +12697,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong
> address, } else {
>                      attrs->secure =
>                          !((env->cp15.vtcr_el2.raw_tcr & (VTCR_NSA |
> VTCR_NSW)) -                        || (env->cp15.vstcr_el2.raw_tcr &
> VSTCR_SA)); +                        || (env->cp15.vstcr_el2.raw_tcr &
> (VSTCR_SA | VSTCR_SW))); }

The VTCR_EL2 specification says that the NSA bit "behaves as 1 for all purposes 
other than reading back the value of the bit when one of the following is true 
(...)
* The value of VTCR_EL2.NSW is 1.
* The value of VSTCR_EL2.SA is 1."

Sorry but I don't see any reason to check the SW bit here.

-- 
Реми Дёни-Курмон
http://www.remlab.net/

Re: [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space

[Richard Henderson]

On 3/27/22 08:24, Rémi Denis-Courmont wrote:
> Le sunnuntaina 27. maaliskuuta 2022, 12.34.26 EEST Idan Horowitz a écrit :
>> As per the AArch64.SS2OutputPASpace() psuedo-code in the ARMv8 ARM when the
>> PA space of the IPA is non secure, the output PA space is secure if and only
>> if all of the bits VTCR.<NSW, NSA>, VSTCR.<SW, SA> are not set.
>>
>> Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
>> ---
>>   target/arm/helper.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 812ca591f4..d0265b760f 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -12697,7 +12697,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong
>> address, } else {
>>                       attrs->secure =
>>                           !((env->cp15.vtcr_el2.raw_tcr & (VTCR_NSA |
>> VTCR_NSW)) -                        || (env->cp15.vstcr_el2.raw_tcr &
>> VSTCR_SA)); +                        || (env->cp15.vstcr_el2.raw_tcr &
>> (VSTCR_SA | VSTCR_SW))); }
> 
> The VTCR_EL2 specification says that the NSA bit "behaves as 1 for all purposes
> other than reading back the value of the bit when one of the following is true
> (...)
> * The value of VTCR_EL2.NSW is 1.
> * The value of VSTCR_EL2.SA is 1."
> 
> Sorry but I don't see any reason to check the SW bit here.

Because the description of SA says that it behaves as 1 if SW is 1.


r~

Re: [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space

[Richard Henderson]

On 3/27/22 03:34, Idan Horowitz wrote:
> As per the AArch64.SS2OutputPASpace() psuedo-code in the ARMv8 ARM when the
> PA space of the IPA is non secure, the output PA space is secure if and only
> if all of the bits VTCR.<NSW, NSA>, VSTCR.<SW, SA> are not set.
> 
> Signed-off-by: Idan Horowitz<idan.horowitz@gmail.com>
> ---
>   target/arm/helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

[PATCH 2/3] target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk

[Idan Horowitz]

As per the AArch64.SS2InitialTTWState() psuedo-code in the ARMv8 ARM the
initial PA space used for stage 2 table walks is assigned based on the SW
and NSW bits of the VSTCR and VTCR registers.
This was already implemented for the recursive stage 2 page table walks
in S1_ptw_translate(), but was missing for the final stage 2 walk.

Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
---
 target/arm/helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d0265b760f..e2695e846a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12657,6 +12657,16 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
                 return ret;
             }
 
+            if (arm_is_secure_below_el3(env)) {
+                if (attrs->secure) {
+                    attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & VSTCR_SW);
+                } else {
+                    attrs->secure = !(env->cp15.vtcr_el2.raw_tcr & VTCR_NSW);
+                }
+            } else {
+                assert(!attrs->secure);
+            }
+
             s2_mmu_idx = attrs->secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
             is_el0 = mmu_idx == ARMMMUIdx_E10_0 || mmu_idx == ARMMMUIdx_SE10_0;
 
-- 
2.35.1

Re: [PATCH 2/3] target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk

[Richard Henderson]

On 3/27/22 03:34, Idan Horowitz wrote:
> As per the AArch64.SS2InitialTTWState() psuedo-code in the ARMv8 ARM the
> initial PA space used for stage 2 table walks is assigned based on the SW
> and NSW bits of the VSTCR and VTCR registers.
> This was already implemented for the recursive stage 2 page table walks
> in S1_ptw_translate(), but was missing for the final stage 2 walk.
> 
> Signed-off-by: Idan Horowitz<idan.horowitz@gmail.com>
> ---
>   target/arm/helper.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)

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

r~

[PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA

[Idan Horowitz]

As per the AArch64.S2Walk() psuedo-code in the ARMv8 ARM, the final
decision as to the output address's PA space based on the SA/SW/NSA/NSA
bits needs to take the input IPA's PA space into account, and not the
PA space of the result of the stage 2 walk itself.

Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
---
 target/arm/helper.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index e2695e846a..16c2628f8f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12644,6 +12644,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
             hwaddr ipa;
             int s2_prot;
             int ret;
+            bool ipa_secure;
             ARMCacheAttrs cacheattrs2 = {};
             ARMMMUIdx s2_mmu_idx;
             bool is_el0;
@@ -12657,14 +12658,15 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
                 return ret;
             }
 
+            ipa_secure = attrs->secure;
             if (arm_is_secure_below_el3(env)) {
-                if (attrs->secure) {
+                if (ipa_secure) {
                     attrs->secure = !(env->cp15.vstcr_el2.raw_tcr & VSTCR_SW);
                 } else {
                     attrs->secure = !(env->cp15.vtcr_el2.raw_tcr & VTCR_NSW);
                 }
             } else {
-                assert(!attrs->secure);
+                assert(!ipa_secure);
             }
 
             s2_mmu_idx = attrs->secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
@@ -12701,7 +12703,7 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
 
             /* Check if IPA translates to secure or non-secure PA space. */
             if (arm_is_secure_below_el3(env)) {
-                if (attrs->secure) {
+                if (ipa_secure) {
                     attrs->secure =
                         !(env->cp15.vstcr_el2.raw_tcr & (VSTCR_SA | VSTCR_SW));
                 } else {
-- 
2.35.1

Re: [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA

[Richard Henderson]

On 3/27/22 03:34, Idan Horowitz wrote:
> As per the AArch64.S2Walk() psuedo-code in the ARMv8 ARM, the final
> decision as to the output address's PA space based on the SA/SW/NSA/NSA
> bits needs to take the input IPA's PA space into account, and not the
> PA space of the result of the stage 2 walk itself.
> 
> Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>

I believe I follow: because the walk uses walkstate.address.paddress.paspace, the ipa 
input parameter is unchanged, and it is ipa that is passed to 
AArch64.S2NextWalkStateLast() to form the output address.

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


r~

Re: [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA

[Idan Horowitz]

On Tue, 29 Mar 2022 at 22:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> I believe I follow: because the walk uses walkstate.address.paddress.paspace, the ipa
> input parameter is unchanged, and it is ipa that is passed to
> AArch64.S2NextWalkStateLast() to form the output address.
>

Indeed, I initially found the issue when a test case passed on arm IP
but raised a data abort in QEMU.
Since fixing this issue solved the inconsistency, I believe this is
the intended behaviour and not a spec bug.

>
> r~

Idan Horowitz

Re: [PATCH 3/3] target/arm: Determine final stage 2 output PA space based on original IPA

[Peter Maydell]

On Tue, 29 Mar 2022 at 20:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/27/22 03:34, Idan Horowitz wrote:
> > As per the AArch64.S2Walk() psuedo-code in the ARMv8 ARM, the final
> > decision as to the output address's PA space based on the SA/SW/NSA/NSA
> > bits needs to take the input IPA's PA space into account, and not the
> > PA space of the result of the stage 2 walk itself.
> >
> > Signed-off-by: Idan Horowitz <idan.horowitz@gmail.com>
>
> I believe I follow: because the walk uses walkstate.address.paddress.paspace, the ipa
> input parameter is unchanged, and it is ipa that is passed to
> AArch64.S2NextWalkStateLast() to form the output address.

Textually, this is described on page D5-4802 of DDI 0487H.a;
the security of the output address of the memory access isn't
affected by the security of the output address of the translation
table walk.

-- PMM

Re: [PATCH 0/3] Bug fixes related to secure 2 stage translation

[Peter Maydell]

On Sun, 27 Mar 2022 at 10:34, Idan Horowitz <idan.horowitz@gmail.com> wrote:
>
>
> Idan Horowitz (3):
>   target/arm: Check VSTCR.SW when assigning the stage 2 output PA space
>   target/arm: Take VSTCR.SW, VTCR.NSW into account in final stage 2 walk
>   target/arm: Determine final stage 2 output PA space based on original
>     IPA
>


Applied to target-arm.next, thanks. (I fixed a couple of typos in
the patch 3 commit message.)

-- PMM

Re: [PATCH 1/3] target/arm: Check VSTCR.SW when assigning the stage 2 output PA space

[Idan Horowitz]

Rémi Denis-Courmont <remi@remlab.net> wrote:
> The VTCR_EL2 specification says that the NSA bit "behaves as 1 for all purposes 
> other than reading back the value of the bit when one of the following is true 
> (...)
> * The value of VTCR_EL2.NSW is 1.
> * The value of VSTCR_EL2.SA is 1."
>
> Sorry but I don't see any reason to check the SW bit here.

That still does not cover the case of NSA=0, NSW=0, SA=0, SW=1.

> -- 
> Реми Дёни-Курмон
> http://www.remlab.net/

Idan Horowitz