Re: [PATCH] powerpc/mm: add exec protection on powerpc 603

From: Christophe LEROY
Date: Thu Nov 29 2018 - 07:12:39 EST



Le 29/11/2018 Ã 12:25, Aneesh Kumar K.V a ÃcritÂ:
On 11/16/18 10:50 PM, Christophe Leroy wrote:
The 603 doesn't have a HASH table, TLB misses are handled by
software. It is then possible to generate page fault when
_PAGE_EXEC is not set like in nohash/32.

In order to support it, set_pte_filter() and
set_access_flags_filter() are made common, and the handling
is made dependent on MMU_FTR_HPTE_TABLE

Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
---
 arch/powerpc/include/asm/book3s/32/hash.h | 1 +
 arch/powerpc/include/asm/book3s/32/pgtable.h | 18 +++++++++---------
 arch/powerpc/include/asm/cputable.h | 8 ++++----
 arch/powerpc/kernel/head_32.S | 2 ++
 arch/powerpc/mm/pgtable.c | 20 +++++++++++---------
 5 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/32/hash.h b/arch/powerpc/include/asm/book3s/32/hash.h
index f2892c7ab73e..2a0a467d2985 100644
--- a/arch/powerpc/include/asm/book3s/32/hash.h
+++ b/arch/powerpc/include/asm/book3s/32/hash.h
@@ -26,6 +26,7 @@
 #define _PAGE_WRITETHRU 0x040 /* W: cache write-through */
 #define _PAGE_DIRTY 0x080 /* C: page changed */
 #define _PAGE_ACCESSED 0x100 /* R: page referenced */
+#define _PAGE_EXECÂÂÂ 0x200ÂÂÂ /* software: exec allowed */
 #define _PAGE_RW 0x400 /* software: user write access allowed */
 #define _PAGE_SPECIAL 0x800 /* software: Special page */
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index c21d33704633..cf844fed4527 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -10,9 +10,9 @@
 /* And here we include common definitions */
 #define _PAGE_KERNEL_RO 0
-#define _PAGE_KERNEL_ROXÂÂÂ 0
+#define _PAGE_KERNEL_ROXÂÂÂ (_PAGE_EXEC)
 #define _PAGE_KERNEL_RW (_PAGE_DIRTY | _PAGE_RW)
-#define _PAGE_KERNEL_RWXÂÂÂ (_PAGE_DIRTY | _PAGE_RW)
+#define _PAGE_KERNEL_RWXÂÂÂ (_PAGE_DIRTY | _PAGE_RW | _PAGE_EXEC)
 #define _PAGE_HPTEFLAGS _PAGE_HASHPTE
@@ -66,11 +66,11 @@ static inline bool pte_user(pte_t pte)
ÂÂ */
 #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)
+#define PAGE_SHARED_XÂÂÂ __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_RW | _PAGE_EXEC)
 #define PAGE_COPY __pgprot(_PAGE_BASE | _PAGE_USER)
-#define PAGE_COPY_XÂÂÂ __pgprot(_PAGE_BASE | _PAGE_USER)
+#define PAGE_COPY_XÂÂÂ __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_EXEC)
 #define PAGE_READONLY __pgprot(_PAGE_BASE | _PAGE_USER)
-#define PAGE_READONLY_XÂÂÂ __pgprot(_PAGE_BASE | _PAGE_USER)
+#define PAGE_READONLY_XÂÂÂ __pgprot(_PAGE_BASE | _PAGE_USER | _PAGE_EXEC)
 /* Permission masks used for kernel mappings */
 #define PAGE_KERNEL __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)
@@ -318,7 +318,7 @@ static inline void __ptep_set_access_flags(struct vm_area_struct *vma,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int psize)
 {
ÂÂÂÂÂ unsigned long set = pte_val(entry) &
-ÂÂÂÂÂÂÂ (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW);
+ÂÂÂÂÂÂÂ (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_EXEC);
ÂÂÂÂÂ pte_update(ptep, 0, set);
@@ -384,7 +384,7 @@ static inline int pte_dirty(pte_t pte)ÂÂÂÂÂÂÂ { return !!(pte_val(pte) & _PAGE_DIRTY);
 static inline int pte_young(pte_t pte) { return !!(pte_val(pte) & _PAGE_ACCESSED); }
 static inline int pte_special(pte_t pte) { return !!(pte_val(pte) & _PAGE_SPECIAL); }
 static inline int pte_none(pte_t pte) { return (pte_val(pte) & ~_PTE_NONE_MASK) == 0; }
-static inline bool pte_exec(pte_t pte)ÂÂÂÂÂÂÂ { return true; }
+static inline bool pte_exec(pte_t pte)ÂÂÂÂÂÂÂ { return pte_val(pte) & _PAGE_EXEC; }
 static inline int pte_present(pte_t pte)
 {
@@ -451,7 +451,7 @@ static inline pte_t pte_wrprotect(pte_t pte)
 static inline pte_t pte_exprotect(pte_t pte)
 {
-ÂÂÂ return pte;
+ÂÂÂ return __pte(pte_val(pte) & ~_PAGE_EXEC);
 }
 static inline pte_t pte_mkclean(pte_t pte)
@@ -466,7 +466,7 @@ static inline pte_t pte_mkold(pte_t pte)
 static inline pte_t pte_mkexec(pte_t pte)
 {
-ÂÂÂ return pte;
+ÂÂÂ return __pte(pte_val(pte) | _PAGE_EXEC);
 }
 static inline pte_t pte_mkpte(pte_t pte)
diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
index 29f49a35d6ee..a0395ccbbe9e 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -296,7 +296,7 @@ static inline void cpu_feature_keys_init(void) { }
 #define CPU_FTRS_PPC601 (CPU_FTR_COMMON | CPU_FTR_601 | \
ÂÂÂÂÂ CPU_FTR_COHERENT_ICACHE | CPU_FTR_UNIFIED_ID_CACHE | CPU_FTR_USE_RTC)
 #define CPU_FTRS_603 (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
-ÂÂÂÂÂÂÂ CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_PPC_LE)
+ÂÂÂÂÂÂÂ CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_PPC_LE | CPU_FTR_NOEXECUTE)
 #define CPU_FTRS_604 (CPU_FTR_COMMON | CPU_FTR_PPC_LE)
 #define CPU_FTRS_740_NOTAU (CPU_FTR_COMMON | \
ÂÂÂÂÂÂÂÂÂ CPU_FTR_MAYBE_CAN_DOZE | CPU_FTR_L2CR | \
@@ -367,15 +367,15 @@ static inline void cpu_feature_keys_init(void) { }
ÂÂÂÂÂÂÂÂÂ CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
ÂÂÂÂÂÂÂÂÂ CPU_FTR_SPEC7450 | CPU_FTR_NAP_DISABLE_L2_PR | \
ÂÂÂÂÂÂÂÂÂ CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
-#define CPU_FTRS_82XXÂÂÂ (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE)
+#define CPU_FTRS_82XXÂÂÂ (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | CPU_FTR_NOEXECUTE)
 #define CPU_FTRS_G2_LE (CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
ÂÂÂÂÂÂÂÂÂ CPU_FTR_MAYBE_CAN_NAP)
 #define CPU_FTRS_E300 (CPU_FTR_MAYBE_CAN_DOZE | \
ÂÂÂÂÂÂÂÂÂ CPU_FTR_MAYBE_CAN_NAP | \
-ÂÂÂÂÂÂÂ CPU_FTR_COMMON)
+ÂÂÂÂÂÂÂ CPU_FTR_COMMONÂ | CPU_FTR_NOEXECUTE)
 #define CPU_FTRS_E300C2 (CPU_FTR_MAYBE_CAN_DOZE | \
ÂÂÂÂÂÂÂÂÂ CPU_FTR_MAYBE_CAN_NAP | \
-ÂÂÂÂÂÂÂ CPU_FTR_COMMON | CPU_FTR_FPU_UNAVAILABLE)
+ÂÂÂÂÂÂÂ CPU_FTR_COMMON | CPU_FTR_FPU_UNAVAILABLEÂ | CPU_FTR_NOEXECUTE)
 #define CPU_FTRS_CLASSIC32 (CPU_FTR_COMMON)
 #define CPU_FTRS_8XX (CPU_FTR_NOEXECUTE)
 #define CPU_FTRS_40X (CPU_FTR_NODSISRALIGN | CPU_FTR_NOEXECUTE)
diff --git a/arch/powerpc/kernel/head_32.S b/arch/powerpc/kernel/head_32.S
index 61ca27929355..50e892763dbb 100644
--- a/arch/powerpc/kernel/head_32.S
+++ b/arch/powerpc/kernel/head_32.S
@@ -515,6 +515,8 @@ InstructionTLBMiss:
ÂÂÂÂÂ lwzÂÂÂ r0,0(r2)ÂÂÂÂÂÂÂ /* get linux-style pte */
ÂÂÂÂÂ andc.ÂÂÂ r1,r1,r0ÂÂÂÂÂÂÂ /* check access & ~permission */
ÂÂÂÂÂ bne-ÂÂÂ InstructionAddressInvalid /* return if access not permitted */
+ÂÂÂ andi.ÂÂÂ r1,r0,_PAGE_EXEC
+ÂÂÂ beq-ÂÂÂ InstructionAddressInvalid /* return if not _PAGE_EXEC */
ÂÂÂÂÂ oriÂÂÂ r0,r0,_PAGE_ACCESSEDÂÂÂ /* set _PAGE_ACCESSED in pte */

Can we get a DataTLB miss and expect to map that in TLB with Exec permission?

There are two independant sets for TLBs, one set for data accesses and one set for instruction fetches.

On a data TLB miss, a data TLB is loaded with tlbld instruction.
On an instruction TLB miss, a insn TLB is loaded with tlbli instruction.

So if I understand your question correctly, the answer is 'no'.



ÂÂÂÂÂ /*
ÂÂÂÂÂÂ * NOTE! We are assuming this is not an SMP system, otherwise
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index 010e1c616cb2..3d86fe9d2ff4 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -74,7 +74,7 @@ static struct page *maybe_pte_to_page(pte_t pte)
ÂÂ * support falls into the same category.
ÂÂ */
-static pte_t set_pte_filter(pte_t pte)
+static pte_t set_pte_filter_hash(pte_t pte)
 {
ÂÂÂÂÂ if (radix_enabled())
ÂÂÂÂÂÂÂÂÂ return pte;
@@ -93,14 +93,12 @@ static pte_t set_pte_filter(pte_t pte)
ÂÂÂÂÂ return pte;
 }
-static pte_t set_access_flags_filter(pte_t pte, struct vm_area_struct *vma,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int dirty)
-{
-ÂÂÂ return pte;
-}
-
 #else /* CONFIG_PPC_BOOK3S */
+static pte_t set_pte_filter_hash(pte_t pte) { return pte; }
+
+#endif /* CONFIG_PPC_BOOK3S */
+
 /* Embedded type MMU with HW exec support. This is a bit more complicated
ÂÂ * as we don't have two bits to spare for _PAGE_EXEC and _PAGE_HWEXEC so
ÂÂ * instead we "filter out" the exec permission for non clean pages.
@@ -109,6 +107,9 @@ static pte_t set_pte_filter(pte_t pte)
 {
ÂÂÂÂÂ struct page *pg;
+ÂÂÂ if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
+ÂÂÂÂÂÂÂ return set_pte_filter_hash(pte);

so 603 doesn't have a coherent icache?

That's right, see asm/cputable.h


+
ÂÂÂÂÂ /* No exec permission in the first place, move on */
ÂÂÂÂÂ if (!pte_exec(pte) || !pte_looks_normal(pte))
ÂÂÂÂÂÂÂÂÂ return pte;
@@ -138,6 +139,9 @@ static pte_t set_access_flags_filter(pte_t pte, struct vm_area_struct *vma,
 {
ÂÂÂÂÂ struct page *pg;
+ÂÂÂ if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
+ÂÂÂÂÂÂÂ return pte;
+
ÂÂÂÂÂ /* So here, we only care about exec faults, as we use them
ÂÂÂÂÂÂ * to recover lost _PAGE_EXEC and perform I$/D$ coherency
ÂÂÂÂÂÂ * if necessary. Also if _PAGE_EXEC is already set, same deal,
@@ -172,8 +176,6 @@ static pte_t set_access_flags_filter(pte_t pte, struct vm_area_struct *vma,
ÂÂÂÂÂ return pte_mkexec(pte);
 }
-#endif /* CONFIG_PPC_BOOK3S */
-
 /*
ÂÂ * set_pte stores a linux PTE into the linux page table.
ÂÂ */


The code looks good from book3s 64 point. I am not familiar with 603 hardware.

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxx>

Thanks
Christophe


-aneesh