[PATCH] Keys: Add possessor permissions to keys [try #3]

The attached patch adds extra permission grants to keys for the possessor of a
key in addition to the owner, group and other permissions bits. This makes
SUID binaries easier to support without going as far as labelling keys and key
targets using the LSM facilities.

This patch adds a second "pointer type" to key structures (struct key_ref *)
that can have the bottom bit of the address set to indicate the possession of
a key. This is propagated through searches from the keyring to the discovered
key. It has been made a separate type so that the compiler can spot attempts
to dereference a potentially incorrect pointer.

The "possession" attribute can't be attached to a key structure directly as
it's not an intrinsic property of a key.

Pointers to keys have been replaced with struct key_ref *'s wherever
possession information needs to be passed through.

This does assume that the bottom bit of the pointer will always be zero on
return from kmem_cache_alloc().

The key reference type has been made into a typedef so that at least it can be
located in the sources, even though it's basically a pointer to an undefined
type. I've also renamed the accessor functions to be more useful, and all
reference variables should now end in "_ref".

Signed-Off-By: David Howells <dhowells@redhat.com>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

1 files changed, 151 insertions, 150 deletions

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index a6516a64b29..4c670ee6acf 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -34,7 +34,7 @@ asmlinkage long sys_add_key(const char __user *_type,
 			    size_t plen,
 			    key_serial_t ringid)
 {
-	struct key *keyring, *key;
+	key_ref_t keyring_ref, key_ref;
 	char type[32], *description;
 	void *payload;
 	long dlen, ret;
@@ -86,25 +86,25 @@ asmlinkage long sys_add_key(const char __user *_type,
 	}
 
 	/* find the target keyring (which must be writable) */
-	keyring = lookup_user_key(NULL, ringid, 1, 0, KEY_WRITE);
-	if (IS_ERR(keyring)) {
-		ret = PTR_ERR(keyring);
+	keyring_ref = lookup_user_key(NULL, ringid, 1, 0, KEY_WRITE);
+	if (IS_ERR(keyring_ref)) {
+		ret = PTR_ERR(keyring_ref);
 		goto error3;
 	}
 
 	/* create or update the requested key and add it to the target
 	 * keyring */
-	key = key_create_or_update(keyring, type, description,
-				   payload, plen, 0);
-	if (!IS_ERR(key)) {
-		ret = key->serial;
-		key_put(key);
+	key_ref = key_create_or_update(keyring_ref, type, description,
+				       payload, plen, 0);
+	if (!IS_ERR(key_ref)) {
+		ret = key_ref_to_ptr(key_ref)->serial;
+		key_ref_put(key_ref);
 	}
 	else {
-		ret = PTR_ERR(key);
+		ret = PTR_ERR(key_ref);
 	}
 
-	key_put(keyring);
+	key_ref_put(keyring_ref);
  error3:
 	kfree(payload);
  error2:
@@ -131,7 +131,8 @@ asmlinkage long sys_request_key(const char __user *_type,
 				key_serial_t destringid)
 {
 	struct key_type *ktype;
-	struct key *key, *dest;
+	struct key *key;
+	key_ref_t dest_ref;
 	char type[32], *description, *callout_info;
 	long dlen, ret;
 
@@ -187,11 +188,11 @@ asmlinkage long sys_request_key(const char __user *_type,
 	}
 
 	/* get the destination keyring if specified */
-	dest = NULL;
+	dest_ref = NULL;
 	if (destringid) {
-		dest = lookup_user_key(NULL, destringid, 1, 0, KEY_WRITE);
-		if (IS_ERR(dest)) {
-			ret = PTR_ERR(dest);
+		dest_ref = lookup_user_key(NULL, destringid, 1, 0, KEY_WRITE);
+		if (IS_ERR(dest_ref)) {
+			ret = PTR_ERR(dest_ref);
 			goto error3;
 		}
 	}
@@ -204,7 +205,8 @@ asmlinkage long sys_request_key(const char __user *_type,
 	}
 
 	/* do the search */
-	key = request_key_and_link(ktype, description, callout_info, dest);
+	key = request_key_and_link(ktype, description, callout_info,
+				   key_ref_to_ptr(dest_ref));
 	if (IS_ERR(key)) {
 		ret = PTR_ERR(key);
 		goto error5;
@@ -216,7 +218,7 @@ asmlinkage long sys_request_key(const char __user *_type,
  error5:
 	key_type_put(ktype);
  error4:
-	key_put(dest);
+	key_ref_put(dest_ref);
  error3:
 	kfree(callout_info);
  error2:
@@ -234,17 +236,17 @@ asmlinkage long sys_request_key(const char __user *_type,
  */
 long keyctl_get_keyring_ID(key_serial_t id, int create)
 {
-	struct key *key;
+	key_ref_t key_ref;
 	long ret;
 
-	key = lookup_user_key(NULL, id, create, 0, KEY_SEARCH);
-	if (IS_ERR(key)) {
-		ret = PTR_ERR(key);
+	key_ref = lookup_user_key(NULL, id, create, 0, KEY_SEARCH);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
 		goto error;
 	}
 
-	ret = key->serial;
-	key_put(key);
+	ret = key_ref_to_ptr(key_ref)->serial;
+	key_ref_put(key_ref);
  error:
 	return ret;
 
@@ -302,7 +304,7 @@ long keyctl_update_key(key_serial_t id,
 		       const void __user *_payload,
 		       size_t plen)
 {
-	struct key *key;
+	key_ref_t key_ref;
 	void *payload;
 	long ret;
 
@@ -324,16 +326,16 @@ long keyctl_update_key(key_serial_t id,
 	}
 
 	/* find the target key (which must be writable) */
-	key = lookup_user_key(NULL, id, 0, 0, KEY_WRITE);
-	if (IS_ERR(key)) {
-		ret = PTR_ERR(key);
+	key_ref = lookup_user_key(NULL, id, 0, 0, KEY_WRITE);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
 		goto error2;
 	}
 
 	/* update the key */
-	ret = key_update(key, payload, plen);
+	ret = key_update(key_ref, payload, plen);
 
-	key_put(key);
+	key_ref_put(key_ref);
  error2:
 	kfree(payload);
  error:
@@ -349,19 +351,19 @@ long keyctl_update_key(key_serial_t id,
  */
 long keyctl_revoke_key(key_serial_t id)
 {
-	struct key *key;
+	key_ref_t key_ref;
 	long ret;
 
-	key = lookup_user_key(NULL, id, 0, 0, KEY_WRITE);
-	if (IS_ERR(key)) {
-		ret = PTR_ERR(key);
+	key_ref = lookup_user_key(NULL, id, 0, 0, KEY_WRITE);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
 		goto error;
 	}
 
-	key_revoke(key);
+	key_revoke(key_ref_to_ptr(key_ref));
 	ret = 0;
 
-	key_put(key);
+	key_ref_put(key_ref);
  error:
 	return ret;
 
@@ -375,18 +377,18 @@ long keyctl_revoke_key(key_serial_t id)
  */
 long keyctl_keyring_clear(key_serial_t ringid)
 {
-	struct key *keyring;
+	key_ref_t keyring_ref;
 	long ret;
 
-	keyring = lookup_user_key(NULL, ringid, 1, 0, KEY_WRITE);
-	if (IS_ERR(keyring)) {
-		ret = PTR_ERR(keyring);
+	keyring_ref = lookup_user_key(NULL, ringid, 1, 0, KEY_WRITE);
+	if (IS_ERR(keyring_ref)) {
+		ret = PTR_ERR(keyring_ref);
 		goto error;
 	}
 
-	ret = keyring_clear(keyring);
+	ret = keyring_clear(key_ref_to_ptr(keyring_ref));
 
-	key_put(keyring);
+	key_ref_put(keyring_ref);
  error:
 	return ret;
 
@@ -401,26 +403,26 @@ long keyctl_keyring_clear(key_serial_t ringid)
  */
 long keyctl_keyring_link(key_serial_t id, key_serial_t ringid)
 {
-	struct key *keyring, *key;
+	key_ref_t keyring_ref, key_ref;
 	long ret;
 
-	keyring = lookup_user_key(NULL, ringid, 1, 0, KEY_WRITE);
-	if (IS_ERR(keyring)) {
-		ret = PTR_ERR(keyring);
+	keyring_ref = lookup_user_key(NULL, ringid, 1, 0, KEY_WRITE);
+	if (IS_ERR(keyring_ref)) {
+		ret = PTR_ERR(keyring_ref);
 		goto error;
 	}
 
-	key = lookup_user_key(NULL, id, 1, 0, KEY_LINK);
-	if (IS_ERR(key)) {
-		ret = PTR_ERR(key);
+	key_ref = lookup_user_key(NULL, id, 1, 0, KEY_LINK);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
 		goto error2;
 	}
 
-	ret = key_link(keyring, key);
+	ret = key_link(key_ref_to_ptr(keyring_ref), key_ref_to_ptr(key_ref));
 
-	key_put(key);
+	key_ref_put(key_ref);
  error2:
-	key_put(keyring);
+	key_ref_put(keyring_ref);
  error:
 	return ret;
 
@@ -435,26 +437,26 @@ long keyctl_keyring_link(key_serial_t id, key_serial_t ringid)
  */
 long keyctl_keyring_unlink(key_serial_t id, key_serial_t ringid)
 {
-	struct key *keyring, *key;
+	key_ref_t keyring_ref, key_ref;
 	long ret;
 
-	keyring = lookup_user_key(NULL, ringid, 0, 0, KEY_WRITE);
-	if (IS_ERR(keyring)) {
-		ret = PTR_ERR(keyring);
+	keyring_ref = lookup_user_key(NULL, ringid, 0, 0, KEY_WRITE);
+	if (IS_ERR(keyring_ref)) {
+		ret = PTR_ERR(keyring_ref);
 		goto error;
 	}
 
-	key = lookup_user_key(NULL, id, 0, 0, 0);
-	if (IS_ERR(key)) {
-		ret = PTR_ERR(key);
+	key_ref = lookup_user_key(NULL, id, 0, 0, 0);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
 		goto error2;
 	}
 
-	ret = key_unlink(keyring, key);
+	ret = key_unlink(key_ref_to_ptr(keyring_ref), key_ref_to_ptr(key_ref));
 
-	key_put(key);
+	key_ref_put(key_ref);
  error2:
-	key_put(keyring);
+	key_ref_put(keyring_ref);
  error:
 	return ret;
 
@@ -476,24 +478,26 @@ long keyctl_describe_key(key_serial_t keyid,
 			 size_t buflen)
 {
 	struct key *key, *instkey;
+	key_ref_t key_ref;
 	char *tmpbuf;
 	long ret;
 
-	key = lookup_user_key(NULL, keyid, 0, 1, KEY_VIEW);
-	if (IS_ERR(key)) {
+	key_ref = lookup_user_key(NULL, keyid, 0, 1, KEY_VIEW);
+	if (IS_ERR(key_ref)) {
 		/* viewing a key under construction is permitted if we have the
 		 * authorisation token handy */
-		if (PTR_ERR(key) == -EACCES) {
+		if (PTR_ERR(key_ref) == -EACCES) {
 			instkey = key_get_instantiation_authkey(keyid);
 			if (!IS_ERR(instkey)) {
 				key_put(instkey);
-				key = lookup_user_key(NULL, keyid, 0, 1, 0);
-				if (!IS_ERR(key))
+				key_ref = lookup_user_key(NULL, keyid,
+							  0, 1, 0);
+				if (!IS_ERR(key_ref))
 					goto okay;
 			}
 		}
 
-		ret = PTR_ERR(key);
+		ret = PTR_ERR(key_ref);
 		goto error;
 	}
 
@@ -504,13 +508,16 @@ okay:
 	if (!tmpbuf)
 		goto error2;
 
+	key = key_ref_to_ptr(key_ref);
+
 	ret = snprintf(tmpbuf, PAGE_SIZE - 1,
-		       "%s;%d;%d;%06x;%s",
-		       key->type->name,
-		       key->uid,
-		       key->gid,
-		       key->perm,
-		       key->description ? key->description :""
+		       "%s;%d;%d;%08x;%s",
+		       key_ref_to_ptr(key_ref)->type->name,
+		       key_ref_to_ptr(key_ref)->uid,
+		       key_ref_to_ptr(key_ref)->gid,
+		       key_ref_to_ptr(key_ref)->perm,
+		       key_ref_to_ptr(key_ref)->description ?
+		       key_ref_to_ptr(key_ref)->description : ""
 		       );
 
 	/* include a NUL char at the end of the data */
@@ -530,7 +537,7 @@ okay:
 
 	kfree(tmpbuf);
  error2:
-	key_put(key);
+	key_ref_put(key_ref);
  error:
 	return ret;
 
@@ -552,7 +559,7 @@ long keyctl_keyring_search(key_serial_t ringid,
 			   key_serial_t destringid)
 {
 	struct key_type *ktype;
-	struct key *keyring, *key, *dest;
+	key_ref_t keyring_ref, key_ref, dest_ref;
 	char type[32], *description;
 	long dlen, ret;
 
@@ -581,18 +588,18 @@ long keyctl_keyring_search(key_serial_t ringid,
 		goto error2;
 
 	/* get the keyring at which to begin the search */
-	keyring = lookup_user_key(NULL, ringid, 0, 0, KEY_SEARCH);
-	if (IS_ERR(keyring)) {
-		ret = PTR_ERR(keyring);
+	keyring_ref = lookup_user_key(NULL, ringid, 0, 0, KEY_SEARCH);
+	if (IS_ERR(keyring_ref)) {
+		ret = PTR_ERR(keyring_ref);
 		goto error2;
 	}
 
 	/* get the destination keyring if specified */
-	dest = NULL;
+	dest_ref = NULL;
 	if (destringid) {
-		dest = lookup_user_key(NULL, destringid, 1, 0, KEY_WRITE);
-		if (IS_ERR(dest)) {
-			ret = PTR_ERR(dest);
+		dest_ref = lookup_user_key(NULL, destringid, 1, 0, KEY_WRITE);
+		if (IS_ERR(dest_ref)) {
+			ret = PTR_ERR(dest_ref);
 			goto error3;
 		}
 	}
@@ -605,9 +612,9 @@ long keyctl_keyring_search(key_serial_t ringid,
 	}
 
 	/* do the search */
-	key = keyring_search(keyring, ktype, description);
-	if (IS_ERR(key)) {
-		ret = PTR_ERR(key);
+	key_ref = keyring_search(keyring_ref, ktype, description);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
 
 		/* treat lack or presence of a negative key the same */
 		if (ret == -EAGAIN)
@@ -616,26 +623,26 @@ long keyctl_keyring_search(key_serial_t ringid,
 	}
 
 	/* link the resulting key to the destination keyring if we can */
-	if (dest) {
+	if (dest_ref) {
 		ret = -EACCES;
-		if (!key_permission(key, KEY_LINK))
+		if (!key_permission(key_ref, KEY_LINK))
 			goto error6;
 
-		ret = key_link(dest, key);
+		ret = key_link(key_ref_to_ptr(dest_ref), key_ref_to_ptr(key_ref));
 		if (ret < 0)
 			goto error6;
 	}
 
-	ret = key->serial;
+	ret = key_ref_to_ptr(key_ref)->serial;
 
  error6:
-	key_put(key);
+	key_ref_put(key_ref);
  error5:
 	key_type_put(ktype);
  error4:
-	key_put(dest);
+	key_ref_put(dest_ref);
  error3:
-	key_put(keyring);
+	key_ref_put(keyring_ref);
  error2:
 	kfree(description);
  error:
@@ -645,16 +652,6 @@ long keyctl_keyring_search(key_serial_t ringid,
 
 /*****************************************************************************/
 /*
- * see if the key we're looking at is the target key
- */
-static int keyctl_read_key_same(const struct key *key, const void *target)
-{
-	return key == target;
-
-} /* end keyctl_read_key_same() */
-
-/*****************************************************************************/
-/*
  * read a user key's payload
  * - the keyring must be readable or the key must be searchable from the
  *   process's keyrings
@@ -665,38 +662,33 @@ static int keyctl_read_key_same(const struct key *key, const void *target)
  */
 long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 {
-	struct key *key, *skey;
+	struct key *key;
+	key_ref_t key_ref;
 	long ret;
 
 	/* find the key first */
-	key = lookup_user_key(NULL, keyid, 0, 0, 0);
-	if (!IS_ERR(key)) {
-		/* see if we can read it directly */
-		if (key_permission(key, KEY_READ))
-			goto can_read_key;
-
-		/* we can't; see if it's searchable from this process's
-		 * keyrings
-		 * - we automatically take account of the fact that it may be
-		 *   dangling off an instantiation key
-		 */
-		skey = search_process_keyrings(key->type, key,
-					       keyctl_read_key_same, current);
-		if (!IS_ERR(skey))
-			goto can_read_key2;
-
-		ret = PTR_ERR(skey);
-		if (ret == -EAGAIN)
-			ret = -EACCES;
-		goto error2;
+	key_ref = lookup_user_key(NULL, keyid, 0, 0, 0);
+	if (IS_ERR(key_ref)) {
+		ret = -ENOKEY;
+		goto error;
 	}
 
-	ret = -ENOKEY;
-	goto error;
+	key = key_ref_to_ptr(key_ref);
+
+	/* see if we can read it directly */
+	if (key_permission(key_ref, KEY_READ))
+		goto can_read_key;
+
+	/* we can't; see if it's searchable from this process's keyrings
+	 * - we automatically take account of the fact that it may be
+	 *   dangling off an instantiation key
+	 */
+	if (!is_key_possessed(key_ref)) {
+		ret = -EACCES;
+		goto error2;
+	}
 
 	/* the key is probably readable - now try to read it */
- can_read_key2:
-	key_put(skey);
  can_read_key:
 	ret = key_validate(key);
 	if (ret == 0) {
@@ -727,18 +719,21 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 long keyctl_chown_key(key_serial_t id, uid_t uid, gid_t gid)
 {
 	struct key *key;
+	key_ref_t key_ref;
 	long ret;
 
 	ret = 0;
 	if (uid == (uid_t) -1 && gid == (gid_t) -1)
 		goto error;
 
-	key = lookup_user_key(NULL, id, 1, 1, 0);
-	if (IS_ERR(key)) {
-		ret = PTR_ERR(key);
+	key_ref = lookup_user_key(NULL, id, 1, 1, 0);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
 		goto error;
 	}
 
+	key = key_ref_to_ptr(key_ref);
+
 	/* make the changes with the locks held to prevent chown/chown races */
 	ret = -EACCES;
 	down_write(&key->sem);
@@ -784,18 +779,21 @@ long keyctl_chown_key(key_serial_t id, uid_t uid, gid_t gid)
 long keyctl_setperm_key(key_serial_t id, key_perm_t perm)
 {
 	struct key *key;
+	key_ref_t key_ref;
 	long ret;
 
 	ret = -EINVAL;
-	if (perm & ~(KEY_USR_ALL | KEY_GRP_ALL | KEY_OTH_ALL))
+	if (perm & ~(KEY_POS_ALL | KEY_USR_ALL | KEY_GRP_ALL | KEY_OTH_ALL))
 		goto error;
 
-	key = lookup_user_key(NULL, id, 1, 1, 0);
-	if (IS_ERR(key)) {
-		ret = PTR_ERR(key);
+	key_ref = lookup_user_key(NULL, id, 1, 1, 0);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
 		goto error;
 	}
 
+	key = key_ref_to_ptr(key_ref);
+
 	/* make the changes with the locks held to prevent chown/chmod races */
 	ret = -EACCES;
 	down_write(&key->sem);
@@ -824,7 +822,8 @@ long keyctl_instantiate_key(key_serial_t id,
 			    key_serial_t ringid)
 {
 	struct request_key_auth *rka;
-	struct key *instkey, *keyring;
+	struct key *instkey;
+	key_ref_t keyring_ref;
 	void *payload;
 	long ret;
 
@@ -857,21 +856,21 @@ long keyctl_instantiate_key(key_serial_t id,
 
 	/* find the destination keyring amongst those belonging to the
 	 * requesting task */
-	keyring = NULL;
+	keyring_ref = NULL;
 	if (ringid) {
-		keyring = lookup_user_key(rka->context, ringid, 1, 0,
-					  KEY_WRITE);
-		if (IS_ERR(keyring)) {
-			ret = PTR_ERR(keyring);
+		keyring_ref = lookup_user_key(rka->context, ringid, 1, 0,
+					      KEY_WRITE);
+		if (IS_ERR(keyring_ref)) {
+			ret = PTR_ERR(keyring_ref);
 			goto error3;
 		}
 	}
 
 	/* instantiate the key and link it into a keyring */
 	ret = key_instantiate_and_link(rka->target_key, payload, plen,
-				       keyring, instkey);
+				       key_ref_to_ptr(keyring_ref), instkey);
 
-	key_put(keyring);
+	key_ref_put(keyring_ref);
  error3:
 	key_put(instkey);
  error2:
@@ -889,7 +888,8 @@ long keyctl_instantiate_key(key_serial_t id,
 long keyctl_negate_key(key_serial_t id, unsigned timeout, key_serial_t ringid)
 {
 	struct request_key_auth *rka;
-	struct key *instkey, *keyring;
+	struct key *instkey;
+	key_ref_t keyring_ref;
 	long ret;
 
 	/* find the instantiation authorisation key */
@@ -903,19 +903,20 @@ long keyctl_negate_key(key_serial_t id, unsigned timeout, key_serial_t ringid)
 
 	/* find the destination keyring if present (which must also be
 	 * writable) */
-	keyring = NULL;
+	keyring_ref = NULL;
 	if (ringid) {
-		keyring = lookup_user_key(NULL, ringid, 1, 0, KEY_WRITE);
-		if (IS_ERR(keyring)) {
-			ret = PTR_ERR(keyring);
+		keyring_ref = lookup_user_key(NULL, ringid, 1, 0, KEY_WRITE);
+		if (IS_ERR(keyring_ref)) {
+			ret = PTR_ERR(keyring_ref);
 			goto error2;
 		}
 	}
 
 	/* instantiate the key and link it into a keyring */
-	ret = key_negate_and_link(rka->target_key, timeout, keyring, instkey);
+	ret = key_negate_and_link(rka->target_key, timeout,
+				  key_ref_to_ptr(keyring_ref), instkey);
 
-	key_put(keyring);
+	key_ref_put(keyring_ref);
  error2:
 	key_put(instkey);
  error: