[v2,8/9] target/arm: Check addresses for disabled regimes

Message ID 20200302175829.2183-9-richard.henderson@linaro.org
State New
Headers show
Series
  • target/arm: Misc cleanups surrounding TBI
Related show

Commit Message

Richard Henderson March 2, 2020, 5:58 p.m.
We fail to validate the upper bits of a virtual address on a
translation disabled regime, as per AArch64.TranslateAddressS1Off.

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

---
 target/arm/helper.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

-- 
2.20.1

Comments

Peter Maydell March 5, 2020, 2:21 p.m. | #1
On Mon, 2 Mar 2020 at 17:58, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> We fail to validate the upper bits of a virtual address on a

> translation disabled regime, as per AArch64.TranslateAddressS1Off.

>

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

> ---

>  target/arm/helper.c | 33 ++++++++++++++++++++++++++++++++-

>  1 file changed, 32 insertions(+), 1 deletion(-)

>

> diff --git a/target/arm/helper.c b/target/arm/helper.c

> index addbec91d8..0ef32d3c24 100644

> --- a/target/arm/helper.c

> +++ b/target/arm/helper.c

> @@ -11634,7 +11634,38 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,

>      /* Definitely a real MMU, not an MPU */

>

>      if (regime_translation_disabled(env, mmu_idx)) {

> -        /* MMU disabled. */

> +        /*

> +         * MMU disabled.  S1 addresses are still checked for bounds.

> +         * C.f. AArch64.TranslateAddressS1Off.

> +         */

> +        if (is_a64(env) && mmu_idx != ARMMMUIdx_Stage2) {


This looks weird -- why do we care about whether the current
EL is aarch64, rather than looking at the controlling EL
for the translation regime ?

thanks
-- PMM
Richard Henderson March 5, 2020, 3:57 p.m. | #2
On 3/5/20 6:21 AM, Peter Maydell wrote:
>>      if (regime_translation_disabled(env, mmu_idx)) {

>> -        /* MMU disabled. */

>> +        /*

>> +         * MMU disabled.  S1 addresses are still checked for bounds.

>> +         * C.f. AArch64.TranslateAddressS1Off.

>> +         */

>> +        if (is_a64(env) && mmu_idx != ARMMMUIdx_Stage2) {

> 

> This looks weird -- why do we care about whether the current

> EL is aarch64, rather than looking at the controlling EL

> for the translation regime ?


You're right, it should be the aa64-ness of the regime_el.
Thanks,


r~

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index addbec91d8..0ef32d3c24 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11634,7 +11634,38 @@  bool get_phys_addr(CPUARMState *env, target_ulong address,
     /* Definitely a real MMU, not an MPU */
 
     if (regime_translation_disabled(env, mmu_idx)) {
-        /* MMU disabled. */
+        /*
+         * MMU disabled.  S1 addresses are still checked for bounds.
+         * C.f. AArch64.TranslateAddressS1Off.
+         */
+        if (is_a64(env) && mmu_idx != ARMMMUIdx_Stage2) {
+            int pamax = arm_pamax(env_archcpu(env));
+            uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
+            int addrtop, tbi;
+
+            tbi = aa64_va_parameter_tbi(tcr, mmu_idx);
+            if (access_type == MMU_INST_FETCH) {
+                tbi &= ~aa64_va_parameter_tbid(tcr, mmu_idx);
+            }
+            tbi = (tbi >> extract64(address, 55, 1)) & 1;
+            addrtop = (tbi ? 55 : 63);
+
+            if (extract64(address, pamax, addrtop - pamax + 1) != 0) {
+                fi->type = ARMFault_AddressSize;
+                fi->level = 0;
+                fi->stage2 = false;
+                return 1;
+            }
+
+            /*
+             * The ARM pseudocode copies bits [51:0] to addrdesc.paddress.
+             * Except for TBI, we've just validated everything above PAMax
+             * is zero.  So we only need to drop TBI.
+             */
+            if (tbi) {
+                address = extract64(address, 0, 56);
+            }
+        }
         *phys_ptr = address;
         *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         *page_size = TARGET_PAGE_SIZE;