Re: [Qemu-ppc] [RFC NO-MERGE 03/12] target/ppc: Move no-execute and guarded page checking into new function

[David Gibson]

On Fri, Feb 17, 2017 at 04:08:03PM +1100, Suraj Jitindar Singh wrote:
> A pte entry has bit fields which can be used to make a page no-execute or
> guarded, if either of these bits are set then an instruction access to this
> page will fail. Currently these bits are checked with the pp_prot function
> however the ISA specifies that the access authority controlled by the
> key-pp value pair should only be checked on an instruction access after
> the no-execute and guard bits have already been verified to permit the
> access.
> 
> Move the no-execute and guard bit checking into a new separate function.
> Note that we can remove the check for the no-execute bit in the slb entry
> since this check was already performed above when we obtained the slb
> entry.
> 
> In the event that the no-execute or guard bits are set, an ISI should be
> generated with the SRR1_NOEXEC_GUARD (0x10000000) bit set in srr1. Add a
> define for this for clarity.
> 
> Signed-off-by: Suraj Jitindar Singh <address@hidden>
> ---
>  target/ppc/mmu-hash64.c | 25 ++++++++++++++++---------
>  target/ppc/mmu.h        |  2 ++
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index de507b9..a7ffedf 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -338,6 +338,14 @@ void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void 
> *hpt, int shift,
>      }
>  }
>  
> +/* Check No-Execute or Guarded Storage */
> +static inline bool ppc_hash64_pte_noexec_guard(PowerPCCPU *cpu,
> +                                               ppc_hash_pte64_t pte)
> +{
> +    return (pte.pte1 & HPTE64_R_N) || (pte.pte1 & HPTE64_R_G);
> +}
> +
> +/* Check Basic Storage Protection */
>  static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
>                                 ppc_slb_t *slb, ppc_hash_pte64_t pte)
>  {
> @@ -381,12 +389,6 @@ static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
>          }
>      }
>  
> -    /* No execute if either noexec or guarded bits set */
> -    if (!(pte.pte1 & HPTE64_R_N) || (pte.pte1 & HPTE64_R_G)
> -        || (slb->vsid & SLB_VSID_N)) {
> -        prot |= PAGE_EXEC;
> -    }
> -
>      return prot;
>  }
>  
> @@ -734,7 +736,7 @@ int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr 
> eaddr,
>      unsigned apshift;
>      hwaddr pte_offset;
>      ppc_hash_pte64_t pte;
> -    int pp_prot, amr_prot, prot;
> +    int exec_prot, pp_prot, amr_prot, prot;
>      uint64_t new_pte1, dsisr;
>      const int need_prot[] = {PAGE_READ, PAGE_WRITE, PAGE_EXEC};
>      hwaddr raddr;
> @@ -842,16 +844,21 @@ skip_slb_search:
>  
>      /* 5. Check access permissions */
>  
> +    exec_prot = ppc_hash64_pte_noexec_guard(cpu, pte) ? 0 : PAGE_EXEC;
>      pp_prot = ppc_hash64_pte_prot(cpu, slb, pte);
>      amr_prot = ppc_hash64_amr_prot(cpu, pte);
> -    prot = pp_prot & amr_prot;
> +    /* Exec permissions CANNOT take away read or write permissions */
> +    prot = exec_prot | PAGE_READ | PAGE_WRITE;
> +    prot &= pp_prot & amr_prot;

Ok.. so, I take back what I said on the last patch.  Looking at this
whole lot again, it becomes clearer that this is basically applying a
sequence of masks to an initial permit-everything mask.

I don't particularly mind how that masking is done - even returning
the not-permitted bits is ok - as long as each of the masks is done
consistently.  So, they *all* return a permitted mask, or the *all*
return a not-permitted mask.  At the moment these aren't consistent,
some return a boolean equivalent, some return a mask and so forth.

>  
>      if ((need_prot[rwx] & ~prot) != 0) {
>          /* Access right violation */
>          qemu_log_mask(CPU_LOG_MMU, "PTE access rejected\n");
>          if (rwx == 2) {
>              int srr1 = 0;
> -            if (PAGE_EXEC & ~pp_prot) {
> +         if (PAGE_EXEC & ~exec_prot) {
> +                srr1 |= SRR1_NOEXEC_GUARD; /* Access violates noexec or 
> guard */
> +            } else if (PAGE_EXEC & ~pp_prot) { /* noexec takes precedence */
>                  srr1 |= SRR1_PROTFAULT; /* Access violates access authority 
> */
>              }
>              if (PAGE_EXEC & ~amr_prot) {
> diff --git a/target/ppc/mmu.h b/target/ppc/mmu.h
> index 1cee142..cfda7f1 100644
> --- a/target/ppc/mmu.h
> +++ b/target/ppc/mmu.h
> @@ -28,6 +28,8 @@
>  /* Interrupt Fields */
>  
>  /* SRR1 */
> +/* Not permitted due to no-execute or guard bit set */
> +#define SRR1_NOEXEC_GUARD        0x10000000
>  #define SRR1_PROTFAULT                0x08000000
>  #define SRR1_IAMR             0x00200000
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature