[PATCH v4 13/20] arm64: reorganise PAGE_/PROT_ macros
Ard Biesheuvel
ardb at kernel.org
Mon Aug 28 03:30:23 PDT 2023
On Thu, 24 Aug 2023 at 15:10, Joey Gouly <joey.gouly at arm.com> wrote:
>
> On Thu, Aug 24, 2023 at 12:18:51PM +0200, Ard Biesheuvel wrote:
> > On Thu, 24 Aug 2023 at 12:16, Joey Gouly <joey.gouly at arm.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Tue, Aug 22, 2023 at 04:10:35PM +0200, Ard Biesheuvel wrote:
> > > > On Tue, 6 Jun 2023 at 17:00, Joey Gouly <joey.gouly at arm.com> wrote:
> > > > >
> > > > > Make these macros available to assembly code, so they can be re-used by the
> > > > > PIE initialisation code.
> > > > >
> > > > > This involves adding some extra macros, prepended with _ that are the raw
> > > > > values not `pgprot` values.
> > > > >
> > > > > A dummy value for PTE_MAYBE_NG is also provided, for use in assembly.
> > > > >
> > > > ...
> > > > > +
> > > > > +#ifdef __ASSEMBLY__
> > > > > +#define PTE_MAYBE_NG 0
> > > > > +#endif
> > > > > +
> > > >
> > > > I am struggling a bit to understand why this is ok. I get that the PIE
> > > > index macros mask off the nG bit even if it is set, but this exposes a
> > > > definition of PROT_DEFAULT and everything based on it to asm code that
> > > > deviates from the one observed by C code.
> > >
> > > Yes, it's a bit of a hack to share as much as possible, and it's "ok" because,
> > > as you said PIE masks that bit out.
> > >
> > > >
> > > > I am running into this because I am adding PTE_MAYBE_SHARED for LPA2
> > > > support (which repurposes the shareability bits as output address
> > > > bits), and I could just #define it to 0x0 as well for assembly, but I
> > > > am not sure this is the right approach.
> > >
> > > Happy to do this differently, if there is a better approach.
> > >
> > > I reverted this patch (fa4cdccaa582), and applied something like (just compile tested):
> > >
> > > diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> > > index c7d77333ce1e..8fceeb111ad1 100644
> > > --- a/arch/arm64/include/asm/pgtable-prot.h
> > > +++ b/arch/arm64/include/asm/pgtable-prot.h
> > > @@ -20,6 +20,17 @@
> > > #define PTE_DEVMAP (_AT(pteval_t, 1) << 57)
> > > #define PTE_PROT_NONE (_AT(pteval_t, 1) << 58) /* only when !PTE_VALID */
> > >
> > > +#define PIE_PAGE_SHARED (PTE_USER | PTE_PXN | PTE_UXN | PTE_WRITE)
> > > +#define PIE_PAGE_SHARED_EXEC (PTE_USER | PTE_PXN | PTE_WRITE)
> > > +#define PIE_PAGE_READONLY (PTE_USER | PTE_PXN | PTE_UXN)
> > > +#define PIE_PAGE_READONLY_EXEC (PTE_USER | PTE_PXN)
> > > +#define PIE_PAGE_EXECONLY (PTE_PXN)
> > > +
> > > +#define PIE_PAGE_KERNEL (PTE_PXN | PTE_UXN | PTE_WRITE)
> > > +#define PIE_PAGE_KERNEL_RO (PTE_PXN | PTE_UXN)
> > > +#define PIE_PAGE_KERNEL_ROX (PTE_UXN)
> > > +#define PIE_PAGE_KERNEL_EXEC (PTE_UXN | PTE_WRITE)
> > > +
> > > /*
> > > * This bit indicates that the entry is present i.e. pmd_page()
> > > * still points to a valid huge page in memory even if the pmd
> > > @@ -83,11 +94,11 @@ extern bool arm64_use_ng_mappings;
> > >
> > > #define PAGE_NONE __pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN)
> > > /* shared+writable pages are clean by default, hence PTE_RDONLY|PTE_WRITE */
> > > -#define PAGE_SHARED __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
> > > -#define PAGE_SHARED_EXEC __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_WRITE)
> > > -#define PAGE_READONLY __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN)
> > > -#define PAGE_READONLY_EXEC __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_RDONLY | PTE_NG | PTE_PXN)
> > > -#define PAGE_EXECONLY __pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PTE_PXN)
> > > +#define PAGE_SHARED __pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PIE_PAGE_SHARED)
> > > +#define PAGE_SHARED_EXEC __pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PIE_PAGE_SHARED_EXEC)
> > > +#define PAGE_READONLY __pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PIE_PAGE_READONLY)
> > > +#define PAGE_READONLY_EXEC __pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PIE_PAGE_READONLY_EXEC)
> > > +#define PAGE_EXECONLY __pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PIE_PAGE_EXECONLY)
> > >
> > > #endif /* __ASSEMBLY__ */
> > >
> > > @@ -124,21 +135,21 @@ extern bool arm64_use_ng_mappings;
> > > /* f: PAGE_SHARED PTE_UXN | PTE_PXN | PTE_WRITE | PTE_USER */
> > >
> > > #define PIE_E0 ( \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_EXECONLY), PIE_X_O) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_READONLY_EXEC), PIE_RX) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_SHARED_EXEC), PIE_RWX) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_READONLY), PIE_R) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_SHARED), PIE_RW))
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_EXECONLY), PIE_X_O) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_READONLY_EXEC), PIE_RX) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_SHARED_EXEC), PIE_RWX) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_READONLY), PIE_R) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_SHARED), PIE_RW))
> > >
> > > #define PIE_E1 ( \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_EXECONLY), PIE_NONE_O) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_READONLY_EXEC), PIE_R) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_SHARED_EXEC), PIE_RW) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_READONLY), PIE_R) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_SHARED), PIE_RW) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_KERNEL_ROX), PIE_RX) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_KERNEL_EXEC), PIE_RWX) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_KERNEL_RO), PIE_R) | \
> > > - PIRx_ELx_PERM(pte_pi_index(_PAGE_KERNEL), PIE_RW))
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_EXECONLY), PIE_NONE_O) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_READONLY_EXEC), PIE_R) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_SHARED_EXEC), PIE_RW) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_READONLY), PIE_R) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_SHARED), PIE_RW) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_KERNEL_ROX), PIE_RX) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_KERNEL_EXEC), PIE_RWX) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_KERNEL_RO), PIE_R) | \
> > > + PIRx_ELx_PERM(pte_pi_index(PIE_PAGE_KERNEL), PIE_RW))
> > >
> > > #endif /* __ASM_PGTABLE_PROT_H */
> > >
> > >
> > > The PAGE_KERNEL bits are harder to share, because they are based on
> > > PROT_NORMAL. But maybe this bit of duplication is better than the #define 0x0
> > > hack I had. Could maybe add a BUILD_BUG_ON somewhere to check that PIE_PAGE_KERNEL*
> > > and PAGE_KERNEL have matching bits?
> > >
> >
> > That seems rather invasive. I was about to send out a patch that does
> > the below instead. Would that work for you?
> >
> > diff --git a/arch/arm64/include/asm/pgtable-prot.h
> > b/arch/arm64/include/asm/pgtable-prot.h
> > index eed814b00a389..282e0ba658f03 100644
> > --- a/arch/arm64/include/asm/pgtable-prot.h
> > +++ b/arch/arm64/include/asm/pgtable-prot.h
> > @@ -57,10 +57,6 @@
> > #define _PAGE_READONLY_EXEC (_PAGE_DEFAULT | PTE_USER | PTE_RDONLY |
> > PTE_NG | PTE_PXN)
> > #define _PAGE_EXECONLY (_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PTE_PXN)
> >
> > -#ifdef __ASSEMBLY__
> > -#define PTE_MAYBE_NG 0
> > -#endif
> > -
> > #ifndef __ASSEMBLY__
> >
> > #include <asm/cpufeature.h>
> > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> > index 22201066749e9..069265a8c4384 100644
> > --- a/arch/arm64/mm/proc.S
> > +++ b/arch/arm64/mm/proc.S
> > @@ -457,11 +457,24 @@ alternative_else_nop_endif
> > ubfx x1, x1, #ID_AA64MMFR3_EL1_S1PIE_SHIFT, #4
> > cbz x1, .Lskip_indirection
> >
> > + /*
> > + * The PROT_* macros describing the various memory types may resolve to
> > + * C expressions if they include the PTE_MAYBE_* macros, and so they
> > + * can only be used from C code. The PIE_E* constants below are also
> > + * defined in terms of those macros, but will mask out those
> > + * PTE_MAYBE_* constants, whether they are set or not. So #define them
> > + * as 0x0 here so we can evaluate the PIE_E* constants in asm context.
> > + */
> > +
> > +#define PTE_MAYBE_NG 0
> > +
> > mov_q x0, PIE_E0
> > msr REG_PIRE0_EL1, x0
> > mov_q x0, PIE_E1
> > msr REG_PIR_EL1, x0
> >
> > +#undef PTE_MAYBE_NG
> > +
> > mov x0, TCR2_EL1x_PIE
> > msr REG_TCR2_EL1, x0
> >
>
> Seems like a way to localise the 'hack', I'm fine with it. We can always take a
> similar approach to what I suggested, if it becomes a problem later.
>
OK
More information about the linux-arm-kernel
mailing list