| Message ID | c41100f9c144dc5b62e5a751b810190c6b5d42fd.1635226743.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] powerpc/nohash: Fix __ptep_set_access_flags() and ptep_set_wrprotect() | expand |
| Related | show |
Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm: > set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC. > set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC. > > Book3e has 2 bits, UX and SX, which defines the exec rights > resp. for user (PR=1) and for kernel (PR=0). > > _PAGE_EXEC is defined as UX only. > > An executable kernel page is set with either _PAGE_KERNEL_RWX > or _PAGE_KERNEL_ROX, which both have SX set and UX cleared. > > So set_memory_nx() call for an executable kernel page does > nothing because UX is already cleared. > > And set_memory_x() on a non-executable kernel page makes it > executable for the user and keeps it non-executable for kernel. > > Also, pte_exec() always returns 'false' on kernel pages, because > it checks _PAGE_EXEC which doesn't include SX, so for instance > the W+X check doesn't work. > > To fix this: > - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER > - sets both UX and SX in _PAGE_EXEC so that pte_user() returns > true whenever one of the two bits is set I don't understand this change. Which pte_user() returns true after this change? Or do you mean pte_exec()? Does this filter through in some cases at least for kernel executable PTEs will get both bits set? Seems cleaner to distinguish user and kernel exec for that but maybe it's a lot of churn? Thanks, Nick > and pte_exprotect() > clears both bits. > - Define a book3e specific version of pte_mkexec() which sets > either SX or UX based on UR. > > Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines") > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > v3: Removed pte_mkexec() from nohash/64/pgtable.h > v2: New > ---
Le 27/10/2021 à 06:44, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm: >> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC. >> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC. >> >> Book3e has 2 bits, UX and SX, which defines the exec rights >> resp. for user (PR=1) and for kernel (PR=0). >> >> _PAGE_EXEC is defined as UX only. >> >> An executable kernel page is set with either _PAGE_KERNEL_RWX >> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared. >> >> So set_memory_nx() call for an executable kernel page does >> nothing because UX is already cleared. >> >> And set_memory_x() on a non-executable kernel page makes it >> executable for the user and keeps it non-executable for kernel. >> >> Also, pte_exec() always returns 'false' on kernel pages, because >> it checks _PAGE_EXEC which doesn't include SX, so for instance >> the W+X check doesn't work. >> >> To fix this: >> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER >> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns >> true whenever one of the two bits is set > > I don't understand this change. Which pte_user() returns true after > this change? Or do you mean pte_exec()? Oops, yes, I mean pte_exec() Unless I have to re-spin, can Michael eventually fix that typo while applying ? > > Does this filter through in some cases at least for kernel executable > PTEs will get both bits set? Seems cleaner to distinguish user and > kernel exec for that but maybe it's a lot of churn? Didn't understand what you mean. I did it like that to be able to continue using _PAGE_EXEC for checking executability regardless of whether this is user or kernel, and then continue using the generic nohash pte_exec() helper. Other solution would be to get rid of _PAGE_EXEC completely for book3e and implement both pte_exec() and pte_mkexec() with _PAGE_BAP_UX and _PAGE_BAP_SX, but I'm not sure it is worth the churn as you say. It would also mean different helpers for book3s/32 when it is using 32 bits PTE (CONFIG_PTE_64BIT=n) Christophe > > Thanks, > Nick > >> and pte_exprotect() >> clears both bits. >> - Define a book3e specific version of pte_mkexec() which sets >> either SX or UX based on UR. >> >> Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines") >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> v3: Removed pte_mkexec() from nohash/64/pgtable.h >> v2: New >> ---
Excerpts from Christophe Leroy's message of October 27, 2021 2:55 pm: > > > Le 27/10/2021 à 06:44, Nicholas Piggin a écrit : >> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm: >>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC. >>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC. >>> >>> Book3e has 2 bits, UX and SX, which defines the exec rights >>> resp. for user (PR=1) and for kernel (PR=0). >>> >>> _PAGE_EXEC is defined as UX only. >>> >>> An executable kernel page is set with either _PAGE_KERNEL_RWX >>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared. >>> >>> So set_memory_nx() call for an executable kernel page does >>> nothing because UX is already cleared. >>> >>> And set_memory_x() on a non-executable kernel page makes it >>> executable for the user and keeps it non-executable for kernel. >>> >>> Also, pte_exec() always returns 'false' on kernel pages, because >>> it checks _PAGE_EXEC which doesn't include SX, so for instance >>> the W+X check doesn't work. >>> >>> To fix this: >>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER >>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns >>> true whenever one of the two bits is set >> >> I don't understand this change. Which pte_user() returns true after >> this change? Or do you mean pte_exec()? > > Oops, yes, I mean pte_exec() > > Unless I have to re-spin, can Michael eventually fix that typo while > applying ? > >> >> Does this filter through in some cases at least for kernel executable >> PTEs will get both bits set? Seems cleaner to distinguish user and >> kernel exec for that but maybe it's a lot of churn? > > Didn't understand what you mean. > > I did it like that to be able to continue using _PAGE_EXEC for checking > executability regardless of whether this is user or kernel, and then > continue using the generic nohash pte_exec() helper. > > Other solution would be to get rid of _PAGE_EXEC completely for book3e > and implement both pte_exec() and pte_mkexec() with _PAGE_BAP_UX and > _PAGE_BAP_SX, but I'm not sure it is worth the churn as you say. It > would also mean different helpers for book3s/32 when it is using 32 bits > PTE (CONFIG_PTE_64BIT=n) That's basically what I mean. And _PAGE_KERNEL_ROX etc would then not set the UX bit. But at least for now it seems to be an improvement. Thanks, Nick
Le 27/10/2021 à 07:27, Nicholas Piggin a écrit : > Excerpts from Christophe Leroy's message of October 27, 2021 2:55 pm: >> >> >> Le 27/10/2021 à 06:44, Nicholas Piggin a écrit : >>> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm: >>>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC. >>>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC. >>>> >>>> Book3e has 2 bits, UX and SX, which defines the exec rights >>>> resp. for user (PR=1) and for kernel (PR=0). >>>> >>>> _PAGE_EXEC is defined as UX only. >>>> >>>> An executable kernel page is set with either _PAGE_KERNEL_RWX >>>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared. >>>> >>>> So set_memory_nx() call for an executable kernel page does >>>> nothing because UX is already cleared. >>>> >>>> And set_memory_x() on a non-executable kernel page makes it >>>> executable for the user and keeps it non-executable for kernel. >>>> >>>> Also, pte_exec() always returns 'false' on kernel pages, because >>>> it checks _PAGE_EXEC which doesn't include SX, so for instance >>>> the W+X check doesn't work. >>>> >>>> To fix this: >>>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER >>>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns >>>> true whenever one of the two bits is set >>> >>> I don't understand this change. Which pte_user() returns true after >>> this change? Or do you mean pte_exec()? >> >> Oops, yes, I mean pte_exec() >> >> Unless I have to re-spin, can Michael eventually fix that typo while >> applying ? >> >>> >>> Does this filter through in some cases at least for kernel executable >>> PTEs will get both bits set? Seems cleaner to distinguish user and >>> kernel exec for that but maybe it's a lot of churn? >> >> Didn't understand what you mean. >> >> I did it like that to be able to continue using _PAGE_EXEC for checking >> executability regardless of whether this is user or kernel, and then >> continue using the generic nohash pte_exec() helper. >> >> Other solution would be to get rid of _PAGE_EXEC completely for book3e >> and implement both pte_exec() and pte_mkexec() with _PAGE_BAP_UX and >> _PAGE_BAP_SX, but I'm not sure it is worth the churn as you say. It >> would also mean different helpers for book3s/32 when it is using 32 bits >> PTE (CONFIG_PTE_64BIT=n) > > That's basically what I mean. And _PAGE_KERNEL_ROX etc would then not > set the UX bit. But at least for now it seems to be an improvement. > That's already the case: #define _PAGE_KERNEL_RWX (_PAGE_BAP_SW | _PAGE_BAP_SR | _PAGE_DIRTY | _PAGE_BAP_SX) #define _PAGE_KERNEL_ROX (_PAGE_BAP_SR | _PAGE_BAP_SX) Christophe
Excerpts from Christophe Leroy's message of October 27, 2021 3:50 pm: > > > Le 27/10/2021 à 07:27, Nicholas Piggin a écrit : >> Excerpts from Christophe Leroy's message of October 27, 2021 2:55 pm: >>> >>> >>> Le 27/10/2021 à 06:44, Nicholas Piggin a écrit : >>>> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm: >>>>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC. >>>>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC. >>>>> >>>>> Book3e has 2 bits, UX and SX, which defines the exec rights >>>>> resp. for user (PR=1) and for kernel (PR=0). >>>>> >>>>> _PAGE_EXEC is defined as UX only. >>>>> >>>>> An executable kernel page is set with either _PAGE_KERNEL_RWX >>>>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared. >>>>> >>>>> So set_memory_nx() call for an executable kernel page does >>>>> nothing because UX is already cleared. >>>>> >>>>> And set_memory_x() on a non-executable kernel page makes it >>>>> executable for the user and keeps it non-executable for kernel. >>>>> >>>>> Also, pte_exec() always returns 'false' on kernel pages, because >>>>> it checks _PAGE_EXEC which doesn't include SX, so for instance >>>>> the W+X check doesn't work. >>>>> >>>>> To fix this: >>>>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER >>>>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns >>>>> true whenever one of the two bits is set >>>> >>>> I don't understand this change. Which pte_user() returns true after >>>> this change? Or do you mean pte_exec()? >>> >>> Oops, yes, I mean pte_exec() >>> >>> Unless I have to re-spin, can Michael eventually fix that typo while >>> applying ? >>> >>>> >>>> Does this filter through in some cases at least for kernel executable >>>> PTEs will get both bits set? Seems cleaner to distinguish user and >>>> kernel exec for that but maybe it's a lot of churn? >>> >>> Didn't understand what you mean. >>> >>> I did it like that to be able to continue using _PAGE_EXEC for checking >>> executability regardless of whether this is user or kernel, and then >>> continue using the generic nohash pte_exec() helper. >>> >>> Other solution would be to get rid of _PAGE_EXEC completely for book3e >>> and implement both pte_exec() and pte_mkexec() with _PAGE_BAP_UX and >>> _PAGE_BAP_SX, but I'm not sure it is worth the churn as you say. It >>> would also mean different helpers for book3s/32 when it is using 32 bits >>> PTE (CONFIG_PTE_64BIT=n) >> >> That's basically what I mean. And _PAGE_KERNEL_ROX etc would then not >> set the UX bit. But at least for now it seems to be an improvement. >> > > That's already the case: > > #define _PAGE_KERNEL_RWX (_PAGE_BAP_SW | _PAGE_BAP_SR | _PAGE_DIRTY | > _PAGE_BAP_SX) > #define _PAGE_KERNEL_ROX (_PAGE_BAP_SR | _PAGE_BAP_SX) So it is, I was looking at the wrong header. Looks okay to me then, for what it's worth. Thanks, Nick
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 27/10/2021 à 06:44, Nicholas Piggin a écrit : >> Excerpts from Christophe Leroy's message of October 26, 2021 3:39 pm: >>> set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC. >>> set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC. >>> >>> Book3e has 2 bits, UX and SX, which defines the exec rights >>> resp. for user (PR=1) and for kernel (PR=0). >>> >>> _PAGE_EXEC is defined as UX only. >>> >>> An executable kernel page is set with either _PAGE_KERNEL_RWX >>> or _PAGE_KERNEL_ROX, which both have SX set and UX cleared. >>> >>> So set_memory_nx() call for an executable kernel page does >>> nothing because UX is already cleared. >>> >>> And set_memory_x() on a non-executable kernel page makes it >>> executable for the user and keeps it non-executable for kernel. >>> >>> Also, pte_exec() always returns 'false' on kernel pages, because >>> it checks _PAGE_EXEC which doesn't include SX, so for instance >>> the W+X check doesn't work. >>> >>> To fix this: >>> - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER >>> - sets both UX and SX in _PAGE_EXEC so that pte_user() returns >>> true whenever one of the two bits is set >> >> I don't understand this change. Which pte_user() returns true after >> this change? Or do you mean pte_exec()? > > Oops, yes, I mean pte_exec() > > Unless I have to re-spin, can Michael eventually fix that typo while > applying ? I did. cheers
diff --git a/arch/powerpc/include/asm/nohash/32/pgtable.h b/arch/powerpc/include/asm/nohash/32/pgtable.h index 11c6849f7864..b67742e2a9b2 100644 --- a/arch/powerpc/include/asm/nohash/32/pgtable.h +++ b/arch/powerpc/include/asm/nohash/32/pgtable.h @@ -193,10 +193,12 @@ static inline pte_t pte_wrprotect(pte_t pte) } #endif +#ifndef pte_mkexec static inline pte_t pte_mkexec(pte_t pte) { return __pte(pte_val(pte) | _PAGE_EXEC); } +#endif #define pmd_none(pmd) (!pmd_val(pmd)) #define pmd_bad(pmd) (pmd_val(pmd) & _PMD_BAD) diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h b/arch/powerpc/include/asm/nohash/64/pgtable.h index d081704b13fb..9d2905a47410 100644 --- a/arch/powerpc/include/asm/nohash/64/pgtable.h +++ b/arch/powerpc/include/asm/nohash/64/pgtable.h @@ -118,11 +118,6 @@ static inline pte_t pte_wrprotect(pte_t pte) return __pte(pte_val(pte) & ~_PAGE_RW); } -static inline pte_t pte_mkexec(pte_t pte) -{ - return __pte(pte_val(pte) | _PAGE_EXEC); -} - #define PMD_BAD_BITS (PTE_TABLE_SIZE-1) #define PUD_BAD_BITS (PMD_TABLE_SIZE-1) diff --git a/arch/powerpc/include/asm/nohash/pte-book3e.h b/arch/powerpc/include/asm/nohash/pte-book3e.h index 813918f40765..f798640422c2 100644 --- a/arch/powerpc/include/asm/nohash/pte-book3e.h +++ b/arch/powerpc/include/asm/nohash/pte-book3e.h @@ -48,7 +48,7 @@ #define _PAGE_WRITETHRU 0x800000 /* W: cache write-through */ /* "Higher level" linux bit combinations */ -#define _PAGE_EXEC _PAGE_BAP_UX /* .. and was cache cleaned */ +#define _PAGE_EXEC (_PAGE_BAP_SX | _PAGE_BAP_UX) /* .. and was cache cleaned */ #define _PAGE_RW (_PAGE_BAP_SW | _PAGE_BAP_UW) /* User write permission */ #define _PAGE_KERNEL_RW (_PAGE_BAP_SW | _PAGE_BAP_SR | _PAGE_DIRTY) #define _PAGE_KERNEL_RO (_PAGE_BAP_SR) @@ -93,11 +93,11 @@ /* Permission masks used to generate the __P and __S table */ #define PAGE_NONE __pgprot(_PAGE_BASE) #define PAGE_SHARED __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW) -#define PAGE_SHARED_X __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW | _PAGE_EXEC) +#define PAGE_SHARED_X __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW | _PAGE_BAP_UX) #define PAGE_COPY __pgprot(_PAGE_BASE | _PAGE_USER) -#define PAGE_COPY_X __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_EXEC) +#define PAGE_COPY_X __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_BAP_UX) #define PAGE_READONLY __pgprot(_PAGE_BASE | _PAGE_USER) -#define PAGE_READONLY_X __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_EXEC) +#define PAGE_READONLY_X __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_BAP_UX) #ifndef __ASSEMBLY__ static inline pte_t pte_mkprivileged(pte_t pte) @@ -113,6 +113,16 @@ static inline pte_t pte_mkuser(pte_t pte) } #define pte_mkuser pte_mkuser + +static inline pte_t pte_mkexec(pte_t pte) +{ + if (pte_val(pte) & _PAGE_BAP_UR) + return __pte((pte_val(pte) & ~_PAGE_BAP_SX) | _PAGE_BAP_UX); + else + return __pte((pte_val(pte) & ~_PAGE_BAP_UX) | _PAGE_BAP_SX); +} +#define pte_mkexec pte_mkexec + #endif /* __ASSEMBLY__ */ #endif /* __KERNEL__ */ diff --git a/arch/powerpc/mm/nohash/tlb_low_64e.S b/arch/powerpc/mm/nohash/tlb_low_64e.S index bf24451f3e71..9235e720e357 100644 --- a/arch/powerpc/mm/nohash/tlb_low_64e.S +++ b/arch/powerpc/mm/nohash/tlb_low_64e.S @@ -222,7 +222,7 @@ tlb_miss_kernel_bolted: tlb_miss_fault_bolted: /* We need to check if it was an instruction miss */ - andi. r10,r11,_PAGE_EXEC|_PAGE_BAP_SX + andi. r10,r11,_PAGE_BAP_UX|_PAGE_BAP_SX bne itlb_miss_fault_bolted dtlb_miss_fault_bolted: tlb_epilog_bolted @@ -239,7 +239,7 @@ itlb_miss_fault_bolted: srdi r15,r16,60 /* get region */ bne- itlb_miss_fault_bolted - li r11,_PAGE_PRESENT|_PAGE_EXEC /* Base perm */ + li r11,_PAGE_PRESENT|_PAGE_BAP_UX /* Base perm */ /* We do the user/kernel test for the PID here along with the RW test */ @@ -614,7 +614,7 @@ itlb_miss_fault_e6500: /* We do the user/kernel test for the PID here along with the RW test */ - li r11,_PAGE_PRESENT|_PAGE_EXEC /* Base perm */ + li r11,_PAGE_PRESENT|_PAGE_BAP_UX /* Base perm */ oris r11,r11,_PAGE_ACCESSED@h cmpldi cr0,r15,0 /* Check for user region */ @@ -734,7 +734,7 @@ normal_tlb_miss_done: normal_tlb_miss_access_fault: /* We need to check if it was an instruction miss */ - andi. r10,r11,_PAGE_EXEC + andi. r10,r11,_PAGE_BAP_UX bne 1f ld r14,EX_TLB_DEAR(r12) ld r15,EX_TLB_ESR(r12)
set_memory_x() calls pte_mkexec() which sets _PAGE_EXEC. set_memory_nx() calls pte_exprotec() which clears _PAGE_EXEC. Book3e has 2 bits, UX and SX, which defines the exec rights resp. for user (PR=1) and for kernel (PR=0). _PAGE_EXEC is defined as UX only. An executable kernel page is set with either _PAGE_KERNEL_RWX or _PAGE_KERNEL_ROX, which both have SX set and UX cleared. So set_memory_nx() call for an executable kernel page does nothing because UX is already cleared. And set_memory_x() on a non-executable kernel page makes it executable for the user and keeps it non-executable for kernel. Also, pte_exec() always returns 'false' on kernel pages, because it checks _PAGE_EXEC which doesn't include SX, so for instance the W+X check doesn't work. To fix this: - change tlb_low_64e.S to use _PAGE_BAP_UX instead of _PAGE_USER - sets both UX and SX in _PAGE_EXEC so that pte_user() returns true whenever one of the two bits is set and pte_exprotect() clears both bits. - Define a book3e specific version of pte_mkexec() which sets either SX or UX based on UR. Fixes: 1f9ad21c3b38 ("powerpc/mm: Implement set_memory() routines") Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- v3: Removed pte_mkexec() from nohash/64/pgtable.h v2: New --- arch/powerpc/include/asm/nohash/32/pgtable.h | 2 ++ arch/powerpc/include/asm/nohash/64/pgtable.h | 5 ----- arch/powerpc/include/asm/nohash/pte-book3e.h | 18 ++++++++++++++---- arch/powerpc/mm/nohash/tlb_low_64e.S | 8 ++++---- 4 files changed, 20 insertions(+), 13 deletions(-)