Re: [Qemu-devel] [PATCH 1/2] tcg-aarch64: user doesn't need R/W access to exec

[Peter Maydell]

On 13 January 2015 at 15:48, Andrew Jones <address@hidden> wrote:
> Table D4-32 shows that execute access from EL0 doesn't depend
> on AP[1].

This commit message is a bit sparse, which confused me
for a bit. It would be worth beefing it up a bit:

target-arm: 64-bit EL0 code can execute from unreadable pages

In AArch64 mode, a page can be executable even if it is not
readable (a difference from AArch32). Instead of bailing out
early if the page is not readable, just add "32 bit and
page not readable" to the list of conditions that make a
page non-executable, and check whether the protections and
the access type are compatible once at the end of the function.

> Signed-off-by: Andrew Jones <address@hidden>
> ---
>  target-arm/helper.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3ef0f1f38eda5..7c30a2669a0f2 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4787,7 +4787,7 @@ static int get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>      hwaddr descaddr, descmask;
>      uint32_t tableattrs;
>      target_ulong page_size;
> -    uint32_t attrs;
> +    uint32_t attrs, ap;
>      int32_t granule_sz = 9;
>      int32_t va_size = 32;
>      int32_t tbi = 0;
> @@ -4952,14 +4952,20 @@ static int get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>          /* Access flag */
>          goto do_fault;
>      }
> +
>      fault_type = permission_fault;
> -    if (is_user && !(attrs & (1 << 4))) {
> -        /* Unprivileged access not enabled */
> -        goto do_fault;
> +    ap = extract32(attrs, 4, 2); /* AP[2:1] */
> +
> +    *prot = 0;
> +    if (!is_user || (ap & 1)) {
> +        *prot |= PAGE_READ;
> +        *prot |= !(ap & 2) ? PAGE_WRITE : 0;

Personally I would find
     if (!(ap & 2)) {
         *prot |= PAGE_WRITE;
     }

clearer.

>      }
> -    *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +
> +    *prot |= PAGE_EXEC;
>      if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 << 12))) 
> ||
>          (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
> +        (!arm_el_is_aa64(env, 1) && is_user && !(ap & 1)) ||
>          (!is_user && (attrs & (1 << 11)))) {
>          /* XN/UXN or PXN. Since we only implement EL0/EL1 we unconditionally
>           * treat XN/UXN as UXN for v8.
> @@ -4969,12 +4975,11 @@ static int get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>          }

There is a "if access_type == 2 goto do_fault" check just
above this hunk which you can delete, because we're now
doing that check in the code you add below.

>          *prot &= ~PAGE_EXEC;
>      }
> -    if (attrs & (1 << 5)) {
> -        /* Write access forbidden */
> -        if (access_type == 1) {
> -            goto do_fault;
> -        }
> -        *prot &= ~PAGE_WRITE;
> +
> +    if ((*prot == 0)
> +            || (!(*prot & PAGE_WRITE) && access_type == 1)
> +            || (!(*prot & PAGE_EXEC) && access_type == 2)) {
> +        goto do_fault;

Why isn't this just
    if (!(*prot & (1 << access_type))) {

? (Or at least, why doesn't it treat PAGE_READ the same way
as the other two bits?) As it is I think we'll treat a page
that is marked exec-not-readable as if it were readable.

thanks
-- PMM