]>
Commit | Line | Data |
---|---|---|
4d83a26e GKH |
1 | From 4d6501dce079c1eb6bf0b1d8f528a5e81770109e Mon Sep 17 00:00:00 2001 |
2 | From: Vegard Nossum <vegard.nossum@oracle.com> | |
3 | Date: Tue, 9 May 2017 09:39:59 +0200 | |
4 | Subject: kthread: Fix use-after-free if kthread fork fails | |
5 | ||
6 | From: Vegard Nossum <vegard.nossum@oracle.com> | |
7 | ||
8 | commit 4d6501dce079c1eb6bf0b1d8f528a5e81770109e upstream. | |
9 | ||
10 | If a kthread forks (e.g. usermodehelper since commit 1da5c46fa965) but | |
11 | fails in copy_process() between calling dup_task_struct() and setting | |
12 | p->set_child_tid, then the value of p->set_child_tid will be inherited | |
13 | from the parent and get prematurely freed by free_kthread_struct(). | |
14 | ||
15 | kthread() | |
16 | - worker_thread() | |
17 | - process_one_work() | |
18 | | - call_usermodehelper_exec_work() | |
19 | | - kernel_thread() | |
20 | | - _do_fork() | |
21 | | - copy_process() | |
22 | | - dup_task_struct() | |
23 | | - arch_dup_task_struct() | |
24 | | - tsk->set_child_tid = current->set_child_tid // implied | |
25 | | - ... | |
26 | | - goto bad_fork_* | |
27 | | - ... | |
28 | | - free_task(tsk) | |
29 | | - free_kthread_struct(tsk) | |
30 | | - kfree(tsk->set_child_tid) | |
31 | - ... | |
32 | - schedule() | |
33 | - __schedule() | |
34 | - wq_worker_sleeping() | |
35 | - kthread_data(task)->flags // UAF | |
36 | ||
37 | The problem started showing up with commit 1da5c46fa965 since it reused | |
38 | ->set_child_tid for the kthread worker data. | |
39 | ||
40 | A better long-term solution might be to get rid of the ->set_child_tid | |
41 | abuse. The comment in set_kthread_struct() also looks slightly wrong. | |
42 | ||
43 | Debugged-by: Jamie Iles <jamie.iles@oracle.com> | |
44 | Fixes: 1da5c46fa965 ("kthread: Make struct kthread kmalloc'ed") | |
45 | Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com> | |
46 | Acked-by: Oleg Nesterov <oleg@redhat.com> | |
47 | Cc: Peter Zijlstra <peterz@infradead.org> | |
48 | Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
49 | Cc: Andy Lutomirski <luto@kernel.org> | |
50 | Cc: Frederic Weisbecker <fweisbec@gmail.com> | |
51 | Cc: Jamie Iles <jamie.iles@oracle.com> | |
52 | Cc: stable@vger.kernel.org | |
53 | Link: http://lkml.kernel.org/r/20170509073959.17858-1-vegard.nossum@oracle.com | |
54 | Signed-off-by: Thomas Gleixner <tglx@linutronix.de> | |
55 | Signed-off-by: Amit Pundir <amit.pundir@linaro.org> | |
56 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
57 | ||
58 | --- | |
59 | kernel/fork.c | 17 ++++++++++++----- | |
60 | 1 file changed, 12 insertions(+), 5 deletions(-) | |
61 | ||
62 | --- a/kernel/fork.c | |
63 | +++ b/kernel/fork.c | |
64 | @@ -1532,6 +1532,18 @@ static __latent_entropy struct task_stru | |
65 | if (!p) | |
66 | goto fork_out; | |
67 | ||
68 | + /* | |
69 | + * This _must_ happen before we call free_task(), i.e. before we jump | |
70 | + * to any of the bad_fork_* labels. This is to avoid freeing | |
71 | + * p->set_child_tid which is (ab)used as a kthread's data pointer for | |
72 | + * kernel threads (PF_KTHREAD). | |
73 | + */ | |
74 | + p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL; | |
75 | + /* | |
76 | + * Clear TID on mm_release()? | |
77 | + */ | |
78 | + p->clear_child_tid = (clone_flags & CLONE_CHILD_CLEARTID) ? child_tidptr : NULL; | |
79 | + | |
80 | ftrace_graph_init_task(p); | |
81 | ||
82 | rt_mutex_init_task(p); | |
83 | @@ -1693,11 +1705,6 @@ static __latent_entropy struct task_stru | |
84 | } | |
85 | } | |
86 | ||
87 | - p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL; | |
88 | - /* | |
89 | - * Clear TID on mm_release()? | |
90 | - */ | |
91 | - p->clear_child_tid = (clone_flags & CLONE_CHILD_CLEARTID) ? child_tidptr : NULL; | |
92 | #ifdef CONFIG_BLOCK | |
93 | p->plug = NULL; | |
94 | #endif |