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
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
|
From 4b244721c11c2f66052ceadd8ef6c48a53290e10 Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 27 Sep 2017 12:50:42 -0700
Subject: [PATCH] KEYS: fix race between updating and finding negative key
In keyring_search_iterator() and in wait_for_key_construction(), we
check whether the key has been negatively instantiated, and if so return
the key's ->reject_error.
However, no lock is held during this, and ->reject_error is in union
with ->payload. And it's impossible for KEY_FLAG_NEGATIVE to be updated
atomically with respect to ->reject_error and ->payload.
Most problematically, when a negative key is positively instantiated via
__key_update() (via sys_add_key()), ->payload is initialized first, then
KEY_FLAG_NEGATIVE is cleared. But that means that ->reject_error can be
observed to have a bogus value, having been overwritten with ->payload,
while the key still appears to be "negative". Clearing
KEY_FLAG_NEGATIVE first wouldn't work either, since then anyone who
accesses the payload under rcu_read_lock() rather than the key semaphore
might observe an uninitialized ->payload. Nor can we just always take
the key's semaphore when checking whether the key is negative, since
keyring searches happen under rcu_read_lock().
Therefore, fix the bug by moving ->reject_error into the high bits of
->flags so that we can read and write it atomically with respect to
KEY_FLAG_NEGATIVE and KEY_FLAG_INSTANTIATED.
This will also allow KEY_FLAG_NEGATIVE to be removed, since tests for
KEY_FLAG_NEGATIVE can be replaced with tests for nonzero reject_error.
But for ease of backporting this fix, that is left for a later patch.
This fixes a kernel crash caused by the following program:
#include <stdlib.h>
#include <unistd.h>
#include <keyutils.h>
int main(void)
{
int ringid = keyctl_join_session_keyring(NULL);
if (fork()) {
for (;;) {
usleep(rand() % 4096);
add_key("user", "desc", "x", 1, ringid);
keyctl_clear(ringid);
}
} else {
for (;;)
request_key("user", "desc", "", ringid);
}
}
Here is the crash:
BUG: unable to handle kernel paging request at fffffffffd39a6b0
IP: __key_link_begin+0x0/0x100
PGD 7a0a067 P4D 7a0a067 PUD 7a0c067 PMD 0
Oops: 0000 [#1] SMP
CPU: 1 PID: 165 Comm: keyctl_negate_r Not tainted 4.14.0-rc1 #377
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
task: ffff9791fd809140 task.stack: ffffacba402bc000
RIP: 0010:__key_link_begin+0x0/0x100
RSP: 0018:ffffacba402bfdc8 EFLAGS: 00010282
RAX: ffff9791fd809140 RBX: fffffffffd39a620 RCX: 0000000000000008
RDX: ffffacba402bfdd0 RSI: fffffffffd39a6a0 RDI: ffff9791fd810600
RBP: ffffacba402bfdf8 R08: 0000000000000063 R09: ffffffff94845620
R10: 8080808080808080 R11: 0000000000000004 R12: ffff9791fd810600
R13: ffff9791fd39a940 R14: fffffffffd39a6a0 R15: 0000000000000000
FS: 00007fbf14a90740(0000) GS:ffff9791ffd00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: fffffffffd39a6b0 CR3: 000000003b910003 CR4: 00000000003606e0
Call Trace:
? key_link+0x28/0xb0
? search_process_keyrings+0x13/0x100
request_key_and_link+0xcb/0x550
? keyring_instantiate+0x110/0x110
? key_default_cmp+0x20/0x20
SyS_request_key+0xc0/0x160
? exit_to_usermode_loop+0x5e/0x80
entry_SYSCALL_64_fastpath+0x1a/0xa5
RIP: 0033:0x7fbf14190bb9
RSP: 002b:00007ffd8e4fe6c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f9
RAX: ffffffffffffffda RBX: 0000000036cc28fb RCX: 00007fbf14190bb9
RDX: 000055748b56ca4a RSI: 000055748b56ca46 RDI: 000055748b56ca4b
RBP: 000055748b56ca4a R08: 0000000000000001 R09: 0000000000000001
R10: 0000000036cc28fb R11: 0000000000000246 R12: 000055748b56c8b0
R13: 00007ffd8e4fe7d0 R14: 0000000000000000 R15: 0000000000000000
Code: c5 0f 85 69 ff ff ff 48 c7 c3 82 ff ff ff eb ab 45 31 ed e9 18 ff ff ff 85 c0 75 8d eb d2 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 <48> 83 7e 10 00 0f 84 c5 00 00 00 55 48 89 e5 41 57 41 56 41 55
RIP: __key_link_begin+0x0/0x100 RSP: ffffacba402bfdc8
CR2: fffffffffd39a6b0
Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
Cc: <stable@vger.kernel.org> [v4.4+]
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
include/linux/key.h | 12 +++++++++++-
security/keys/key.c | 26 +++++++++++++++++++-------
security/keys/keyctl.c | 3 +++
security/keys/keyring.c | 4 ++--
security/keys/request_key.c | 11 +++++++----
5 files changed, 42 insertions(+), 14 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index e315e16b6ff8..b7b590d7c480 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -189,6 +189,17 @@ struct key {
#define KEY_FLAG_KEEP 10 /* set if key should not be removed */
#define KEY_FLAG_UID_KEYRING 11 /* set if key is a user or user session keyring */
+ /*
+ * If the key is negatively instantiated, then bits 20-31 hold the error
+ * code which should be returned when someone tries to use the key
+ * (unless they allow negative keys). The error code is stored as a
+ * positive number, so it must be negated before being returned.
+ *
+ * Note that a key can go from negative to positive but not vice versa.
+ */
+#define KEY_FLAGS_REJECT_ERROR_SHIFT 20
+#define KEY_FLAGS_REJECT_ERROR_MASK 0xFFF00000
+
/* the key type and key description string
* - the desc is used to match a key against search criteria
* - it should be a printable string
@@ -213,7 +224,6 @@ struct key {
struct list_head name_link;
struct assoc_array keys;
};
- int reject_error;
};
/* This is set on a keyring to restrict the addition of a link to a key
diff --git a/security/keys/key.c b/security/keys/key.c
index eb914a838840..786158d3442e 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -401,6 +401,20 @@ int key_payload_reserve(struct key *key, size_t datalen)
}
EXPORT_SYMBOL(key_payload_reserve);
+static void mark_key_instantiated(struct key *key, unsigned int reject_error)
+{
+ unsigned long old, new;
+
+ do {
+ old = READ_ONCE(key->flags);
+ new = (old & ~((1 << KEY_FLAG_NEGATIVE) |
+ KEY_FLAGS_REJECT_ERROR_MASK)) |
+ (1 << KEY_FLAG_INSTANTIATED) |
+ (reject_error ? (1 << KEY_FLAG_NEGATIVE) : 0) |
+ (reject_error << KEY_FLAGS_REJECT_ERROR_SHIFT);
+ } while (cmpxchg_release(&key->flags, old, new) != old);
+}
+
/*
* Instantiate a key and link it into the target keyring atomically. Must be
* called with the target keyring's semaphore writelocked. The target key's
@@ -431,7 +445,7 @@ static int __key_instantiate_and_link(struct key *key,
if (ret == 0) {
/* mark the key as being instantiated */
atomic_inc(&key->user->nikeys);
- set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+ mark_key_instantiated(key, 0);
if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
awaken = 1;
@@ -580,10 +594,8 @@ int key_reject_and_link(struct key *key,
if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
/* mark the key as being negatively instantiated */
atomic_inc(&key->user->nikeys);
- key->reject_error = -error;
- smp_wmb();
- set_bit(KEY_FLAG_NEGATIVE, &key->flags);
- set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
+ mark_key_instantiated(key, error);
+
now = current_kernel_time();
key->expiry = now.tv_sec + timeout;
key_schedule_gc(key->expiry + key_gc_delay);
@@ -753,7 +765,7 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
ret = key->type->update(key, prep);
if (ret == 0)
/* updating a negative key instantiates it */
- clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+ mark_key_instantiated(key, 0);
up_write(&key->sem);
@@ -987,7 +999,7 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
ret = key->type->update(key, &prep);
if (ret == 0)
/* updating a negative key instantiates it */
- clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
+ mark_key_instantiated(key, 0);
up_write(&key->sem);
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 365ff85d7e27..19a09e121089 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1223,6 +1223,9 @@ long keyctl_reject_key(key_serial_t id, unsigned timeout, unsigned error,
error == ERESTART_RESTARTBLOCK)
return -EINVAL;
+ BUILD_BUG_ON(MAX_ERRNO > (KEY_FLAGS_REJECT_ERROR_MASK >>
+ KEY_FLAGS_REJECT_ERROR_SHIFT));
+
/* the appropriate instantiation authorisation key must have been
* assumed before calling this */
ret = -EPERM;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 129a4175760b..e54ad0ed7aa4 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -598,8 +598,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
if (ctx->flags & KEYRING_SEARCH_DO_STATE_CHECK) {
/* we set a different error code if we pass a negative key */
if (kflags & (1 << KEY_FLAG_NEGATIVE)) {
- smp_rmb();
- ctx->result = ERR_PTR(key->reject_error);
+ ctx->result = ERR_PTR(-(int)(kflags >>
+ KEY_FLAGS_REJECT_ERROR_SHIFT));
kleave(" = %d [neg]", ctx->skipped_ret);
goto skipped;
}
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 63e63a42db3c..0aab68344837 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -590,15 +590,18 @@ struct key *request_key_and_link(struct key_type *type,
int wait_for_key_construction(struct key *key, bool intr)
{
int ret;
+ unsigned long flags;
ret = wait_on_bit(&key->flags, KEY_FLAG_USER_CONSTRUCT,
intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
if (ret)
return -ERESTARTSYS;
- if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
- smp_rmb();
- return key->reject_error;
- }
+
+ /* Pairs with RELEASE in mark_key_instantiated() */
+ flags = smp_load_acquire(&key->flags);
+ if (flags & (1 << KEY_FLAG_NEGATIVE))
+ return -(int)(flags >> KEY_FLAGS_REJECT_ERROR_SHIFT);
+
return key_validate(key);
}
EXPORT_SYMBOL(wait_for_key_construction);
--
2.13.6
|
