powerpc/pkeys: copy pkey-tracking-information at fork()

At fork(), the pkey tracking information is not copied over to the mm_struct of the child. This can cause the child to erroneously allocate keys that were already allocated. Any allocated execute-only key is lost aswell.

Add code; called by dup_mmap(), to copy the pkey state from parent to child explicitly.

This problem was originally found by Dave Hansen on x86, which turns out to be a problem on powerpc aswell.

Testing:
Passes the new selftest added by Dave Hansen. I have temporarily captured that selftest at
https://github.com/rampai/memorykeys/ -b memkey.v15-rc6

Signed-off-by Ram Pai <[email protected]>

---
 arch/powerpc/include/asm/mmu_context.h |   15 +++++++++------
 arch/powerpc/mm/pkeys.c                |    7 +++++++
 2 files changed, 16 insertions(+), 6 deletions(-)

arch/powerpc/include/asm/mmu_context.h +9 -6

index 0381394..cb16146 100644
@@ -217,12 +217,6 @@ static inline void enter_lazy_tlb(struct mm_struct *mm,
 #endif
 }
 
-static inline int arch_dup_mmap(struct mm_struct *oldmm,
-				struct mm_struct *mm)
-{
-	return 0;
-}
-
 #ifndef CONFIG_PPC_BOOK3S_64
 static inline void arch_exit_mmap(struct mm_struct *mm)
 {

@@ -247,6 +241,7 @@ static inline void arch_bprm_mm_init(struct mm_struct *mm,
 #ifdef CONFIG_PPC_MEM_KEYS
 bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
 			       bool execute, bool foreign);
+void arch_dup_pkeys(struct mm_struct *oldmm, struct mm_struct *mm);
 #else /* CONFIG_PPC_MEM_KEYS */
 static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
 		bool write, bool execute, bool foreign)

@@ -259,6 +254,7 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
 #define thread_pkey_regs_save(thread)
 #define thread_pkey_regs_restore(new_thread, old_thread)
 #define thread_pkey_regs_init(thread)
+#define arch_dup_pkeys(oldmm, mm)
 
 static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
 {

@@ -267,5 +263,12 @@ static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
 
 #endif /* CONFIG_PPC_MEM_KEYS */
 
+static inline int arch_dup_mmap(struct mm_struct *oldmm,
+				struct mm_struct *mm)
+{
+	arch_dup_pkeys(oldmm, mm);
+	return 0;
+}
+
 #endif /* __KERNEL__ */
 #endif /* __ASM_POWERPC_MMU_CONTEXT_H */

arch/powerpc/mm/pkeys.c +0 -0

index b271b28..5d65c47 100644

arch/powerpc/mm/pkeys.c +0 -0

index b271b28..5d65c47 100644

Hi Ram,

Thanks for fixing this.

Quoted 6 lines · Ram Pai

arch/powerpc/mm/pkeys.c +0 -0

index b271b28..5d65c47 100644
@@ -414,3 +414,10 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
 

arch/powerpc/mm/pkeys.c bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write, · line 423

Quoted 9 lines

arch/powerpc/mm/pkeys.c +7 -0

 	return pkey_access_permitted(vma_pkey(vma), write, execute);
 }
+
+void arch_dup_pkeys(struct mm_struct *oldmm, struct mm_struct *mm)
+{
+	/* Duplicate the oldmm pkey state in mm: */
+	mm_pkey_allocation_map(mm) = mm_pkey_allocation_map(oldmm);
+	mm->context.execute_only_pkey = oldmm->context.execute_only_pkey;
+}

Shouldn't this check if pkeys are actually in use?

eg:

arch/powerpc/mm/pkeys.c +3 -0

index cf87dddefbdc..587807763737 100644
@@ -418,6 +418,9 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
 
 void arch_dup_pkeys(struct mm_struct *oldmm, struct mm_struct *mm)
 {
+	if (static_branch_likely(&pkey_disabled))
+		return;
+
 	/* Duplicate the oldmm pkey state in mm: */
 	mm_pkey_allocation_map(mm) = mm_pkey_allocation_map(oldmm);
 	mm->context.execute_only_pkey = oldmm->context.execute_only_pkey;

Ideally we'd actually do it in the inline so that the function call to arch_dup_pkeys() can be avoided. But it looks like header dependencies might make that hard.

cheers

Pkey tracking information is not copied over to the mm_struct of the child during fork(). This can cause the child to erroneously allocate keys that were already allocated. Any allocated execute-only key is lost aswell.

Add code; called by dup_mmap(), to copy the pkey state from parent to child explicitly.

This problem was originally found by Dave Hansen on x86, which turns out to be a problem on powerpc aswell.

Reviewed-by Thiago Jung Bauermann <[email protected]>

Signed-off-by Ram Pai <[email protected]>

v2: do not copy if pkeys is disabled.
-- comment by Michael Ellermen

Pkey tracking information is not copied over to the mm_struct of the child during fork(). This can cause the child to erroneously allocate keys that were already allocated. Any allocated execute-only key is lost aswell.

Add code; called by dup_mmap(), to copy the pkey state from parent to child explicitly.

This problem was originally found by Dave Hansen on x86, which turns out to be a problem on powerpc aswell.

Reviewed-by Thiago Jung Bauermann <[email protected]>

Signed-off-by Ram Pai <[email protected]>