| Message ID | 20250319072203.327431-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Fix page permissions for platforms running at EL2 | expand |
Hi Ilias, On Wed, 19 Mar 2025 at 08:22, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > We currently set both and print both PXN and UXN bits when removing > execution for pages. This happens even in the existing per platform > definitions of 'struct mm_region'. > > That's not entirely correct though. For stage-1 translations, if a > platform runs on a translation regime with a single privilege level or the > the translation regime supports two privilege levels and we are not > in EL1&0 with HCR_EL2.{NV, NV1} = {1, 1} only BIT54 (XN) is needed > and BIT53(PXN) is reserved 0. > > Currently we support Non-Secure EL2, Non-secure EL2&0 and Non-secure > EL1&0. > > We already have get_effective_el() which returns 1 if we are > - Running in EL1 so we assume an EL1 translation regime but without > checking HCR_EL2.{NV, NV1} != {1,1} > - Running in EL2 with HCR_EL2.E2H = 1 > > The only problem with the above is that if we are in EL1&0 and > HCR_EL2.{NV1, NV} == {1, 1}, then > - Bit[54] holds the PXN instead of the UXN > - The Effective value of UXN is 0 > - Bit[53] is RES0 > > So let's re-use that function and set PXN only when we are in > and EL[2|1]&0 translation regime. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > arch/arm/cpu/armv8/cache_v8.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > Perhaps add a test? Regards, Simon
Hi Simon, On Wed, 19 Mar 2025 at 17:04, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Wed, 19 Mar 2025 at 08:22, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > We currently set both and print both PXN and UXN bits when removing > > execution for pages. This happens even in the existing per platform > > definitions of 'struct mm_region'. > > > > That's not entirely correct though. For stage-1 translations, if a > > platform runs on a translation regime with a single privilege level or the > > the translation regime supports two privilege levels and we are not > > in EL1&0 with HCR_EL2.{NV, NV1} = {1, 1} only BIT54 (XN) is needed > > and BIT53(PXN) is reserved 0. > > > > Currently we support Non-Secure EL2, Non-secure EL2&0 and Non-secure > > EL1&0. > > > > We already have get_effective_el() which returns 1 if we are > > - Running in EL1 so we assume an EL1 translation regime but without > > checking HCR_EL2.{NV, NV1} != {1,1} > > - Running in EL2 with HCR_EL2.E2H = 1 > > > > The only problem with the above is that if we are in EL1&0 and > > HCR_EL2.{NV1, NV} == {1, 1}, then > > - Bit[54] holds the PXN instead of the UXN > > - The Effective value of UXN is 0 > > - Bit[53] is RES0 > > > > So let's re-use that function and set PXN only when we are in > > and EL[2|1]&0 translation regime. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > arch/arm/cpu/armv8/cache_v8.c | 28 ++++++++++++++++++++++++---- > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > Perhaps add a test? Sure, I'll send one, but it obviously can't run on sandbox. We can run in QEMU with an without virt, but we also care for real hardware (which is all problematic AFAICT -- at least the ones that have an EL2 translation regime). Any pointers on where I should add the test? A new file under test? Thanks /Ilias > > Regards, > Simon
Hi Ilias, On Wed, 19 Mar 2025 at 16:22, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Wed, 19 Mar 2025 at 17:04, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Wed, 19 Mar 2025 at 08:22, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > We currently set both and print both PXN and UXN bits when removing > > > execution for pages. This happens even in the existing per platform > > > definitions of 'struct mm_region'. > > > > > > That's not entirely correct though. For stage-1 translations, if a > > > platform runs on a translation regime with a single privilege level or the > > > the translation regime supports two privilege levels and we are not > > > in EL1&0 with HCR_EL2.{NV, NV1} = {1, 1} only BIT54 (XN) is needed > > > and BIT53(PXN) is reserved 0. > > > > > > Currently we support Non-Secure EL2, Non-secure EL2&0 and Non-secure > > > EL1&0. > > > > > > We already have get_effective_el() which returns 1 if we are > > > - Running in EL1 so we assume an EL1 translation regime but without > > > checking HCR_EL2.{NV, NV1} != {1,1} > > > - Running in EL2 with HCR_EL2.E2H = 1 > > > > > > The only problem with the above is that if we are in EL1&0 and > > > HCR_EL2.{NV1, NV} == {1, 1}, then > > > - Bit[54] holds the PXN instead of the UXN > > > - The Effective value of UXN is 0 > > > - Bit[53] is RES0 > > > > > > So let's re-use that function and set PXN only when we are in > > > and EL[2|1]&0 translation regime. > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > --- > > > arch/arm/cpu/armv8/cache_v8.c | 28 ++++++++++++++++++++++++---- > > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > > > > Perhaps add a test? > > Sure, I'll send one, but it obviously can't run on sandbox. > We can run in QEMU with an without virt, but we also care for real > hardware (which is all problematic AFAICT -- at least the ones that > have an EL2 translation regime). > > Any pointers on where I should add the test? A new file under test? I am more talking about the algorithm than the effect. You could write something which calls part of your code (an exported function) and then checks part of the resulting table(s) in memory. For example, if you pass in the value of find_pte() then you could write something which calls pgprot_set_attrs() and then does a few spot checks on the resulting (fake) page tables, e.g. to check that your fix holds. With effort you could run such a test on sandbox, but it might be easier to enable it on QEMU on ARM, bearing in mind that it would not be messing with the real page tables, just checking the algorithm. Probably only a few dozen lines of test code, but it would help provide a basis for people to build on as things grow over time. There also seems to be some code-duplication in this file. Regards, Simon
diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c index 12ae9bd06039..1c1e33bec24f 100644 --- a/arch/arm/cpu/armv8/cache_v8.c +++ b/arch/arm/cpu/armv8/cache_v8.c @@ -575,8 +575,12 @@ static void pretty_print_block_attrs(u64 pte) if (perm_attrs & PTE_BLOCK_PXN) cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "PXN "); - if (perm_attrs & PTE_BLOCK_UXN) - cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "UXN "); + if (perm_attrs & PTE_BLOCK_UXN) { + if (get_effective_el() == 1) + cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "UXN "); + else + cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "XN "); + } if (perm_attrs & PTE_BLOCK_RO) cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "RO"); if (!mem_attrs[0]) @@ -1039,13 +1043,29 @@ int pgprot_set_attrs(phys_addr_t addr, size_t size, enum pgprot_attrs perm) switch (perm) { case MMU_ATTR_RO: - attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO; + /* + * get_effective_el() will return 1 if + * - Running in EL1 so we assume an EL1 translation regime + * with HCR_EL2.{NV, NV1} != {1,1} + * - Running in EL2 with HCR_EL2.E2H = 1 so we assume an + * EL2&0 translation regime. Since we don't have accesses + * from EL0 we don't have to check HCR_EL2.TGE + * + * Both of these requires PXN to be set + */ + if (get_effective_el() == 1) + attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO; + else + attrs |= PTE_BLOCK_UXN | PTE_BLOCK_RO; break; case MMU_ATTR_RX: attrs |= PTE_BLOCK_RO; break; case MMU_ATTR_RW: - attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN; + if (get_effective_el() == 1) + attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN; + else + attrs |= PTE_BLOCK_UXN; break; default: log_err("Unknown attribute %d\n", perm);
We currently set both and print both PXN and UXN bits when removing execution for pages. This happens even in the existing per platform definitions of 'struct mm_region'. That's not entirely correct though. For stage-1 translations, if a platform runs on a translation regime with a single privilege level or the the translation regime supports two privilege levels and we are not in EL1&0 with HCR_EL2.{NV, NV1} = {1, 1} only BIT54 (XN) is needed and BIT53(PXN) is reserved 0. Currently we support Non-Secure EL2, Non-secure EL2&0 and Non-secure EL1&0. We already have get_effective_el() which returns 1 if we are - Running in EL1 so we assume an EL1 translation regime but without checking HCR_EL2.{NV, NV1} != {1,1} - Running in EL2 with HCR_EL2.E2H = 1 The only problem with the above is that if we are in EL1&0 and HCR_EL2.{NV1, NV} == {1, 1}, then - Bit[54] holds the PXN instead of the UXN - The Effective value of UXN is 0 - Bit[53] is RES0 So let's re-use that function and set PXN only when we are in and EL[2|1]&0 translation regime. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- arch/arm/cpu/armv8/cache_v8.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) -- 2.47.2