From: Greg Kroah-Hartman Date: Wed, 25 Oct 2017 09:48:16 +0000 (+0200) Subject: 4.9-stable patches X-Git-Tag: v3.18.78~1 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4baa735ddf0020537f6e62e382ae410a9c80af40;p=thirdparty%2Fkernel%2Fstable-queue.git 4.9-stable patches added patches: fs-cache-fix-dereference-of-null-user_key_payload.patch fscrypt-fix-dereference-of-null-user_key_payload.patch keys-fix-race-between-updating-and-finding-a-negative-key.patch --- diff --git a/queue-4.9/fs-cache-fix-dereference-of-null-user_key_payload.patch b/queue-4.9/fs-cache-fix-dereference-of-null-user_key_payload.patch new file mode 100644 index 00000000000..205c7e34d10 --- /dev/null +++ b/queue-4.9/fs-cache-fix-dereference-of-null-user_key_payload.patch @@ -0,0 +1,45 @@ +From d124b2c53c7bee6569d2a2d0b18b4a1afde00134 Mon Sep 17 00:00:00 2001 +From: Eric Biggers +Date: Mon, 9 Oct 2017 12:40:00 -0700 +Subject: FS-Cache: fix dereference of NULL user_key_payload + +From: Eric Biggers + +commit d124b2c53c7bee6569d2a2d0b18b4a1afde00134 upstream. + +When the file /proc/fs/fscache/objects (available with +CONFIG_FSCACHE_OBJECT_LIST=y) is opened, we request a user key with +description "fscache:objlist", then access its payload. However, a +revoked key has a NULL payload, and we failed to check for this. +request_key() *does* skip revoked keys, but there is still a window +where the key can be revoked before we access its payload. + +Fix it by checking for a NULL payload, treating it like a key which was +already revoked at the time it was requested. + +Fixes: 4fbf4291aa15 ("FS-Cache: Allow the current state of all objects to be dumped") +Reviewed-by: James Morris +Signed-off-by: Eric Biggers +Signed-off-by: David Howells +Signed-off-by: Greg Kroah-Hartman + +--- + fs/fscache/object-list.c | 7 +++++++ + 1 file changed, 7 insertions(+) + +--- a/fs/fscache/object-list.c ++++ b/fs/fscache/object-list.c +@@ -330,6 +330,13 @@ static void fscache_objlist_config(struc + rcu_read_lock(); + + confkey = user_key_payload(key); ++ if (!confkey) { ++ /* key was revoked */ ++ rcu_read_unlock(); ++ key_put(key); ++ goto no_config; ++ } ++ + buf = confkey->data; + + for (len = confkey->datalen - 1; len >= 0; len--) { diff --git a/queue-4.9/fscrypt-fix-dereference-of-null-user_key_payload.patch b/queue-4.9/fscrypt-fix-dereference-of-null-user_key_payload.patch new file mode 100644 index 00000000000..4800c0b17b5 --- /dev/null +++ b/queue-4.9/fscrypt-fix-dereference-of-null-user_key_payload.patch @@ -0,0 +1,43 @@ +From d60b5b7854c3d135b869f74fb93eaf63cbb1991a Mon Sep 17 00:00:00 2001 +From: Eric Biggers +Date: Mon, 9 Oct 2017 12:46:18 -0700 +Subject: fscrypt: fix dereference of NULL user_key_payload + +From: Eric Biggers + +commit d60b5b7854c3d135b869f74fb93eaf63cbb1991a upstream. + +When an fscrypt-encrypted file is opened, we request the file's master +key from the keyrings service as a logon key, then access its payload. +However, a revoked key has a NULL payload, and we failed to check for +this. request_key() *does* skip revoked keys, but there is still a +window where the key can be revoked before we acquire its semaphore. + +Fix it by checking for a NULL payload, treating it like a key which was +already revoked at the time it was requested. + +Fixes: 88bd6ccdcdd6 ("ext4 crypto: add encryption key management facilities") +Reviewed-by: James Morris +Signed-off-by: Eric Biggers +Signed-off-by: David Howells +Signed-off-by: Greg Kroah-Hartman + + +--- + fs/crypto/keyinfo.c | 5 +++++ + 1 file changed, 5 insertions(+) + +--- a/fs/crypto/keyinfo.c ++++ b/fs/crypto/keyinfo.c +@@ -108,6 +108,11 @@ static int validate_user_key(struct fscr + goto out; + } + ukp = user_key_payload(keyring_key); ++ if (!ukp) { ++ /* key was revoked before we acquired its semaphore */ ++ res = -EKEYREVOKED; ++ goto out; ++ } + if (ukp->datalen != sizeof(struct fscrypt_key)) { + res = -EINVAL; + goto out; diff --git a/queue-4.9/keys-fix-race-between-updating-and-finding-a-negative-key.patch b/queue-4.9/keys-fix-race-between-updating-and-finding-a-negative-key.patch new file mode 100644 index 00000000000..bdf02746d0e --- /dev/null +++ b/queue-4.9/keys-fix-race-between-updating-and-finding-a-negative-key.patch @@ -0,0 +1,466 @@ +From 363b02dab09b3226f3bd1420dad9c72b79a42a76 Mon Sep 17 00:00:00 2001 +From: David Howells +Date: Wed, 4 Oct 2017 16:43:25 +0100 +Subject: KEYS: Fix race between updating and finding a negative key + +From: David Howells + +commit 363b02dab09b3226f3bd1420dad9c72b79a42a76 upstream. + +Consolidate KEY_FLAG_INSTANTIATED, KEY_FLAG_NEGATIVE and the rejection +error into one field such that: + + (1) The instantiation state can be modified/read atomically. + + (2) The error can be accessed atomically with the state. + + (3) The error isn't stored unioned with the payload pointers. + +This deals with the problem that the state is spread over three different +objects (two bits and a separate variable) and reading or updating them +atomically isn't practical, given that not only can uninstantiated keys +change into instantiated or rejected keys, but rejected keys can also turn +into instantiated keys - and someone accessing the key might not be using +any locking. + +The main side effect of this problem is that what was held in the payload +may change, depending on the state. For instance, you might observe the +key to be in the rejected state. You then read the cached error, but if +the key semaphore wasn't locked, the key might've become instantiated +between the two reads - and you might now have something in hand that isn't +actually an error code. + +The state is now KEY_IS_UNINSTANTIATED, KEY_IS_POSITIVE or a negative error +code if the key is negatively instantiated. The key_is_instantiated() +function is replaced with key_is_positive() to avoid confusion as negative +keys are also 'instantiated'. + +Additionally, barriering is included: + + (1) Order payload-set before state-set during instantiation. + + (2) Order state-read before payload-read when using the key. + +Further separate barriering is necessary if RCU is being used to access the +payload content after reading the payload pointers. + +Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data") +Reported-by: Eric Biggers +Signed-off-by: David Howells +Reviewed-by: Eric Biggers +Signed-off-by: Greg Kroah-Hartman +--- + include/linux/key.h | 47 +++++++++++++++++++------------ + net/dns_resolver/dns_key.c | 2 - + security/keys/big_key.c | 4 +- + security/keys/encrypted-keys/encrypted.c | 2 - + security/keys/gc.c | 8 ++--- + security/keys/key.c | 31 +++++++++++++------- + security/keys/keyctl.c | 9 ++--- + security/keys/keyring.c | 10 +++--- + security/keys/proc.c | 7 +++- + security/keys/process_keys.c | 2 - + security/keys/request_key.c | 7 +--- + security/keys/request_key_auth.c | 2 - + security/keys/trusted.c | 2 - + security/keys/user_defined.c | 4 +- + 14 files changed, 80 insertions(+), 57 deletions(-) + +--- a/include/linux/key.h ++++ b/include/linux/key.h +@@ -126,6 +126,11 @@ static inline bool is_key_possessed(cons + return (unsigned long) key_ref & 1UL; + } + ++enum key_state { ++ KEY_IS_UNINSTANTIATED, ++ KEY_IS_POSITIVE, /* Positively instantiated */ ++}; ++ + /*****************************************************************************/ + /* + * authentication token / access credential / keyring +@@ -157,6 +162,7 @@ struct key { + * - may not match RCU dereferenced payload + * - payload should contain own length + */ ++ short state; /* Key state (+) or rejection error (-) */ + + #ifdef KEY_DEBUGGING + unsigned magic; +@@ -165,18 +171,16 @@ struct key { + #endif + + unsigned long flags; /* status flags (change with bitops) */ +-#define KEY_FLAG_INSTANTIATED 0 /* set if key has been instantiated */ +-#define KEY_FLAG_DEAD 1 /* set if key type has been deleted */ +-#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 */ +-#define KEY_FLAG_ROOT_CAN_INVAL 9 /* set if key can be invalidated by root without permission */ +-#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 */ ++#define KEY_FLAG_DEAD 0 /* set if key type has been deleted */ ++#define KEY_FLAG_REVOKED 1 /* set if key had been revoked */ ++#define KEY_FLAG_IN_QUOTA 2 /* set if key consumes quota */ ++#define KEY_FLAG_USER_CONSTRUCT 3 /* set if key is being constructed in userspace */ ++#define KEY_FLAG_ROOT_CAN_CLEAR 4 /* set if key can be cleared by root without permission */ ++#define KEY_FLAG_INVALIDATED 5 /* set if key has been invalidated */ ++#define KEY_FLAG_BUILTIN 6 /* set if key is built in to the kernel */ ++#define KEY_FLAG_ROOT_CAN_INVAL 7 /* set if key can be invalidated by root without permission */ ++#define KEY_FLAG_KEEP 8 /* set if key should not be removed */ ++#define KEY_FLAG_UID_KEYRING 9 /* set if key is a user or user session keyring */ + + /* the key type and key description string + * - the desc is used to match a key against search criteria +@@ -202,7 +206,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 +@@ -343,17 +346,27 @@ extern void key_set_timeout(struct key * + #define KEY_NEED_SETATTR 0x20 /* Require permission to change attributes */ + #define KEY_NEED_ALL 0x3f /* All the above permissions */ + ++static inline short key_read_state(const struct key *key) ++{ ++ /* Barrier versus mark_key_instantiated(). */ ++ return smp_load_acquire(&key->state); ++} ++ + /** +- * key_is_instantiated - Determine if a key has been positively instantiated ++ * key_is_positive - Determine if a key has been positively instantiated + * @key: The key to check. + * + * Return true if the specified key has been positively instantiated, false + * otherwise. + */ +-static inline bool key_is_instantiated(const struct key *key) ++static inline bool key_is_positive(const struct key *key) ++{ ++ return key_read_state(key) == KEY_IS_POSITIVE; ++} ++ ++static inline bool key_is_negative(const struct key *key) + { +- return test_bit(KEY_FLAG_INSTANTIATED, &key->flags) && +- !test_bit(KEY_FLAG_NEGATIVE, &key->flags); ++ return key_read_state(key) < 0; + } + + #define rcu_dereference_key(KEY) \ +--- a/net/dns_resolver/dns_key.c ++++ b/net/dns_resolver/dns_key.c +@@ -224,7 +224,7 @@ static int dns_resolver_match_preparse(s + static void dns_resolver_describe(const struct key *key, struct seq_file *m) + { + seq_puts(m, key->description); +- if (key_is_instantiated(key)) { ++ if (key_is_positive(key)) { + int err = PTR_ERR(key->payload.data[dns_key_error]); + + if (err) +--- a/security/keys/big_key.c ++++ b/security/keys/big_key.c +@@ -245,7 +245,7 @@ void big_key_revoke(struct key *key) + + /* clear the quota */ + key_payload_reserve(key, 0); +- if (key_is_instantiated(key) && ++ if (key_is_positive(key) && + (size_t)key->payload.data[big_key_len] > BIG_KEY_FILE_THRESHOLD) + vfs_truncate(path, 0); + } +@@ -277,7 +277,7 @@ void big_key_describe(const struct key * + + seq_puts(m, key->description); + +- if (key_is_instantiated(key)) ++ if (key_is_positive(key)) + seq_printf(m, ": %zu [%s]", + datalen, + datalen > BIG_KEY_FILE_THRESHOLD ? "file" : "buff"); +--- a/security/keys/encrypted-keys/encrypted.c ++++ b/security/keys/encrypted-keys/encrypted.c +@@ -874,7 +874,7 @@ static int encrypted_update(struct key * + 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; +--- a/security/keys/gc.c ++++ b/security/keys/gc.c +@@ -129,15 +129,15 @@ static noinline void key_gc_unused_keys( + while (!list_empty(keys)) { + struct key *key = + list_entry(keys->next, struct key, graveyard_link); ++ short state = key->state; ++ + list_del(&key->graveyard_link); + + kdebug("- %u", key->serial); + 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 (state == KEY_IS_POSITIVE && key->type->destroy) + key->type->destroy(key); + + security_key_free(key); +@@ -151,7 +151,7 @@ static noinline void key_gc_unused_keys( + } + + atomic_dec(&key->user->nkeys); +- if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) ++ if (state != KEY_IS_UNINSTANTIATED) + atomic_dec(&key->user->nikeys); + + key_user_put(key->user); +--- a/security/keys/key.c ++++ b/security/keys/key.c +@@ -401,6 +401,18 @@ int key_payload_reserve(struct key *key, + EXPORT_SYMBOL(key_payload_reserve); + + /* ++ * Change the key state to being instantiated. ++ */ ++static void mark_key_instantiated(struct key *key, int reject_error) ++{ ++ /* Commit the payload before setting the state; barrier versus ++ * key_read_state(). ++ */ ++ smp_store_release(&key->state, ++ (reject_error < 0) ? reject_error : KEY_IS_POSITIVE); ++} ++ ++/* + * 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 + * semaphore need not be locked as instantiation is serialised by +@@ -423,14 +435,14 @@ static int __key_instantiate_and_link(st + mutex_lock(&key_construction_mutex); + + /* can't instantiate twice */ +- if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) { ++ if (key->state == KEY_IS_UNINSTANTIATED) { + /* instantiate the key */ + ret = key->type->instantiate(key, prep); + + 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; +@@ -572,13 +584,10 @@ int key_reject_and_link(struct key *key, + mutex_lock(&key_construction_mutex); + + /* can't instantiate twice */ +- if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) { ++ if (key->state == KEY_IS_UNINSTANTIATED) { + /* 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); +@@ -750,8 +759,8 @@ static inline key_ref_t __key_update(key + + ret = key->type->update(key, prep); + if (ret == 0) +- /* updating a negative key instantiates it */ +- clear_bit(KEY_FLAG_NEGATIVE, &key->flags); ++ /* Updating a negative key positively instantiates it */ ++ mark_key_instantiated(key, 0); + + up_write(&key->sem); + +@@ -995,8 +1004,8 @@ int key_update(key_ref_t key_ref, const + + ret = key->type->update(key, &prep); + if (ret == 0) +- /* updating a negative key instantiates it */ +- clear_bit(KEY_FLAG_NEGATIVE, &key->flags); ++ /* Updating a negative key positively instantiates it */ ++ mark_key_instantiated(key, 0); + + up_write(&key->sem); + +--- a/security/keys/keyctl.c ++++ b/security/keys/keyctl.c +@@ -766,10 +766,9 @@ long keyctl_read_key(key_serial_t keyid, + + key = key_ref_to_ptr(key_ref); + +- if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) { +- ret = -ENOKEY; +- goto error2; +- } ++ ret = key_read_state(key); ++ if (ret < 0) ++ goto error2; /* Negatively instantiated */ + + /* see if we can read it directly */ + ret = key_permission(key_ref, KEY_NEED_READ); +@@ -901,7 +900,7 @@ long keyctl_chown_key(key_serial_t id, u + atomic_dec(&key->user->nkeys); + atomic_inc(&newowner->nkeys); + +- if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) { ++ if (key->state != KEY_IS_UNINSTANTIATED) { + atomic_dec(&key->user->nikeys); + atomic_inc(&newowner->nikeys); + } +--- a/security/keys/keyring.c ++++ b/security/keys/keyring.c +@@ -407,7 +407,7 @@ static void keyring_describe(const struc + else + seq_puts(m, "[anon]"); + +- if (key_is_instantiated(keyring)) { ++ if (key_is_positive(keyring)) { + if (keyring->keys.nr_leaves_on_tree != 0) + seq_printf(m, ": %lu", keyring->keys.nr_leaves_on_tree); + else +@@ -546,7 +546,8 @@ static int keyring_search_iterator(const + { + struct keyring_search_context *ctx = iterator_data; + const struct key *key = keyring_ptr_to_key(object); +- unsigned long kflags = key->flags; ++ unsigned long kflags = READ_ONCE(key->flags); ++ short state = READ_ONCE(key->state); + + kenter("{%d}", key->serial); + +@@ -590,9 +591,8 @@ static int keyring_search_iterator(const + + 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); ++ if (state < 0) { ++ ctx->result = ERR_PTR(state); + kleave(" = %d [neg]", ctx->skipped_ret); + goto skipped; + } +--- a/security/keys/proc.c ++++ b/security/keys/proc.c +@@ -182,6 +182,7 @@ static int proc_keys_show(struct seq_fil + unsigned long timo; + key_ref_t key_ref, skey_ref; + char xbuf[16]; ++ short state; + int rc; + + struct keyring_search_context ctx = { +@@ -240,17 +241,19 @@ static int proc_keys_show(struct seq_fil + sprintf(xbuf, "%luw", timo / (60*60*24*7)); + } + ++ state = key_read_state(key); ++ + #define showflag(KEY, LETTER, FLAG) \ + (test_bit(FLAG, &(KEY)->flags) ? LETTER : '-') + + seq_printf(m, "%08x %c%c%c%c%c%c%c %5d %4s %08x %5d %5d %-9.9s ", + key->serial, +- showflag(key, 'I', KEY_FLAG_INSTANTIATED), ++ state != KEY_IS_UNINSTANTIATED ? 'I' : '-', + showflag(key, 'R', KEY_FLAG_REVOKED), + 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), ++ state < 0 ? 'N' : '-', + showflag(key, 'i', KEY_FLAG_INVALIDATED), + atomic_read(&key->usage), + xbuf, +--- a/security/keys/process_keys.c ++++ b/security/keys/process_keys.c +@@ -729,7 +729,7 @@ try_again: + + ret = -EIO; + if (!(lflags & KEY_LOOKUP_PARTIAL) && +- !test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) ++ key_read_state(key) == KEY_IS_UNINSTANTIATED) + goto invalid_key; + + /* check the permissions */ +--- a/security/keys/request_key.c ++++ b/security/keys/request_key.c +@@ -594,10 +594,9 @@ int wait_for_key_construction(struct key + intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE); + if (ret) + return -ERESTARTSYS; +- if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) { +- smp_rmb(); +- return key->reject_error; +- } ++ ret = key_read_state(key); ++ if (ret < 0) ++ return ret; + return key_validate(key); + } + EXPORT_SYMBOL(wait_for_key_construction); +--- a/security/keys/request_key_auth.c ++++ b/security/keys/request_key_auth.c +@@ -73,7 +73,7 @@ static void request_key_auth_describe(co + + seq_puts(m, "key:"); + seq_puts(m, key->description); +- if (key_is_instantiated(key)) ++ if (key_is_positive(key)) + seq_printf(m, " pid:%d ci:%zu", rka->pid, rka->callout_len); + } + +--- a/security/keys/trusted.c ++++ b/security/keys/trusted.c +@@ -1067,7 +1067,7 @@ static int trusted_update(struct key *ke + 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) +--- a/security/keys/user_defined.c ++++ b/security/keys/user_defined.c +@@ -106,7 +106,7 @@ int user_update(struct key *key, struct + + /* attach the new data, displacing the old */ + key->expiry = prep->expiry; +- if (!test_bit(KEY_FLAG_NEGATIVE, &key->flags)) ++ if (key_is_positive(key)) + zap = rcu_dereference_key(key); + rcu_assign_keypointer(key, prep->payload.data[0]); + prep->payload.data[0] = NULL; +@@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(user_destroy); + void user_describe(const struct key *key, struct seq_file *m) + { + seq_puts(m, key->description); +- if (key_is_instantiated(key)) ++ if (key_is_positive(key)) + seq_printf(m, ": %u", key->datalen); + } + diff --git a/queue-4.9/series b/queue-4.9/series index 367f2797e1d..073f8af8329 100644 --- a/queue-4.9/series +++ b/queue-4.9/series @@ -46,3 +46,6 @@ xfs-reinit-btree-pointer-on-attr-tree-inactivation-walk.patch xfs-handle-error-if-xfs_btree_get_bufs-fails.patch xfs-cancel-dirty-pages-on-invalidation.patch xfs-trim-writepage-mapping-to-within-eof.patch +fscrypt-fix-dereference-of-null-user_key_payload.patch +keys-fix-race-between-updating-and-finding-a-negative-key.patch +fs-cache-fix-dereference-of-null-user_key_payload.patch