[PATCH 3.16 046/204] KEYS: fix cred refcount leak in request_key_auth_new()

From: Ben Hutchings
Date: Thu Dec 28 2017 - 13:02:55 EST


3.16.52-rc1 review patch. If anyone has any objections, please let me know.

------------------

From: Eric Biggers <ebiggers@xxxxxxxxxx>

commit 44d8143340a99b167c74365e844516b73523c087 upstream.

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 <ebiggers@xxxxxxxxxx>
Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
[bwh: Backported to 3.16: adjust context]
Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
---
security/keys/request_key_auth.c | 68 ++++++++++++++++++----------------------
1 file changed, 31 insertions(+), 37 deletions(-)

--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -107,6 +107,18 @@ static void request_key_auth_revoke(stru
}
}

+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.
*/
@@ -116,15 +128,7 @@ static void request_key_auth_destroy(str

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);
}

/*
@@ -138,22 +142,17 @@ struct key *request_key_auth_new(struct
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;
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 */
@@ -163,8 +162,12 @@ struct key *request_key_auth_new(struct

/* 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;
rka->cred = get_cred(irka->cred);
@@ -192,32 +195,23 @@ struct key *request_key_auth_new(struct
KEY_USR_VIEW, KEY_ALLOC_NOT_IN_QUOTA);
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, atomic_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);
+error:
kleave("= %d", ret);
return ERR_PTR(ret);
}