Skip to contentSkip to conversation

KEYS: fix error handling in request_key_auth_new()

PATCH In review Eric Biggers 2 participants 6 messages Sep 21, 2017 – Sep 25, 2017

Subsystems (2)
Security (LSM) Keys/keyrings
lore
Share & tools
lore

Eric Biggers

author
#0 lore raw

v2 cover KEYS: fix error handling in request_key_auth_new()

From: Eric Biggers <[email protected]>

Fix a reference leak and a NULL pointer dereference in the error handling paths of request_key_auth_new().

Eric Biggers (3):
KEYS: fix cred refcount leak in request_key_auth_new()
KEYS: don't revoke uninstantiated key in request_key_auth_new()
KEYS: use kmemdup() in request_key_auth_new()

 security/keys/request_key_auth.c | 73 ++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 41 deletions(-)
Signature · 1 line
2.14.1.821.g8fa685d3b7-goog
author
#1 lore raw

PATCH v2 1/3 KEYS: fix cred refcount leak in request_key_auth_new()

From: Eric Biggers <[email protected]>

In request_key_auth_new(), if key_alloc() or key_instantiate_and_link() were to fail, we would leak a reference to the 'struct cred'. Currently this can only happen if key_alloc() fails to allocate memory. But it still should be fixed, as it is a more severe bug waiting to happen.

Fix it by cleaning things up to use a helper function which frees a 'struct request_key_auth' correctly.

Fixes d84f4f992cbd ("CRED: Inaugurate COW credentials")
Signed-off-by Eric Biggers <[email protected]>
---
 security/keys/request_key_auth.c | 67 ++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 37 deletions(-)
security/keys/request_key_auth.c +30 -37
index afe9d22ab361..8f12645945ca 100644
@@ -120,6 +120,18 @@ static void request_key_auth_revoke(struct key *key)
 	}
 }
 
+static void free_request_key_auth(struct request_key_auth *rka)
+{
+	if (!rka)
+		return;
+	key_put(rka->target_key);
+	key_put(rka->dest_keyring);
+	if (rka->cred)
+		put_cred(rka->cred);
+	kfree(rka->callout_info);
+	kfree(rka);
+}
+
 /*
  * Destroy an instantiation authorisation token key.
  */

@@ -129,15 +141,7 @@ static void request_key_auth_destroy(struct key *key)
 
 	kenter("{%d}", key->serial);
 
-	if (rka->cred) {
-		put_cred(rka->cred);
-		rka->cred = NULL;
-	}
-
-	key_put(rka->target_key);
-	key_put(rka->dest_keyring);
-	kfree(rka->callout_info);
-	kfree(rka);
+	free_request_key_auth(rka);
 }
 
 /*

@@ -151,22 +155,17 @@ struct key *request_key_auth_new(struct key *target, const void *callout_info,
 	const struct cred *cred = current->cred;
 	struct key *authkey = NULL;
 	char desc[20];
-	int ret;
+	int ret = -ENOMEM;
 
 	kenter("%d,", target->serial);
 
 	/* allocate a auth record */
-	rka = kmalloc(sizeof(*rka), GFP_KERNEL);
-	if (!rka) {
-		kleave(" = -ENOMEM");
-		return ERR_PTR(-ENOMEM);
-	}
+	rka = kzalloc(sizeof(*rka), GFP_KERNEL);
+	if (!rka)
+		goto error_free_rka;
 	rka->callout_info = kmalloc(callout_len, GFP_KERNEL);
-	if (!rka->callout_info) {
-		kleave(" = -ENOMEM");
-		kfree(rka);
-		return ERR_PTR(-ENOMEM);
-	}
+	if (!rka->callout_info)
+		goto error_free_rka;
 
 	/* see if the calling process is already servicing the key request of
 	 * another process */

@@ -176,8 +175,12 @@ struct key *request_key_auth_new(struct key *target, const void *callout_info,
 
 		/* if the auth key has been revoked, then the key we're
 		 * servicing is already instantiated */
-		if (test_bit(KEY_FLAG_REVOKED, &cred->request_key_auth->flags))
-			goto auth_key_revoked;
+		if (test_bit(KEY_FLAG_REVOKED,
+			     &cred->request_key_auth->flags)) {
+			up_read(&cred->request_key_auth->sem);
+			ret = -EKEYREVOKED;
+			goto error_free_rka;
+		}
 
 		irka = cred->request_key_auth->payload.data[0];
 		rka->cred = get_cred(irka->cred);

@@ -205,32 +208,22 @@ struct key *request_key_auth_new(struct key *target, const void *callout_info,
 			    KEY_USR_VIEW, KEY_ALLOC_NOT_IN_QUOTA, NULL);
 	if (IS_ERR(authkey)) {
 		ret = PTR_ERR(authkey);
-		goto error_alloc;
+		goto error_free_rka;
 	}
 
 	/* construct the auth key */
 	ret = key_instantiate_and_link(authkey, rka, 0, NULL, NULL);
 	if (ret < 0)
-		goto error_inst;
+		goto error_put_authkey;
 
 	kleave(" = {%d,%d}", authkey->serial, refcount_read(&authkey->usage));
 	return authkey;
 
-auth_key_revoked:
-	up_read(&cred->request_key_auth->sem);
-	kfree(rka->callout_info);
-	kfree(rka);
-	kleave("= -EKEYREVOKED");
-	return ERR_PTR(-EKEYREVOKED);
-
-error_inst:
+error_put_authkey:
 	key_revoke(authkey);
 	key_put(authkey);
-error_alloc:
-	key_put(rka->target_key);
-	key_put(rka->dest_keyring);
-	kfree(rka->callout_info);
-	kfree(rka);
+error_free_rka:
+	free_request_key_auth(rka);
 	kleave("= %d", ret);
 	return ERR_PTR(ret);
 }
Signature · 1 line
2.14.1.821.g8fa685d3b7-goog
maintainer
#2 lore raw

Re: [PATCH v2 1/3] KEYS: fix cred refcount leak in request_key_auth_new()

security/keys/request_key_auth.c struct key *request_key_auth_new(struct key *target, const void *callout_info, · line 165

Quoted 3 lines · Eric Biggers
security/keys/request_key_auth.c +3 -0
+	rka = kzalloc(sizeof(*rka), GFP_KERNEL);
+	if (!rka)
+		goto error_free_rka;

But you haven't allocated an rka if you're in the error path. I'll fix that to jump over the release.

David

author
#3 lore raw

PATCH v2 2/3 KEYS: don't revoke uninstantiated key in request_key_auth_new()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

If key_instantiate_and_link() were to fail (which fortunately isn't possible currently), the call to key_revoke(authkey) would crash with a NULL pointer dereference in request_key_auth_revoke() because the key has not yet been instantiated.

Fix this by removing the call to key_revoke(). key_put() is sufficient, as it's not possible for an uninstantiated authkey to have been used for anything yet.

Fixes b5f545c880a2 ("[PATCH] keys: Permit running process to instantiate keys")
Signed-off-by Eric Biggers <[email protected]>
---
 security/keys/request_key_auth.c | 1 -
 1 file changed, 1 deletion(-)
security/keys/request_key_auth.c +0 -1
index 8f12645945ca..2df92b619a38 100644
@@ -220,7 +220,6 @@ struct key *request_key_auth_new(struct key *target, const void *callout_info,
 	return authkey;
 
 error_put_authkey:
-	key_revoke(authkey);
 	key_put(authkey);
 error_free_rka:
 	free_request_key_auth(rka);
Signature · 1 line
2.14.1.821.g8fa685d3b7-goog
maintainer
#4 lore raw

Re: [PATCH v2 2/3] KEYS: don't revoke uninstantiated key in request_key_auth_new()

Quoted 3 lines · Eric Biggers

Fix this by removing the call to key_revoke(). key_put() is sufficient, as it's not possible for an uninstantiated authkey to have been used for anything yet.

Note that the owner can call keyctl_describe() upon it, though that shouldn't wait because KEY_LOOKUP_PARTIAL would be specified.

However, this is probably fair enough, and it should be gc'd quickly because it'll lose its last ref.

David

author
#5 lore raw

PATCH v2 3/3 KEYS: use kmemdup() in request_key_auth_new()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

kmemdup() is preferred to kmalloc() followed by memcpy().

Signed-off-by Eric Biggers <[email protected]>
---
 security/keys/request_key_auth.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
security/keys/request_key_auth.c +2 -3
index 2df92b619a38..9d536f9e5222 100644
@@ -163,9 +163,10 @@ struct key *request_key_auth_new(struct key *target, const void *callout_info,
 	rka = kzalloc(sizeof(*rka), GFP_KERNEL);
 	if (!rka)
 		goto error_free_rka;
-	rka->callout_info = kmalloc(callout_len, GFP_KERNEL);
+	rka->callout_info = kmemdup(callout_info, callout_len, GFP_KERNEL);
 	if (!rka->callout_info)
 		goto error_free_rka;
+	rka->callout_len = callout_len;
 
 	/* see if the calling process is already servicing the key request of
 	 * another process */

@@ -196,8 +197,6 @@ struct key *request_key_auth_new(struct key *target, const void *callout_info,
 
 	rka->target_key = key_get(target);
 	rka->dest_keyring = key_get(dest_keyring);
-	memcpy(rka->callout_info, callout_info, callout_len);
-	rka->callout_len = callout_len;
 
 	/* allocate the auth key */
 	sprintf(desc, "%x", target->serial);
Signature · 1 line
2.14.1.821.g8fa685d3b7-goog