From: Eric Biggers <ebiggers@xxxxxxxxxx> In keyring_search_iterator() and in wait_for_key_construction(), we check whether the key has been negatively instantiated, and if so retrieve the ->reject_error, which is in union with ->payload. The problem is that no lock can be held during this, and it's impossible to update KEY_FLAG_NEGATIVE atomically with respect to ->reject_error and ->payload. Updating ->reject_error or ->payload first as key_update(), __key_update(), and key_reject_and_link() do is insufficient because ->reject_error can be observed to have a bogus value, having been overwritten with ->payload, before KEY_FLAG_NEGATIVE is cleared. Conversely, if we were to change KEY_FLAG_NEGATIVE first, then someone could use ->reject_error or ->payload before it has been set. Fix the bug by moving ->reject_error out of the union with ->payload, then using nonzero ->reject_error to mean that the key is negative. This eliminates the need for KEY_FLAG_NEGATIVE, which we remove as well so that we don't have to handle memory ordering between KEY_FLAG_NEGATIVE and ->reject_error. We *do* still need to handle memory ordering between KEY_FLAG_INSTANTIATED and ->reject_error, but that was needed before (and for KEY_FLAG_NEGATIVE as well --- though it wasn't done correctly, which was another bug). The following program reproduces the primary bug, which dates back to v4.4 when ->reject_error and ->payload were placed in union: #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); } } It causes the following 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@xxxxxxxxxxxxxxx> [v4.4+] Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx> --- include/linux/key.h | 34 ++++++++++++++++++++++++++++---- security/keys/encrypted-keys/encrypted.c | 2 +- security/keys/gc.c | 4 +--- security/keys/key.c | 16 ++++++++++----- security/keys/keyctl.c | 2 +- security/keys/keyring.c | 5 ++--- security/keys/proc.c | 2 +- security/keys/request_key.c | 8 ++++---- security/keys/trusted.c | 2 +- security/keys/user_defined.c | 2 +- 10 files changed, 53 insertions(+), 24 deletions(-) diff --git a/include/linux/key.h b/include/linux/key.h index e315e16b6ff8..e4b05ade798a 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -181,7 +181,6 @@ struct key { #define KEY_FLAG_REVOKED 2 /* set if key had been revoked */ #define KEY_FLAG_IN_QUOTA 3 /* set if key consumes quota */ #define KEY_FLAG_USER_CONSTRUCT 4 /* set if key is being constructed in userspace */ -#define KEY_FLAG_NEGATIVE 5 /* set if key is negative */ #define KEY_FLAG_ROOT_CAN_CLEAR 6 /* set if key can be cleared by root without permission */ #define KEY_FLAG_INVALIDATED 7 /* set if key has been invalidated */ #define KEY_FLAG_BUILTIN 8 /* set if key is built in to the kernel */ @@ -213,9 +212,22 @@ struct key { struct list_head name_link; struct assoc_array keys; }; - int reject_error; }; + /* + * This is set to a negative error value if the key is negatively + * instantiated. + * + * This could in theory be in union with ->payload, with a separate flag + * indicating that the key is negative. But that doesn't work because + * sometimes we need to check this without holding a lock. + * + * Note that keys can go from negative to positive but not vice versa. + * That is, if KEY_FLAG_INSTANTIATED is set and this field is 0, then + * the key is positive, and it cannot turn negative out from under you. + */ + int reject_error; + /* This is set on a keyring to restrict the addition of a link to a key * to it. If this structure isn't provided then it is assumed that the * keyring is open to any addition. It is ignored for non-keyring @@ -353,6 +365,18 @@ extern void key_set_timeout(struct key *, unsigned); #define KEY_NEED_SETATTR 0x20 /* Require permission to change attributes */ #define KEY_NEED_ALL 0x3f /* All the above permissions */ +/** + * key_is_negative - Determine if a key has been negatively instantiated + * @key: The key to check. + * + * Return true if the specified key has been negatively instantiated, false + * otherwise. + */ +static inline bool key_is_negative(const struct key *key) +{ + return READ_ONCE(key->reject_error) != 0; +} + /** * key_is_instantiated - Determine if a key has been positively instantiated * @key: The key to check. @@ -362,8 +386,10 @@ extern void key_set_timeout(struct key *, unsigned); */ static inline bool key_is_instantiated(const struct key *key) { - return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) && - !test_bit(KEY_FLAG_NEGATIVE, &key->flags); + if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) + return false; + smp_rmb(); /* pairs with smp_wmb() in key_reject_and_link() */ + return !key_is_negative(key); } #define dereference_key_rcu(KEY) \ diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 69855ba0d3b3..f54b92868bc3 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -847,7 +847,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep) size_t datalen = prep->datalen; int ret = 0; - if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) + if (key_is_negative(key)) return -ENOKEY; if (datalen <= 0 || datalen > 32767 || !prep->data) return -EINVAL; diff --git a/security/keys/gc.c b/security/keys/gc.c index 87cb260e4890..0adc52be3ea9 100644 --- a/security/keys/gc.c +++ b/security/keys/gc.c @@ -135,9 +135,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys) key_check(key); /* Throw away the key data if the key is instantiated */ - if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags) && - !test_bit(KEY_FLAG_NEGATIVE, &key->flags) && - key->type->destroy) + if (key_is_instantiated(key) && key->type->destroy) key->type->destroy(key); security_key_free(key); diff --git a/security/keys/key.c b/security/keys/key.c index eb914a838840..bed5b6c2ef20 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -578,11 +578,17 @@ int key_reject_and_link(struct key *key, /* can't instantiate twice */ if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) { - /* mark the key as being negatively instantiated */ atomic_inc(&key->user->nikeys); - key->reject_error = -error; + + /* mark the key as being negatively instantiated */ + WRITE_ONCE(key->reject_error, -error); + + /* + * pairs with smp_rmb() in key_is_instantiated() and + * wait_for_key_construction() + */ smp_wmb(); - set_bit(KEY_FLAG_NEGATIVE, &key->flags); + set_bit(KEY_FLAG_INSTANTIATED, &key->flags); now = current_kernel_time(); key->expiry = now.tv_sec + timeout; @@ -753,7 +759,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); + WRITE_ONCE(key->reject_error, 0); up_write(&key->sem); @@ -987,7 +993,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); + WRITE_ONCE(key->reject_error, 0); up_write(&key->sem); diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 365ff85d7e27..c9d94f61d183 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -766,7 +766,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) key = key_ref_to_ptr(key_ref); - if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) { + if (key_is_negative(key)) { ret = -ENOKEY; goto error2; } diff --git a/security/keys/keyring.c b/security/keys/keyring.c index 4fa82a8a9c0e..5a13858d74fb 100644 --- a/security/keys/keyring.c +++ b/security/keys/keyring.c @@ -597,9 +597,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(READ_ONCE(key->reject_error)); + if (ctx->result) { kleave(" = %d [neg]", ctx->skipped_ret); goto skipped; } diff --git a/security/keys/proc.c b/security/keys/proc.c index de834309d100..e5e41f3c547d 100644 --- a/security/keys/proc.c +++ b/security/keys/proc.c @@ -246,7 +246,7 @@ static int proc_keys_show(struct seq_file *m, void *v) showflag(key, 'D', KEY_FLAG_DEAD), showflag(key, 'Q', KEY_FLAG_IN_QUOTA), showflag(key, 'U', KEY_FLAG_USER_CONSTRUCT), - showflag(key, 'N', KEY_FLAG_NEGATIVE), + key_is_negative(key) ? 'N' : '-', showflag(key, 'i', KEY_FLAG_INVALIDATED), refcount_read(&key->usage), xbuf, diff --git a/security/keys/request_key.c b/security/keys/request_key.c index 63e63a42db3c..5cddca6bef37 100644 --- a/security/keys/request_key.c +++ b/security/keys/request_key.c @@ -595,10 +595,10 @@ int wait_for_key_construction(struct key *key, bool intr) intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE); if (ret) return -ERESTARTSYS; - if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) { - smp_rmb(); - return key->reject_error; - } + smp_rmb(); /* pairs with smp_wmb() in key_reject_and_link() */ + ret = READ_ONCE(key->reject_error); + if (ret) + return ret; return key_validate(key); } EXPORT_SYMBOL(wait_for_key_construction); diff --git a/security/keys/trusted.c b/security/keys/trusted.c index ddfaebf60fc8..bd85315cbfeb 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -1066,7 +1066,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) char *datablob; int ret = 0; - if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) + if (key_is_negative(key)) return -ENOKEY; p = key->payload.data[0]; if (!p->migratable) diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c index 3d8c68eba516..a5506400836c 100644 --- a/security/keys/user_defined.c +++ b/security/keys/user_defined.c @@ -114,7 +114,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep) /* attach the new data, displacing the old */ key->expiry = prep->expiry; - if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags)) + if (!key_is_negative(key)) zap = dereference_key_locked(key); rcu_assign_keypointer(key, prep->payload.data[0]); prep->payload.data[0] = NULL; -- 2.14.1.690.gbb1197296e-goog