x86/ldt: Prevent LDT inheritance on exec()
v2 cover x86/ldt: Prevent LDT inheritance on exec()
LDT should not be inherited on exec(), only on fork().
To sanitize that, the LDT initialization for a new process must be split into parts and the actual duplication moved to arch_dup_mmap() which is only called on fork(). This requires that arch_dup_mmap() gains a return value.
Changes vs. V1:
Moved the duplication to arch_dup_mmap() as suggested by Linus.
Thanks,
tglx
--- arch/powerpc/include/asm/mmu_context.h | 5 +++-- arch/x86/kernel/ldt.c | 17 +++++------------ b/arch/um/include/asm/mmu_context.h | 3 ++- b/arch/unicore32/include/asm/mmu_context.h | 5 +++-- b/arch/x86/include/asm/mmu_context.h | 24 ++++++++++++++++-------- include/asm-generic/mm_hooks.h | 5 +++-- kernel/fork.c | 3 +-- tools/testing/selftests/x86/ldt_gdt.c | 9 +++------ 8 files changed, 36 insertions(+), 35 deletions(-)
PATCH v2 1/2 arch: Allow arch_dup_mmap() to fail
In order to sanitize the LDT initialization on x86 arch_dup_mmap() must be allowed to fail Fix up all instances.
--- arch/powerpc/include/asm/mmu_context.h | 5 +++-- arch/um/include/asm/mmu_context.h | 3 ++- arch/unicore32/include/asm/mmu_context.h | 5 +++-- arch/x86/include/asm/mmu_context.h | 4 ++-- include/asm-generic/mm_hooks.h | 5 +++-- kernel/fork.c | 3 +-- 6 files changed, 14 insertions(+), 11 deletions(-)
arch/powerpc/include/asm/mmu_context.h +3 -2
@@ -160,9 +160,10 @@ static inline void enter_lazy_tlb(struct#endif } -static inline void arch_dup_mmap (struct mm_struct *oldmm, -struct mm_struct *mm) +static inline int arch_dup_mmap (struct mm_struct *oldmm, +struct mm_struct *mm) { +return 0 ; }#ifndef CONFIG_PPC_BOOK3S_64
arch/um/include/asm/mmu_context.h +2 -1
@@ -15,9 +15,10 @@ extern void uml_setup_stubs(struct mm_st/* * Needed since we do not use the asm-generic/mm_hooks.h: */ -static inline void arch_dup_mmap (struct mm_struct *oldmm,struct mm_struct *mm) +static inline int arch_dup_mmap (struct mm_struct *oldmm,struct mm_struct *mm) {uml_setup_stubs (mm); +return 0 ; }extern void arch_exit_mmap (struct mm_struct *mm); static inline void arch_unmap(struct mm_struct *mm,
arch/unicore32/include/asm/mmu_context.h +3 -2
@@ -81,9 +81,10 @@ do { \ } \ }while (0 ) -static inline void arch_dup_mmap(struct mm_struct *oldmm, -struct mm_struct *mm) +static inline int arch_dup_mmap(struct mm_struct *oldmm, +struct mm_struct *mm) { +return 0 ; } static inline void arch_unmap(struct mm_struct *mm,
arch/x86/include/asm/mmu_context.h +2 -2
@@ -176,10 +176,10 @@ do { \ } while (0)#endif -static inline void arch_dup_mmap (struct mm_struct *oldmm, -struct mm_struct *mm) +static inline int arch_dup_mmap (struct mm_struct *oldmm,struct mm_struct *mm) {paravirt_arch_dup_mmap (oldmm, mm); +return 0 ; }static inline void arch_exit_mmap (struct mm_struct *mm)
include/asm-generic/mm_hooks.h +3 -2
@@ -7,9 +7,10 @@#ifndef _ASM_GENERIC_MM_HOOKS_H#define _ASM_GENERIC_MM_HOOKS_H -static inline void arch_dup_mmap (struct mm_struct *oldmm, -struct mm_struct *mm) +static inline int arch_dup_mmap (struct mm_struct *oldmm, +struct mm_struct *mm) { +return 0 ; }static inline void arch_exit_mmap (struct mm_struct *mm)
kernel/fork.c +1 -2
@@ -721,8 +721,7 @@ static __latent_entropy int dup_mmap(str goto out; }/* a new mm has just been created */ -arch_dup_mmap (oldmm, mm); - retval =0 ; + retval =arch_dup_mmap (oldmm, mm); out:up_write (&mm->mmap_sem);flush_tlb_mm (oldmm);
PATCH v2 2/2 x86/ldt: Prevent ldt inheritance on exec
In reply to Thomas GleixnerFrom: Thomas Gleixner <[email protected]>
The LDT is inheritet independent of fork or exec, but that makes no sense at all because exec is supposed to start the process clean.
The reason why this happens is that init_new_context_ldt() is called from init_new_context() which obviously needs to be called for both fork() and exec().
It would be surprising if anything relies on that behaviour, so it seems to be safe to remove that misfeature.
Split the context initialization into two parts. Clear the ldt pointer and initialize the mutex from the general context init and move the LDT duplication to arch_dup_mmap() which is only called on fork(). Fix up the selftest accordingly.
--- arch/x86/include/asm/mmu_context.h | 22 +++++++++++++++------- arch/x86/kernel/ldt.c | 17 +++++------------ tools/testing/selftests/x86/ldt_gdt.c | 9 +++------ 3 files changed, 23 insertions(+), 25 deletions(-)
arch/x86/include/asm/mmu_context.h +15 -7
@@ -57,11 +57,17 @@ struct ldt_struct {/* * Used for LDT copy/destruction. */ -int init_new_context_ldt (struct task_struct *tsk,struct mm_struct *mm); +static inline void init_new_context_ldt (struct mm_struct *mm) +{ + mm->context.ldt =NULL ; +mutex_init (&mm->context.lock); +} +int ldt_dup_context (struct mm_struct *oldmm,struct mm_struct *mm);void destroy_context_ldt (struct mm_struct *mm);#else /* CONFIG_MODIFY_LDT_SYSCALL */ -static inline int init_new_context_ldt (struct task_struct *tsk, -struct mm_struct *mm) +static inline void init_new_context_ldt (struct mm_struct *mm) { } +static inline int ldt_dup_context (struct mm_struct *oldmm, +struct mm_struct *mm) {return 0 ; }
@@ -135,16 +141,18 @@ static inline int init_new_context(struc mm->context.ctx_id =atomic64_inc_return (&last_mm_ctx_id);atomic64_set (&mm->context.tlb_gen,0 ); -#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS +#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS if (cpu_feature_enabled (X86_FEATURE_OSPKE )) {/* pkey 0 is the default and always allocated */ mm->context.pkey_allocation_map =0x1 ;/* -1 means unallocated or invalid */ mm->context.execute_only_pkey =-1 ; } -#endif -return init_new_context_ldt (tsk, mm); +#endif +init_new_context_ldt (mm); +return 0 ; } +static inline void destroy_context (struct mm_struct *mm) {destroy_context_ldt (mm);
@@ -179,7 +187,7 @@ do { \static inline int arch_dup_mmap (struct mm_struct *oldmm,struct mm_struct *mm) {paravirt_arch_dup_mmap (oldmm, mm); -return 0 ; +return ldt_dup_context (oldmm, mm); }static inline void arch_exit_mmap (struct mm_struct *mm)
arch/x86/kernel/ldt.c +5 -12
@@ -124,27 +124,20 @@ static void free_ldt_struct(struct ldt_s }/* -* we do not have to muck with descriptors here, that is -* done in switch_mm() as needed. +* Called on fork from arch_dup_mmap(). Just copy the current LDT state, +* the new task is not running, so nothing can be installed. */ -int init_new_context_ldt (struct task_struct *tsk,struct mm_struct *mm) +int ldt_dup_context (struct mm_struct *old_mm,struct mm_struct *mm) {struct ldt_struct *new_ldt; -struct mm_struct *old_mm;int retval =0 ; -mutex_init (&mm->context.lock); - old_mm = current->mm; -if (!old_mm) { - mm->context.ldt =NULL ; +if (!old_mm)return 0 ; - }mutex_lock (&old_mm->context.lock); -if (!old_mm->context.ldt) { - mm->context.ldt =NULL ; +if (!old_mm->context.ldt) goto out_unlock; - } new_ldt =alloc_ldt_struct (old_mm->context.ldt->nr_entries);if (!new_ldt) {
tools/testing/selftests/x86/ldt_gdt.c +3 -6
@@ -627,13 +627,10 @@ static void do_multicpu_tests(void)static int finish_exec_test (void ) {/* -* In a sensible world, this would be check_invalid_segment(0, 1); -* For better or for worse, though, the LDT is inherited across exec. -* We can probably change this safely, but for now we test it. +* Older kernel versions did inherit the LDT on exec() which is +* wrong because exec() starts from a clean state. */ -check_valid_segment (0 ,1 , -AR_DPL3 |AR_TYPE_XRCODE |AR_S |AR_P |AR_DB , -42 , true); +check_invalid_segment (0 ,1 );return nerrs ?1 :0 ; }
Re: [patch V2 2/2] x86/ldt: Prevent ldt inheritance on exec
Quoted 5 lines · Thomas Gleixner
arch/x86/kernel/ldt.c +1
-1+int ldt_dup_context (struct mm_struct *old_mm,struct mm_struct *mm) {struct ldt_struct *new_ldt; -struct mm_struct *old_mm;int retval =0 ;
arch/x86/kernel/ldt.c static void free_ldt_struct(struct ldt_s · line 138
Quoted 10 lines
arch/x86/kernel/ldt.c +1
-5-mutex_init (&mm->context.lock); - old_mm = current->mm; -if (!old_mm) { - mm->context.ldt =NULL ; +if (!old_mm)return 0 ; - }mutex_lock (&old_mm->context.lock);
Bah. That's broken. It now nests into old_mm->mmap_sem which is the reverse lock order than in ldt_write. Will fix.
Thanks,
tglx
Re: [patch V2 2/2] x86/ldt: Prevent ldt inheritance on exec
Quoted 15 lines · Thomas Gleixner
Quoted 14 lines · Thomas Gleixner
arch/x86/kernel/ldt.c +2
-6+int ldt_dup_context (struct mm_struct *old_mm,struct mm_struct *mm) {struct ldt_struct *new_ldt; -struct mm_struct *old_mm;int retval =0 ; -mutex_init (&mm->context.lock); - old_mm = current->mm; -if (!old_mm) { - mm->context.ldt =NULL ; +if (!old_mm)return 0 ; - }
Quoted 4 lines
Quoted 1 line
mutex_lock(&old_mm->context.lock);Bah. That's broken. It now nests into
old_mm->mmap_semwhich is the reverse lock order than inldt_write. Will fix.
Confused myself. mmap_sem is not taken in mainline ldt_write. It's just in the stuff I'm working on.
Thanks,
tglx
Peter Zijlstra
maintainerRe: [patch V2 2/2] x86/ldt: Prevent ldt inheritance on exec
In reply to Thomas GleixnerQuoted 15 lines · Thomas Gleixner
Quoted 14 lines · Thomas Gleixner
arch/x86/kernel/ldt.c +2
-6+int ldt_dup_context (struct mm_struct *old_mm,struct mm_struct *mm) {struct ldt_struct *new_ldt; -struct mm_struct *old_mm;int retval =0 ; -mutex_init (&mm->context.lock); - old_mm = current->mm; -if (!old_mm) { - mm->context.ldt =NULL ; +if (!old_mm)return 0 ; - }
Quoted 4 lines
Quoted 1 line
mutex_lock(&old_mm->context.lock);Bah. That's broken. It now nests into
old_mm->mmap_semwhich is the reverse lock order than inldt_write. Will fix.
But read_ldt() will still nest mmap_sem inside context.lock, no? Lockdep doesn't care about old_mm vs new_mm.
Re: [patch V2 2/2] x86/ldt: Prevent ldt inheritance on exec
Quoted 16 lines · Peter Zijlstra
Quoted 15 lines · Thomas Gleixner
Quoted 14 lines · Thomas Gleixner
arch/x86/kernel/ldt.c +2
-6+int ldt_dup_context (struct mm_struct *old_mm,struct mm_struct *mm) {struct ldt_struct *new_ldt; -struct mm_struct *old_mm;int retval =0 ; -mutex_init (&mm->context.lock); - old_mm = current->mm; -if (!old_mm) { - mm->context.ldt =NULL ; +if (!old_mm)return 0 ; - }
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
mutex_lock(&old_mm->context.lock);Bah. That's broken. It now nests into
old_mm->mmap_semwhich is the reverse lock order than inldt_write. Will fix.But
read_ldt()will still nestmmap_seminside context.lock, no? Lockdep doesn't care aboutold_mmvsnew_mm.
Yeah, found that and fixed that already. Next version will be perfect :)