x86: fix free_thread_info() with uninitalized thread_info

PATCH New Akinobu Mita 1 participant 5 messages Dec 27, 2008

Subsystems (4)

x86 MM core Scheduler exec and binfmt

lore

Share & tools

lore

Akinobu Mita

author

2008-12-27 05:16 UTC #0 lore raw

cover x86: fix free_thread_info() with uninitalized thread_info

This patch series fixes a problem described below. The actual fix is only made by the patch 3/4. The rest of patches help it make simple and there is no actual behavioral change.

x86 arch specific free_thread_info() accesses thread_info->task to call free_thread_xstate(). But the thread_info may not be initialized yet. So invalid pointer derefence may happen in free_thread_xstate().

It happens in the following scenario in dup_task_struct()

1. call alloc_task_struct() to allocate empty task_struct
2. call alloc_thread_info() to allocate empty thread_info
3. call arch_dup_task_struct()

x86 arch specific arch_dup_task_struct() copies task_struct from source task_struct. it also allocates empty xstate and copy from source if source task_struct has ->thread.xstate.

If the xstate allocation failed, arch_dup_task_struct() returns error.

4. call free_thread_info() to deallocate thread_info

x86 arch specific free_thread_info() calls free_thread_xstate() with thread_info->task. But the thread_info is not initialized yet.

Akinobu Mita

author

2008-12-27 05:17 UTC #1 lore raw

PATCH 1/4 convert task_struct allocator macros to inline functions

Usually inline function is better than function-like macro.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 kernel/fork.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

kernel/fork.c +12 -2

Index: 2.6/kernel/fork.c
===================================================================
@@ -92,9 +92,19 @@ int nr_processes(void)
 }
 
 #ifndef __HAVE_ARCH_TASK_STRUCT_ALLOCATOR
-# define alloc_task_struct()	kmem_cache_alloc(task_struct_cachep, GFP_KERNEL)
-# define free_task_struct(tsk)	kmem_cache_free(task_struct_cachep, (tsk))
+
 static struct kmem_cache *task_struct_cachep;
+
+static inline struct task_struct *alloc_task_struct(void)
+{
+	return kmem_cache_alloc(task_struct_cachep, GFP_KERNEL);
+}
+
+static inline void free_task_struct(struct task_struct *tsk)
+{
+	kmem_cache_free(task_struct_cachep, tsk);
+}
+
 #endif
 
 #ifndef __HAVE_ARCH_THREAD_INFO_ALLOCATOR

Akinobu Mita

author

2008-12-27 05:18 UTC #2 lore raw

PATCH 2/4 x86: arch specific task_struct allocator

In reply to Akinobu Mita

This patch defines x86 specific task_struct allocator. It is just duplication of generic task-struct allocator for now. Forthcoming patch will make actual change to it.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 arch/x86/include/asm/thread_info.h |    6 ++++++
 arch/x86/kernel/process.c          |   16 ++++++++++++++++
 2 files changed, 22 insertions(+)

arch/x86/kernel/process.c +16 -0

Index: 2.6/arch/x86/kernel/process.c
===================================================================
@@ -44,8 +44,24 @@ void free_thread_info(struct thread_info
 	free_pages((unsigned long)ti, get_order(THREAD_SIZE));
 }
 
+static struct kmem_cache *task_struct_cachep;
+
+struct task_struct *alloc_task_struct(void)
+{
+	return kmem_cache_alloc(task_struct_cachep, GFP_KERNEL);
+}
+
+void free_task_struct(struct task_struct *tsk)
+{
+	kmem_cache_free(task_struct_cachep, tsk);
+}
+
 void arch_task_cache_init(void)
 {
+	task_struct_cachep = kmem_cache_create("task_struct",
+				sizeof(struct task_struct), ARCH_MIN_TASKALIGN,
+				SLAB_PANIC, NULL);
+
         task_xstate_cachep =
         	kmem_cache_create("task_xstate", xstate_size,
 				  __alignof__(union thread_xstate),

arch/x86/include/asm/thread_info.h +6 -0

Index: 2.6/arch/x86/include/asm/thread_info.h
===================================================================
@@ -260,5 +260,11 @@ extern void arch_task_cache_init(void);
 extern void free_thread_info(struct thread_info *ti);
 extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
 #define arch_task_cache_init arch_task_cache_init
+
+#define __HAVE_ARCH_TASK_STRUCT_ALLOCATOR
+
+extern struct task_struct *alloc_task_struct(void);
+extern void free_task_struct(struct task_struct *tsk);
+
 #endif
 #endif /* _ASM_X86_THREAD_INFO_H */

Akinobu Mita

author

2008-12-27 05:19 UTC #3 lore raw

PATCH 3/4 x86: call free_thread_xstate() in free_task_struct()

In reply to Akinobu Mita

It happens in the following scenario in dup_task_struct()

1. call alloc_task_struct() to allocate empty task_struct
2. call alloc_thread_info() to allocate empty thread_info
3. call arch_dup_task_struct()

x86 arch specific arch_dup_task_struct() copies task_struct from source task_struct. it also allocates empty xstate and copy from source if source task_struct has ->thread.xstate.

If the xstate allocation failed, arch_dup_task_struct() returns error.

4. call free_thread_info() to deallocate thread_info

x86 arch specific free_thread_info() calls free_thread_xstate() with thread_info->task. But the thread_info is not initialized yet.

This patch resolves the issue by moving the free_thread_xstate() call into free_task_struct().

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 arch/x86/kernel/process.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/kernel/process.c +1 -1

Index: 2.6/arch/x86/kernel/process.c
===================================================================
@@ -40,7 +40,6 @@ void free_thread_xstate(struct task_stru
 
 void free_thread_info(struct thread_info *ti)
 {
-	free_thread_xstate(ti->task);
 	free_pages((unsigned long)ti, get_order(THREAD_SIZE));
 }

@@ -53,6 +52,7 @@ struct task_struct *alloc_task_struct(vo
 
 void free_task_struct(struct task_struct *tsk)
 {
+	free_thread_xstate(tsk);
 	kmem_cache_free(task_struct_cachep, tsk);
 }

Akinobu Mita

author

2008-12-27 05:20 UTC #4 lore raw

PATCH 4/4 x86: use generic thread_info allocator

In reply to Akinobu Mita

Use generic thread_info allocator for x86.
Because thread_info allocator for x86 and generic one are same now.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Signed-off-by Akinobu Mita <[email protected]>

---
 arch/x86/include/asm/page_32.h     |    6 +++---
 arch/x86/include/asm/page_64.h     |    4 ++--
 arch/x86/include/asm/thread_info.h |   13 -------------
 arch/x86/kernel/process.c          |    5 -----
 4 files changed, 5 insertions(+), 23 deletions(-)

arch/x86/include/asm/thread_info.h +0 ~~-13~~

Index: 2.6/arch/x86/include/asm/thread_info.h
===================================================================
@@ -147,18 +147,6 @@ struct thread_info {
 
 #define PREEMPT_ACTIVE		0x10000000
 
-/* thread information allocation */
-#ifdef CONFIG_DEBUG_STACK_USAGE
-#define THREAD_FLAGS (GFP_KERNEL | __GFP_ZERO)
-#else
-#define THREAD_FLAGS GFP_KERNEL
-#endif
-
-#define __HAVE_ARCH_THREAD_INFO_ALLOCATOR
-
-#define alloc_thread_info(tsk)						\
-	((struct thread_info *)__get_free_pages(THREAD_FLAGS, THREAD_ORDER))
-
 #ifdef CONFIG_X86_32
 
 #define STACK_WARN	(THREAD_SIZE/8)

@@ -257,7 +245,6 @@ static inline void set_restore_sigmask(v
 
 #ifndef __ASSEMBLY__
 extern void arch_task_cache_init(void);
-extern void free_thread_info(struct thread_info *ti);
 extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
 #define arch_task_cache_init arch_task_cache_init

arch/x86/kernel/process.c +0 -5

Index: 2.6/arch/x86/kernel/process.c
===================================================================
@@ -38,11 +38,6 @@ void free_thread_xstate(struct task_stru
 	}
 }
 
-void free_thread_info(struct thread_info *ti)
-{
-	free_pages((unsigned long)ti, get_order(THREAD_SIZE));
-}
-
 static struct kmem_cache *task_struct_cachep;
 
 struct task_struct *alloc_task_struct(void)

arch/x86/include/asm/page_32.h +3 -3

Index: 2.6/arch/x86/include/asm/page_32.h
===================================================================
@@ -14,11 +14,11 @@
 #define __PAGE_OFFSET		_AC(CONFIG_PAGE_OFFSET, UL)
 
 #ifdef CONFIG_4KSTACKS
-#define THREAD_ORDER	0
+#define THREAD_SIZE_ORDER	0
 #else
-#define THREAD_ORDER	1
+#define THREAD_SIZE_ORDER	1
 #endif
-#define THREAD_SIZE 	(PAGE_SIZE << THREAD_ORDER)
+#define THREAD_SIZE 	(PAGE_SIZE << THREAD_SIZE_ORDER)
 
 #define STACKFAULT_STACK 0
 #define DOUBLEFAULT_STACK 1

arch/x86/include/asm/page_64.h +2 -2

Index: 2.6/arch/x86/include/asm/page_64.h
===================================================================
@@ -3,8 +3,8 @@
 
 #define PAGETABLE_LEVELS	4
 
-#define THREAD_ORDER	1
-#define THREAD_SIZE  (PAGE_SIZE << THREAD_ORDER)
+#define THREAD_SIZE_ORDER	1
+#define THREAD_SIZE  (PAGE_SIZE << THREAD_SIZE_ORDER)
 #define CURRENT_MASK (~(THREAD_SIZE - 1))
 
 #define EXCEPTION_STACK_ORDER 0