diff mbox

[v9,05/11] KEYS: Use structure to capture key restriction function and data

Message ID 20161116223806.14601-6-mathew.j.martineau@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mat Martineau Nov. 16, 2016, 10:38 p.m. UTC
Replace struct key's restrict_link function pointer with a pointer to
the new struct key_restriction. The structure contains pointers to the
restriction function as well as relevant data for evaluating the
restriction. The optional key_restriction free_data function is called
at keyring destruction time.

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 certs/system_keyring.c  | 21 ++++++++++++++++++++-
 include/linux/key.h     |  8 ++++----
 security/keys/key.c     | 23 ++++++++++++++---------
 security/keys/keyring.c | 20 +++++++++++++++-----
 4 files changed, 53 insertions(+), 19 deletions(-)

Comments

David Howells Nov. 24, 2016, 5:47 p.m. UTC | #1
Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:

> +static struct key_restriction *get_builtin_and_secondary_restriction(void)
> +{
> +	struct key_restriction *restriction;
> +
> +	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
> +
> +	if (!restriction)
> +		panic("Can't allocate secondary trusted keyring restriction\n");
> +
> +	restriction->check = restrict_link_by_builtin_and_secondary_trusted;
> +
> +	return restriction;
> +}

Should this be static?  And if not, should it have its destructor set?

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mat Martineau Nov. 28, 2016, 7:47 p.m. UTC | #2
On Thu, 24 Nov 2016, David Howells wrote:

> Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
>
>> +static struct key_restriction *get_builtin_and_secondary_restriction(void)
>> +{
>> +	struct key_restriction *restriction;
>> +
>> +	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
>> +
>> +	if (!restriction)
>> +		panic("Can't allocate secondary trusted keyring restriction\n");
>> +
>> +	restriction->check = restrict_link_by_builtin_and_secondary_trusted;
>> +
>> +	return restriction;
>> +}
>
> Should this be static?  And if not, should it have its destructor set?

This function is only intended for use by system_trusted_keyring_init(), I 
was trying to avoid adding another ifdef to system_trusted_keyring_init(). 
It should be static (and I need to add __init). I will clarify the 
intended usage in the comment above this function.

I will also update the patch set based on your feedback for patches 1, 2, 
7, and 8.

--
Mat Martineau
Intel OTC
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index e904653..adec145 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -14,6 +14,7 @@ 
 #include <linux/sched.h>
 #include <linux/cred.h>
 #include <linux/err.h>
+#include <linux/slab.h>
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
 #include <crypto/pkcs7.h>
@@ -68,6 +69,24 @@  int restrict_link_by_builtin_and_secondary_trusted(
 	return restrict_link_by_signature(dest_keyring, type, payload,
 					  secondary_trusted_keys);
 }
+
+/**
+ * Allocate a struct key_restriction for the "builtin and secondary trust"
+ * keyring.
+ */
+static struct key_restriction *get_builtin_and_secondary_restriction(void)
+{
+	struct key_restriction *restriction;
+
+	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
+
+	if (!restriction)
+		panic("Can't allocate secondary trusted keyring restriction\n");
+
+	restriction->check = restrict_link_by_builtin_and_secondary_trusted;
+
+	return restriction;
+}
 #endif
 
 /*
@@ -95,7 +114,7 @@  static __init int system_trusted_keyring_init(void)
 			       KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
 			       KEY_USR_WRITE),
 			      KEY_ALLOC_NOT_IN_QUOTA,
-			      restrict_link_by_builtin_and_secondary_trusted,
+			      get_builtin_and_secondary_restriction(),
 			      NULL);
 	if (IS_ERR(secondary_trusted_keys))
 		panic("Can't allocate secondary trusted keyring\n");
diff --git a/include/linux/key.h b/include/linux/key.h
index 768d189..5667aad 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -216,7 +216,7 @@  struct key {
 	};
 
 	/* This is set on a keyring to restrict the addition of a link to a key
-	 * to it.  If this method isn't provided then it is assumed that the
+	 * to it.  If this structure isn't provided then it is assumed that the
 	 * keyring is open to any addition.  It is ignored for non-keyring
 	 * keys.
 	 *
@@ -225,7 +225,7 @@  struct key {
 	 * overrides this, allowing the kernel to add extra keys without
 	 * restriction.
 	 */
-	restrict_link_func_t restrict_link;
+	struct key_restriction *restrict_link;
 };
 
 extern struct key *key_alloc(struct key_type *type,
@@ -234,7 +234,7 @@  extern struct key *key_alloc(struct key_type *type,
 			     const struct cred *cred,
 			     key_perm_t perm,
 			     unsigned long flags,
-			     restrict_link_func_t restrict_link);
+			     struct key_restriction *restrict_link);
 
 
 #define KEY_ALLOC_IN_QUOTA		0x0000	/* add to quota, reject if would overrun */
@@ -310,7 +310,7 @@  extern struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid
 				 const struct cred *cred,
 				 key_perm_t perm,
 				 unsigned long flags,
-				 restrict_link_func_t restrict_link,
+				 struct key_restriction *restrict_link,
 				 struct key *dest);
 
 extern int restrict_link_reject(struct key *keyring,
diff --git a/security/keys/key.c b/security/keys/key.c
index 993fd6d..1be33bd 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -201,12 +201,15 @@  static inline void key_alloc_serial(struct key *key)
  * @cred: The credentials specifying UID namespace.
  * @perm: The permissions mask of the new key.
  * @flags: Flags specifying quota properties.
- * @restrict_link: Optional link restriction method for new keyrings.
+ * @restrict_link: Optional link restriction for new keyrings.
  *
  * Allocate a key of the specified type with the attributes given.  The key is
  * returned in an uninstantiated state and the caller needs to instantiate the
  * key before returning.
  *
+ * The restrict_link structure (if not NULL) will be freed when the
+ * keyring is destroyed, so it must be dynamically allocated.
+ *
  * The user's key count quota is updated to reflect the creation of the key and
  * the user's key data quota has the default for the key type reserved.  The
  * instantiation function should amend this as necessary.  If insufficient
@@ -225,7 +228,7 @@  static inline void key_alloc_serial(struct key *key)
 struct key *key_alloc(struct key_type *type, const char *desc,
 		      kuid_t uid, kgid_t gid, const struct cred *cred,
 		      key_perm_t perm, unsigned long flags,
-		      restrict_link_func_t restrict_link)
+		      struct key_restriction *restrict_link)
 {
 	struct key_user *user = NULL;
 	struct key *key;
@@ -497,9 +500,11 @@  int key_instantiate_and_link(struct key *key,
 	}
 
 	if (keyring) {
-		if (keyring->restrict_link) {
-			ret = keyring->restrict_link(keyring, key->type,
-						     &prep.payload, NULL);
+		if (keyring->restrict_link && keyring->restrict_link->check) {
+			struct key_restriction *keyres = keyring->restrict_link;
+
+			ret = keyres->check(keyring, key->type, &prep.payload,
+					    keyres->data);
 			if (ret < 0)
 				goto error;
 		}
@@ -809,7 +814,7 @@  key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	struct key *keyring, *key = NULL;
 	key_ref_t key_ref;
 	int ret;
-	restrict_link_func_t restrict_link = NULL;
+	struct key_restriction *restrict_link = NULL;
 
 	/* look up the key type to see if it's one of the registered kernel
 	 * types */
@@ -855,9 +860,9 @@  key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	}
 	index_key.desc_len = strlen(index_key.description);
 
-	if (restrict_link) {
-		ret = restrict_link(keyring, index_key.type, &prep.payload,
-				    NULL);
+	if (restrict_link && restrict_link->check) {
+		ret = restrict_link->check(keyring, index_key.type,
+					   &prep.payload, restrict_link->data);
 		if (ret < 0) {
 			key_ref = ERR_PTR(ret);
 			goto error_free_prep;
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index b57798d..07f680b 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -394,6 +394,15 @@  static void keyring_destroy(struct key *keyring)
 		write_unlock(&keyring_name_lock);
 	}
 
+	if (keyring->restrict_link) {
+		struct key_restriction *keyres = keyring->restrict_link;
+
+		if (keyres->free_data)
+			keyres->free_data(keyres->data);
+
+		kfree(keyres);
+	}
+
 	assoc_array_destroy(&keyring->keys, &keyring_assoc_array_ops);
 }
 
@@ -492,7 +501,7 @@  static long keyring_read(const struct key *keyring,
 struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid,
 			  const struct cred *cred, key_perm_t perm,
 			  unsigned long flags,
-			  restrict_link_func_t restrict_link,
+			  struct key_restriction *restrict_link,
 			  struct key *dest)
 {
 	struct key *keyring;
@@ -523,8 +532,8 @@  EXPORT_SYMBOL(keyring_alloc);
  * passing KEY_ALLOC_BYPASS_RESTRICTION to key_instantiate_and_link() when
  * adding a key to a keyring.
  *
- * This is meant to be passed as the restrict_link parameter to
- * keyring_alloc().
+ * This is meant to be stored in a key_restriction structure which is passed
+ * in the restrict_link parameter to keyring_alloc().
  */
 int restrict_link_reject(struct key *keyring,
 			 const struct key_type *type,
@@ -1220,9 +1229,10 @@  void __key_link_end(struct key *keyring,
  */
 static int __key_link_check_restriction(struct key *keyring, struct key *key)
 {
-	if (!keyring->restrict_link)
+	if (!keyring->restrict_link || !keyring->restrict_link->check)
 		return 0;
-	return keyring->restrict_link(keyring, key->type, &key->payload, NULL);
+	return keyring->restrict_link->check(keyring, key->type, &key->payload,
+					     keyring->restrict_link->data);
 }
 
 /**