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

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

[Ram Pai]

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 <linuxram@us.ibm.com>
---
 arch/powerpc/include/asm/mmu_context.h |   15 +++++++++------
 arch/powerpc/mm/pkeys.c                |    7 +++++++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
index 0381394..cb16146 100644
--- a/arch/powerpc/include/asm/mmu_context.h
+++ b/arch/powerpc/include/asm/mmu_context.h
@@ -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 */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index b271b28..5d65c47 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -414,3 +414,10 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
 
 	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;
+}
-- 
1.7.1

Re: [PATCH] powerpc/pkeys: copy pkey-tracking-information at fork()

[Thiago Jung Bauermann]

Ram Pai <linuxram@us.ibm.com> writes:

> 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 <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/asm/mmu_context.h |   15 +++++++++------
>  arch/powerpc/mm/pkeys.c                |    7 +++++++
>  2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> index 0381394..cb16146 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -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 */
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index b271b28..5d65c47 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -414,3 +414,10 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
>
>  	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;
> +}

This function is small and perhaps could have been a static inline in
<asm/mmu_context.h>, but arch_dup_mmap() doesn't seem to be in a hot
path so the segregation of implementation details to pkeys.c is nice
from an organization point of view.

Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH] powerpc/pkeys: copy pkey-tracking-information at fork()

[Florian Weimer]

* Ram Pai:

> +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;
> +}

Looks reasonable to me.

Thanks,
Florian (who is not a kernel developer though)

Re: [PATCH] powerpc/pkeys: copy pkey-tracking-information at fork()

[Michael Ellerman]

Hi Ram,

Thanks for fixing this.

Ram Pai <linuxram@us.ibm.com> writes:
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index b271b28..5d65c47 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -414,3 +414,10 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
>  
>  	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:

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index cf87dddefbdc..587807763737 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -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

Re: [PATCH] powerpc/pkeys: copy pkey-tracking-information at fork()

[Ram Pai]

On Fri, Dec 21, 2018 at 12:19:13AM +1100, Michael Ellerman wrote:
> Hi Ram,
> 
> Thanks for fixing this.
> 
> Ram Pai <linuxram@us.ibm.com> writes:
> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> > index b271b28..5d65c47 100644
> > --- a/arch/powerpc/mm/pkeys.c
> > +++ b/arch/powerpc/mm/pkeys.c
> > @@ -414,3 +414,10 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
> >  
> >  	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?

Yes. That will make it better. However not checking it, will not hurt. Just that
it will do some unnecessary copying.

Will spin out a new patch with the fix, and send it your way.
RP


> 
> eg:
> 
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index cf87dddefbdc..587807763737 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -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

-- 
Ram Pai

[PATCH v2] powerpc/pkeys: copy pkey-tracking-information at fork()

[Ram Pai]

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 <bauerman@linux.ibm.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
    
v2: do not copy if pkeys is disabled.
    	-- comment by Michael Ellermen

diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
index 0381394..cb16146 100644
--- a/arch/powerpc/include/asm/mmu_context.h
+++ b/arch/powerpc/include/asm/mmu_context.h
@@ -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 */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index b271b28..25a8dd9 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -414,3 +414,13 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write,
 
 	return pkey_access_permitted(vma_pkey(vma), write, execute);
 }
+
+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;
+}

Re: [PATCH v2] powerpc/pkeys: copy pkey-tracking-information at fork()

[Michael Ellerman]

Ram Pai <linuxram@us.ibm.com> writes:

> 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 <bauerman@linux.ibm.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>     
> v2: do not copy if pkeys is disabled.
>     	-- comment by Michael Ellermen

Thanks.

I changed the subject to:

  powerpc/pkeys: Fix handling of pkey state across fork()

And added tags:

  Fixes: cf43d3b26452 ("powerpc: Enable pkey subsystem")
  Cc: stable@vger.kernel.org # v4.16+

cheers

Re: [v2] powerpc/pkeys: copy pkey-tracking-information at fork()

[Michael Ellerman]

On Thu, 2018-12-20 at 20:03:30 UTC, Ram Pai wrote:
> 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 <bauerman@linux.ibm.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/2cd4bd192ee94848695c1c052d8791

cheers