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

[Andrew Jones <drjones@redhat.com>]

On Fri, Jan 16, 2015 at 02:52:21PM +0000, Peter Maydell wrote:
> On 13 January 2015 at 15:48, Andrew Jones <drjones@redhat.com> 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.

OK

> 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  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.

OK

> 
> >      }
> > -    *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.

Right. Will do, alternatively I should have brought the PAGE_EXEC
handling below in with patch 2/2, which was my plan, but forgot
to split it out.

> 
> >          *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))) {

yeah, that would be better

> 
> ? (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.

Oh yes, we should check PAGE_READ as well

Thanks for the review. I see from another mail that you'll be sending some
patches I should base the next version on. So I'll hold off on sending a
revised patch until I see that.

drew