| Message ID | 20170303002559.8280-2-mathew.j.martineau@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Mat Martineau <mathew.j.martineau@linux.intel.com> wrote: > By adding a rwsem to struct key_type, key_types_sem only needs to be > held when key_types_list is being accessed. key_type_put() is modified > to only release the given key_type (the key_type parameter to this > function was previously ignored). Perhaps a better way of doing this is to hold the key_type's module instead whilst we have it looked up. That way we can drop the lock as before returning from key_type_lookup(). key_type_put() should just then need to release the module ref. We *could* even hold the key_type ref as long as a key is in existence, and thereby hold the module that implements the key_type - but that means a module may get pinned by a key that you can't find or remove. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 3 Mar 2017, David Howells wrote: > Mat Martineau <mathew.j.martineau@linux.intel.com> wrote: > >> By adding a rwsem to struct key_type, key_types_sem only needs to be >> held when key_types_list is being accessed. key_type_put() is modified >> to only release the given key_type (the key_type parameter to this >> function was previously ignored). > > Perhaps a better way of doing this is to hold the key_type's module instead > whilst we have it looked up. That way we can drop the lock as before > returning from key_type_lookup(). key_type_put() should just then need to > release the module ref. It does appear that current in-tree key types are only unregistered by module exit functions, but I don't see that's it is a stated or enforced requirement. If the module reference count is used instead of a semaphore and non-module key types were allowed to be unregistered, we'd end up with a race condition in the garbage collector which could result in a keyring having a restriction based on a key type that is not registered. It wouldn't result in dereferencing invalid memory, but would be confusing behavior. Now that I've gone back and taken a closer look, this discussion may be moot. Keyring restrictions are now added to existing keyrings instead of processed during keyring creation so this patch is probably no longer needed. If lockdep doesn't complain, I plan to drop this patch. While I like the idea of reducing contention for key_types_sem by holding it for less time, adding another lock to the mix has consequences too. -- Mat Martineau Intel OTC -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mat Martineau <mathew.j.martineau@linux.intel.com> wrote: > It does appear that current in-tree key types are only unregistered by module > exit functions, but I don't see that's it is a stated or enforced > requirement. If the module reference count is used instead of a semaphore and > non-module key types were allowed to be unregistered, we'd end up with a race > condition in the garbage collector which could result in a keyring having a > restriction based on a key type that is not registered. It wouldn't result in > dereferencing invalid memory, but would be confusing behavior. Good point. You'd have to have some sort of counter of keys that you can wait on in unregister_key_type() to find out when all the keys of your type are dead. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/key-type.h b/include/linux/key-type.h index eaee981c5558..1f5b863c2bce 100644 --- a/include/linux/key-type.h +++ b/include/linux/key-type.h @@ -150,6 +150,7 @@ struct key_type { /* internal fields */ struct list_head link; /* link in types list */ struct lock_class_key lock_class; /* key->sem lock class */ + struct rw_semaphore sem; /* for safe key type removal */ }; extern struct key_type key_type_keyring; diff --git a/security/keys/gc.c b/security/keys/gc.c index addf060399e0..467af2cd1393 100644 --- a/security/keys/gc.c +++ b/security/keys/gc.c @@ -99,7 +99,7 @@ static void key_gc_timer_func(unsigned long data) * collect dead links and the third to clean up the dead keys. We have to be * careful as there may already be a cycle in progress. * - * The caller must be holding key_types_sem. + * The caller must be holding key_types_sem and ktype->sem. */ void key_gc_keytype(struct key_type *ktype) { diff --git a/security/keys/key.c b/security/keys/key.c index 346fbf201c22..d0b0c3f9ae23 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -672,8 +672,8 @@ struct key *key_lookup(key_serial_t id) /* * Find and lock the specified key type against removal. * - * We return with the sem read-locked if successful. If the type wasn't - * available -ENOKEY is returned instead. + * We return with the key type's sem read-locked if successful. If the + * type wasn't available -ENOKEY is returned instead. */ struct key_type *key_type_lookup(const char *type) { @@ -684,14 +684,18 @@ struct key_type *key_type_lookup(const char *type) /* look up the key type to see if it's one of the registered kernel * types */ list_for_each_entry(ktype, &key_types_list, link) { - if (strcmp(ktype->name, type) == 0) + /* ktype->sem is only write locked when unregistering, so + * ignore a key type with a contended lock. + */ + if ((strcmp(ktype->name, type) == 0) && + down_read_trylock(&ktype->sem)) goto found_kernel_type; } - up_read(&key_types_sem); ktype = ERR_PTR(-ENOKEY); found_kernel_type: + up_read(&key_types_sem); return ktype; } @@ -720,7 +724,8 @@ EXPORT_SYMBOL_GPL(key_set_timeout); */ void key_type_put(struct key_type *ktype) { - up_read(&key_types_sem); + if (ktype) + up_read(&ktype->sem); } /* @@ -1102,6 +1107,7 @@ int register_key_type(struct key_type *ktype) int ret; memset(&ktype->lock_class, 0, sizeof(ktype->lock_class)); + init_rwsem(&ktype->sem); ret = -EEXIST; down_write(&key_types_sem); @@ -1135,14 +1141,22 @@ EXPORT_SYMBOL(register_key_type); void unregister_key_type(struct key_type *ktype) { down_write(&key_types_sem); + down_write(&ktype->sem); list_del_init(&ktype->link); downgrade_write(&key_types_sem); key_gc_keytype(ktype); pr_notice("Key type %s unregistered\n", ktype->name); + up_write(&ktype->sem); up_read(&key_types_sem); } EXPORT_SYMBOL(unregister_key_type); +static void __init add_special_key_type(struct key_type *ktype) +{ + init_rwsem(&ktype->sem); + list_add_tail(&ktype->link, &key_types_list); +} + /* * Initialise the key management state. */ @@ -1153,10 +1167,10 @@ void __init key_init(void) 0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL); /* add the special key types */ - list_add_tail(&key_type_keyring.link, &key_types_list); - list_add_tail(&key_type_dead.link, &key_types_list); - list_add_tail(&key_type_user.link, &key_types_list); - list_add_tail(&key_type_logon.link, &key_types_list); + add_special_key_type(&key_type_keyring); + add_special_key_type(&key_type_dead); + add_special_key_type(&key_type_user); + add_special_key_type(&key_type_logon); /* record the root user tracking */ rb_link_node(&root_key_user.node,
The key_types_sem read-write semaphore has been performing two jobs, protecting the key_types_list and making sure individual key types are not unregistered while they are in use. It is held for most of key_create_or_update, which means a second key type cannot be looked up by name while a key is being created or updated without re-acquiring the semaphore and triggering a lockdep warning: [ 445.752215] ============================================= [ 445.752946] [ INFO: possible recursive locking detected ] [ 445.753745] 4.8.0-rc1+ #34 Not tainted [ 445.754087] --------------------------------------------- [ 445.754540] test-key/948 is trying to acquire lock: [ 445.754950] (key_types_sem){++++.+}, at: [<ffffffff903c4dcb>] key_type_lookup+0x1b/0x80 [ 445.755854] [ 445.755854] but task is already holding lock: [ 445.756515] (key_types_sem){++++.+}, at: [<ffffffff903c4dcb>] key_type_lookup+0x1b/0x80 By adding a rwsem to struct key_type, key_types_sem only needs to be held when key_types_list is being accessed. key_type_put() is modified to only release the given key_type (the key_type parameter to this function was previously ignored). Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> --- include/linux/key-type.h | 1 + security/keys/gc.c | 2 +- security/keys/key.c | 32 +++++++++++++++++++++++--------- 3 files changed, 25 insertions(+), 10 deletions(-)