Message-ID: <156095857859.25264.1843382792417643880.stgit@warthog.procyon.org.uk>
Date:   Wed, 19 Jun 2019 16:36:18 +0100
From:   David Howells <dhowells@...hat.com>
To:     keyrings@...r.kernel.org
Cc:     dhowells@...hat.com, linux-afs@...ts.infradead.org,
        linux-fsdevel@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH 3/6] keys: Move the RCU locks outwards from the keyring
 search functions [ver #2]

Move the RCU locks outwards from the keyring search functions so that it
will become possible to provide an RCU-capable partial request_key()
function in a later commit.

Signed-off-by: David Howells <dhowells@...hat.com>
---

 Documentation/security/keys/request-key.rst |    2 -
 include/keys/request_key_auth-type.h        |    1 
 security/keys/internal.h                    |    6 +--
 security/keys/keyring.c                     |   16 ++++---
 security/keys/proc.c                        |    4 +-
 security/keys/process_keys.c                |   41 ++++++++----------
 security/keys/request_key.c                 |    8 +++-
 security/keys/request_key_auth.c            |   60 ++++++++++++++++-----------
 8 files changed, 77 insertions(+), 61 deletions(-)

diff --git a/Documentation/security/keys/request-key.rst b/Documentation/security/keys/request-key.rst
index 600ad67d1707..07af991463b5 100644
--- a/Documentation/security/keys/request-key.rst
+++ b/Documentation/security/keys/request-key.rst
@@ -148,7 +148,7 @@ The Search Algorithm
 
 A search of any particular keyring proceeds in the following fashion:
 
-  1) When the key management code searches for a key (keyring_search_aux) it
+  1) When the key management code searches for a key (keyring_search_rcu) it
      firstly calls key_permission(SEARCH) on the keyring it's starting with,
      if this denies permission, it doesn't search further.
 
diff --git a/include/keys/request_key_auth-type.h b/include/keys/request_key_auth-type.h
index a726dd3f1dc6..2a046062bb42 100644
--- a/include/keys/request_key_auth-type.h
+++ b/include/keys/request_key_auth-type.h
@@ -18,6 +18,7 @@
  * Authorisation record for request_key().
  */
 struct request_key_auth {
+	struct rcu_head		rcu;
 	struct key		*target_key;
 	struct key		*dest_keyring;
 	const struct cred	*cred;
diff --git a/security/keys/internal.h b/security/keys/internal.h
index d04bff631227..3d5c08db74d2 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -139,11 +139,11 @@ struct keyring_search_context {
 
 extern bool key_default_cmp(const struct key *key,
 			    const struct key_match_data *match_data);
-extern key_ref_t keyring_search_aux(key_ref_t keyring_ref,
+extern key_ref_t keyring_search_rcu(key_ref_t keyring_ref,
 				    struct keyring_search_context *ctx);
 
-extern key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx);
-extern key_ref_t search_process_keyrings(struct keyring_search_context *ctx);
+extern key_ref_t search_cred_keyrings_rcu(struct keyring_search_context *ctx);
+extern key_ref_t search_process_keyrings_rcu(struct keyring_search_context *ctx);
 
 extern struct key *find_keyring_by_name(const char *name, bool uid_keyring);
 
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 67066bb58b83..afa6d4024c67 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -835,7 +835,7 @@ static bool search_nested_keyrings(struct key *keyring,
 }
 
 /**
- * keyring_search_aux - Search a keyring tree for a key matching some criteria
+ * keyring_search_rcu - Search a keyring tree for a matching key under RCU
  * @keyring_ref: A pointer to the keyring with possession indicator.
  * @ctx: The keyring search context.
  *
@@ -847,7 +847,9 @@ static bool search_nested_keyrings(struct key *keyring,
  * addition, the LSM gets to forbid keyring searches and key matches.
  *
  * The search is performed as a breadth-then-depth search up to the prescribed
- * limit (KEYRING_SEARCH_MAX_DEPTH).
+ * limit (KEYRING_SEARCH_MAX_DEPTH).  The caller must hold the RCU read lock to
+ * prevent keyrings from being destroyed or rearranged whilst they are being
+ * searched.
  *
  * Keys are matched to the type provided and are then filtered by the match
  * function, which is given the description to use in any way it sees fit.  The
@@ -866,7 +868,7 @@ static bool search_nested_keyrings(struct key *keyring,
  * In the case of a successful return, the possession attribute from
  * @keyring_ref is propagated to the returned key reference.
  */
-key_ref_t keyring_search_aux(key_ref_t keyring_ref,
+key_ref_t keyring_search_rcu(key_ref_t keyring_ref,
 			     struct keyring_search_context *ctx)
 {
 	struct key *keyring;
@@ -888,11 +890,9 @@ key_ref_t keyring_search_aux(key_ref_t keyring_ref,
 			return ERR_PTR(err);
 	}
 
-	rcu_read_lock();
 	ctx->now = ktime_get_real_seconds();
 	if (search_nested_keyrings(keyring, ctx))
 		__key_get(key_ref_to_ptr(ctx->result));
-	rcu_read_unlock();
 	return ctx->result;
 }
 
@@ -902,7 +902,7 @@ key_ref_t keyring_search_aux(key_ref_t keyring_ref,
  * @type: The type of keyring we want to find.
  * @description: The name of the keyring we want to find.
  *
- * As keyring_search_aux() above, but using the current task's credentials and
+ * As keyring_search_rcu() above, but using the current task's credentials and
  * type's default matching function and preferred search method.
  */
 key_ref_t keyring_search(key_ref_t keyring,
@@ -928,7 +928,9 @@ key_ref_t keyring_search(key_ref_t keyring,
 			return ERR_PTR(ret);
 	}
 
-	key = keyring_search_aux(keyring, &ctx);
+	rcu_read_lock();
+	key = keyring_search_rcu(keyring, &ctx);
+	rcu_read_unlock();
 
 	if (type->match_free)
 		type->match_free(&ctx.match_data);
diff --git a/security/keys/proc.c b/security/keys/proc.c
index 78ac305d715e..f081dceae3b9 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -179,7 +179,9 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	 * skip if the key does not indicate the possessor can view it
 	 */
 	if (key->perm & KEY_POS_VIEW) {
-		skey_ref = search_my_process_keyrings(&ctx);
+		rcu_read_lock();
+		skey_ref = search_cred_keyrings_rcu(&ctx);
+		rcu_read_unlock();
 		if (!IS_ERR(skey_ref)) {
 			key_ref_put(skey_ref);
 			key_ref = make_key_ref(key, 1);
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 39aaa21462bf..f8ffb06d0297 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -318,7 +318,8 @@ void key_fsgid_changed(struct cred *new_cred)
 
 /*
  * Search the process keyrings attached to the supplied cred for the first
- * matching key.
+ * matching key under RCU conditions (the caller must be holding the RCU read
+ * lock).
  *
  * The search criteria are the type and the match function.  The description is
  * given to the match function as a parameter, but doesn't otherwise influence
@@ -337,7 +338,7 @@ void key_fsgid_changed(struct cred *new_cred)
  * In the case of a successful return, the possession attribute is set on the
  * returned key reference.
  */
-key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
+key_ref_t search_cred_keyrings_rcu(struct keyring_search_context *ctx)
 {
 	key_ref_t key_ref, ret, err;
 	const struct cred *cred = ctx->cred;
@@ -355,7 +356,7 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
 
 	/* search the thread keyring first */
 	if (cred->thread_keyring) {
-		key_ref = keyring_search_aux(
+		key_ref = keyring_search_rcu(
 			make_key_ref(cred->thread_keyring, 1), ctx);
 		if (!IS_ERR(key_ref))
 			goto found;
@@ -373,7 +374,7 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
 
 	/* search the process keyring second */
 	if (cred->process_keyring) {
-		key_ref = keyring_search_aux(
+		key_ref = keyring_search_rcu(
 			make_key_ref(cred->process_keyring, 1), ctx);
 		if (!IS_ERR(key_ref))
 			goto found;
@@ -394,7 +395,7 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
 
 	/* search the session keyring */
 	if (cred->session_keyring) {
-		key_ref = keyring_search_aux(
+		key_ref = keyring_search_rcu(
 			make_key_ref(cred->session_keyring, 1), ctx);
 
 		if (!IS_ERR(key_ref))
@@ -415,7 +416,7 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
 	}
 	/* or search the user-session keyring */
 	else if (READ_ONCE(cred->user->session_keyring)) {
-		key_ref = keyring_search_aux(
+		key_ref = keyring_search_rcu(
 			make_key_ref(READ_ONCE(cred->user->session_keyring), 1),
 			ctx);
 		if (!IS_ERR(key_ref))
@@ -448,16 +449,16 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
  * the keys attached to the assumed authorisation key using its credentials if
  * one is available.
  *
- * Return same as search_my_process_keyrings().
+ * The caller must be holding the RCU read lock.
+ *
+ * Return same as search_cred_keyrings_rcu().
  */
-key_ref_t search_process_keyrings(struct keyring_search_context *ctx)
+key_ref_t search_process_keyrings_rcu(struct keyring_search_context *ctx)
 {
 	struct request_key_auth *rka;
 	key_ref_t key_ref, ret = ERR_PTR(-EACCES), err;
 
-	might_sleep();
-
-	key_ref = search_my_process_keyrings(ctx);
+	key_ref = search_cred_keyrings_rcu(ctx);
 	if (!IS_ERR(key_ref))
 		goto found;
 	err = key_ref;
@@ -472,24 +473,17 @@ key_ref_t search_process_keyrings(struct keyring_search_context *ctx)
 	    ) {
 		const struct cred *cred = ctx->cred;
 
-		/* defend against the auth key being revoked */
-		down_read(&cred->request_key_auth->sem);
-
-		if (key_validate(ctx->cred->request_key_auth) == 0) {
+		if (key_validate(cred->request_key_auth) == 0) {
 			rka = ctx->cred->request_key_auth->payload.data[0];
 
+			//// was search_process_keyrings() [ie. recursive]
 			ctx->cred = rka->cred;
-			key_ref = search_process_keyrings(ctx);
+			key_ref = search_cred_keyrings_rcu(ctx);
 			ctx->cred = cred;
 
-			up_read(&cred->request_key_auth->sem);
-
 			if (!IS_ERR(key_ref))
 				goto found;
-
 			ret = key_ref;
-		} else {
-			up_read(&cred->request_key_auth->sem);
 		}
 	}
 
@@ -504,7 +498,6 @@ key_ref_t search_process_keyrings(struct keyring_search_context *ctx)
 found:
 	return key_ref;
 }
-
 /*
  * See if the key we're looking at is the target key.
  */
@@ -691,7 +684,9 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 		ctx.index_key			= key->index_key;
 		ctx.match_data.raw_data		= key;
 		kdebug("check possessed");
-		skey_ref = search_process_keyrings(&ctx);
+		rcu_read_lock();
+		skey_ref = search_process_keyrings_rcu(&ctx);
+		rcu_read_unlock();
 		kdebug("possessed=%p", skey_ref);
 
 		if (!IS_ERR(skey_ref)) {
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 244e538d113f..bf1d223ec21c 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -385,7 +385,9 @@ static int construct_alloc_key(struct keyring_search_context *ctx,
 	 * waited for locks */
 	mutex_lock(&key_construction_mutex);
 
-	key_ref = search_process_keyrings(ctx);
+	rcu_read_lock();
+	key_ref = search_process_keyrings_rcu(ctx);
+	rcu_read_unlock();
 	if (!IS_ERR(key_ref))
 		goto key_already_present;
 
@@ -561,7 +563,9 @@ struct key *request_key_and_link(struct key_type *type,
 	}
 
 	/* search all the process keyrings for a key */
-	key_ref = search_process_keyrings(&ctx);
+	rcu_read_lock();
+	key_ref = search_process_keyrings_rcu(&ctx);
+	rcu_read_unlock();
 
 	if (!IS_ERR(key_ref)) {
 		if (dest_keyring) {
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ec5226557023..99ed7a8a273d 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -58,7 +58,7 @@ static void request_key_auth_free_preparse(struct key_preparsed_payload *prep)
 static int request_key_auth_instantiate(struct key *key,
 					struct key_preparsed_payload *prep)
 {
-	key->payload.data[0] = (struct request_key_auth *)prep->data;
+	rcu_assign_keypointer(key, (struct request_key_auth *)prep->data);
 	return 0;
 }
 
@@ -68,7 +68,7 @@ static int request_key_auth_instantiate(struct key *key,
 static void request_key_auth_describe(const struct key *key,
 				      struct seq_file *m)
 {
-	struct request_key_auth *rka = get_request_key_auth(key);
+	struct request_key_auth *rka = dereference_key_rcu(key);
 
 	seq_puts(m, "key:");
 	seq_puts(m, key->description);
@@ -83,7 +83,7 @@ static void request_key_auth_describe(const struct key *key,
 static long request_key_auth_read(const struct key *key,
 				  char __user *buffer, size_t buflen)
 {
-	struct request_key_auth *rka = get_request_key_auth(key);
+	struct request_key_auth *rka = dereference_key_locked(key);
 	size_t datalen;
 	long ret;
 
@@ -102,23 +102,6 @@ static long request_key_auth_read(const struct key *key,
 	return ret;
 }
 
-/*
- * Handle revocation of an authorisation token key.
- *
- * Called with the key sem write-locked.
- */
-static void request_key_auth_revoke(struct key *key)
-{
-	struct request_key_auth *rka = get_request_key_auth(key);
-
-	kenter("{%d}", key->serial);
-
-	if (rka->cred) {
-		put_cred(rka->cred);
-		rka->cred = NULL;
-	}
-}
-
 static void free_request_key_auth(struct request_key_auth *rka)
 {
 	if (!rka)
@@ -131,16 +114,43 @@ static void free_request_key_auth(struct request_key_auth *rka)
 	kfree(rka);
 }
 
+/*
+ * Dispose of the request_key_auth record under RCU conditions
+ */
+static void request_key_auth_rcu_disposal(struct rcu_head *rcu)
+{
+	struct request_key_auth *rka =
+		container_of(rcu, struct request_key_auth, rcu);
+
+	free_request_key_auth(rka);
+}
+
+/*
+ * Handle revocation of an authorisation token key.
+ *
+ * Called with the key sem write-locked.
+ */
+static void request_key_auth_revoke(struct key *key)
+{
+	struct request_key_auth *rka = dereference_key_locked(key);
+
+	kenter("{%d}", key->serial);
+	rcu_assign_keypointer(key, NULL);
+	call_rcu(&rka->rcu, request_key_auth_rcu_disposal);
+}
+
 /*
  * Destroy an instantiation authorisation token key.
  */
 static void request_key_auth_destroy(struct key *key)
 {
-	struct request_key_auth *rka = get_request_key_auth(key);
+	struct request_key_auth *rka = rcu_access_pointer(key->payload.rcu_data0);
 
 	kenter("{%d}", key->serial);
-
-	free_request_key_auth(rka);
+	if (rka) {
+		rcu_assign_keypointer(key, NULL);
+		call_rcu(&rka->rcu, request_key_auth_rcu_disposal);
+	}
 }
 
 /*
@@ -249,7 +259,9 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
 
 	ctx.index_key.desc_len = sprintf(description, "%x", target_id);
 
-	authkey_ref = search_process_keyrings(&ctx);
+	rcu_read_lock();
+	authkey_ref = search_process_keyrings_rcu(&ctx);
+	rcu_read_unlock();
 
 	if (IS_ERR(authkey_ref)) {
 		authkey = ERR_CAST(authkey_ref);

Powered by blists - more mailing lists