[PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[Timofey Kutergin]

    - Use CPSR.PAN to check for PAN state in aarch32 mode
    - throw permission fault during address translation when PAN is
      enabled and kernel tries to access user acessible page
    - ignore SCTLR_XP bit for armv7 and armv8 (conflicts with SCTLR_SPAN).

Signed-off-by: Timofey Kutergin <tkutergin@gmail.com>
---
 target/arm/helper.c | 13 +++++++++++--
 target/arm/ptw.c    | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index c672903f43..4301478ed8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10992,6 +10992,15 @@ ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env, bool secstate)
 }
 #endif
 
+static bool arm_pan_enabled(CPUARMState *env)
+{
+    if (is_a64(env)) {
+        return env->pstate & PSTATE_PAN;
+    } else {
+        return env->uncached_cpsr & CPSR_PAN;
+    }
+}
+
 ARMMMUIdx arm_mmu_idx_el(CPUARMState *env, int el)
 {
     ARMMMUIdx idx;
@@ -11012,7 +11021,7 @@ ARMMMUIdx arm_mmu_idx_el(CPUARMState *env, int el)
         }
         break;
     case 1:
-        if (env->pstate & PSTATE_PAN) {
+        if (arm_pan_enabled(env)) {
             idx = ARMMMUIdx_E10_1_PAN;
         } else {
             idx = ARMMMUIdx_E10_1;
@@ -11021,7 +11030,7 @@ ARMMMUIdx arm_mmu_idx_el(CPUARMState *env, int el)
     case 2:
         /* Note that TGE does not apply at EL2.  */
         if (arm_hcr_el2_eff(env) & HCR_E2H) {
-            if (env->pstate & PSTATE_PAN) {
+            if (arm_pan_enabled(env)) {
                 idx = ARMMMUIdx_E20_2_PAN;
             } else {
                 idx = ARMMMUIdx_E20_2;
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 6c5ed56a10..a82accab40 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -433,12 +433,11 @@ static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
  * @mmu_idx:     MMU index indicating required translation regime
  * @ap:          The 3-bit access permissions (AP[2:0])
  * @domain_prot: The 2-bit domain access permissions
+ * @is_user: TRUE if accessing from PL0
  */
-static int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
-                         int ap, int domain_prot)
+static int ap_to_rw_prot_is_user(CPUARMState *env, ARMMMUIdx mmu_idx,
+                         int ap, int domain_prot, bool is_user)
 {
-    bool is_user = regime_is_user(env, mmu_idx);
-
     if (domain_prot == 3) {
         return PAGE_READ | PAGE_WRITE;
     }
@@ -482,6 +481,20 @@ static int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
     }
 }
 
+/*
+ * Translate section/page access permissions to page R/W protection flags
+ * @env:         CPUARMState
+ * @mmu_idx:     MMU index indicating required translation regime
+ * @ap:          The 3-bit access permissions (AP[2:0])
+ * @domain_prot: The 2-bit domain access permissions
+ */
+static int ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
+                         int ap, int domain_prot)
+{
+   return ap_to_rw_prot_is_user(env, mmu_idx, ap, domain_prot,
+                                regime_is_user(env, mmu_idx));
+}
+
 /*
  * Translate section/page access permissions to page R/W protection flags.
  * @ap:      The 2-bit simple AP (AP[2:1])
@@ -644,6 +657,7 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
     hwaddr phys_addr;
     uint32_t dacr;
     bool ns;
+    int user_prot;
 
     /* Pagetable walk.  */
     /* Lookup l1 descriptor.  */
@@ -749,8 +763,10 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
                 goto do_fault;
             }
             result->f.prot = simple_ap_to_rw_prot(env, mmu_idx, ap >> 1);
+            user_prot = simple_ap_to_rw_prot_is_user(ap >> 1, 1);
         } else {
             result->f.prot = ap_to_rw_prot(env, mmu_idx, ap, domain_prot);
+            user_prot = ap_to_rw_prot_is_user(env, mmu_idx, ap, domain_prot, 1);
         }
         if (result->f.prot && !xn) {
             result->f.prot |= PAGE_EXEC;
@@ -760,6 +776,14 @@ static bool get_phys_addr_v6(CPUARMState *env, S1Translate *ptw,
             fi->type = ARMFault_Permission;
             goto do_fault;
         }
+        if (regime_is_pan(env, mmu_idx) &&
+            !regime_is_user(env, mmu_idx) &&
+            user_prot &&
+            access_type != MMU_INST_FETCH) {
+            /* Privileged Access Never fault */
+            fi->type = ARMFault_Permission;
+            goto do_fault;
+        }
     }
     if (ns) {
         /* The NS bit will (as required by the architecture) have no effect if
@@ -2606,7 +2630,8 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
     if (regime_using_lpae_format(env, mmu_idx)) {
         return get_phys_addr_lpae(env, ptw, address, access_type, false,
                                   result, fi);
-    } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
+    } else if (arm_feature(env, ARM_FEATURE_V7) ||
+               regime_sctlr(env, mmu_idx) & SCTLR_XP) {
         return get_phys_addr_v6(env, ptw, address, access_type, result, fi);
     } else {
         return get_phys_addr_v5(env, ptw, address, access_type, result, fi);
-- 
2.25.1

Re: [PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[Peter Maydell]

On Thu, 27 Oct 2022 at 12:26, Timofey Kutergin <tkutergin@gmail.com> wrote:
>
>     - Use CPSR.PAN to check for PAN state in aarch32 mode
>     - throw permission fault during address translation when PAN is
>       enabled and kernel tries to access user acessible page
>     - ignore SCTLR_XP bit for armv7 and armv8 (conflicts with SCTLR_SPAN).
>
> Signed-off-by: Timofey Kutergin <tkutergin@gmail.com>
> ---
>  target/arm/helper.c | 13 +++++++++++--
>  target/arm/ptw.c    | 35 ++++++++++++++++++++++++++++++-----
>  2 files changed, 41 insertions(+), 7 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Not sure if I'll be able to get this in before softfreeze,
but since it's a bug fix it'll be in 7.2.

thanks
-- PMM

Re: [PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[Peter Maydell]

On Fri, 28 Oct 2022 at 19:17, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 27 Oct 2022 at 12:26, Timofey Kutergin <tkutergin@gmail.com> wrote:
> >
> >     - Use CPSR.PAN to check for PAN state in aarch32 mode
> >     - throw permission fault during address translation when PAN is
> >       enabled and kernel tries to access user acessible page
> >     - ignore SCTLR_XP bit for armv7 and armv8 (conflicts with SCTLR_SPAN).
> >
> > Signed-off-by: Timofey Kutergin <tkutergin@gmail.com>
> > ---
> >  target/arm/helper.c | 13 +++++++++++--
> >  target/arm/ptw.c    | 35 ++++++++++++++++++++++++++++++-----
> >  2 files changed, 41 insertions(+), 7 deletions(-)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Not sure if I'll be able to get this in before softfreeze,
> but since it's a bug fix it'll be in 7.2.



Applied to target-arm.next, thanks.

-- PMM

[PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[Timofey Kutergin]

- synchronize PSTATE.PAN with changes in CPSR.PAN in aarch32 mode
- set PAN bit automatically on exception entry if SCTLR_SPAN bit
  is set
- throw permission fault during address translation when PAN is
  enabled and kernel tries to access user acessible page
- ignore SCTLR_XP bit for armv7 and armv8 (conflicts with SCTLR_SPAN).

Signed-off-by: Timofey Kutergin <tkutergin@gmail.com>
---
 target/arm/helper.c |  6 ++++++
 target/arm/ptw.c    | 11 ++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index dde64a487a..5299f67e3f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9052,6 +9052,11 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
     }
     mask &= ~CACHED_CPSR_BITS;
     env->uncached_cpsr = (env->uncached_cpsr & ~mask) | (val & mask);
+    if (env->uncached_cpsr & CPSR_PAN) {
+        env->pstate |= PSTATE_PAN;
+    } else {
+        env->pstate &= ~PSTATE_PAN;
+    }
     if (rebuild_hflags) {
         arm_rebuild_hflags(env);
     }
@@ -9592,6 +9597,7 @@ static void take_aarch32_exception(CPUARMState *env, int new_mode,
                 /* ... the target is EL1 and SCTLR.SPAN is 0.  */
                 if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPAN)) {
                     env->uncached_cpsr |= CPSR_PAN;
+                    env->pstate |= PSTATE_PAN;
                 }
                 break;
             }
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 23f16f4ff7..204a73350f 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -659,6 +659,13 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
             goto do_fault;
         }
 
+        if (regime_is_pan(env, mmu_idx) && !regime_is_user(env, mmu_idx) &&
+            simple_ap_to_rw_prot_is_user(ap >> 1, 1) &&
+            access_type != MMU_INST_FETCH) {
+            fi->type = ARMFault_Permission;
+            goto do_fault;
+        }
+
         if (arm_feature(env, ARM_FEATURE_V6K) &&
                 (regime_sctlr(env, mmu_idx) & SCTLR_AFE)) {
             /* The simplified model uses AP[0] as an access control bit.  */
@@ -2506,7 +2513,9 @@ bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
     if (regime_using_lpae_format(env, mmu_idx)) {
         return get_phys_addr_lpae(env, address, access_type, mmu_idx,
                                   is_secure, false, result, fi);
-    } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
+    } else if (arm_feature(env, ARM_FEATURE_V7) ||
+               arm_feature(env, ARM_FEATURE_V8) || (
+               regime_sctlr(env, mmu_idx) & SCTLR_XP)) {
         return get_phys_addr_v6(env, address, access_type, mmu_idx,
                                 is_secure, result, fi);
     } else {
-- 
2.25.1

Re: [PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[Peter Maydell]

On Wed, 19 Oct 2022 at 13:15, Timofey Kutergin <tkutergin@gmail.com> wrote:
>
> - synchronize PSTATE.PAN with changes in CPSR.PAN in aarch32 mode
> - set PAN bit automatically on exception entry if SCTLR_SPAN bit
>   is set
> - throw permission fault during address translation when PAN is
>   enabled and kernel tries to access user acessible page
> - ignore SCTLR_XP bit for armv7 and armv8 (conflicts with SCTLR_SPAN).
>
> Signed-off-by: Timofey Kutergin <tkutergin@gmail.com>
> ---
>  target/arm/helper.c |  6 ++++++
>  target/arm/ptw.c    | 11 ++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)

Thanks for this patch. I think you've caught all the places
we aren't correctly implementing AArch32 PAN handling.

> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index dde64a487a..5299f67e3f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9052,6 +9052,11 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
>      }
>      mask &= ~CACHED_CPSR_BITS;
>      env->uncached_cpsr = (env->uncached_cpsr & ~mask) | (val & mask);
> +    if (env->uncached_cpsr & CPSR_PAN) {
> +        env->pstate |= PSTATE_PAN;
> +    } else {
> +        env->pstate &= ~PSTATE_PAN;
> +    }

This approach means we're storing the PAN bit in two places,
both in env->uncached_cpsr and in env->pstate. We don't do
this for any other bits as far as I can see. I think we should
either:
 (1) have the code that changes behaviour based on PAN look
     at either env->pstate or env->uncached_cpsr depending
     on whether we're AArch64 or AArch32
 (2) always store the state in env->pstate only, and handle
     this in read/write of the CPSR the same way we do with
     other "cached" bits

I think the intention of the current code is (1), and the
only place we get this wrong is in arm_mmu_idx_el(),
which is checking env->pstate only. (The other places that
directly check env->pstate are all in AArch64-only code,
and various AArch32-only bits of code already check
env->uncached_cpsr.) A function like

bool arm_pan_enabled(CPUARMState *env)
{
    if (is_a64(env)) {
        return env->pstate & PSTATE_PAN;
    } else {
        return env->uncached_cpsr & CPSR_PAN;
    }
}

and then using that in arm_mmu_idx_el() should I think
mean you don't need to change either cpsr_write() or
take_aarch32_exception().

>      if (rebuild_hflags) {
>          arm_rebuild_hflags(env);
>      }
> @@ -9592,6 +9597,7 @@ static void take_aarch32_exception(CPUARMState *env, int new_mode,
>                  /* ... the target is EL1 and SCTLR.SPAN is 0.  */
>                  if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPAN)) {
>                      env->uncached_cpsr |= CPSR_PAN;
> +                    env->pstate |= PSTATE_PAN;
>                  }
>                  break;
>              }
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 23f16f4ff7..204a73350f 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -659,6 +659,13 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
>              goto do_fault;
>          }
>
> +        if (regime_is_pan(env, mmu_idx) && !regime_is_user(env, mmu_idx) &&
> +            simple_ap_to_rw_prot_is_user(ap >> 1, 1) &&
> +            access_type != MMU_INST_FETCH) {
> +            fi->type = ARMFault_Permission;
> +            goto do_fault;
> +        }

This assumes we're using the SCTLR.AFE==1 simplified
permissions model, but PAN should apply even if we're using the
old model. So we need a ap_to_rw_prot_is_user() to check the
permissions in that model.

The check is also being done before the Access fault check, but
the architecture says that Access faults take priority over
Permission faults.

> +
>          if (arm_feature(env, ARM_FEATURE_V6K) &&
>                  (regime_sctlr(env, mmu_idx) & SCTLR_AFE)) {
>              /* The simplified model uses AP[0] as an access control bit.  */
> @@ -2506,7 +2513,9 @@ bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
>      if (regime_using_lpae_format(env, mmu_idx)) {
>          return get_phys_addr_lpae(env, address, access_type, mmu_idx,
>                                    is_secure, false, result, fi);
> -    } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
> +    } else if (arm_feature(env, ARM_FEATURE_V7) ||
> +               arm_feature(env, ARM_FEATURE_V8) || (

V8 always implies V7, so we only need to check V7 here.

> +               regime_sctlr(env, mmu_idx) & SCTLR_XP)) {
>          return get_phys_addr_v6(env, address, access_type, mmu_idx,
>                                  is_secure, result, fi);
>      } else {

thanks
-- PMM

Re: [PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[Timofey Kutergin]

[-- Attachment #1: Type: text/plain, Size: 5317 bytes --]

Hi Peter,
> V8 always implies V7, so we only need to check V7 here.
From silicon perspective - yes, but as I see in qemu,
ARM_FEATURE_V7 and ARM_FEATURE_V8 are independent bits which do not affect
each
other in arm_feature() and set_feature() so they should be tested
separately.
Did I miss something?

Thanks
Best regards
Timofey



On Tue, Oct 25, 2022 at 4:45 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Wed, 19 Oct 2022 at 13:15, Timofey Kutergin <tkutergin@gmail.com>
> wrote:
> >
> > - synchronize PSTATE.PAN with changes in CPSR.PAN in aarch32 mode
> > - set PAN bit automatically on exception entry if SCTLR_SPAN bit
> >   is set
> > - throw permission fault during address translation when PAN is
> >   enabled and kernel tries to access user acessible page
> > - ignore SCTLR_XP bit for armv7 and armv8 (conflicts with SCTLR_SPAN).
> >
> > Signed-off-by: Timofey Kutergin <tkutergin@gmail.com>
> > ---
> >  target/arm/helper.c |  6 ++++++
> >  target/arm/ptw.c    | 11 ++++++++++-
> >  2 files changed, 16 insertions(+), 1 deletion(-)
>
> Thanks for this patch. I think you've caught all the places
> we aren't correctly implementing AArch32 PAN handling.
>
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index dde64a487a..5299f67e3f 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -9052,6 +9052,11 @@ void cpsr_write(CPUARMState *env, uint32_t val,
> uint32_t mask,
> >      }
> >      mask &= ~CACHED_CPSR_BITS;
> >      env->uncached_cpsr = (env->uncached_cpsr & ~mask) | (val & mask);
> > +    if (env->uncached_cpsr & CPSR_PAN) {
> > +        env->pstate |= PSTATE_PAN;
> > +    } else {
> > +        env->pstate &= ~PSTATE_PAN;
> > +    }
>
> This approach means we're storing the PAN bit in two places,
> both in env->uncached_cpsr and in env->pstate. We don't do
> this for any other bits as far as I can see. I think we should
> either:
>  (1) have the code that changes behaviour based on PAN look
>      at either env->pstate or env->uncached_cpsr depending
>      on whether we're AArch64 or AArch32
>  (2) always store the state in env->pstate only, and handle
>      this in read/write of the CPSR the same way we do with
>      other "cached" bits
>
> I think the intention of the current code is (1), and the
> only place we get this wrong is in arm_mmu_idx_el(),
> which is checking env->pstate only. (The other places that
> directly check env->pstate are all in AArch64-only code,
> and various AArch32-only bits of code already check
> env->uncached_cpsr.) A function like
>
> bool arm_pan_enabled(CPUARMState *env)
> {
>     if (is_a64(env)) {
>         return env->pstate & PSTATE_PAN;
>     } else {
>         return env->uncached_cpsr & CPSR_PAN;
>     }
> }
>
> and then using that in arm_mmu_idx_el() should I think
> mean you don't need to change either cpsr_write() or
> take_aarch32_exception().
>
> >      if (rebuild_hflags) {
> >          arm_rebuild_hflags(env);
> >      }
> > @@ -9592,6 +9597,7 @@ static void take_aarch32_exception(CPUARMState
> *env, int new_mode,
> >                  /* ... the target is EL1 and SCTLR.SPAN is 0.  */
> >                  if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPAN)) {
> >                      env->uncached_cpsr |= CPSR_PAN;
> > +                    env->pstate |= PSTATE_PAN;
> >                  }
> >                  break;
> >              }
> > diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> > index 23f16f4ff7..204a73350f 100644
> > --- a/target/arm/ptw.c
> > +++ b/target/arm/ptw.c
> > @@ -659,6 +659,13 @@ static bool get_phys_addr_v6(CPUARMState *env,
> uint32_t address,
> >              goto do_fault;
> >          }
> >
> > +        if (regime_is_pan(env, mmu_idx) && !regime_is_user(env,
> mmu_idx) &&
> > +            simple_ap_to_rw_prot_is_user(ap >> 1, 1) &&
> > +            access_type != MMU_INST_FETCH) {
> > +            fi->type = ARMFault_Permission;
> > +            goto do_fault;
> > +        }
>
> This assumes we're using the SCTLR.AFE==1 simplified
> permissions model, but PAN should apply even if we're using the
> old model. So we need a ap_to_rw_prot_is_user() to check the
> permissions in that model.
>
> The check is also being done before the Access fault check, but
> the architecture says that Access faults take priority over
> Permission faults.
>
> > +
> >          if (arm_feature(env, ARM_FEATURE_V6K) &&
> >                  (regime_sctlr(env, mmu_idx) & SCTLR_AFE)) {
> >              /* The simplified model uses AP[0] as an access control
> bit.  */
> > @@ -2506,7 +2513,9 @@ bool get_phys_addr_with_secure(CPUARMState *env,
> target_ulong address,
> >      if (regime_using_lpae_format(env, mmu_idx)) {
> >          return get_phys_addr_lpae(env, address, access_type, mmu_idx,
> >                                    is_secure, false, result, fi);
> > -    } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
> > +    } else if (arm_feature(env, ARM_FEATURE_V7) ||
> > +               arm_feature(env, ARM_FEATURE_V8) || (
>
> V8 always implies V7, so we only need to check V7 here.
>
> > +               regime_sctlr(env, mmu_idx) & SCTLR_XP)) {
> >          return get_phys_addr_v6(env, address, access_type, mmu_idx,
> >                                  is_secure, result, fi);
> >      } else {
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 7504 bytes --]

Re: [PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[Peter Maydell]

On Thu, 27 Oct 2022 at 10:22, Timofey Kutergin <tkutergin@gmail.com> wrote:
> > V8 always implies V7, so we only need to check V7 here.

> From silicon perspective - yes, but as I see in qemu,
> ARM_FEATURE_V7 and ARM_FEATURE_V8 are independent bits which do not affect each
> other in arm_feature() and set_feature() so they should be tested separately.
> Did I miss something?

In arm_cpu_realizefn() there is code which sets feature flags
that are always implied by other feature flags. There we set
the V7VE flag if V8 is set, and the V7 flag if V7VE is set.
So we can rely on any v8 CPU having the V7 feature flag set.

thanks
-- PMM

Re: [PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[Timofey Kutergin]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

Understood, thank you a lot :)

Best regards
Timofey


On Thu, Oct 27, 2022 at 12:35 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 27 Oct 2022 at 10:22, Timofey Kutergin <tkutergin@gmail.com>
> wrote:
> > > V8 always implies V7, so we only need to check V7 here.
>
> > From silicon perspective - yes, but as I see in qemu,
> > ARM_FEATURE_V7 and ARM_FEATURE_V8 are independent bits which do not
> affect each
> > other in arm_feature() and set_feature() so they should be tested
> separately.
> > Did I miss something?
>
> In arm_cpu_realizefn() there is code which sets feature flags
> that are always implied by other feature flags. There we set
> the V7VE flag if V8 is set, and the V7 flag if V7VE is set.
> So we can rely on any v8 CPU having the V7 feature flag set.
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 1246 bytes --]

[PATCH] target/arm: Fixed Privileged Access Never (PAN) for aarch32

[Timofey Kutergin]

- synchronize PSTATE.PAN with changes in CPSR.PAN in aarch32 mode
- set PAN bit automatically on exception entry if SCTLR_SPAN bit
  is set
- throw permission fault during address translation when PAN is
  enabled and kernel tries to access user acessible page
- ignore SCTLR_XP bit for armv7 and armv8 (conflicts with SCTLR_SPAN).
---
 target/arm/helper.c |  6 ++++++
 target/arm/ptw.c    | 11 ++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index dde64a487a..5299f67e3f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9052,6 +9052,11 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask,
     }
     mask &= ~CACHED_CPSR_BITS;
     env->uncached_cpsr = (env->uncached_cpsr & ~mask) | (val & mask);
+    if (env->uncached_cpsr & CPSR_PAN) {
+        env->pstate |= PSTATE_PAN;
+    } else {
+        env->pstate &= ~PSTATE_PAN;
+    }
     if (rebuild_hflags) {
         arm_rebuild_hflags(env);
     }
@@ -9592,6 +9597,7 @@ static void take_aarch32_exception(CPUARMState *env, int new_mode,
                 /* ... the target is EL1 and SCTLR.SPAN is 0.  */
                 if (!(env->cp15.sctlr_el[new_el] & SCTLR_SPAN)) {
                     env->uncached_cpsr |= CPSR_PAN;
+                    env->pstate |= PSTATE_PAN;
                 }
                 break;
             }
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 23f16f4ff7..204a73350f 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -659,6 +659,13 @@ static bool get_phys_addr_v6(CPUARMState *env, uint32_t address,
             goto do_fault;
         }
 
+        if (regime_is_pan(env, mmu_idx) && !regime_is_user(env, mmu_idx) &&
+            simple_ap_to_rw_prot_is_user(ap >> 1, 1) &&
+            access_type != MMU_INST_FETCH) {
+            fi->type = ARMFault_Permission;
+            goto do_fault;
+        }
+
         if (arm_feature(env, ARM_FEATURE_V6K) &&
                 (regime_sctlr(env, mmu_idx) & SCTLR_AFE)) {
             /* The simplified model uses AP[0] as an access control bit.  */
@@ -2506,7 +2513,9 @@ bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
     if (regime_using_lpae_format(env, mmu_idx)) {
         return get_phys_addr_lpae(env, address, access_type, mmu_idx,
                                   is_secure, false, result, fi);
-    } else if (regime_sctlr(env, mmu_idx) & SCTLR_XP) {
+    } else if (arm_feature(env, ARM_FEATURE_V7) ||
+               arm_feature(env, ARM_FEATURE_V8) || (
+               regime_sctlr(env, mmu_idx) & SCTLR_XP)) {
         return get_phys_addr_v6(env, address, access_type, mmu_idx,
                                 is_secure, result, fi);
     } else {
-- 
2.25.1