Message ID | 1421164091-19989-2-git-send-email-drjones@redhat.com |
---|---|
State | New |
Headers | show |
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. > 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. > } > - *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
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
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; } - *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, } *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; } *phys_ptr = descaddr;
Table D4-32 shows that execute access from EL0 doesn't depend on AP[1]. Signed-off-by: Andrew Jones <drjones@redhat.com> --- target-arm/helper.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)