1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
|
From 35ee9ac513280f46eeb1196bac82ed5320380412 Mon Sep 17 00:00:00 2001
From: Jann Horn <jannh@google.com>
Date: Thu, 3 Dec 2020 02:25:05 +0100
Subject: [PATCH] tty: Fix ->session locking
commit c8bcd9c5be24fb9e6132e97da5a35e55a83e36b9 upstream.
Currently, locking of ->session is very inconsistent; most places
protect it using the legacy tty mutex, but disassociate_ctty(),
__do_SAK(), tiocspgrp() and tiocgsid() don't.
Two of the writers hold the ctrl_lock (because they already need it for
->pgrp), but __proc_set_tty() doesn't do that yet.
On a PREEMPT=y system, an unprivileged user can theoretically abuse
this broken locking to read 4 bytes of freed memory via TIOCGSID if
tiocgsid() is preempted long enough at the right point. (Other things
might also go wrong, especially if root-only ioctls are involved; I'm
not sure about that.)
Change the locking on ->session such that:
- tty_lock() is held by all writers: By making disassociate_ctty()
hold it. This should be fine because the same lock can already be
taken through the call to tty_vhangup_session().
The tricky part is that we need to shorten the area covered by
siglock to be able to take tty_lock() without ugly retry logic; as
far as I can tell, this should be fine, since nothing in the
signal_struct is touched in the `if (tty)` branch.
- ctrl_lock is held by all writers: By changing __proc_set_tty() to
hold the lock a little longer.
- All readers that aren't holding tty_lock() hold ctrl_lock: By
adding locking to tiocgsid() and __do_SAK(), and expanding the area
covered by ctrl_lock in tiocspgrp().
Cc: stable@kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/tty/tty_io.c | 7 ++++++-
drivers/tty/tty_jobctrl.c | 44 +++++++++++++++++++++++++++------------
include/linux/tty.h | 4 ++++
3 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 36c1c59cc72a..642765bf1023 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2894,10 +2894,14 @@ void __do_SAK(struct tty_struct *tty)
struct task_struct *g, *p;
struct pid *session;
int i;
+ unsigned long flags;
if (!tty)
return;
- session = tty->session;
+
+ spin_lock_irqsave(&tty->ctrl_lock, flags);
+ session = get_pid(tty->session);
+ spin_unlock_irqrestore(&tty->ctrl_lock, flags);
tty_ldisc_flush(tty);
@@ -2929,6 +2933,7 @@ void __do_SAK(struct tty_struct *tty)
task_unlock(p);
} while_each_thread(g, p);
read_unlock(&tasklist_lock);
+ put_pid(session);
#endif
}
diff --git a/drivers/tty/tty_jobctrl.c b/drivers/tty/tty_jobctrl.c
index af508957ff05..813be2c05262 100644
--- a/drivers/tty/tty_jobctrl.c
+++ b/drivers/tty/tty_jobctrl.c
@@ -103,8 +103,8 @@ static void __proc_set_tty(struct tty_struct *tty)
put_pid(tty->session);
put_pid(tty->pgrp);
tty->pgrp = get_pid(task_pgrp(current));
- spin_unlock_irqrestore(&tty->ctrl_lock, flags);
tty->session = get_pid(task_session(current));
+ spin_unlock_irqrestore(&tty->ctrl_lock, flags);
if (current->signal->tty) {
tty_debug(tty, "current tty %s not NULL!!\n",
current->signal->tty->name);
@@ -293,20 +293,23 @@ void disassociate_ctty(int on_exit)
spin_lock_irq(¤t->sighand->siglock);
put_pid(current->signal->tty_old_pgrp);
current->signal->tty_old_pgrp = NULL;
-
tty = tty_kref_get(current->signal->tty);
+ spin_unlock_irq(¤t->sighand->siglock);
+
if (tty) {
unsigned long flags;
+
+ tty_lock(tty);
spin_lock_irqsave(&tty->ctrl_lock, flags);
put_pid(tty->session);
put_pid(tty->pgrp);
tty->session = NULL;
tty->pgrp = NULL;
spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+ tty_unlock(tty);
tty_kref_put(tty);
}
- spin_unlock_irq(¤t->sighand->siglock);
/* Now clear signal->tty under the lock */
read_lock(&tasklist_lock);
session_clear_tty(task_session(current));
@@ -477,14 +480,19 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
return -ENOTTY;
if (retval)
return retval;
- if (!current->signal->tty ||
- (current->signal->tty != real_tty) ||
- (real_tty->session != task_session(current)))
- return -ENOTTY;
+
if (get_user(pgrp_nr, p))
return -EFAULT;
if (pgrp_nr < 0)
return -EINVAL;
+
+ spin_lock_irq(&real_tty->ctrl_lock);
+ if (!current->signal->tty ||
+ (current->signal->tty != real_tty) ||
+ (real_tty->session != task_session(current))) {
+ retval = -ENOTTY;
+ goto out_unlock_ctrl;
+ }
rcu_read_lock();
pgrp = find_vpid(pgrp_nr);
retval = -ESRCH;
@@ -494,12 +502,12 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
if (session_of_pgrp(pgrp) != task_session(current))
goto out_unlock;
retval = 0;
- spin_lock_irq(&real_tty->ctrl_lock);
put_pid(real_tty->pgrp);
real_tty->pgrp = get_pid(pgrp);
- spin_unlock_irq(&real_tty->ctrl_lock);
out_unlock:
rcu_read_unlock();
+out_unlock_ctrl:
+ spin_unlock_irq(&real_tty->ctrl_lock);
return retval;
}
@@ -511,20 +519,30 @@ static int tiocspgrp(struct tty_struct *tty, struct tty_struct *real_tty, pid_t
*
* Obtain the session id of the tty. If there is no session
* return an error.
- *
- * Locking: none. Reference to current->signal->tty is safe.
*/
static int tiocgsid(struct tty_struct *tty, struct tty_struct *real_tty, pid_t __user *p)
{
+ unsigned long flags;
+ pid_t sid;
+
/*
* (tty == real_tty) is a cheap way of
* testing if the tty is NOT a master pty.
*/
if (tty == real_tty && current->signal->tty != real_tty)
return -ENOTTY;
+
+ spin_lock_irqsave(&real_tty->ctrl_lock, flags);
if (!real_tty->session)
- return -ENOTTY;
- return put_user(pid_vnr(real_tty->session), p);
+ goto err;
+ sid = pid_vnr(real_tty->session);
+ spin_unlock_irqrestore(&real_tty->ctrl_lock, flags);
+
+ return put_user(sid, p);
+
+err:
+ spin_unlock_irqrestore(&real_tty->ctrl_lock, flags);
+ return -ENOTTY;
}
/*
diff --git a/include/linux/tty.h b/include/linux/tty.h
index a99e9b8e4e31..eb33d948788c 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -306,6 +306,10 @@ struct tty_struct {
struct termiox *termiox; /* May be NULL for unsupported */
char name[64];
struct pid *pgrp; /* Protected by ctrl lock */
+ /*
+ * Writes protected by both ctrl lock and legacy mutex, readers must use
+ * at least one of them.
+ */
struct pid *session;
unsigned long flags;
int count;
--
2.17.1
|
