linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/11] keys: Namespacing
@ 2019-04-24 16:13 David Howells
  2019-04-24 16:13 ` [PATCH 01/11] keys: Invalidate used request_key authentication keys David Howells
                   ` (11 more replies)
  0 siblings, 12 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:13 UTC (permalink / raw)
  To: ebiederm
  Cc: linux-cifs, linux-nfs, netdev, linux-afs, Jann Horn, keyrings,
	dhowells, linux-security-module, linux-fsdevel, linux-kernel,
	dwalsh, vgoyal


Here are some patches to make keys and keyrings more namespace aware.  Note
that the branch is dependent on security/next-general.

Firstly some miscellaneous patches to make the process easier:

 (1) Invalidate rather than revoke request_key() authentication keys to
     recycle them more quickly.

 (2) Remove request_key_async*() as they aren't used and would have to be
     namespaced.

 (3) Simplify key index_key handling so that the word-sized chunks
     assoc_array requires don't have to be shifted about, making it easier
     to add more bits into the key.

 (4) Cache the hash value so that we don't have to calculate on every key
     we examine during a search (it involves a bunch of multiplications).

 (5) Allow keying_search() to search non-recursively.

Then the main patches:

 (6) Make it so that keyring names are per-user_namespace from the point of
     view of KEYCTL_JOIN_SESSION_KEYRING so that they're not accessible
     cross-user_namespace.

 (7) Move the user and user-session keyrings to the user_namespace rather
     than the user_struct.  This prevents them propagating directly across
     user_namespaces boundaries (ie. the KEY_SPEC_* flags will only pick
     from the current user_namespace).

 (8) Make it possible to include the target namespace in which the key shall
     operate in the index_key.  This will allow the possibility of multiple
     keys with the same description, but different target domains to be held
     in the same keyring.

 (9) Make it so that keys are implicitly invalidated by removal of a domain
     tag, causing them to be garbage collected.

(10) Institute a network namespace domain tag that allows keys to be
     differentiated by the network namespace in which they operate.  New keys
     that are of a type marked 'KEY_TYPE_NET_DOMAIN' are assigned the network
     domain in force when they are created.

(11) Make it so that the desired network namespace can be handed down into the
     request_key() mechanism.  This allows AFS, NFS, etc. to request keys
     specific to the network namespace of the superblock.

     This also means that the keys in the DNS record cache are thenceforth
     namespaced, provided network filesystems pass the appropriate network
     namespace down into dns_query().

     For DNS, AFS and NFS are good; CIFS and Ceph are not.  Other cache
     keyrings, such as idmapper keyrings, also need to set the domain tag.


The patches can be found on the following branch:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-namespace

David
---
David Howells (11):
      keys: Invalidate used request_key authentication keys
      keys: Kill off request_key_async{,_with_auxdata}
      keys: Simplify key description management
      keys: Cache the hash value to avoid lots of recalculation
      keys: Add a 'recurse' flag for keyring searches
      keys: Namespace keyring names
      keys: Move the user and user-session keyrings to the user_namespace
      keys: Include target namespace in match criteria
      keys: Garbage collect keys for which the domain has been removed
      keys: Network namespace domain tag
      keys: Pass the network namespace into request_key mechanism


 Documentation/security/keys/core.rst     |   10 +
 certs/blacklist.c                        |    2 
 crypto/asymmetric_keys/asymmetric_type.c |    2 
 fs/afs/addr_list.c                       |    4 
 fs/afs/dynroot.c                         |    7 -
 fs/cifs/dns_resolve.c                    |    3 
 fs/nfs/dns_resolve.c                     |    2 
 include/linux/dns_resolver.h             |    3 
 include/linux/key-type.h                 |    3 
 include/linux/key.h                      |   49 ++++--
 include/linux/sched/user.h               |   14 --
 include/linux/user_namespace.h           |    8 +
 include/net/net_namespace.h              |    4 
 kernel/user.c                            |   10 -
 kernel/user_namespace.c                  |    9 -
 lib/digsig.c                             |    2 
 net/ceph/messenger.c                     |    3 
 net/core/net_namespace.c                 |   18 ++
 net/dns_resolver/dns_key.c               |    1 
 net/dns_resolver/dns_query.c             |    6 -
 net/rxrpc/key.c                          |    6 -
 net/rxrpc/security.c                     |    2 
 security/integrity/digsig_asymmetric.c   |    4 
 security/keys/gc.c                       |    2 
 security/keys/internal.h                 |   10 +
 security/keys/key.c                      |    9 +
 security/keys/keyctl.c                   |    4 
 security/keys/keyring.c                  |  263 +++++++++++++++++-------------
 security/keys/persistent.c               |   10 +
 security/keys/proc.c                     |    3 
 security/keys/process_keys.c             |  252 ++++++++++++++++++-----------
 security/keys/request_key.c              |  114 +++++++------
 security/keys/request_key_auth.c         |    3 
 33 files changed, 502 insertions(+), 340 deletions(-)


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 01/11] keys: Invalidate used request_key authentication keys
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
@ 2019-04-24 16:13 ` David Howells
  2019-04-24 16:13 ` [PATCH 02/11] keys: Kill off request_key_async{,_with_auxdata} David Howells
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:13 UTC (permalink / raw)
  To: ebiederm
  Cc: keyrings, dhowells, linux-security-module, linux-fsdevel,
	linux-kernel, dwalsh, vgoyal

Invalidate used request_key authentication keys rather than revoking them
so that they get cleaned up immediately rather than potentially hanging
around.  There doesn't seem any need to keep the revoked keys around.

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

 security/keys/key.c         |    4 ++--
 security/keys/request_key.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/keys/key.c b/security/keys/key.c
index 696f1c092c50..d705b950ce2a 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -459,7 +459,7 @@ static int __key_instantiate_and_link(struct key *key,
 
 			/* disable the authorisation key */
 			if (authkey)
-				key_revoke(authkey);
+				key_invalidate(authkey);
 
 			if (prep->expiry != TIME64_MAX) {
 				key->expiry = prep->expiry;
@@ -607,7 +607,7 @@ int key_reject_and_link(struct key *key,
 
 		/* disable the authorisation key */
 		if (authkey)
-			key_revoke(authkey);
+			key_invalidate(authkey);
 	}
 
 	mutex_unlock(&key_construction_mutex);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 75d87f9e0f49..a7b698394257 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -222,7 +222,7 @@ static int construct_key(struct key *key, const void *callout_info,
 	/* check that the actor called complete_request_key() prior to
 	 * returning an error */
 	WARN_ON(ret < 0 &&
-		!test_bit(KEY_FLAG_REVOKED, &authkey->flags));
+		!test_bit(KEY_FLAG_INVALIDATED, &authkey->flags));
 
 	key_put(authkey);
 	kleave(" = %d", ret);


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 02/11] keys: Kill off request_key_async{,_with_auxdata}
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
  2019-04-24 16:13 ` [PATCH 01/11] keys: Invalidate used request_key authentication keys David Howells
@ 2019-04-24 16:13 ` David Howells
  2019-04-24 16:13 ` [PATCH 03/11] keys: Simplify key description management David Howells
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:13 UTC (permalink / raw)
  To: ebiederm
  Cc: keyrings, dhowells, linux-security-module, linux-fsdevel,
	linux-kernel, dwalsh, vgoyal

Kill off request_key_async{,_with_auxdata}() as they're not currently used.

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

 include/linux/key.h         |   11 ---------
 security/keys/request_key.c |   50 -------------------------------------------
 2 files changed, 61 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 7099985e35a9..aaa93fe3f587 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -280,17 +280,6 @@ extern struct key *request_key_with_auxdata(struct key_type *type,
 					    size_t callout_len,
 					    void *aux);
 
-extern struct key *request_key_async(struct key_type *type,
-				     const char *description,
-				     const void *callout_info,
-				     size_t callout_len);
-
-extern struct key *request_key_async_with_auxdata(struct key_type *type,
-						  const char *description,
-						  const void *callout_info,
-						  size_t callout_len,
-						  void *aux);
-
 extern int wait_for_key_construction(struct key *key, bool intr);
 
 extern int key_validate(const struct key *key);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index a7b698394257..a5b74b7758f7 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -687,53 +687,3 @@ struct key *request_key_with_auxdata(struct key_type *type,
 	return key;
 }
 EXPORT_SYMBOL(request_key_with_auxdata);
-
-/*
- * request_key_async - Request a key (allow async construction)
- * @type: Type of key.
- * @description: The searchable description of the key.
- * @callout_info: The data to pass to the instantiation upcall (or NULL).
- * @callout_len: The length of callout_info.
- *
- * As for request_key_and_link() except that it does not add the returned key
- * to a keyring if found, new keys are always allocated in the user's quota and
- * no auxiliary data can be passed.
- *
- * The caller should call wait_for_key_construction() to wait for the
- * completion of the returned key if it is still undergoing construction.
- */
-struct key *request_key_async(struct key_type *type,
-			      const char *description,
-			      const void *callout_info,
-			      size_t callout_len)
-{
-	return request_key_and_link(type, description, callout_info,
-				    callout_len, NULL, NULL,
-				    KEY_ALLOC_IN_QUOTA);
-}
-EXPORT_SYMBOL(request_key_async);
-
-/*
- * request a key with auxiliary data for the upcaller (allow async construction)
- * @type: Type of key.
- * @description: The searchable description of the key.
- * @callout_info: The data to pass to the instantiation upcall (or NULL).
- * @callout_len: The length of callout_info.
- * @aux: Auxiliary data for the upcall.
- *
- * As for request_key_and_link() except that it does not add the returned key
- * to a keyring if found and new keys are always allocated in the user's quota.
- *
- * The caller should call wait_for_key_construction() to wait for the
- * completion of the returned key if it is still undergoing construction.
- */
-struct key *request_key_async_with_auxdata(struct key_type *type,
-					   const char *description,
-					   const void *callout_info,
-					   size_t callout_len,
-					   void *aux)
-{
-	return request_key_and_link(type, description, callout_info,
-				    callout_len, aux, NULL, KEY_ALLOC_IN_QUOTA);
-}
-EXPORT_SYMBOL(request_key_async_with_auxdata);


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 03/11] keys: Simplify key description management
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
  2019-04-24 16:13 ` [PATCH 01/11] keys: Invalidate used request_key authentication keys David Howells
  2019-04-24 16:13 ` [PATCH 02/11] keys: Kill off request_key_async{,_with_auxdata} David Howells
@ 2019-04-24 16:13 ` David Howells
  2019-04-24 16:14 ` [PATCH 04/11] keys: Cache the hash value to avoid lots of recalculation David Howells
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:13 UTC (permalink / raw)
  To: ebiederm
  Cc: keyrings, dhowells, linux-security-module, linux-fsdevel,
	linux-kernel, dwalsh, vgoyal

Simplify key description management by cramming the word containing the
length with the first few chars of the description also.  This simplifies
the code that generates the index-key used by assoc_array.  It should speed
up key searching a bit too.

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

 include/linux/key.h        |   14 ++++++++-
 security/keys/internal.h   |    6 ++++
 security/keys/key.c        |    2 +
 security/keys/keyring.c    |   70 +++++++++++++-------------------------------
 security/keys/persistent.c |    1 +
 5 files changed, 43 insertions(+), 50 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index aaa93fe3f587..33d87cb2d469 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -86,9 +86,20 @@ struct keyring_list;
 struct keyring_name;
 
 struct keyring_index_key {
+	union {
+		struct {
+#ifdef __LITTLE_ENDIAN /* Put desc_len at the LSB of x */
+			u8	desc_len;
+			char	desc[sizeof(long) - 1];	/* First few chars of description */
+#else
+			char	desc[sizeof(long) - 1];	/* First few chars of description */
+			u8	desc_len;
+#endif
+		};
+		unsigned long x;
+	};
 	struct key_type		*type;
 	const char		*description;
-	size_t			desc_len;
 };
 
 union key_payload {
@@ -202,6 +213,7 @@ struct key {
 	union {
 		struct keyring_index_key index_key;
 		struct {
+			unsigned long	len_desc;
 			struct key_type	*type;		/* type of key */
 			char		*description;
 		};
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8f533c81aa8d..02592d24f13a 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -90,6 +90,12 @@ extern struct mutex key_construction_mutex;
 extern wait_queue_head_t request_key_conswq;
 
 
+static inline void key_set_index_key(struct keyring_index_key *index_key)
+{
+	size_t n = min_t(size_t, index_key->desc_len, sizeof(index_key->desc));
+	memcpy(index_key->desc, index_key->description, n);
+}
+
 extern struct key_type *key_type_lookup(const char *type);
 extern void key_type_put(struct key_type *ktype);
 
diff --git a/security/keys/key.c b/security/keys/key.c
index d705b950ce2a..1d0250a8990e 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -285,6 +285,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	key->index_key.description = kmemdup(desc, desclen + 1, GFP_KERNEL);
 	if (!key->index_key.description)
 		goto no_memory_3;
+	key_set_index_key(&key->index_key);
 
 	refcount_set(&key->usage, 1);
 	init_rwsem(&key->sem);
@@ -859,6 +860,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 			goto error_free_prep;
 	}
 	index_key.desc_len = strlen(index_key.description);
+	key_set_index_key(&index_key);
 
 	ret = __key_link_begin(keyring, &index_key, &edit);
 	if (ret < 0) {
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index e14f09e3a4b0..7bb1f499996c 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -179,9 +179,9 @@ static unsigned long hash_key_type_and_desc(const struct keyring_index_key *inde
 	int n, desc_len = index_key->desc_len;
 
 	type = (unsigned long)index_key->type;
-
 	acc = mult_64x32_and_fold(type, desc_len + 13);
 	acc = mult_64x32_and_fold(acc, 9207);
+
 	for (;;) {
 		n = desc_len;
 		if (n <= 0)
@@ -215,23 +215,13 @@ static unsigned long hash_key_type_and_desc(const struct keyring_index_key *inde
 /*
  * Build the next index key chunk.
  *
- * On 32-bit systems the index key is laid out as:
- *
- *	0	4	5	9...
- *	hash	desclen	typeptr	desc[]
- *
- * On 64-bit systems:
- *
- *	0	8	9	17...
- *	hash	desclen	typeptr	desc[]
- *
  * We return it one word-sized chunk at a time.
  */
 static unsigned long keyring_get_key_chunk(const void *data, int level)
 {
 	const struct keyring_index_key *index_key = data;
 	unsigned long chunk = 0;
-	long offset = 0;
+	const u8 *d;
 	int desc_len = index_key->desc_len, n = sizeof(chunk);
 
 	level /= ASSOC_ARRAY_KEY_CHUNK_SIZE;
@@ -239,33 +229,23 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
 	case 0:
 		return hash_key_type_and_desc(index_key);
 	case 1:
-		return ((unsigned long)index_key->type << 8) | desc_len;
+		return index_key->x;
 	case 2:
-		if (desc_len == 0)
-			return (u8)((unsigned long)index_key->type >>
-				    (ASSOC_ARRAY_KEY_CHUNK_SIZE - 8));
-		n--;
-		offset = 1;
-		/* fall through */
+		return (unsigned long)index_key->type;
 	default:
-		offset += sizeof(chunk) - 1;
-		offset += (level - 3) * sizeof(chunk);
-		if (offset >= desc_len)
+		level -= 3;
+		if (desc_len <= sizeof(index_key->desc))
 			return 0;
-		desc_len -= offset;
+
+		d = index_key->description + sizeof(index_key->desc);
+		d += level * sizeof(long);
+		desc_len -= sizeof(index_key->desc);
 		if (desc_len > n)
 			desc_len = n;
-		offset += desc_len;
 		do {
 			chunk <<= 8;
-			chunk |= ((u8*)index_key->description)[--offset];
+			chunk |= *d++;
 		} while (--desc_len > 0);
-
-		if (level == 2) {
-			chunk <<= 8;
-			chunk |= (u8)((unsigned long)index_key->type >>
-				      (ASSOC_ARRAY_KEY_CHUNK_SIZE - 8));
-		}
 		return chunk;
 	}
 }
@@ -304,39 +284,28 @@ static int keyring_diff_objects(const void *object, const void *data)
 	seg_b = hash_key_type_and_desc(b);
 	if ((seg_a ^ seg_b) != 0)
 		goto differ;
+	level += ASSOC_ARRAY_KEY_CHUNK_SIZE / 8;
 
 	/* The number of bits contributed by the hash is controlled by a
 	 * constant in the assoc_array headers.  Everything else thereafter we
 	 * can deal with as being machine word-size dependent.
 	 */
-	level += ASSOC_ARRAY_KEY_CHUNK_SIZE / 8;
-	seg_a = a->desc_len;
-	seg_b = b->desc_len;
+	seg_a = a->x;
+	seg_b = b->x;
 	if ((seg_a ^ seg_b) != 0)
 		goto differ;
+	level += sizeof(unsigned long);
 
 	/* The next bit may not work on big endian */
-	level++;
 	seg_a = (unsigned long)a->type;
 	seg_b = (unsigned long)b->type;
 	if ((seg_a ^ seg_b) != 0)
 		goto differ;
-
 	level += sizeof(unsigned long);
-	if (a->desc_len == 0)
-		goto same;
 
-	i = 0;
-	if (((unsigned long)a->description | (unsigned long)b->description) &
-	    (sizeof(unsigned long) - 1)) {
-		do {
-			seg_a = *(unsigned long *)(a->description + i);
-			seg_b = *(unsigned long *)(b->description + i);
-			if ((seg_a ^ seg_b) != 0)
-				goto differ_plus_i;
-			i += sizeof(unsigned long);
-		} while (i < (a->desc_len & (sizeof(unsigned long) - 1)));
-	}
+	i = sizeof(a->desc);
+	if (a->desc_len <= i)
+		goto same;
 
 	for (; i < a->desc_len; i++) {
 		seg_a = *(unsigned char *)(a->description + i);
@@ -662,6 +631,9 @@ static bool search_nested_keyrings(struct key *keyring,
 	BUG_ON((ctx->flags & STATE_CHECKS) == 0 ||
 	       (ctx->flags & STATE_CHECKS) == STATE_CHECKS);
 
+	if (ctx->index_key.description)
+		key_set_index_key(&ctx->index_key);
+
 	/* Check to see if this top-level keyring is what we are looking for
 	 * and whether it is valid or not.
 	 */
diff --git a/security/keys/persistent.c b/security/keys/persistent.c
index d0cb5b32eff7..fc29ec59efa7 100644
--- a/security/keys/persistent.c
+++ b/security/keys/persistent.c
@@ -87,6 +87,7 @@ static long key_get_persistent(struct user_namespace *ns, kuid_t uid,
 	index_key.type = &key_type_keyring;
 	index_key.description = buf;
 	index_key.desc_len = sprintf(buf, "_persistent.%u", from_kuid(ns, uid));
+	key_set_index_key(&index_key);
 
 	if (ns->persistent_keyring_register) {
 		reg_ref = make_key_ref(ns->persistent_keyring_register, true);


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 04/11] keys: Cache the hash value to avoid lots of recalculation
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
                   ` (2 preceding siblings ...)
  2019-04-24 16:13 ` [PATCH 03/11] keys: Simplify key description management David Howells
@ 2019-04-24 16:14 ` David Howells
  2019-04-24 16:14 ` [PATCH 05/11] keys: Add a 'recurse' flag for keyring searches David Howells
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:14 UTC (permalink / raw)
  To: ebiederm
  Cc: keyrings, dhowells, linux-security-module, linux-fsdevel,
	linux-kernel, dwalsh, vgoyal

Cache the hash of the key's type and description in the index key so that
we're not recalculating it every time we look at a key during a search.
The hash function does a bunch of multiplications, so evading those is
probably worthwhile - especially as this is done for every key examined
during a search.

This also allows the methods used by assoc_array to get chunks of index-key
to be simplified.

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

 include/linux/key.h      |    3 +++
 security/keys/internal.h |    8 +-------
 security/keys/key.c      |    2 +-
 security/keys/keyring.c  |   28 ++++++++++++++++++++--------
 4 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 33d87cb2d469..b39f5876b66d 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -86,6 +86,8 @@ struct keyring_list;
 struct keyring_name;
 
 struct keyring_index_key {
+	/* [!] If this structure is altered, the union in struct key must change too! */
+	unsigned long		hash;			/* Hash value */
 	union {
 		struct {
 #ifdef __LITTLE_ENDIAN /* Put desc_len at the LSB of x */
@@ -213,6 +215,7 @@ struct key {
 	union {
 		struct keyring_index_key index_key;
 		struct {
+			unsigned long	hash;
 			unsigned long	len_desc;
 			struct key_type	*type;		/* type of key */
 			char		*description;
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 02592d24f13a..3154ba59a2f0 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -89,13 +89,7 @@ extern spinlock_t key_serial_lock;
 extern struct mutex key_construction_mutex;
 extern wait_queue_head_t request_key_conswq;
 
-
-static inline void key_set_index_key(struct keyring_index_key *index_key)
-{
-	size_t n = min_t(size_t, index_key->desc_len, sizeof(index_key->desc));
-	memcpy(index_key->desc, index_key->description, n);
-}
-
+extern void key_set_index_key(struct keyring_index_key *index_key);
 extern struct key_type *key_type_lookup(const char *type);
 extern void key_type_put(struct key_type *ktype);
 
diff --git a/security/keys/key.c b/security/keys/key.c
index 1d0250a8990e..1568b0028ba4 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -285,12 +285,12 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	key->index_key.description = kmemdup(desc, desclen + 1, GFP_KERNEL);
 	if (!key->index_key.description)
 		goto no_memory_3;
+	key->index_key.type = type;
 	key_set_index_key(&key->index_key);
 
 	refcount_set(&key->usage, 1);
 	init_rwsem(&key->sem);
 	lockdep_set_class(&key->sem, &type->lock_class);
-	key->index_key.type = type;
 	key->user = user;
 	key->quotalen = quotalen;
 	key->datalen = type->def_datalen;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 7bb1f499996c..7f5f8d76dd7d 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -168,7 +168,7 @@ static u64 mult_64x32_and_fold(u64 x, u32 y)
 /*
  * Hash a key type and description.
  */
-static unsigned long hash_key_type_and_desc(const struct keyring_index_key *index_key)
+static void hash_key_type_and_desc(struct keyring_index_key *index_key)
 {
 	const unsigned level_shift = ASSOC_ARRAY_LEVEL_STEP;
 	const unsigned long fan_mask = ASSOC_ARRAY_FAN_MASK;
@@ -206,10 +206,22 @@ static unsigned long hash_key_type_and_desc(const struct keyring_index_key *inde
 	 * zero for keyrings and non-zero otherwise.
 	 */
 	if (index_key->type != &key_type_keyring && (hash & fan_mask) == 0)
-		return hash | (hash >> (ASSOC_ARRAY_KEY_CHUNK_SIZE - level_shift)) | 1;
-	if (index_key->type == &key_type_keyring && (hash & fan_mask) != 0)
-		return (hash + (hash << level_shift)) & ~fan_mask;
-	return hash;
+		hash |= (hash >> (ASSOC_ARRAY_KEY_CHUNK_SIZE - level_shift)) | 1;
+	else if (index_key->type == &key_type_keyring && (hash & fan_mask) != 0)
+		hash = (hash + (hash << level_shift)) & ~fan_mask;
+	index_key->hash = hash;
+}
+
+/*
+ * Finalise an index key to include a part of the description actually in the
+ * index key and to add in the hash too.
+ */
+void key_set_index_key(struct keyring_index_key *index_key)
+{
+	size_t n = min_t(size_t, index_key->desc_len, sizeof(index_key->desc));
+	memcpy(index_key->desc, index_key->description, n);
+
+	hash_key_type_and_desc(index_key);
 }
 
 /*
@@ -227,7 +239,7 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
 	level /= ASSOC_ARRAY_KEY_CHUNK_SIZE;
 	switch (level) {
 	case 0:
-		return hash_key_type_and_desc(index_key);
+		return index_key->hash;
 	case 1:
 		return index_key->x;
 	case 2:
@@ -280,8 +292,8 @@ static int keyring_diff_objects(const void *object, const void *data)
 	int level, i;
 
 	level = 0;
-	seg_a = hash_key_type_and_desc(a);
-	seg_b = hash_key_type_and_desc(b);
+	seg_a = a->hash;
+	seg_b = b->hash;
 	if ((seg_a ^ seg_b) != 0)
 		goto differ;
 	level += ASSOC_ARRAY_KEY_CHUNK_SIZE / 8;


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 05/11] keys: Add a 'recurse' flag for keyring searches
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
                   ` (3 preceding siblings ...)
  2019-04-24 16:14 ` [PATCH 04/11] keys: Cache the hash value to avoid lots of recalculation David Howells
@ 2019-04-24 16:14 ` David Howells
  2019-04-25  4:27   ` Andrew Zaborowski
  2019-04-25 11:02   ` David Howells
  2019-04-24 16:14 ` [PATCH 06/11] keys: Namespace keyring names David Howells
                   ` (6 subsequent siblings)
  11 siblings, 2 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:14 UTC (permalink / raw)
  To: ebiederm
  Cc: keyrings, dhowells, linux-security-module, linux-fsdevel,
	linux-kernel, dwalsh, vgoyal

Add a 'recurse' flag for keyring searches so that the flag can be omitted
and recursion disabled, thereby allowing just the nominated keyring to be
searched and none of the children.

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

 Documentation/security/keys/core.rst     |   10 ++++++----
 certs/blacklist.c                        |    2 +-
 crypto/asymmetric_keys/asymmetric_type.c |    2 +-
 include/linux/key.h                      |    3 ++-
 lib/digsig.c                             |    2 +-
 net/rxrpc/security.c                     |    2 +-
 security/integrity/digsig_asymmetric.c   |    4 ++--
 security/keys/internal.h                 |    1 +
 security/keys/keyctl.c                   |    2 +-
 security/keys/keyring.c                  |   12 ++++++++++--
 security/keys/proc.c                     |    3 ++-
 security/keys/process_keys.c             |    3 ++-
 security/keys/request_key.c              |    3 ++-
 security/keys/request_key_auth.c         |    3 ++-
 14 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
index 9521c4207f01..99079b664036 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -1159,11 +1159,13 @@ payload contents" for more information.
 
 	key_ref_t keyring_search(key_ref_t keyring_ref,
 				 const struct key_type *type,
-				 const char *description)
+				 const char *description,
+				 bool recurse)
 
-    This searches the keyring tree specified for a matching key. Error ENOKEY
-    is returned upon failure (use IS_ERR/PTR_ERR to determine). If successful,
-    the returned key will need to be released.
+    This searches the specified keyring only (recurse == false) or keyring tree
+    (recurse == true) specified for a matching key. Error ENOKEY is returned
+    upon failure (use IS_ERR/PTR_ERR to determine). If successful, the returned
+    key will need to be released.
 
     The possession attribute from the keyring reference is used to control
     access through the permissions mask and is propagated to the returned key
diff --git a/certs/blacklist.c b/certs/blacklist.c
index 3a507b9e2568..181cb7fa9540 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -128,7 +128,7 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
 	*p = 0;
 
 	kref = keyring_search(make_key_ref(blacklist_keyring, true),
-			      &key_type_blacklist, buffer);
+			      &key_type_blacklist, buffer, false);
 	if (!IS_ERR(kref)) {
 		key_ref_put(kref);
 		ret = -EKEYREJECTED;
diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index 69a0788a7de5..084027ef3121 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -87,7 +87,7 @@ struct key *find_asymmetric_key(struct key *keyring,
 	pr_debug("Look up: \"%s\"\n", req);
 
 	ref = keyring_search(make_key_ref(keyring, 1),
-			     &key_type_asymmetric, req);
+			     &key_type_asymmetric, req, true);
 	if (IS_ERR(ref))
 		pr_debug("Request for key '%s' err %ld\n", req, PTR_ERR(ref));
 	kfree(req);
diff --git a/include/linux/key.h b/include/linux/key.h
index b39f5876b66d..bdd179169508 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -333,7 +333,8 @@ extern int keyring_clear(struct key *keyring);
 
 extern key_ref_t keyring_search(key_ref_t keyring,
 				struct key_type *type,
-				const char *description);
+				const char *description,
+				bool no_recurse);
 
 extern int keyring_add_key(struct key *keyring,
 			   struct key *key);
diff --git a/lib/digsig.c b/lib/digsig.c
index 6ba6fcd92dd1..abec995a0982 100644
--- a/lib/digsig.c
+++ b/lib/digsig.c
@@ -221,7 +221,7 @@ int digsig_verify(struct key *keyring, const char *sig, int siglen,
 		/* search in specific keyring */
 		key_ref_t kref;
 		kref = keyring_search(make_key_ref(keyring, 1UL),
-						&key_type_user, name);
+				      &key_type_user, name, true);
 		if (IS_ERR(kref))
 			key = ERR_CAST(kref);
 		else
diff --git a/net/rxrpc/security.c b/net/rxrpc/security.c
index c4479afe8ae7..2cfc7125bc41 100644
--- a/net/rxrpc/security.c
+++ b/net/rxrpc/security.c
@@ -148,7 +148,7 @@ int rxrpc_init_server_conn_security(struct rxrpc_connection *conn)
 
 	/* look through the service's keyring */
 	kref = keyring_search(make_key_ref(rx->securities, 1UL),
-			      &key_type_rxrpc_s, kdesc);
+			      &key_type_rxrpc_s, kdesc, true);
 	if (IS_ERR(kref)) {
 		read_unlock(&local->services_lock);
 		_leave(" = %ld [search]", PTR_ERR(kref));
diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index d775e03fbbcc..fa52fbaef520 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -39,7 +39,7 @@ static struct key *request_asymmetric_key(struct key *keyring, uint32_t keyid)
 		key_ref_t kref;
 
 		kref = keyring_search(make_key_ref(key, 1),
-				     &key_type_asymmetric, name);
+				      &key_type_asymmetric, name, true);
 		if (!IS_ERR(kref)) {
 			pr_err("Key '%s' is in ima_blacklist_keyring\n", name);
 			return ERR_PTR(-EKEYREJECTED);
@@ -51,7 +51,7 @@ static struct key *request_asymmetric_key(struct key *keyring, uint32_t keyid)
 		key_ref_t kref;
 
 		kref = keyring_search(make_key_ref(keyring, 1),
-				      &key_type_asymmetric, name);
+				      &key_type_asymmetric, name, true);
 		if (IS_ERR(kref))
 			key = ERR_CAST(kref);
 		else
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 3154ba59a2f0..8ac7573bf23b 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -123,6 +123,7 @@ struct keyring_search_context {
 #define KEYRING_SEARCH_NO_CHECK_PERM	0x0008	/* Don't check permissions */
 #define KEYRING_SEARCH_DETECT_TOO_DEEP	0x0010	/* Give an error on excessive depth */
 #define KEYRING_SEARCH_SKIP_EXPIRED	0x0020	/* Ignore expired keys (intention to replace) */
+#define KEYRING_SEARCH_RECURSE		0x0040	/* Search child keyrings also */
 
 	int (*iterator)(const void *object, void *iterator_data);
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 3e4053a217c3..d2023d550bd6 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -704,7 +704,7 @@ long keyctl_keyring_search(key_serial_t ringid,
 	}
 
 	/* do the search */
-	key_ref = keyring_search(keyring_ref, ktype, description);
+	key_ref = keyring_search(keyring_ref, ktype, description, true);
 	if (IS_ERR(key_ref)) {
 		ret = PTR_ERR(key_ref);
 
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 7f5f8d76dd7d..29a6e3255157 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -685,6 +685,9 @@ static bool search_nested_keyrings(struct key *keyring,
 	 * Non-keyrings avoid the leftmost branch of the root entirely (root
 	 * slots 1-15).
 	 */
+	if (!(ctx->flags & KEYRING_SEARCH_RECURSE))
+		goto not_this_keyring;
+
 	ptr = READ_ONCE(keyring->keys.root);
 	if (!ptr)
 		goto not_this_keyring;
@@ -885,13 +888,15 @@ key_ref_t keyring_search_aux(key_ref_t keyring_ref,
  * @keyring: The root of the keyring tree to be searched.
  * @type: The type of keyring we want to find.
  * @description: The name of the keyring we want to find.
+ * @recurse: True to search the children of @keyring also
  *
  * As keyring_search_aux() 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,
 			 struct key_type *type,
-			 const char *description)
+			 const char *description,
+			 bool recurse)
 {
 	struct keyring_search_context ctx = {
 		.index_key.type		= type,
@@ -906,6 +911,8 @@ key_ref_t keyring_search(key_ref_t keyring,
 	key_ref_t key;
 	int ret;
 
+	if (recurse)
+		ctx.flags |= KEYRING_SEARCH_RECURSE;
 	if (type->match_preparse) {
 		ret = type->match_preparse(&ctx.match_data);
 		if (ret < 0)
@@ -1170,7 +1177,8 @@ static int keyring_detect_cycle(struct key *A, struct key *B)
 		.flags			= (KEYRING_SEARCH_NO_STATE_CHECK |
 					   KEYRING_SEARCH_NO_UPDATE_TIME |
 					   KEYRING_SEARCH_NO_CHECK_PERM |
-					   KEYRING_SEARCH_DETECT_TOO_DEEP),
+					   KEYRING_SEARCH_DETECT_TOO_DEEP |
+					   KEYRING_SEARCH_RECURSE),
 	};
 
 	rcu_read_lock();
diff --git a/security/keys/proc.c b/security/keys/proc.c
index 78ac305d715e..8695f6108da9 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -170,7 +170,8 @@ static int proc_keys_show(struct seq_file *m, void *v)
 		.match_data.cmp		= lookup_user_key_possessed,
 		.match_data.raw_data	= key,
 		.match_data.lookup_type	= KEYRING_SEARCH_LOOKUP_DIRECT,
-		.flags			= KEYRING_SEARCH_NO_STATE_CHECK,
+		.flags			= (KEYRING_SEARCH_NO_STATE_CHECK |
+					   KEYRING_SEARCH_RECURSE),
 	};
 
 	key_ref = make_key_ref(key, 0);
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index f05f7125a7d5..70fa20888ca8 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -540,7 +540,8 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 	struct keyring_search_context ctx = {
 		.match_data.cmp		= lookup_user_key_possessed,
 		.match_data.lookup_type	= KEYRING_SEARCH_LOOKUP_DIRECT,
-		.flags			= KEYRING_SEARCH_NO_STATE_CHECK,
+		.flags			= (KEYRING_SEARCH_NO_STATE_CHECK |
+					   KEYRING_SEARCH_RECURSE),
 	};
 	struct request_key_auth *rka;
 	struct key *key;
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index a5b74b7758f7..39802540ffff 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -537,7 +537,8 @@ struct key *request_key_and_link(struct key_type *type,
 		.match_data.raw_data	= description,
 		.match_data.lookup_type	= KEYRING_SEARCH_LOOKUP_DIRECT,
 		.flags			= (KEYRING_SEARCH_DO_STATE_CHECK |
-					   KEYRING_SEARCH_SKIP_EXPIRED),
+					   KEYRING_SEARCH_SKIP_EXPIRED |
+					   KEYRING_SEARCH_RECURSE),
 	};
 	struct key *key;
 	key_ref_t key_ref;
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index bda6201c6c45..b83930581f4d 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -242,7 +242,8 @@ struct key *key_get_instantiation_authkey(key_serial_t target_id)
 		.match_data.cmp		= key_default_cmp,
 		.match_data.raw_data	= description,
 		.match_data.lookup_type	= KEYRING_SEARCH_LOOKUP_DIRECT,
-		.flags			= KEYRING_SEARCH_DO_STATE_CHECK,
+		.flags			= (KEYRING_SEARCH_DO_STATE_CHECK |
+					   KEYRING_SEARCH_RECURSE),
 	};
 	struct key *authkey;
 	key_ref_t authkey_ref;


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 06/11] keys: Namespace keyring names
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
                   ` (4 preceding siblings ...)
  2019-04-24 16:14 ` [PATCH 05/11] keys: Add a 'recurse' flag for keyring searches David Howells
@ 2019-04-24 16:14 ` David Howells
  2019-04-24 16:14 ` [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace David Howells
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:14 UTC (permalink / raw)
  To: ebiederm
  Cc: keyrings, dhowells, linux-security-module, linux-fsdevel,
	linux-kernel, dwalsh, vgoyal

Keyring names are held in a single global list that any process can pick
from by means of keyctl_join_session_keyring (provided the keyring grants
Search permission).  This isn't very container friendly, however.

Make the following changes:

 (1) Make default session, process and thread keyring names begin with a
     '.' instead of '_'.

 (2) Keyrings whose names begin with a '.' aren't added to the list.  Such
     keyrings are system specials.

 (3) Replace the global list with per-user_namespace lists.  A keyring adds
     its name to the list for the user_namespace that it is currently in.

 (4) When a user_namespace is deleted, it just removes itself from the
     keyring name list.

The global keyring_name_lock is retained for accessing the name lists.
This allows (4) to work.

This can be tested by:

	# keyctl newring foo @s
	995906392
	# unshare -U
	$ keyctl show
	...
	 995906392 --alswrv  65534 65534   \_ keyring: foo
	...
	$ keyctl session foo
	Joined session keyring: 935622349

As can be seen, a new session keyring was created.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Eric W. Biederman <ebiederm@xmission.com>
---

 include/linux/key.h            |    2 +
 include/linux/user_namespace.h |    5 ++
 kernel/user.c                  |    3 +
 kernel/user_namespace.c        |    7 ++-
 security/keys/keyring.c        |   99 +++++++++++++++++-----------------------
 5 files changed, 57 insertions(+), 59 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index bdd179169508..52334cc7c18d 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -353,6 +353,7 @@ extern void key_set_timeout(struct key *, unsigned);
 
 extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
 				 key_perm_t perm);
+extern void key_free_user_ns(struct user_namespace *);
 
 /*
  * The permissions required on a key that we're looking up.
@@ -426,6 +427,7 @@ extern void key_init(void);
 #define key_fsuid_changed(t)		do { } while(0)
 #define key_fsgid_changed(t)		do { } while(0)
 #define key_init()			do { } while(0)
+#define key_free_user_ns(ns)		do { } while(0)
 
 #endif /* CONFIG_KEYS */
 #endif /* __KERNEL__ */
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index d6b74b91096b..90457015fa3f 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -64,6 +64,11 @@ struct user_namespace {
 	struct ns_common	ns;
 	unsigned long		flags;
 
+#ifdef CONFIG_KEYS
+	/* List of joinable keyrings in this namespace */
+	struct list_head	keyring_name_list;
+#endif
+
 	/* Register of per-UID persistent keyrings for this namespace */
 #ifdef CONFIG_PERSISTENT_KEYRINGS
 	struct key		*persistent_keyring_register;
diff --git a/kernel/user.c b/kernel/user.c
index 0df9b1640b2a..6a6ccb99f31a 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -62,6 +62,9 @@ struct user_namespace init_user_ns = {
 	.ns.ops = &userns_operations,
 #endif
 	.flags = USERNS_INIT_FLAGS,
+#ifdef CONFIG_KEYS
+	.keyring_name_list = LIST_HEAD_INIT(init_user_ns.keyring_name_list),
+#endif
 #ifdef CONFIG_PERSISTENT_KEYRINGS
 	.persistent_keyring_register_sem =
 	__RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 923414a246e9..bda6e890ad88 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -133,6 +133,9 @@ int create_user_ns(struct cred *new)
 	ns->flags = parent_ns->flags;
 	mutex_unlock(&userns_state_mutex);
 
+#ifdef CONFIG_KEYS
+	INIT_LIST_HEAD(&ns->keyring_name_list);
+#endif
 #ifdef CONFIG_PERSISTENT_KEYRINGS
 	init_rwsem(&ns->persistent_keyring_register_sem);
 #endif
@@ -196,9 +199,7 @@ static void free_user_ns(struct work_struct *work)
 			kfree(ns->projid_map.reverse);
 		}
 		retire_userns_sysctls(ns);
-#ifdef CONFIG_PERSISTENT_KEYRINGS
-		key_put(ns->persistent_keyring_register);
-#endif
+		key_free_user_ns(ns);
 		ns_free_inum(&ns->ns);
 		kmem_cache_free(user_ns_cachep, ns);
 		dec_user_namespaces(ucounts);
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 29a6e3255157..48eda80afe89 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -16,6 +16,7 @@
 #include <linux/security.h>
 #include <linux/seq_file.h>
 #include <linux/err.h>
+#include <linux/user_namespace.h>
 #include <keys/keyring-type.h>
 #include <keys/user-type.h>
 #include <linux/assoc_array_priv.h>
@@ -28,11 +29,6 @@
  */
 #define KEYRING_SEARCH_MAX_DEPTH 6
 
-/*
- * We keep all named keyrings in a hash to speed looking them up.
- */
-#define KEYRING_NAME_HASH_SIZE	(1 << 5)
-
 /*
  * We mark pointers we pass to the associative array with bit 1 set if
  * they're keyrings and clear otherwise.
@@ -55,17 +51,20 @@ static inline void *keyring_key_to_ptr(struct key *key)
 	return key;
 }
 
-static struct list_head	keyring_name_hash[KEYRING_NAME_HASH_SIZE];
 static DEFINE_RWLOCK(keyring_name_lock);
 
-static inline unsigned keyring_hash(const char *desc)
+/*
+ * Clean up the bits of user_namespace that belong to us.
+ */
+void key_free_user_ns(struct user_namespace *ns)
 {
-	unsigned bucket = 0;
-
-	for (; *desc; desc++)
-		bucket += (unsigned char)*desc;
+	write_lock(&keyring_name_lock);
+	list_del_init(&ns->keyring_name_list);
+	write_unlock(&keyring_name_lock);
 
-	return bucket & (KEYRING_NAME_HASH_SIZE - 1);
+#ifdef CONFIG_PERSISTENT_KEYRINGS
+	key_put(ns->persistent_keyring_register);
+#endif
 }
 
 /*
@@ -104,23 +103,17 @@ static DECLARE_RWSEM(keyring_serialise_link_sem);
 
 /*
  * Publish the name of a keyring so that it can be found by name (if it has
- * one).
+ * one and it doesn't begin with a dot).
  */
 static void keyring_publish_name(struct key *keyring)
 {
-	int bucket;
-
-	if (keyring->description) {
-		bucket = keyring_hash(keyring->description);
+	struct user_namespace *ns = current_user_ns();
 
+	if (keyring->description &&
+	    keyring->description[0] &&
+	    keyring->description[0] != '.') {
 		write_lock(&keyring_name_lock);
-
-		if (!keyring_name_hash[bucket].next)
-			INIT_LIST_HEAD(&keyring_name_hash[bucket]);
-
-		list_add_tail(&keyring->name_link,
-			      &keyring_name_hash[bucket]);
-
+		list_add_tail(&keyring->name_link, &ns->keyring_name_list);
 		write_unlock(&keyring_name_lock);
 	}
 }
@@ -1091,50 +1084,44 @@ key_ref_t find_key_to_update(key_ref_t keyring_ref,
  */
 struct key *find_keyring_by_name(const char *name, bool uid_keyring)
 {
+	struct user_namespace *ns = current_user_ns();
 	struct key *keyring;
-	int bucket;
 
 	if (!name)
 		return ERR_PTR(-EINVAL);
 
-	bucket = keyring_hash(name);
-
 	read_lock(&keyring_name_lock);
 
-	if (keyring_name_hash[bucket].next) {
-		/* search this hash bucket for a keyring with a matching name
-		 * that's readable and that hasn't been revoked */
-		list_for_each_entry(keyring,
-				    &keyring_name_hash[bucket],
-				    name_link
-				    ) {
-			if (!kuid_has_mapping(current_user_ns(), keyring->user->uid))
-				continue;
-
-			if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
-				continue;
+	/* Search this hash bucket for a keyring with a matching name that
+	 * grants Search permission and that hasn't been revoked
+	 */
+	list_for_each_entry(keyring, &ns->keyring_name_list, name_link) {
+		if (!kuid_has_mapping(ns, keyring->user->uid))
+			continue;
 
-			if (strcmp(keyring->description, name) != 0)
-				continue;
+		if (test_bit(KEY_FLAG_REVOKED, &keyring->flags))
+			continue;
 
-			if (uid_keyring) {
-				if (!test_bit(KEY_FLAG_UID_KEYRING,
-					      &keyring->flags))
-					continue;
-			} else {
-				if (key_permission(make_key_ref(keyring, 0),
-						   KEY_NEED_SEARCH) < 0)
-					continue;
-			}
+		if (strcmp(keyring->description, name) != 0)
+			continue;
 
-			/* we've got a match but we might end up racing with
-			 * key_cleanup() if the keyring is currently 'dead'
-			 * (ie. it has a zero usage count) */
-			if (!refcount_inc_not_zero(&keyring->usage))
+		if (uid_keyring) {
+			if (!test_bit(KEY_FLAG_UID_KEYRING,
+				      &keyring->flags))
+				continue;
+		} else {
+			if (key_permission(make_key_ref(keyring, 0),
+					   KEY_NEED_SEARCH) < 0)
 				continue;
-			keyring->last_used_at = ktime_get_real_seconds();
-			goto out;
 		}
+
+		/* we've got a match but we might end up racing with
+		 * key_cleanup() if the keyring is currently 'dead'
+		 * (ie. it has a zero usage count) */
+		if (!refcount_inc_not_zero(&keyring->usage))
+			continue;
+		keyring->last_used_at = ktime_get_real_seconds();
+		goto out;
 	}
 
 	keyring = ERR_PTR(-ENOKEY);


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
                   ` (5 preceding siblings ...)
  2019-04-24 16:14 ` [PATCH 06/11] keys: Namespace keyring names David Howells
@ 2019-04-24 16:14 ` David Howells
  2019-04-24 22:03   ` Jann Horn
                     ` (2 more replies)
  2019-04-24 16:14 ` [PATCH 08/11] keys: Include target namespace in match criteria David Howells
                   ` (4 subsequent siblings)
  11 siblings, 3 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:14 UTC (permalink / raw)
  To: ebiederm
  Cc: Jann Horn, keyrings, dhowells, linux-security-module,
	linux-fsdevel, linux-kernel, dwalsh, vgoyal

Move the user and user-session keyrings to the user_namespace struct rather
than pinning them from the user_struct struct.  This prevents these
keyrings from propagating across user-namespaces boundaries with regard to
the KEY_SPEC_* flags, thereby making them more useful in a containerised
environment.

The issue is that a single user_struct may be represent UIDs in several
different namespaces.

The way the patch does this is by attaching a 'register keyring' in each
user_namespace and then sticking the user and user-session keyrings into
that.  It can then be searched to retrieve them.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jann Horn <jannh@google.com>
---

 include/linux/sched/user.h     |   14 --
 include/linux/user_namespace.h |    3 
 kernel/user.c                  |    9 -
 kernel/user_namespace.c        |    4 -
 security/keys/internal.h       |    3 
 security/keys/keyring.c        |    1 
 security/keys/persistent.c     |    8 +
 security/keys/process_keys.c   |  249 +++++++++++++++++++++++++---------------
 security/keys/request_key.c    |   21 ++-
 9 files changed, 181 insertions(+), 131 deletions(-)

diff --git a/include/linux/sched/user.h b/include/linux/sched/user.h
index 468d2565a9fe..917d88edb7b9 100644
--- a/include/linux/sched/user.h
+++ b/include/linux/sched/user.h
@@ -7,8 +7,6 @@
 #include <linux/refcount.h>
 #include <linux/ratelimit.h>
 
-struct key;
-
 /*
  * Some day this will be a full-fledged user tracking system..
  */
@@ -30,18 +28,6 @@ struct user_struct {
 	unsigned long unix_inflight;	/* How many files in flight in unix sockets */
 	atomic_long_t pipe_bufs;  /* how many pages are allocated in pipe buffers */
 
-#ifdef CONFIG_KEYS
-	/*
-	 * These pointers can only change from NULL to a non-NULL value once.
-	 * Writes are protected by key_user_keyring_mutex.
-	 * Unlocked readers should use READ_ONCE() unless they know that
-	 * install_user_keyrings() has been called successfully (which sets
-	 * these members to non-NULL values, preventing further modifications).
-	 */
-	struct key *uid_keyring;	/* UID specific keyring */
-	struct key *session_keyring;	/* UID's default session keyring */
-#endif
-
 	/* Hash table maintenance information */
 	struct hlist_node uidhash_node;
 	kuid_t uid;
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index 90457015fa3f..44a5a4a8a269 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -67,12 +67,13 @@ struct user_namespace {
 #ifdef CONFIG_KEYS
 	/* List of joinable keyrings in this namespace */
 	struct list_head	keyring_name_list;
+	struct key		*user_keyring_register;
+	struct rw_semaphore	keyring_sem;
 #endif
 
 	/* Register of per-UID persistent keyrings for this namespace */
 #ifdef CONFIG_PERSISTENT_KEYRINGS
 	struct key		*persistent_keyring_register;
-	struct rw_semaphore	persistent_keyring_register_sem;
 #endif
 	struct work_struct	work;
 #ifdef CONFIG_SYSCTL
diff --git a/kernel/user.c b/kernel/user.c
index 6a6ccb99f31a..63bde2144338 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -64,10 +64,7 @@ struct user_namespace init_user_ns = {
 	.flags = USERNS_INIT_FLAGS,
 #ifdef CONFIG_KEYS
 	.keyring_name_list = LIST_HEAD_INIT(init_user_ns.keyring_name_list),
-#endif
-#ifdef CONFIG_PERSISTENT_KEYRINGS
-	.persistent_keyring_register_sem =
-	__RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
+	.keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem),
 #endif
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
@@ -143,8 +140,6 @@ static void free_user(struct user_struct *up, unsigned long flags)
 {
 	uid_hash_remove(up);
 	spin_unlock_irqrestore(&uidhash_lock, flags);
-	key_put(up->uid_keyring);
-	key_put(up->session_keyring);
 	kmem_cache_free(uid_cachep, up);
 }
 
@@ -202,8 +197,6 @@ struct user_struct *alloc_uid(kuid_t uid)
 		spin_lock_irq(&uidhash_lock);
 		up = uid_hash_find(uid, hashent);
 		if (up) {
-			key_put(new->uid_keyring);
-			key_put(new->session_keyring);
 			kmem_cache_free(uid_cachep, new);
 		} else {
 			uid_hash_insert(new, hashent);
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index bda6e890ad88..c87c2ecc7085 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -135,9 +135,7 @@ int create_user_ns(struct cred *new)
 
 #ifdef CONFIG_KEYS
 	INIT_LIST_HEAD(&ns->keyring_name_list);
-#endif
-#ifdef CONFIG_PERSISTENT_KEYRINGS
-	init_rwsem(&ns->persistent_keyring_register_sem);
+	init_rwsem(&ns->keyring_sem);
 #endif
 	ret = -ENOMEM;
 	if (!setup_userns_sysctls(ns))
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8ac7573bf23b..ea48d8b30794 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -144,7 +144,8 @@ extern key_ref_t search_process_keyrings(struct keyring_search_context *ctx);
 
 extern struct key *find_keyring_by_name(const char *name, bool uid_keyring);
 
-extern int install_user_keyrings(void);
+extern int look_up_user_keyrings(struct key **, struct key **);
+extern struct key *get_user_session_keyring(void);
 extern int install_thread_keyring_to_cred(struct cred *);
 extern int install_process_keyring_to_cred(struct cred *);
 extern int install_session_keyring_to_cred(struct cred *, struct key *);
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 48eda80afe89..1fc9219ed002 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -62,6 +62,7 @@ void key_free_user_ns(struct user_namespace *ns)
 	list_del_init(&ns->keyring_name_list);
 	write_unlock(&keyring_name_lock);
 
+	key_put(ns->user_keyring_register);
 #ifdef CONFIG_PERSISTENT_KEYRINGS
 	key_put(ns->persistent_keyring_register);
 #endif
diff --git a/security/keys/persistent.c b/security/keys/persistent.c
index fc29ec59efa7..90303fe4a394 100644
--- a/security/keys/persistent.c
+++ b/security/keys/persistent.c
@@ -91,9 +91,9 @@ static long key_get_persistent(struct user_namespace *ns, kuid_t uid,
 
 	if (ns->persistent_keyring_register) {
 		reg_ref = make_key_ref(ns->persistent_keyring_register, true);
-		down_read(&ns->persistent_keyring_register_sem);
+		down_read(&ns->keyring_sem);
 		persistent_ref = find_key_to_update(reg_ref, &index_key);
-		up_read(&ns->persistent_keyring_register_sem);
+		up_read(&ns->keyring_sem);
 
 		if (persistent_ref)
 			goto found;
@@ -102,9 +102,9 @@ static long key_get_persistent(struct user_namespace *ns, kuid_t uid,
 	/* It wasn't in the register, so we'll need to create it.  We might
 	 * also need to create the register.
 	 */
-	down_write(&ns->persistent_keyring_register_sem);
+	down_write(&ns->keyring_sem);
 	persistent_ref = key_create_persistent(ns, uid, &index_key);
-	up_write(&ns->persistent_keyring_register_sem);
+	up_write(&ns->keyring_sem);
 	if (!IS_ERR(persistent_ref))
 		goto found;
 
diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 70fa20888ca8..ff62d90415ae 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -8,7 +8,7 @@
  * as published by the Free Software Foundation; either version
  * 2 of the License, or (at your option) any later version.
  */
-
+#define __KDEBUG
 #include <linux/init.h>
 #include <linux/sched.h>
 #include <linux/sched/user.h>
@@ -19,15 +19,13 @@
 #include <linux/security.h>
 #include <linux/user_namespace.h>
 #include <linux/uaccess.h>
+#include <linux/init_task.h>
 #include <keys/request_key_auth-type.h>
 #include "internal.h"
 
 /* Session keyring create vs join semaphore */
 static DEFINE_MUTEX(key_session_mutex);
 
-/* User keyring creation semaphore */
-static DEFINE_MUTEX(key_user_keyring_mutex);
-
 /* The root user's tracking struct */
 struct key_user root_key_user = {
 	.usage		= REFCOUNT_INIT(3),
@@ -39,98 +37,173 @@ struct key_user root_key_user = {
 };
 
 /*
- * Install the user and user session keyrings for the current process's UID.
+ * Get or create a user register keyring.
  */
-int install_user_keyrings(void)
+static struct key *get_user_register(struct user_namespace *user_ns)
 {
-	struct user_struct *user;
-	const struct cred *cred;
-	struct key *uid_keyring, *session_keyring;
+	struct key *reg_keyring = user_ns->user_keyring_register;
+
+	if (reg_keyring)
+		return reg_keyring;
+
+	down_write(&user_ns->keyring_sem);
+
+	/* Make sure there's a register keyring.  It gets owned by the
+	 * user_namespace's owner.
+	 */
+	reg_keyring = user_ns->user_keyring_register;
+	if (!reg_keyring) {
+		reg_keyring = keyring_alloc(".user_reg", user_ns->owner, INVALID_GID,
+					    &init_cred,
+					    KEY_POS_WRITE | KEY_POS_SEARCH |
+					    KEY_USR_VIEW | KEY_USR_READ,
+					    0,
+					    NULL, NULL);
+		if (!IS_ERR(reg_keyring))
+			user_ns->user_keyring_register = reg_keyring;
+	}
+
+	up_write(&user_ns->keyring_sem);
+
+	/* We don't return a ref since the keyring is pinned by the user_ns */
+	return reg_keyring;
+}
+
+/*
+ * Look up the user and user session keyrings for the current process's UID,
+ * creating them if they don't exist.
+ */
+int look_up_user_keyrings(struct key **_user_keyring,
+			  struct key **_user_session_keyring)
+{
+	const struct cred *cred = current_cred();
+	struct user_namespace *user_ns = current_user_ns();
+	struct key *reg_keyring, *uid_keyring, *session_keyring;
 	key_perm_t user_keyring_perm;
+	key_ref_t uid_keyring_r, session_keyring_r;
+	uid_t uid = from_kuid(user_ns, cred->user->uid);
 	char buf[20];
 	int ret;
-	uid_t uid;
 
 	user_keyring_perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL;
-	cred = current_cred();
-	user = cred->user;
-	uid = from_kuid(cred->user_ns, user->uid);
 
-	kenter("%p{%u}", user, uid);
+	kenter("%u", uid);
 
-	if (READ_ONCE(user->uid_keyring) && READ_ONCE(user->session_keyring)) {
-		kleave(" = 0 [exist]");
-		return 0;
-	}
+	reg_keyring = get_user_register(user_ns);
+	if (IS_ERR(reg_keyring))
+		return PTR_ERR(reg_keyring);
 
-	mutex_lock(&key_user_keyring_mutex);
+	down_write(&user_ns->keyring_sem);
 	ret = 0;
 
-	if (!user->uid_keyring) {
-		/* get the UID-specific keyring
-		 * - there may be one in existence already as it may have been
-		 *   pinned by a session, but the user_struct pointing to it
-		 *   may have been destroyed by setuid */
-		sprintf(buf, "_uid.%u", uid);
-
-		uid_keyring = find_keyring_by_name(buf, true);
+	/* Get the user keyring.  Note that there may be one in existence
+	 * already as it may have been pinned by a session, but the user_struct
+	 * pointing to it may have been destroyed by setuid.
+	 */
+	sprintf(buf, "_uid.%u", uid);
+	uid_keyring_r = keyring_search(make_key_ref(reg_keyring, true),
+				       &key_type_keyring, buf, false);
+	kdebug("_uid %p", uid_keyring_r);
+	if (uid_keyring_r == ERR_PTR(-EAGAIN)) {
+		uid_keyring = keyring_alloc(buf, cred->user->uid, INVALID_GID,
+					    cred, user_keyring_perm,
+					    KEY_ALLOC_UID_KEYRING |
+					    KEY_ALLOC_IN_QUOTA,
+					    NULL, reg_keyring);
 		if (IS_ERR(uid_keyring)) {
-			uid_keyring = keyring_alloc(buf, user->uid, INVALID_GID,
-						    cred, user_keyring_perm,
-						    KEY_ALLOC_UID_KEYRING |
-							KEY_ALLOC_IN_QUOTA,
-						    NULL, NULL);
-			if (IS_ERR(uid_keyring)) {
-				ret = PTR_ERR(uid_keyring);
-				goto error;
-			}
+			ret = PTR_ERR(uid_keyring);
+			goto error;
 		}
+	} else if (IS_ERR(uid_keyring_r)) {
+		ret = PTR_ERR(uid_keyring_r);
+		goto error;
+	} else {
+		uid_keyring = key_ref_to_ptr(uid_keyring_r);
+	}
 
-		/* get a default session keyring (which might also exist
-		 * already) */
-		sprintf(buf, "_uid_ses.%u", uid);
-
-		session_keyring = find_keyring_by_name(buf, true);
+	/* Get a default session keyring (which might also exist already) */
+	sprintf(buf, "_uid_ses.%u", uid);
+	session_keyring_r = keyring_search(make_key_ref(reg_keyring, true),
+					   &key_type_keyring, buf, false);
+	kdebug("_uid_ses %p", session_keyring_r);
+	if (session_keyring_r == ERR_PTR(-EAGAIN)) {
+		session_keyring = keyring_alloc(buf, cred->user->uid, INVALID_GID,
+						cred, user_keyring_perm,
+						KEY_ALLOC_UID_KEYRING |
+						KEY_ALLOC_IN_QUOTA,
+						NULL, NULL);
 		if (IS_ERR(session_keyring)) {
-			session_keyring =
-				keyring_alloc(buf, user->uid, INVALID_GID,
-					      cred, user_keyring_perm,
-					      KEY_ALLOC_UID_KEYRING |
-						  KEY_ALLOC_IN_QUOTA,
-					      NULL, NULL);
-			if (IS_ERR(session_keyring)) {
-				ret = PTR_ERR(session_keyring);
-				goto error_release;
-			}
-
-			/* we install a link from the user session keyring to
-			 * the user keyring */
-			ret = key_link(session_keyring, uid_keyring);
-			if (ret < 0)
-				goto error_release_both;
+			ret = PTR_ERR(session_keyring);
+			goto error_release;
 		}
 
-		/* install the keyrings */
-		/* paired with READ_ONCE() */
-		smp_store_release(&user->uid_keyring, uid_keyring);
-		/* paired with READ_ONCE() */
-		smp_store_release(&user->session_keyring, session_keyring);
+		/* We install a link from the user session keyring to
+		 * the user keyring.
+		 */
+		ret = key_link(session_keyring, uid_keyring);
+		if (ret < 0)
+			goto error_release_session;
+
+		/* And only then link the user-session keyring to the
+		 * register.
+		 */
+		ret = key_link(reg_keyring, session_keyring);
+		if (ret < 0)
+			goto error_release_session;
+	} else if (IS_ERR(session_keyring_r)) {
+		ret = PTR_ERR(session_keyring_r);
+		goto error_release;
+	} else {
+		session_keyring = key_ref_to_ptr(session_keyring_r);
 	}
 
-	mutex_unlock(&key_user_keyring_mutex);
+	up_write(&user_ns->keyring_sem);
+
+	if (_user_session_keyring)
+		*_user_session_keyring = session_keyring;
+	else
+		key_put(session_keyring);
+	if (_user_keyring)
+		*_user_keyring = uid_keyring;
+	else
+		key_put(uid_keyring);
 	kleave(" = 0");
 	return 0;
 
-error_release_both:
+error_release_session:
 	key_put(session_keyring);
 error_release:
 	key_put(uid_keyring);
 error:
-	mutex_unlock(&key_user_keyring_mutex);
+	up_write(&user_ns->keyring_sem);
 	kleave(" = %d", ret);
 	return ret;
 }
 
+/*
+ * Get the user session keyring if it exists, but don't create it if it
+ * doesn't.
+ */
+struct key *get_user_session_keyring(void)
+{
+	const struct cred *cred = current_cred();
+	struct user_namespace *user_ns = current_user_ns();
+	struct key *reg_keyring = user_ns->user_keyring_register;
+	key_ref_t session_keyring_r;
+	char buf[20];
+
+	if (!reg_keyring)
+		return NULL;
+
+	sprintf(buf, "_uid_ses.%u", from_kuid(user_ns, cred->user->uid));
+
+	session_keyring_r = keyring_search(make_key_ref(reg_keyring, true),
+					   &key_type_keyring, buf, false);
+	if (IS_ERR(session_keyring_r))
+		return NULL;
+	return key_ref_to_ptr(session_keyring_r);
+}
+
 /*
  * Install a thread keyring to the given credentials struct if it didn't have
  * one already.  This is allowed to overrun the quota.
@@ -341,6 +414,7 @@ void key_fsgid_changed(struct task_struct *tsk)
  */
 key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
 {
+	struct key *user_session;
 	key_ref_t key_ref, ret, err;
 	const struct cred *cred = ctx->cred;
 
@@ -416,10 +490,9 @@ 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(
-			make_key_ref(READ_ONCE(cred->user->session_keyring), 1),
-			ctx);
+	else if ((user_session = get_user_session_keyring())) {
+		key_ref = keyring_search_aux(make_key_ref(user_session, 1),
+					     ctx);
 		if (!IS_ERR(key_ref))
 			goto found;
 
@@ -435,6 +508,8 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
 			err = key_ref;
 			break;
 		}
+
+		key_put(user_session);
 	}
 
 	/* no key - decide on the error we're going to go for */
@@ -544,7 +619,7 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 					   KEYRING_SEARCH_RECURSE),
 	};
 	struct request_key_auth *rka;
-	struct key *key;
+	struct key *key, *user_session;
 	key_ref_t key_ref, skey_ref;
 	int ret;
 
@@ -593,20 +668,20 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 		if (!ctx.cred->session_keyring) {
 			/* always install a session keyring upon access if one
 			 * doesn't exist yet */
-			ret = install_user_keyrings();
+			ret = look_up_user_keyrings(NULL, &user_session);
 			if (ret < 0)
 				goto error;
 			if (lflags & KEY_LOOKUP_CREATE)
 				ret = join_session_keyring(NULL);
 			else
-				ret = install_session_keyring(
-					ctx.cred->user->session_keyring);
+				ret = install_session_keyring(user_session);
 
+			key_put(user_session);
 			if (ret < 0)
 				goto error;
 			goto reget_creds;
-		} else if (ctx.cred->session_keyring ==
-			   READ_ONCE(ctx.cred->user->session_keyring) &&
+		} else if (test_bit(KEY_FLAG_UID_KEYRING,
+				    &ctx.cred->session_keyring->flags) &&
 			   lflags & KEY_LOOKUP_CREATE) {
 			ret = join_session_keyring(NULL);
 			if (ret < 0)
@@ -620,26 +695,16 @@ key_ref_t lookup_user_key(key_serial_t id, unsigned long lflags,
 		break;
 
 	case KEY_SPEC_USER_KEYRING:
-		if (!READ_ONCE(ctx.cred->user->uid_keyring)) {
-			ret = install_user_keyrings();
-			if (ret < 0)
-				goto error;
-		}
-
-		key = ctx.cred->user->uid_keyring;
-		__key_get(key);
+		ret = look_up_user_keyrings(&key, NULL);
+		if (ret < 0)
+			goto error;
 		key_ref = make_key_ref(key, 1);
 		break;
 
 	case KEY_SPEC_USER_SESSION_KEYRING:
-		if (!READ_ONCE(ctx.cred->user->session_keyring)) {
-			ret = install_user_keyrings();
-			if (ret < 0)
-				goto error;
-		}
-
-		key = ctx.cred->user->session_keyring;
-		__key_get(key);
+		ret = look_up_user_keyrings(NULL, &key);
+		if (ret < 0)
+			goto error;
 		key_ref = make_key_ref(key, 1);
 		break;
 
@@ -888,7 +953,7 @@ void key_change_session_keyring(struct callback_head *twork)
  */
 static int __init init_root_keyring(void)
 {
-	return install_user_keyrings();
+	return look_up_user_keyrings(NULL, NULL);
 }
 
 late_initcall(init_root_keyring);
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 39802540ffff..d95627888f5a 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -96,7 +96,7 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
 	struct request_key_auth *rka = get_request_key_auth(authkey);
 	const struct cred *cred = current_cred();
 	key_serial_t prkey, sskey;
-	struct key *key = rka->target_key, *keyring, *session;
+	struct key *key = rka->target_key, *keyring, *session, *user_session;
 	char *argv[9], *envp[3], uid_str[12], gid_str[12];
 	char key_str[12], keyring_str[3][12];
 	char desc[20];
@@ -104,9 +104,9 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
 
 	kenter("{%d},{%d},%s", key->serial, authkey->serial, rka->op);
 
-	ret = install_user_keyrings();
+	ret = look_up_user_keyrings(NULL, &user_session);
 	if (ret < 0)
-		goto error_alloc;
+		goto error_us;
 
 	/* allocate a new session keyring */
 	sprintf(desc, "_req.%u", key->serial);
@@ -144,7 +144,7 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
 
 	session = cred->session_keyring;
 	if (!session)
-		session = cred->user->session_keyring;
+		session = user_session;
 	sskey = session->serial;
 
 	sprintf(keyring_str[2], "%d", sskey);
@@ -187,6 +187,8 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
 
 error_alloc:
 	complete_request_key(authkey, ret);
+error_us:
+	key_put(user_session);
 	kleave(" = %d", ret);
 	return ret;
 }
@@ -289,16 +291,19 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
 
 			if (dest_keyring)
 				break;
+			/* Fall through */
 
 			/* fall through */
 		case KEY_REQKEY_DEFL_USER_SESSION_KEYRING:
-			dest_keyring =
-				key_get(READ_ONCE(cred->user->session_keyring));
+			ret = look_up_user_keyrings(NULL, &dest_keyring);
+			if (ret < 0)
+				return ret;
 			break;
 
 		case KEY_REQKEY_DEFL_USER_KEYRING:
-			dest_keyring =
-				key_get(READ_ONCE(cred->user->uid_keyring));
+			ret = look_up_user_keyrings(&dest_keyring, NULL);
+			if (ret < 0)
+				return ret;
 			break;
 
 		case KEY_REQKEY_DEFL_GROUP_KEYRING:


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 08/11] keys: Include target namespace in match criteria
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
                   ` (6 preceding siblings ...)
  2019-04-24 16:14 ` [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace David Howells
@ 2019-04-24 16:14 ` David Howells
  2019-04-24 16:14 ` [PATCH 09/11] keys: Garbage collect keys for which the domain has been removed David Howells
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:14 UTC (permalink / raw)
  To: ebiederm
  Cc: keyrings, dhowells, linux-security-module, linux-fsdevel,
	linux-kernel, dwalsh, vgoyal

Currently a key has a standard matching criteria of { type, description } and
this is used to only allow keys with unique criteria in a keyring.  This
means, however, that you cannot have keys with the same type and description
but a different target namespace in the same keyring.

This is a potential problem for a containerised environment where, say, a
container is made up of some parts of its mount space involving netfs
superblocks from two different network namespaces.

This is also a problem for shared system management keyrings such as the DNS
records keyring or the NFS idmapper keyring that might contain keys from
different network namespaces.

Fix this by including a namespace component in a key's matching criteria.
Keyring types are marked to indicate which, if any, namespace is relevant to
keys of that type, and that namespace is set when the key is created from the
current task's namespace set.

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

 include/linux/key.h        |   11 +++++++++++
 security/keys/gc.c         |    2 +-
 security/keys/key.c        |    1 +
 security/keys/keyring.c    |   36 ++++++++++++++++++++++++++++++++++--
 security/keys/persistent.c |    1 +
 5 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 52334cc7c18d..2b298532dad0 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -82,9 +82,16 @@ struct cred;
 
 struct key_type;
 struct key_owner;
+struct key_tag;
 struct keyring_list;
 struct keyring_name;
 
+struct key_tag {
+	struct rcu_head		rcu;
+	refcount_t		usage;
+	bool			removed;	/* T when subject removed */
+};
+
 struct keyring_index_key {
 	/* [!] If this structure is altered, the union in struct key must change too! */
 	unsigned long		hash;			/* Hash value */
@@ -101,6 +108,7 @@ struct keyring_index_key {
 		unsigned long x;
 	};
 	struct key_type		*type;
+	struct key_tag		*domain_tag;	/* Domain of operation */
 	const char		*description;
 };
 
@@ -218,6 +226,7 @@ struct key {
 			unsigned long	hash;
 			unsigned long	len_desc;
 			struct key_type	*type;		/* type of key */
+			struct key_tag	*domain_tag;	/* Domain of operation */
 			char		*description;
 		};
 	};
@@ -268,6 +277,7 @@ extern struct key *key_alloc(struct key_type *type,
 extern void key_revoke(struct key *key);
 extern void key_invalidate(struct key *key);
 extern void key_put(struct key *key);
+extern bool key_put_tag(struct key_tag *tag);
 
 static inline struct key *__key_get(struct key *key)
 {
@@ -428,6 +438,7 @@ extern void key_init(void);
 #define key_fsgid_changed(t)		do { } while(0)
 #define key_init()			do { } while(0)
 #define key_free_user_ns(ns)		do { } while(0)
+#define key_put_subject(s)		do { } while(0)
 
 #endif /* CONFIG_KEYS */
 #endif /* __KERNEL__ */
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 634e96b380e8..83d279fb7793 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -154,7 +154,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 			atomic_dec(&key->user->nikeys);
 
 		key_user_put(key->user);
-
+		key_put_tag(key->domain_tag);
 		kfree(key->description);
 
 		memzero_explicit(key, sizeof(*key));
diff --git a/security/keys/key.c b/security/keys/key.c
index 1568b0028ba4..8cd0f98e4fa3 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -317,6 +317,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 		goto security_error;
 
 	/* publish the key by giving it a serial number */
+	refcount_inc(&key->domain_tag->usage);
 	atomic_inc(&user->nkeys);
 	key_alloc_serial(key);
 
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 1fc9219ed002..0898d6d91d41 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -175,6 +175,9 @@ static void hash_key_type_and_desc(struct keyring_index_key *index_key)
 	type = (unsigned long)index_key->type;
 	acc = mult_64x32_and_fold(type, desc_len + 13);
 	acc = mult_64x32_and_fold(acc, 9207);
+	piece = (unsigned long)index_key->domain_tag;
+	acc = mult_64x32_and_fold(acc, piece);
+	acc = mult_64x32_and_fold(acc, 9207);
 
 	for (;;) {
 		n = desc_len;
@@ -208,16 +211,36 @@ static void hash_key_type_and_desc(struct keyring_index_key *index_key)
 
 /*
  * Finalise an index key to include a part of the description actually in the
- * index key and to add in the hash too.
+ * index key, to set the domain tag and to calculate the hash.
  */
 void key_set_index_key(struct keyring_index_key *index_key)
 {
+	static struct key_tag default_domain_tag = { .usage = REFCOUNT_INIT(1), };
 	size_t n = min_t(size_t, index_key->desc_len, sizeof(index_key->desc));
+
 	memcpy(index_key->desc, index_key->description, n);
 
+	index_key->domain_tag = &default_domain_tag;
 	hash_key_type_and_desc(index_key);
 }
 
+/**
+ * key_put_tag - Release a ref on a tag.
+ * @tag: The tag to release.
+ *
+ * This releases a reference the given tag and returns true if that ref was the
+ * last one.
+ */
+bool key_put_tag(struct key_tag *tag)
+{
+	if (refcount_dec_and_test(&tag->usage)) {
+		kfree_rcu(tag, rcu);
+		return true;
+	}
+
+	return false;
+}
+
 /*
  * Build the next index key chunk.
  *
@@ -238,8 +261,10 @@ static unsigned long keyring_get_key_chunk(const void *data, int level)
 		return index_key->x;
 	case 2:
 		return (unsigned long)index_key->type;
+	case 3:
+		return (unsigned long)index_key->domain_tag;
 	default:
-		level -= 3;
+		level -= 4;
 		if (desc_len <= sizeof(index_key->desc))
 			return 0;
 
@@ -268,6 +293,7 @@ static bool keyring_compare_object(const void *object, const void *data)
 	const struct key *key = keyring_ptr_to_key(object);
 
 	return key->index_key.type == index_key->type &&
+		key->index_key.domain_tag == index_key->domain_tag &&
 		key->index_key.desc_len == index_key->desc_len &&
 		memcmp(key->index_key.description, index_key->description,
 		       index_key->desc_len) == 0;
@@ -309,6 +335,12 @@ static int keyring_diff_objects(const void *object, const void *data)
 		goto differ;
 	level += sizeof(unsigned long);
 
+	seg_a = (unsigned long)a->domain_tag;
+	seg_b = (unsigned long)b->domain_tag;
+	if ((seg_a ^ seg_b) != 0)
+		goto differ;
+	level += sizeof(unsigned long);
+
 	i = sizeof(a->desc);
 	if (a->desc_len <= i)
 		goto same;
diff --git a/security/keys/persistent.c b/security/keys/persistent.c
index 90303fe4a394..9944d855a28d 100644
--- a/security/keys/persistent.c
+++ b/security/keys/persistent.c
@@ -84,6 +84,7 @@ static long key_get_persistent(struct user_namespace *ns, kuid_t uid,
 	long ret;
 
 	/* Look in the register if it exists */
+	memset(&index_key, 0, sizeof(index_key));
 	index_key.type = &key_type_keyring;
 	index_key.description = buf;
 	index_key.desc_len = sprintf(buf, "_persistent.%u", from_kuid(ns, uid));


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 09/11] keys: Garbage collect keys for which the domain has been removed
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
                   ` (7 preceding siblings ...)
  2019-04-24 16:14 ` [PATCH 08/11] keys: Include target namespace in match criteria David Howells
@ 2019-04-24 16:14 ` David Howells
  2019-04-24 16:14 ` [PATCH 10/11] keys: Network namespace domain tag David Howells
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:14 UTC (permalink / raw)
  To: ebiederm
  Cc: keyrings, dhowells, linux-security-module, linux-fsdevel,
	linux-kernel, dwalsh, vgoyal

If a key operation domain (such as a network namespace) has been removed
then attempt to garbage collect all the keys that use it.

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

 include/linux/key.h      |    1 +
 security/keys/internal.h |    3 ++-
 security/keys/keyring.c  |   15 +++++++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 2b298532dad0..e79802d4c928 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -278,6 +278,7 @@ extern void key_revoke(struct key *key);
 extern void key_invalidate(struct key *key);
 extern void key_put(struct key *key);
 extern bool key_put_tag(struct key_tag *tag);
+extern void key_remove_domain(struct key_tag *domain_tag);
 
 static inline struct key *__key_get(struct key *key)
 {
diff --git a/security/keys/internal.h b/security/keys/internal.h
index ea48d8b30794..c07fcc756fdd 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -205,7 +205,8 @@ static inline bool key_is_dead(const struct key *key, time64_t limit)
 	return
 		key->flags & ((1 << KEY_FLAG_DEAD) |
 			      (1 << KEY_FLAG_INVALIDATED)) ||
-		(key->expiry > 0 && key->expiry <= limit);
+		(key->expiry > 0 && key->expiry <= limit) ||
+		key->domain_tag->removed;
 }
 
 /*
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 0898d6d91d41..d2ad27535624 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -241,6 +241,21 @@ bool key_put_tag(struct key_tag *tag)
 	return false;
 }
 
+/**
+ * key_remove_domain - Kill off a key domain and gc its keys
+ * @domain_tag: The domain tag to release.
+ *
+ * This marks a domain tag as being dead and releases a ref on it.  If that
+ * wasn't the last reference, the garbage collector is poked to try and delete
+ * all keys that were in the domain.
+ */
+void key_remove_domain(struct key_tag *domain_tag)
+{
+	domain_tag->removed = true;
+	if (!key_put_tag(domain_tag))
+		key_schedule_gc_links();
+}
+
 /*
  * Build the next index key chunk.
  *


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 10/11] keys: Network namespace domain tag
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
                   ` (8 preceding siblings ...)
  2019-04-24 16:14 ` [PATCH 09/11] keys: Garbage collect keys for which the domain has been removed David Howells
@ 2019-04-24 16:14 ` David Howells
  2019-04-24 16:15 ` [PATCH 11/11] keys: Pass the network namespace into request_key mechanism David Howells
  2019-04-24 21:54 ` [PATCH 10/11] keys: Network namespace domain tag David Howells
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:14 UTC (permalink / raw)
  To: ebiederm
  Cc: netdev, linux-nfs, linux-cifs, linux-afs, keyrings, dhowells,
	linux-security-module, linux-fsdevel, linux-kernel, dwalsh,
	vgoyal

Create key domain tags for network namespaces and make it possible to
automatically tag keys that are used by networked services (e.g. AF_RXRPC,
AFS, DNS) with the default network namespace if not set by the caller.

This allows keys with the same description but in different namespaces to
coexist within a keyring.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: netdev@vger.kernel.org
cc: linux-nfs@vger.kernel.org
cc: linux-cifs@vger.kernel.org
cc: linux-afs@lists.infradead.org
---

 include/linux/key-type.h    |    3 +++
 include/net/net_namespace.h |    4 ++++
 net/core/net_namespace.c    |   18 ++++++++++++++++++
 net/dns_resolver/dns_key.c  |    1 +
 net/rxrpc/key.c             |    2 ++
 security/keys/keyring.c     |    7 ++++++-
 6 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index e49d1de0614e..2148a6bf58f1 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -74,6 +74,9 @@ struct key_type {
 	 */
 	size_t def_datalen;
 
+	unsigned int flags;
+#define KEY_TYPE_NET_DOMAIN	0x00000001 /* Keys of this type have a net namespace domain */
+
 	/* vet a description */
 	int (*vet_description)(const char *description);
 
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index a68ced28d8f4..b96de08a3ace 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -45,6 +45,7 @@ struct net_generic;
 struct uevent_sock;
 struct netns_ipvs;
 struct bpf_prog;
+struct key_subject;
 
 
 #define NETDEV_HASHBITS    8
@@ -70,6 +71,9 @@ struct net {
 						 */
 	struct llist_node	cleanup_list;	/* namespaces on death row */
 
+#ifdef CONFIG_KEYS
+	struct key_tag		*key_domain;	/* Key domain of operation tag */
+#endif
 	struct user_namespace   *user_ns;	/* Owning user namespace */
 	struct ucounts		*ucounts;
 	spinlock_t		nsid_lock;
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 17f36317363d..9c4c3dfe2576 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -38,9 +38,16 @@ EXPORT_SYMBOL_GPL(net_namespace_list);
 DECLARE_RWSEM(net_rwsem);
 EXPORT_SYMBOL_GPL(net_rwsem);
 
+#ifdef CONFIG_KEYS
+static struct key_tag init_net_key_domain = { .usage = REFCOUNT_INIT(1) };
+#endif
+
 struct net init_net = {
 	.count		= REFCOUNT_INIT(1),
 	.dev_base_head	= LIST_HEAD_INIT(init_net.dev_base_head),
+#ifdef CONFIG_KEYS
+	.key_domain	= &init_net_key_domain,
+#endif
 };
 EXPORT_SYMBOL(init_net);
 
@@ -385,10 +392,20 @@ static struct net *net_alloc(void)
 	if (!net)
 		goto out_free;
 
+#ifdef CONFIG_KEYS
+	net->key_domain = kzalloc(sizeof(struct key_tag), GFP_KERNEL);
+	if (!net->key_domain)
+		goto out_free_2;
+#endif
+
 	rcu_assign_pointer(net->gen, ng);
 out:
 	return net;
 
+#ifdef CONFIG_KEYS
+out_free_2:
+	kmem_cache_free(net_cachep, net);
+#endif
 out_free:
 	kfree(ng);
 	goto out;
@@ -565,6 +582,7 @@ static void cleanup_net(struct work_struct *work)
 	list_for_each_entry_safe(net, tmp, &net_exit_list, exit_list) {
 		list_del_init(&net->exit_list);
 		dec_net_namespaces(net->ucounts);
+		key_remove_domain(net->key_domain);
 		put_user_ns(net->user_ns);
 		net_drop_ns(net);
 	}
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index a65d553e730d..3e1a90669006 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -314,6 +314,7 @@ static long dns_resolver_read(const struct key *key,
 
 struct key_type key_type_dns_resolver = {
 	.name		= "dns_resolver",
+	.flags		= KEY_TYPE_NET_DOMAIN,
 	.preparse	= dns_resolver_preparse,
 	.free_preparse	= dns_resolver_free_preparse,
 	.instantiate	= generic_key_instantiate,
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index e7f6b8823eb6..2722189ec273 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -43,6 +43,7 @@ static long rxrpc_read(const struct key *, char __user *, size_t);
  */
 struct key_type key_type_rxrpc = {
 	.name		= "rxrpc",
+	.flags		= KEY_TYPE_NET_DOMAIN,
 	.preparse	= rxrpc_preparse,
 	.free_preparse	= rxrpc_free_preparse,
 	.instantiate	= generic_key_instantiate,
@@ -58,6 +59,7 @@ EXPORT_SYMBOL(key_type_rxrpc);
  */
 struct key_type key_type_rxrpc_s = {
 	.name		= "rxrpc_s",
+	.flags		= KEY_TYPE_NET_DOMAIN,
 	.vet_description = rxrpc_vet_description_s,
 	.preparse	= rxrpc_preparse_s,
 	.free_preparse	= rxrpc_free_preparse_s,
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index d2ad27535624..ffa368594a03 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -17,10 +17,12 @@
 #include <linux/seq_file.h>
 #include <linux/err.h>
 #include <linux/user_namespace.h>
+#include <linux/nsproxy.h>
 #include <keys/keyring-type.h>
 #include <keys/user-type.h>
 #include <linux/assoc_array_priv.h>
 #include <linux/uaccess.h>
+#include <net/net_namespace.h>
 #include "internal.h"
 
 /*
@@ -220,7 +222,10 @@ void key_set_index_key(struct keyring_index_key *index_key)
 
 	memcpy(index_key->desc, index_key->description, n);
 
-	index_key->domain_tag = &default_domain_tag;
+	if (index_key->type->flags & KEY_TYPE_NET_DOMAIN)
+		index_key->domain_tag = current->nsproxy->net_ns->key_domain;
+	else
+		index_key->domain_tag = &default_domain_tag;
 	hash_key_type_and_desc(index_key);
 }
 


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 11/11] keys: Pass the network namespace into request_key mechanism
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
                   ` (9 preceding siblings ...)
  2019-04-24 16:14 ` [PATCH 10/11] keys: Network namespace domain tag David Howells
@ 2019-04-24 16:15 ` David Howells
  2019-04-24 21:54 ` [PATCH 10/11] keys: Network namespace domain tag David Howells
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 16:15 UTC (permalink / raw)
  To: ebiederm
  Cc: netdev, linux-nfs, linux-cifs, linux-afs, keyrings, dhowells,
	linux-security-module, linux-fsdevel, linux-kernel, dwalsh,
	vgoyal

Create a request_key_net() function and use it to pass the network
namespace domain tag into DNS revolver keys and rxrpc/AFS keys so that keys
for different domains can coexist in the same keyring.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: netdev@vger.kernel.org
cc: linux-nfs@vger.kernel.org
cc: linux-cifs@vger.kernel.org
cc: linux-afs@lists.infradead.org
---

 fs/afs/addr_list.c           |    4 +--
 fs/afs/dynroot.c             |    7 +++--
 fs/cifs/dns_resolve.c        |    3 +-
 fs/nfs/dns_resolve.c         |    2 +
 include/linux/dns_resolver.h |    3 +-
 include/linux/key.h          |    6 ++++
 net/ceph/messenger.c         |    3 +-
 net/dns_resolver/dns_query.c |    6 +++-
 net/rxrpc/key.c              |    4 +--
 security/keys/internal.h     |    1 +
 security/keys/keyctl.c       |    2 +
 security/keys/keyring.c      |   11 +++++---
 security/keys/request_key.c  |   58 ++++++++++++++++++++++++++++++++++++++----
 13 files changed, 86 insertions(+), 24 deletions(-)

diff --git a/fs/afs/addr_list.c b/fs/afs/addr_list.c
index 967db336d11a..bf8ddac5f402 100644
--- a/fs/afs/addr_list.c
+++ b/fs/afs/addr_list.c
@@ -250,8 +250,8 @@ struct afs_vlserver_list *afs_dns_query(struct afs_cell *cell, time64_t *_expiry
 
 	_enter("%s", cell->name);
 
-	ret = dns_query("afsdb", cell->name, cell->name_len, "srv=1",
-			&result, _expiry);
+	ret = dns_query(cell->net->net, "afsdb", cell->name, cell->name_len,
+			"srv=1", &result, _expiry);
 	if (ret < 0) {
 		_leave(" = %d [dns]", ret);
 		return ERR_PTR(ret);
diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
index a9ba81ddf154..07d010cd28e2 100644
--- a/fs/afs/dynroot.c
+++ b/fs/afs/dynroot.c
@@ -28,6 +28,7 @@ const struct file_operations afs_dynroot_file_operations = {
 static int afs_probe_cell_name(struct dentry *dentry)
 {
 	struct afs_cell *cell;
+	struct afs_net *net = afs_d2net(dentry);
 	const char *name = dentry->d_name.name;
 	size_t len = dentry->d_name.len;
 	int ret;
@@ -40,13 +41,13 @@ static int afs_probe_cell_name(struct dentry *dentry)
 		len--;
 	}
 
-	cell = afs_lookup_cell_rcu(afs_d2net(dentry), name, len);
+	cell = afs_lookup_cell_rcu(net, name, len);
 	if (!IS_ERR(cell)) {
-		afs_put_cell(afs_d2net(dentry), cell);
+		afs_put_cell(net, cell);
 		return 0;
 	}
 
-	ret = dns_query("afsdb", name, len, "srv=1", NULL, NULL);
+	ret = dns_query(net->net, "afsdb", name, len, "srv=1", NULL, NULL);
 	if (ret == -ENODATA)
 		ret = -EDESTADDRREQ;
 	return ret;
diff --git a/fs/cifs/dns_resolve.c b/fs/cifs/dns_resolve.c
index 7ede7306599f..1239aa1b5d27 100644
--- a/fs/cifs/dns_resolve.c
+++ b/fs/cifs/dns_resolve.c
@@ -77,7 +77,8 @@ dns_resolve_server_name_to_ip(const char *unc, char **ip_addr)
 		goto name_is_IP_address;
 
 	/* Perform the upcall */
-	rc = dns_query(NULL, hostname, len, NULL, ip_addr, NULL);
+	rc = dns_query(current->nsproxy->net_ns, NULL, hostname, len,
+		       NULL, ip_addr, NULL);
 	if (rc < 0)
 		cifs_dbg(FYI, "%s: unable to resolve: %*.*s\n",
 			 __func__, len, len, hostname);
diff --git a/fs/nfs/dns_resolve.c b/fs/nfs/dns_resolve.c
index a7d3df85736d..8611d4b81b0e 100644
--- a/fs/nfs/dns_resolve.c
+++ b/fs/nfs/dns_resolve.c
@@ -22,7 +22,7 @@ ssize_t nfs_dns_resolve_name(struct net *net, char *name, size_t namelen,
 	char *ip_addr = NULL;
 	int ip_len;
 
-	ip_len = dns_query(NULL, name, namelen, NULL, &ip_addr, NULL);
+	ip_len = dns_query(net, NULL, name, namelen, NULL, &ip_addr, NULL);
 	if (ip_len > 0)
 		ret = rpc_pton(net, ip_addr, ip_len, sa, salen);
 	else
diff --git a/include/linux/dns_resolver.h b/include/linux/dns_resolver.h
index 34a744a1bafc..3855395fa3c0 100644
--- a/include/linux/dns_resolver.h
+++ b/include/linux/dns_resolver.h
@@ -26,7 +26,8 @@
 
 #include <uapi/linux/dns_resolver.h>
 
-extern int dns_query(const char *type, const char *name, size_t namelen,
+struct net;
+extern int dns_query(struct net *net, const char *type, const char *name, size_t namelen,
 		     const char *options, char **_result, time64_t *_expiry);
 
 #endif /* _LINUX_DNS_RESOLVER_H */
diff --git a/include/linux/key.h b/include/linux/key.h
index e79802d4c928..5cc4440ccb68 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -36,6 +36,7 @@ typedef int32_t key_serial_t;
 typedef uint32_t key_perm_t;
 
 struct key;
+struct net;
 
 #ifdef CONFIG_KEYS
 
@@ -306,6 +307,11 @@ extern struct key *request_key_with_auxdata(struct key_type *type,
 					    size_t callout_len,
 					    void *aux);
 
+extern struct key *request_key_net(struct key_type *type,
+				   const char *description,
+				   struct net *net,
+				   const char *callout_info);
+
 extern int wait_for_key_construction(struct key *key, bool intr);
 
 extern int key_validate(const struct key *key);
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 7e71b0df1fbc..d4549d42675b 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1885,7 +1885,8 @@ static int ceph_dns_resolve_name(const char *name, size_t namelen,
 		return -EINVAL;
 
 	/* do dns_resolve upcall */
-	ip_len = dns_query(NULL, name, end - name, NULL, &ip_addr, NULL);
+	ip_len = dns_query(current->nsproxy->net_ns,
+			   NULL, name, end - name, NULL, &ip_addr, NULL);
 	if (ip_len > 0)
 		ret = ceph_pton(ip_addr, ip_len, ss, -1, NULL);
 	else
diff --git a/net/dns_resolver/dns_query.c b/net/dns_resolver/dns_query.c
index 76338c38738a..d88ea98da63e 100644
--- a/net/dns_resolver/dns_query.c
+++ b/net/dns_resolver/dns_query.c
@@ -48,6 +48,7 @@
 
 /**
  * dns_query - Query the DNS
+ * @net: The network namespace to operate in.
  * @type: Query type (or NULL for straight host->IP lookup)
  * @name: Name to look up
  * @namelen: Length of name
@@ -68,7 +69,8 @@
  *
  * Returns the size of the result on success, -ve error code otherwise.
  */
-int dns_query(const char *type, const char *name, size_t namelen,
+int dns_query(struct net *net,
+	      const char *type, const char *name, size_t namelen,
 	      const char *options, char **_result, time64_t *_expiry)
 {
 	struct key *rkey;
@@ -122,7 +124,7 @@ int dns_query(const char *type, const char *name, size_t namelen,
 	 * add_key() to preinstall malicious redirections
 	 */
 	saved_cred = override_creds(dns_resolver_cache);
-	rkey = request_key(&key_type_dns_resolver, desc, options);
+	rkey = request_key_net(&key_type_dns_resolver, desc, net, options);
 	revert_creds(saved_cred);
 	kfree(desc);
 	if (IS_ERR(rkey)) {
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 2722189ec273..1cc6b0c6cc42 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -914,7 +914,7 @@ int rxrpc_request_key(struct rxrpc_sock *rx, char __user *optval, int optlen)
 	if (IS_ERR(description))
 		return PTR_ERR(description);
 
-	key = request_key(&key_type_rxrpc, description, NULL);
+	key = request_key_net(&key_type_rxrpc, description, sock_net(&rx->sk), NULL);
 	if (IS_ERR(key)) {
 		kfree(description);
 		_leave(" = %ld", PTR_ERR(key));
@@ -945,7 +945,7 @@ int rxrpc_server_keyring(struct rxrpc_sock *rx, char __user *optval,
 	if (IS_ERR(description))
 		return PTR_ERR(description);
 
-	key = request_key(&key_type_keyring, description, NULL);
+	key = request_key_net(&key_type_keyring, description, sock_net(&rx->sk), NULL);
 	if (IS_ERR(key)) {
 		kfree(description);
 		_leave(" = %ld", PTR_ERR(key));
diff --git a/security/keys/internal.h b/security/keys/internal.h
index c07fcc756fdd..1fc8c283bdf4 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -152,6 +152,7 @@ extern int install_session_keyring_to_cred(struct cred *, struct key *);
 
 extern struct key *request_key_and_link(struct key_type *type,
 					const char *description,
+					struct key_tag *domain_tag,
 					const void *callout_info,
 					size_t callout_len,
 					void *aux,
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index d2023d550bd6..29c4fa71616b 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -210,7 +210,7 @@ SYSCALL_DEFINE4(request_key, const char __user *, _type,
 	}
 
 	/* do the search */
-	key = request_key_and_link(ktype, description, callout_info,
+	key = request_key_and_link(ktype, description, NULL, callout_info,
 				   callout_len, NULL, key_ref_to_ptr(dest_ref),
 				   KEY_ALLOC_IN_QUOTA);
 	if (IS_ERR(key)) {
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index ffa368594a03..2597ae756191 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -222,10 +222,13 @@ void key_set_index_key(struct keyring_index_key *index_key)
 
 	memcpy(index_key->desc, index_key->description, n);
 
-	if (index_key->type->flags & KEY_TYPE_NET_DOMAIN)
-		index_key->domain_tag = current->nsproxy->net_ns->key_domain;
-	else
-		index_key->domain_tag = &default_domain_tag;
+	if (!index_key->domain_tag) {
+		if (index_key->type->flags & KEY_TYPE_NET_DOMAIN)
+			index_key->domain_tag = current->nsproxy->net_ns->key_domain;
+		else
+			index_key->domain_tag = &default_domain_tag;
+	}
+
 	hash_key_type_and_desc(index_key);
 }
 
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index d95627888f5a..4cc27eee9795 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/keyctl.h>
 #include <linux/slab.h>
+#include <net/net_namespace.h>
 #include "internal.h"
 #include <keys/request_key_auth-type.h>
 
@@ -502,16 +503,18 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
  * request_key_and_link - Request a key and cache it in a keyring.
  * @type: The type of key we want.
  * @description: The searchable description of the key.
+ * @domain_tag: The domain in which the key operates.
  * @callout_info: The data to pass to the instantiation upcall (or NULL).
  * @callout_len: The length of callout_info.
  * @aux: Auxiliary data for the upcall.
  * @dest_keyring: Where to cache the key.
  * @flags: Flags to key_alloc().
  *
- * A key matching the specified criteria is searched for in the process's
- * keyrings and returned with its usage count incremented if found.  Otherwise,
- * if callout_info is not NULL, a key will be allocated and some service
- * (probably in userspace) will be asked to instantiate it.
+ * A key matching the specified criteria (type, description, domain_tag) is
+ * searched for in the process's keyrings and returned with its usage count
+ * incremented if found.  Otherwise, if callout_info is not NULL, a key will be
+ * allocated and some service (probably in userspace) will be asked to
+ * instantiate it.
  *
  * If successfully found or created, the key will be linked to the destination
  * keyring if one is provided.
@@ -527,6 +530,7 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx,
  */
 struct key *request_key_and_link(struct key_type *type,
 				 const char *description,
+				 struct key_tag *domain_tag,
 				 const void *callout_info,
 				 size_t callout_len,
 				 void *aux,
@@ -645,7 +649,8 @@ struct key *request_key(struct key_type *type,
 
 	if (callout_info)
 		callout_len = strlen(callout_info);
-	key = request_key_and_link(type, description, callout_info, callout_len,
+	key = request_key_and_link(type, description, NULL,
+				   callout_info, callout_len,
 				   NULL, NULL, KEY_ALLOC_IN_QUOTA);
 	if (!IS_ERR(key)) {
 		ret = wait_for_key_construction(key, false);
@@ -681,7 +686,8 @@ struct key *request_key_with_auxdata(struct key_type *type,
 	struct key *key;
 	int ret;
 
-	key = request_key_and_link(type, description, callout_info, callout_len,
+	key = request_key_and_link(type, description, NULL,
+				   callout_info, callout_len,
 				   aux, NULL, KEY_ALLOC_IN_QUOTA);
 	if (!IS_ERR(key)) {
 		ret = wait_for_key_construction(key, false);
@@ -693,3 +699,43 @@ struct key *request_key_with_auxdata(struct key_type *type,
 	return key;
 }
 EXPORT_SYMBOL(request_key_with_auxdata);
+
+/**
+ * request_key_net - Request a key for a net namespace and wait for construction
+ * @type: Type of key.
+ * @description: The searchable description of the key.
+ * @net: The network namespace that is the key's domain of operation.
+ * @callout_info: The data to pass to the instantiation upcall (or NULL).
+ *
+ * As for request_key() except that it does not add the returned key to a
+ * keyring if found, new keys are always allocated in the user's quota, the
+ * callout_info must be a NUL-terminated string and no auxiliary data can be
+ * passed.  Only keys that operate the specified network namespace are used.
+ *
+ * Furthermore, it then works as wait_for_key_construction() to wait for the
+ * completion of keys undergoing construction with a non-interruptible wait.
+ */
+struct key *request_key_net(struct key_type *type,
+			    const char *description,
+			    struct net *net,
+			    const char *callout_info)
+{
+	struct key *key;
+	size_t callout_len = 0;
+	int ret;
+
+	if (callout_info)
+		callout_len = strlen(callout_info);
+	key = request_key_and_link(type, description, net->key_domain,
+				   callout_info, callout_len,
+				   NULL, NULL, KEY_ALLOC_IN_QUOTA);
+	if (!IS_ERR(key)) {
+		ret = wait_for_key_construction(key, false);
+		if (ret < 0) {
+			key_put(key);
+			return ERR_PTR(ret);
+		}
+	}
+	return key;
+}
+EXPORT_SYMBOL(request_key_net);


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 10/11] keys: Network namespace domain tag
  2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
                   ` (10 preceding siblings ...)
  2019-04-24 16:15 ` [PATCH 11/11] keys: Pass the network namespace into request_key mechanism David Howells
@ 2019-04-24 21:54 ` David Howells
  11 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 21:54 UTC (permalink / raw)
  Cc: dhowells, ebiederm, netdev, linux-nfs, linux-cifs, linux-afs,
	keyrings, linux-security-module, linux-fsdevel, linux-kernel,
	dwalsh, vgoyal

David Howells <dhowells@redhat.com> wrote:

> @@ -385,10 +392,20 @@ static struct net *net_alloc(void)
>  	if (!net)
>  		goto out_free;
>  
> +#ifdef CONFIG_KEYS
> +	net->key_domain = kzalloc(sizeof(struct key_tag), GFP_KERNEL);
> +	if (!net->key_domain)
> +		goto out_free_2;
> +#endif

Initialisation of net->key_domain->usage to 1 is needed here.  I've fixed that
and repushed the patches to the branch.

David

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace
  2019-04-24 16:14 ` [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace David Howells
@ 2019-04-24 22:03   ` Jann Horn
  2019-04-24 22:24   ` David Howells
  2019-04-25 11:38   ` David Howells
  2 siblings, 0 replies; 18+ messages in thread
From: Jann Horn @ 2019-04-24 22:03 UTC (permalink / raw)
  To: David Howells
  Cc: Eric W. Biederman, keyrings, linux-security-module,
	linux-fsdevel, kernel list, dwalsh, vgoyal

On Wed, Apr 24, 2019 at 6:14 PM David Howells <dhowells@redhat.com> wrote:
> Move the user and user-session keyrings to the user_namespace struct rather
> than pinning them from the user_struct struct.  This prevents these
> keyrings from propagating across user-namespaces boundaries with regard to
> the KEY_SPEC_* flags, thereby making them more useful in a containerised
> environment.
>
> The issue is that a single user_struct may be represent UIDs in several
> different namespaces.
>
> The way the patch does this is by attaching a 'register keyring' in each
> user_namespace and then sticking the user and user-session keyrings into
> that.  It can then be searched to retrieve them.

Overall, this looks good to me, apart from some details.

The user_keyring_register keyring is basically just used like an
xarray/idr/... that maps from namespaced UIDs to keyrings, right? (Not
saying it's a bad idea, just want to make sure I understand it
correctly.)

> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index 90457015fa3f..44a5a4a8a269 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -67,12 +67,13 @@ struct user_namespace {
>  #ifdef CONFIG_KEYS
>         /* List of joinable keyrings in this namespace */
>         struct list_head        keyring_name_list;
> +       struct key              *user_keyring_register;

Maybe a comment about locking semantics above user_keyring_register?
"Only written once, may be read locklessly with READ_ONCE()", or
something like that?

> +       struct rw_semaphore     keyring_sem;
>  #endif
[...]
> diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
> index 70fa20888ca8..ff62d90415ae 100644
> --- a/security/keys/process_keys.c
> +++ b/security/keys/process_keys.c
> @@ -8,7 +8,7 @@
>   * as published by the Free Software Foundation; either version
>   * 2 of the License, or (at your option) any later version.
>   */
> -
> +#define __KDEBUG

Was that supposed to be in here, or did you commit that accidentally?

[...]
> @@ -39,98 +37,173 @@ struct key_user root_key_user = {
>  };
>
>  /*
> - * Install the user and user session keyrings for the current process's UID.
> + * Get or create a user register keyring.
>   */
> -int install_user_keyrings(void)
> +static struct key *get_user_register(struct user_namespace *user_ns)
>  {
> -       struct user_struct *user;
> -       const struct cred *cred;
> -       struct key *uid_keyring, *session_keyring;
> +       struct key *reg_keyring = user_ns->user_keyring_register;

This is a lockless read of a field that may be written concurrently;
this should be READ_ONCE(). (Especially on alpha, I think the memory
ordering will actually be incorrect without READ_ONCE().)

> +       if (reg_keyring)
> +               return reg_keyring;
> +
> +       down_write(&user_ns->keyring_sem);
> +
> +       /* Make sure there's a register keyring.  It gets owned by the
> +        * user_namespace's owner.
> +        */
> +       reg_keyring = user_ns->user_keyring_register;
> +       if (!reg_keyring) {
> +               reg_keyring = keyring_alloc(".user_reg", user_ns->owner, INVALID_GID,
> +                                           &init_cred,
> +                                           KEY_POS_WRITE | KEY_POS_SEARCH |
> +                                           KEY_USR_VIEW | KEY_USR_READ,
> +                                           0,
> +                                           NULL, NULL);
> +               if (!IS_ERR(reg_keyring))
> +                       user_ns->user_keyring_register = reg_keyring;

This is a write of a pointer that may be read concurrently; this
should be smp_store_release().

> +       }
> +
> +       up_write(&user_ns->keyring_sem);
> +
> +       /* We don't return a ref since the keyring is pinned by the user_ns */
> +       return reg_keyring;
> +}
> +
> +/*
> + * Look up the user and user session keyrings for the current process's UID,
> + * creating them if they don't exist.
> + */
> +int look_up_user_keyrings(struct key **_user_keyring,
> +                         struct key **_user_session_keyring)
> +{
> +       const struct cred *cred = current_cred();
> +       struct user_namespace *user_ns = current_user_ns();
> +       struct key *reg_keyring, *uid_keyring, *session_keyring;
>         key_perm_t user_keyring_perm;
> +       key_ref_t uid_keyring_r, session_keyring_r;
> +       uid_t uid = from_kuid(user_ns, cred->user->uid);
>         char buf[20];
>         int ret;
> -       uid_t uid;
>
>         user_keyring_perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL;
> -       cred = current_cred();
> -       user = cred->user;
> -       uid = from_kuid(cred->user_ns, user->uid);
>
> -       kenter("%p{%u}", user, uid);
> +       kenter("%u", uid);
>
> -       if (READ_ONCE(user->uid_keyring) && READ_ONCE(user->session_keyring)) {
> -               kleave(" = 0 [exist]");
> -               return 0;
> -       }
> +       reg_keyring = get_user_register(user_ns);
> +       if (IS_ERR(reg_keyring))
> +               return PTR_ERR(reg_keyring);
>
> -       mutex_lock(&key_user_keyring_mutex);
> +       down_write(&user_ns->keyring_sem);
>         ret = 0;
>
> -       if (!user->uid_keyring) {
> -               /* get the UID-specific keyring
> -                * - there may be one in existence already as it may have been
> -                *   pinned by a session, but the user_struct pointing to it
> -                *   may have been destroyed by setuid */
> -               sprintf(buf, "_uid.%u", uid);
> -
> -               uid_keyring = find_keyring_by_name(buf, true);
> +       /* Get the user keyring.  Note that there may be one in existence
> +        * already as it may have been pinned by a session, but the user_struct
> +        * pointing to it may have been destroyed by setuid.
> +        */
> +       sprintf(buf, "_uid.%u", uid);

I know that the sprintf() calls here and below can't overrun the
buffer, but it'd be nice if you could use "snprintf(buf, sizeof(buf),
...)" anyway.

[...]
>  }
>
> +/*
> + * Get the user session keyring if it exists, but don't create it if it
> + * doesn't.
> + */
> +struct key *get_user_session_keyring(void)
> +{
> +       const struct cred *cred = current_cred();
> +       struct user_namespace *user_ns = current_user_ns();
> +       struct key *reg_keyring = user_ns->user_keyring_register;

(READ_ONCE() again)

> +       key_ref_t session_keyring_r;
> +       char buf[20];
> +
> +       if (!reg_keyring)
> +               return NULL;
> +
> +       sprintf(buf, "_uid_ses.%u", from_kuid(user_ns, cred->user->uid));
> +
> +       session_keyring_r = keyring_search(make_key_ref(reg_keyring, true),
> +                                          &key_type_keyring, buf, false);
> +       if (IS_ERR(session_keyring_r))
> +               return NULL;
> +       return key_ref_to_ptr(session_keyring_r);
> +}
[...]
> @@ -416,10 +490,9 @@ 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(
> -                       make_key_ref(READ_ONCE(cred->user->session_keyring), 1),
> -                       ctx);
> +       else if ((user_session = get_user_session_keyring())) {
> +               key_ref = keyring_search_aux(make_key_ref(user_session, 1),
> +                                            ctx);
>                 if (!IS_ERR(key_ref))
>                         goto found;

I'm not sure I understand this code. In the "goto found" case, the
key_put() below is skipped, right? Is that intentional?

>
> @@ -435,6 +508,8 @@ key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx)
>                         err = key_ref;
>                         break;
>                 }
> +
> +               key_put(user_session);
>         }
>
>         /* no key - decide on the error we're going to go for */
[...]
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 39802540ffff..d95627888f5a 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -96,7 +96,7 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
>         struct request_key_auth *rka = get_request_key_auth(authkey);
>         const struct cred *cred = current_cred();
>         key_serial_t prkey, sskey;
> -       struct key *key = rka->target_key, *keyring, *session;
> +       struct key *key = rka->target_key, *keyring, *session, *user_session;
>         char *argv[9], *envp[3], uid_str[12], gid_str[12];
>         char key_str[12], keyring_str[3][12];
>         char desc[20];
> @@ -104,9 +104,9 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
>
>         kenter("{%d},{%d},%s", key->serial, authkey->serial, rka->op);
>
> -       ret = install_user_keyrings();
> +       ret = look_up_user_keyrings(NULL, &user_session);
>         if (ret < 0)
> -               goto error_alloc;
> +               goto error_us;
>
>         /* allocate a new session keyring */
>         sprintf(desc, "_req.%u", key->serial);
> @@ -144,7 +144,7 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
>
>         session = cred->session_keyring;
>         if (!session)
> -               session = cred->user->session_keyring;
> +               session = user_session;
>         sskey = session->serial;
>
>         sprintf(keyring_str[2], "%d", sskey);
> @@ -187,6 +187,8 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
>
>  error_alloc:
>         complete_request_key(authkey, ret);
> +error_us:
> +       key_put(user_session);
>         kleave(" = %d", ret);
>         return ret;
>  }

This looks weird. If the look_up_user_keyrings() fails, user_session
might still be an uninitialized pointer, right? And then the "goto
error_us" jumps down here and calls key_put() on that?

> @@ -289,16 +291,19 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
>
>                         if (dest_keyring)
>                                 break;
> +                       /* Fall through */
>
>                         /* fall through */
>                 case KEY_REQKEY_DEFL_USER_SESSION_KEYRING:

Why two "fall through" comments?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace
  2019-04-24 16:14 ` [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace David Howells
  2019-04-24 22:03   ` Jann Horn
@ 2019-04-24 22:24   ` David Howells
  2019-04-25 11:38   ` David Howells
  2 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-24 22:24 UTC (permalink / raw)
  To: Jann Horn
  Cc: dhowells, Eric W. Biederman, keyrings, linux-security-module,
	linux-fsdevel, kernel list, dwalsh, vgoyal

Jann Horn <jannh@google.com> wrote:

> Overall, this looks good to me, apart from some details.
> 
> The user_keyring_register keyring is basically just used like an
> xarray/idr/... that maps from namespaced UIDs to keyrings, right? (Not
> saying it's a bad idea, just want to make sure I understand it
> correctly.)

Well, a keyring is a wrapper around an assoc_array object, the keyring search
functions do the access checks and the keys garbage collector does the
cleanup.  Also, each UID is mapped to two keyrings.

I'll have a look at applying the rest of your comments tomorrow.

Thanks,
David

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 05/11] keys: Add a 'recurse' flag for keyring searches
  2019-04-24 16:14 ` [PATCH 05/11] keys: Add a 'recurse' flag for keyring searches David Howells
@ 2019-04-25  4:27   ` Andrew Zaborowski
  2019-04-25 11:02   ` David Howells
  1 sibling, 0 replies; 18+ messages in thread
From: Andrew Zaborowski @ 2019-04-25  4:27 UTC (permalink / raw)
  To: David Howells
  Cc: ebiederm, keyrings, linux-security-module, linux-fsdevel,
	linux-kernel, dwalsh, vgoyal

On Wed, 24 Apr 2019 at 18:14, David Howells <dhowells@redhat.com> wrote:
> Add a 'recurse' flag for keyring searches so that the flag can be omitted
> and recursion disabled, thereby allowing just the nominated keyring to be
> searched and none of the children.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  Documentation/security/keys/core.rst     |   10 ++++++----
>  certs/blacklist.c                        |    2 +-
>  crypto/asymmetric_keys/asymmetric_type.c |    2 +-
>  include/linux/key.h                      |    3 ++-
>  lib/digsig.c                             |    2 +-
>  net/rxrpc/security.c                     |    2 +-
>  security/integrity/digsig_asymmetric.c   |    4 ++--
>  security/keys/internal.h                 |    1 +
>  security/keys/keyctl.c                   |    2 +-
>  security/keys/keyring.c                  |   12 ++++++++++--
>  security/keys/proc.c                     |    3 ++-
>  security/keys/process_keys.c             |    3 ++-
>  security/keys/request_key.c              |    3 ++-
>  security/keys/request_key_auth.c         |    3 ++-
>  14 files changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
> index 9521c4207f01..99079b664036 100644
> --- a/Documentation/security/keys/core.rst
> +++ b/Documentation/security/keys/core.rst
> @@ -1159,11 +1159,13 @@ payload contents" for more information.
>
>         key_ref_t keyring_search(key_ref_t keyring_ref,
>                                  const struct key_type *type,
> -                                const char *description)
> +                                const char *description,
> +                                bool recurse)
>
> -    This searches the keyring tree specified for a matching key. Error ENOKEY
> -    is returned upon failure (use IS_ERR/PTR_ERR to determine). If successful,
> -    the returned key will need to be released.
> +    This searches the specified keyring only (recurse == false) or keyring tree
> +    (recurse == true) specified for a matching key. Error ENOKEY is returned
> +    upon failure (use IS_ERR/PTR_ERR to determine). If successful, the returned
> +    key will need to be released.
>
>      The possession attribute from the keyring reference is used to control
>      access through the permissions mask and is propagated to the returned key
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 3a507b9e2568..181cb7fa9540 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -128,7 +128,7 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
>         *p = 0;
>
>         kref = keyring_search(make_key_ref(blacklist_keyring, true),
> -                             &key_type_blacklist, buffer);
> +                             &key_type_blacklist, buffer, false);
>         if (!IS_ERR(kref)) {
>                 key_ref_put(kref);
>                 ret = -EKEYREJECTED;
> diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
> index 69a0788a7de5..084027ef3121 100644
> --- a/crypto/asymmetric_keys/asymmetric_type.c
> +++ b/crypto/asymmetric_keys/asymmetric_type.c
> @@ -87,7 +87,7 @@ struct key *find_asymmetric_key(struct key *keyring,
>         pr_debug("Look up: \"%s\"\n", req);
>
>         ref = keyring_search(make_key_ref(keyring, 1),
> -                            &key_type_asymmetric, req);
> +                            &key_type_asymmetric, req, true);
>         if (IS_ERR(ref))
>                 pr_debug("Request for key '%s' err %ld\n", req, PTR_ERR(ref));
>         kfree(req);
> diff --git a/include/linux/key.h b/include/linux/key.h
> index b39f5876b66d..bdd179169508 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -333,7 +333,8 @@ extern int keyring_clear(struct key *keyring);
>
>  extern key_ref_t keyring_search(key_ref_t keyring,
>                                 struct key_type *type,
> -                               const char *description);
> +                               const char *description,
> +                               bool no_recurse);

No functional difference but it's "recurse" everywhere else.

Best regards

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 05/11] keys: Add a 'recurse' flag for keyring searches
  2019-04-24 16:14 ` [PATCH 05/11] keys: Add a 'recurse' flag for keyring searches David Howells
  2019-04-25  4:27   ` Andrew Zaborowski
@ 2019-04-25 11:02   ` David Howells
  1 sibling, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-25 11:02 UTC (permalink / raw)
  To: Andrew Zaborowski
  Cc: dhowells, ebiederm, keyrings, linux-security-module,
	linux-fsdevel, linux-kernel, dwalsh, vgoyal

Andrew Zaborowski <andrew.zaborowski@intel.com> wrote:

> >  extern key_ref_t keyring_search(key_ref_t keyring,
> >                                 struct key_type *type,
> > -                               const char *description);
> > +                               const char *description,
> > +                               bool no_recurse);
> 
> No functional difference but it's "recurse" everywhere else.

Fixed, thanks.

David

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace
  2019-04-24 16:14 ` [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace David Howells
  2019-04-24 22:03   ` Jann Horn
  2019-04-24 22:24   ` David Howells
@ 2019-04-25 11:38   ` David Howells
  2 siblings, 0 replies; 18+ messages in thread
From: David Howells @ 2019-04-25 11:38 UTC (permalink / raw)
  To: Jann Horn
  Cc: dhowells, Eric W. Biederman, keyrings, linux-security-module,
	linux-fsdevel, kernel list, dwalsh, vgoyal

Jann Horn <jannh@google.com> wrote:

> > +       struct key              *user_keyring_register;
> 
> Maybe a comment about locking semantics above user_keyring_register?
> "Only written once, may be read locklessly with READ_ONCE()", or
> something like that?

Ok.

> > -
> > +#define __KDEBUG
> 
> Was that supposed to be in here, or did you commit that accidentally?

Accidental.

> > -       struct key *uid_keyring, *session_keyring;
> > +       struct key *reg_keyring = user_ns->user_keyring_register;
> 
> This is a lockless read of a field that may be written concurrently;
> this should be READ_ONCE(). (Especially on alpha, I think the memory
> ordering will actually be incorrect without READ_ONCE().)

Yeah, you're right about both of these that you pointed out.  It's not needed
when the user_ns->keyring_sem is taken for writing, however.

> > +               if (!IS_ERR(reg_keyring))
> > +                       user_ns->user_keyring_register = reg_keyring;
> 
> This is a write of a pointer that may be read concurrently; this
> should be smp_store_release().

Yep.

> > +       else if ((user_session = get_user_session_keyring())) {
> > +               key_ref = keyring_search_aux(make_key_ref(user_session, 1),
> > +                                            ctx);
> >                 if (!IS_ERR(key_ref))
> >                         goto found;
> 
> I'm not sure I understand this code. In the "goto found" case, the
> key_put() below is skipped, right? Is that intentional?

Actually, the key_put() should be directly after the keyring_search_aux()
call, before the error check.

> >  error_alloc:
> >         complete_request_key(authkey, ret);
> > +error_us:
> > +       key_put(user_session);
> >         kleave(" = %d", ret);
> >         return ret;
> >  }
> 
> This looks weird. If the look_up_user_keyrings() fails, user_session
> might still be an uninitialized pointer, right? And then the "goto
> error_us" jumps down here and calls key_put() on that?

The call to complete_request_key() should be after error_us and the key_put()
should be before it.

> > @@ -289,16 +291,19 @@ static int construct_get_dest_keyring(struct key **_dest_keyring)
> >
> >                         if (dest_keyring)
> >                                 break;
> > +                       /* Fall through */
> >
> >                         /* fall through */
> >                 case KEY_REQKEY_DEFL_USER_SESSION_KEYRING:
> 
> Why two "fall through" comments?

Someone else added one and when I rebased, I don't think I got a conflict.

David

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-04-25 11:38 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-24 16:13 [PATCH 00/11] keys: Namespacing David Howells
2019-04-24 16:13 ` [PATCH 01/11] keys: Invalidate used request_key authentication keys David Howells
2019-04-24 16:13 ` [PATCH 02/11] keys: Kill off request_key_async{,_with_auxdata} David Howells
2019-04-24 16:13 ` [PATCH 03/11] keys: Simplify key description management David Howells
2019-04-24 16:14 ` [PATCH 04/11] keys: Cache the hash value to avoid lots of recalculation David Howells
2019-04-24 16:14 ` [PATCH 05/11] keys: Add a 'recurse' flag for keyring searches David Howells
2019-04-25  4:27   ` Andrew Zaborowski
2019-04-25 11:02   ` David Howells
2019-04-24 16:14 ` [PATCH 06/11] keys: Namespace keyring names David Howells
2019-04-24 16:14 ` [PATCH 07/11] keys: Move the user and user-session keyrings to the user_namespace David Howells
2019-04-24 22:03   ` Jann Horn
2019-04-24 22:24   ` David Howells
2019-04-25 11:38   ` David Howells
2019-04-24 16:14 ` [PATCH 08/11] keys: Include target namespace in match criteria David Howells
2019-04-24 16:14 ` [PATCH 09/11] keys: Garbage collect keys for which the domain has been removed David Howells
2019-04-24 16:14 ` [PATCH 10/11] keys: Network namespace domain tag David Howells
2019-04-24 16:15 ` [PATCH 11/11] keys: Pass the network namespace into request_key mechanism David Howells
2019-04-24 21:54 ` [PATCH 10/11] keys: Network namespace domain tag David Howells

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).