[RFC][PATCH 0/9] encrypted keys & key control op

[RFC][PATCH 0/9] encrypted keys & key control op

[David Howells]

Hi Mimi, Dmitry,

Here's a series of patches, the last three of which attempt to fix up a
problem with encrypted keys update method.  The preceding patches are fixes or
are preparatory for other changes that I want to put underneath.

I really want to make all key types use ->preparse() to avoid a deadlock in
the garbage collector with quota recycling, but encrypted_update() requires
access to the old key - which you can't have in ->preparse() because the
keyring isn't locked (and this lock is the root of the gc deadlock anyway).

Further, the existence of encrypted_update() means that add_key() will
sometimes get things wrong with encrypted keys (add_key() will call ->update()
if a matching key already exists rather than creating a new key).  But you
can't pre-search for the existence of a key and mould the payload accordingly
because that means you can race against both add_key() and keyctl_unlink().

My solution is to add a new keyctl function that allows the caller to perform
a type-specific operation on a key:

	long keyctl_control(key_serial_t key_id,
			    const char *command,
			    char *reply_buffer,
			    size_t reply_size);

This would then take a NUL-terminated string indicating the command and
arguments and potentially return a reply up to the buffer length.

For instance:

	keyctl_control(1234, "encrypted change-master-key fred's key", NULL, 0);

or, from the shell:

	keyctl control 1234 "encrypted change-master-key fred's key"

(I think that requiring the command string to be prefixed with the expected
key type is probably a good idea).

The control op could also be used for other things like pushing a key into a
TPM.

What do you think?

David
---
David Howells (9):
      KEYS: The RSA public key algorithm needs to select MPILIB
      KEYS: Provide a generic instantiation function
      KEYS: struct key_preparsed_payload should have two payload pointers
      KEYS: Allow expiry time to be set when preparsing a key
      KEYS: Call ->free_preparse() even after ->preparse() returns an error
      KEYS: Trusted: Use key preparsing
      KEYS: Add a keyctl function to alter/control a key in type-dependent way
      KEYS: Implement keyctl control for encrypted keys
      KEYS: Fix encrypted key type update method


 Documentation/security/keys.txt          |   48 +++++++-
 crypto/asymmetric_keys/Kconfig           |    1 
 crypto/asymmetric_keys/asymmetric_type.c |   27 ----
 crypto/asymmetric_keys/x509_public_key.c |    2 
 include/linux/key-type.h                 |   11 ++
 include/uapi/linux/keyctl.h              |    1 
 security/keys/compat.c                   |    6 +
 security/keys/encrypted-keys/encrypted.c |  111 +++++++++++++-----
 security/keys/internal.h                 |    2 
 security/keys/key.c                      |   49 +++++++-
 security/keys/keyctl.c                   |  104 ++++++++++++++++
 security/keys/trusted.c                  |  190 ++++++++++++++----------------
 12 files changed, 385 insertions(+), 167 deletions(-)

[PATCH 1/9] KEYS: The RSA public key algorithm needs to select MPILIB

[David Howells]

The RSA public key algorithm needs to select MPILIB directly in Kconfig as the
'select' directive is not recursive and is thus MPILIB is not enabled by
selecting MPILIB_EXTRA.

Without this, the following errors can occur:

	crypto/built-in.o: In function `RSA_verify_signature':
	rsa.c:(.text+0x1d347): undefined reference to `mpi_get_nbits'
	rsa.c:(.text+0x1d354): undefined reference to `mpi_get_nbits'
	rsa.c:(.text+0x1d36e): undefined reference to `mpi_cmp_ui'
	rsa.c:(.text+0x1d382): undefined reference to `mpi_cmp'
	rsa.c:(.text+0x1d391): undefined reference to `mpi_alloc'
	rsa.c:(.text+0x1d3b0): undefined reference to `mpi_powm'
	rsa.c:(.text+0x1d3c3): undefined reference to `mpi_free'
	rsa.c:(.text+0x1d3d8): undefined reference to `mpi_get_buffer'
	rsa.c:(.text+0x1d4d4): undefined reference to `mpi_free'
	rsa.c:(.text+0x1d503): undefined reference to `mpi_get_nbits'

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
---

 crypto/asymmetric_keys/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/asymmetric_keys/Kconfig b/crypto/asymmetric_keys/Kconfig
index 82e7d6b0c276..03a6eb95ab50 100644
--- a/crypto/asymmetric_keys/Kconfig
+++ b/crypto/asymmetric_keys/Kconfig
@@ -23,6 +23,7 @@ config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
 config PUBLIC_KEY_ALGO_RSA
 	tristate "RSA public-key algorithm"
 	select MPILIB_EXTRA
+	select MPILIB
 	help
 	  This option enables support for the RSA algorithm (PKCS#1, RFC3447).
 

[PATCH 2/9] KEYS: Provide a generic instantiation function

[David Howells]

Provide a generic instantiation function for key types that use the preparse
hook.  This makes it easier to prereserve key quota before keyrings get locked
to retain the new key.

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

 crypto/asymmetric_keys/asymmetric_type.c |   25 +------------------------
 include/linux/key-type.h                 |    2 ++
 security/keys/key.c                      |   30 ++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index b77eb5304788..c1fe0fcee8e3 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -164,29 +164,6 @@ static void asymmetric_key_free_preparse(struct key_preparsed_payload *prep)
 }
 
 /*
- * Instantiate a asymmetric_key defined key.  The key was preparsed, so we just
- * have to transfer the data here.
- */
-static int asymmetric_key_instantiate(struct key *key, struct key_preparsed_payload *prep)
-{
-	int ret;
-
-	pr_devel("==>%s()\n", __func__);
-
-	ret = key_payload_reserve(key, prep->quotalen);
-	if (ret == 0) {
-		key->type_data.p[0] = prep->type_data[0];
-		key->type_data.p[1] = prep->type_data[1];
-		key->payload.data = prep->payload;
-		prep->type_data[0] = NULL;
-		prep->type_data[1] = NULL;
-		prep->payload = NULL;
-	}
-	pr_devel("<==%s() = %d\n", __func__, ret);
-	return ret;
-}
-
-/*
  * dispose of the data dangling from the corpse of a asymmetric key
  */
 static void asymmetric_key_destroy(struct key *key)
@@ -205,7 +182,7 @@ struct key_type key_type_asymmetric = {
 	.name		= "asymmetric",
 	.preparse	= asymmetric_key_preparse,
 	.free_preparse	= asymmetric_key_free_preparse,
-	.instantiate	= asymmetric_key_instantiate,
+	.instantiate	= generic_key_instantiate,
 	.match		= asymmetric_key_match,
 	.destroy	= asymmetric_key_destroy,
 	.describe	= asymmetric_key_describe,
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index a74c3a84dfdd..88503dca2a57 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -159,5 +159,7 @@ static inline int key_negate_and_link(struct key *key,
 	return key_reject_and_link(key, timeout, ENOKEY, keyring, instkey);
 }
 
+extern int generic_key_instantiate(struct key *key, struct key_preparsed_payload *prep);
+
 #endif /* CONFIG_KEYS */
 #endif /* _LINUX_KEY_TYPE_H */
diff --git a/security/keys/key.c b/security/keys/key.c
index 55d110f0aced..4ea216652d52 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -1030,6 +1030,36 @@ void key_invalidate(struct key *key)
 EXPORT_SYMBOL(key_invalidate);
 
 /**
+ * generic_key_instantiate - Simple instantiation of a key from preparsed data
+ * @key: The key to be instantiated
+ * @prep: The preparsed data to load.
+ *
+ * Instantiate a key from preparsed data.  We assume we can just copy the data
+ * in directly and clear the old pointers.
+ *
+ * This can be pointed to directly by the key type instantiate op pointer.
+ */
+int generic_key_instantiate(struct key *key, struct key_preparsed_payload *prep)
+{
+	int ret;
+
+	pr_devel("==>%s()\n", __func__);
+
+	ret = key_payload_reserve(key, prep->quotalen);
+	if (ret == 0) {
+		key->type_data.p[0] = prep->type_data[0];
+		key->type_data.p[1] = prep->type_data[1];
+		rcu_assign_keypointer(key, prep->payload);
+		prep->type_data[0] = NULL;
+		prep->type_data[1] = NULL;
+		prep->payload = NULL;
+	}
+	pr_devel("<==%s() = %d\n", __func__, ret);
+	return ret;
+}
+EXPORT_SYMBOL(generic_key_instantiate);
+
+/**
  * register_key_type - Register a type of key.
  * @ktype: The new key type.
  *

[PATCH 3/9] KEYS: struct key_preparsed_payload should have two payload pointers

[David Howells]

struct key_preparsed_payload should have two payload pointers to correspond
with those in struct key.

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

 crypto/asymmetric_keys/asymmetric_type.c |    2 +-
 crypto/asymmetric_keys/x509_public_key.c |    2 +-
 include/linux/key-type.h                 |    2 +-
 security/keys/key.c                      |    6 ++++--
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index c1fe0fcee8e3..21960a4e74e8 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -156,7 +156,7 @@ static void asymmetric_key_free_preparse(struct key_preparsed_payload *prep)
 	pr_devel("==>%s()\n", __func__);
 
 	if (subtype) {
-		subtype->destroy(prep->payload);
+		subtype->destroy(prep->payload[0]);
 		module_put(subtype->owner);
 	}
 	kfree(prep->type_data[1]);
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index f83300b6e8c1..ae3de8bfaa70 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -256,7 +256,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 	__module_get(public_key_subtype.owner);
 	prep->type_data[0] = &public_key_subtype;
 	prep->type_data[1] = cert->fingerprint;
-	prep->payload = cert->pub;
+	prep->payload[0] = cert->pub;
 	prep->description = desc;
 	prep->quotalen = 100;
 
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 88503dca2a57..d2b4845d74bf 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -41,7 +41,7 @@ struct key_construction {
 struct key_preparsed_payload {
 	char		*description;	/* Proposed key description (or NULL) */
 	void		*type_data[2];	/* Private key-type data */
-	void		*payload;	/* Proposed payload */
+	void		*payload[2];	/* Proposed payload */
 	const void	*data;		/* Raw data */
 	size_t		datalen;	/* Raw datalen */
 	size_t		quotalen;	/* Quota length for proposed payload */
diff --git a/security/keys/key.c b/security/keys/key.c
index 4ea216652d52..64dc9cf6848e 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -1049,10 +1049,12 @@ int generic_key_instantiate(struct key *key, struct key_preparsed_payload *prep)
 	if (ret == 0) {
 		key->type_data.p[0] = prep->type_data[0];
 		key->type_data.p[1] = prep->type_data[1];
-		rcu_assign_keypointer(key, prep->payload);
+		rcu_assign_keypointer(key, prep->payload[0]);
+		key->payload.data2[1] = prep->payload[1];
 		prep->type_data[0] = NULL;
 		prep->type_data[1] = NULL;
-		prep->payload = NULL;
+		prep->payload[0] = NULL;
+		prep->payload[1] = NULL;
 	}
 	pr_devel("<==%s() = %d\n", __func__, ret);
 	return ret;

[PATCH 4/9] KEYS: Allow expiry time to be set when preparsing a key

[David Howells]

Allow a key type's preparsing routine to set the expiry time for a key.

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

 Documentation/security/keys.txt |   10 +++++++---
 include/linux/key-type.h        |    1 +
 security/keys/key.c             |    8 ++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/security/keys.txt b/Documentation/security/keys.txt
index a4c33f1a7c6d..315cf96a41a2 100644
--- a/Documentation/security/keys.txt
+++ b/Documentation/security/keys.txt
@@ -1150,20 +1150,24 @@ The structure has a number of fields, some of which are mandatory:
 		const void	*data;
 		size_t		datalen;
 		size_t		quotalen;
+		time_t		expiry;
 	};
 
      Before calling the method, the caller will fill in data and datalen with
      the payload blob parameters; quotalen will be filled in with the default
-     quota size from the key type and the rest will be cleared.
+     quota size from the key type; expiry will be set to TIME_T_MAX and the
+     rest will be cleared.
 
      If a description can be proposed from the payload contents, that should be
      attached as a string to the description field.  This will be used for the
      key description if the caller of add_key() passes NULL or "".
 
      The method can attach anything it likes to type_data[] and payload.  These
-     are merely passed along to the instantiate() or update() operations.
+     are merely passed along to the instantiate() or update() operations.  If
+     set, the expiry time will be applied to the key if it is instantiated from
+     this data.
 
-     The method should return 0 if success ful or a negative error code
+     The method should return 0 if successful or a negative error code
      otherwise.
 
      
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index d2b4845d74bf..44792ee649de 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -45,6 +45,7 @@ struct key_preparsed_payload {
 	const void	*data;		/* Raw data */
 	size_t		datalen;	/* Raw datalen */
 	size_t		quotalen;	/* Quota length for proposed payload */
+	time_t		expiry;		/* Expiry time of key */
 	bool		trusted;	/* True if key is trusted */
 };
 
diff --git a/security/keys/key.c b/security/keys/key.c
index 64dc9cf6848e..1af0edacd804 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -443,6 +443,11 @@ static int __key_instantiate_and_link(struct key *key,
 			/* disable the authorisation key */
 			if (authkey)
 				key_revoke(authkey);
+
+			if (prep->expiry != TIME_T_MAX) {
+				key->expiry = prep->expiry;
+				key_schedule_gc(prep->expiry + key_gc_delay);
+			}
 		}
 	}
 
@@ -485,6 +490,7 @@ int key_instantiate_and_link(struct key *key,
 	prep.data = data;
 	prep.datalen = datalen;
 	prep.quotalen = key->type->def_datalen;
+	prep.expiry = TIME_T_MAX;
 	if (key->type->preparse) {
 		ret = key->type->preparse(&prep);
 		if (ret < 0)
@@ -817,6 +823,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	prep.datalen = plen;
 	prep.quotalen = index_key.type->def_datalen;
 	prep.trusted = flags & KEY_ALLOC_TRUSTED;
+	prep.expiry = TIME_T_MAX;
 	if (index_key.type->preparse) {
 		ret = index_key.type->preparse(&prep);
 		if (ret < 0) {
@@ -947,6 +954,7 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 	prep.data = payload;
 	prep.datalen = plen;
 	prep.quotalen = key->type->def_datalen;
+	prep.expiry = TIME_T_MAX;
 	if (key->type->preparse) {
 		ret = key->type->preparse(&prep);
 		if (ret < 0)

[PATCH 5/9] KEYS: Call ->free_preparse() even after ->preparse() returns an error

[David Howells]

Call the ->free_preparse() key type op even after ->preparse() returns an
error as it does cleaning up type stuff.

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

 Documentation/security/keys.txt |    4 +++-
 security/keys/key.c             |    9 ++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/security/keys.txt b/Documentation/security/keys.txt
index 315cf96a41a2..3bbec087fc5f 100644
--- a/Documentation/security/keys.txt
+++ b/Documentation/security/keys.txt
@@ -1176,7 +1176,9 @@ The structure has a number of fields, some of which are mandatory:
      This method is only required if the preparse() method is provided,
      otherwise it is unused.  It cleans up anything attached to the
      description, type_data and payload fields of the key_preparsed_payload
-     struct as filled in by the preparse() method.
+     struct as filled in by the preparse() method.  If will always be called
+     after preparse() returns successfully, even if instantiate() or update()
+     succeed.
 
 
  (*) int (*instantiate)(struct key *key, struct key_preparsed_payload *prep);
diff --git a/security/keys/key.c b/security/keys/key.c
index 1af0edacd804..d77c09f0c48e 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -500,7 +500,7 @@ int key_instantiate_and_link(struct key *key,
 	if (keyring) {
 		ret = __key_link_begin(keyring, &key->index_key, &edit);
 		if (ret < 0)
-			goto error_free_preparse;
+			goto error;
 	}
 
 	ret = __key_instantiate_and_link(key, &prep, keyring, authkey, &edit);
@@ -508,10 +508,9 @@ int key_instantiate_and_link(struct key *key,
 	if (keyring)
 		__key_link_end(keyring, &key->index_key, edit);
 
-error_free_preparse:
+error:
 	if (key->type->preparse)
 		key->type->free_preparse(&prep);
-error:
 	return ret;
 }
 
@@ -828,7 +827,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 		ret = index_key.type->preparse(&prep);
 		if (ret < 0) {
 			key_ref = ERR_PTR(ret);
-			goto error_put_type;
+			goto error_free_prep;
 		}
 		if (!index_key.description)
 			index_key.description = prep.description;
@@ -970,9 +969,9 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen)
 
 	up_write(&key->sem);
 
+error:
 	if (key->type->preparse)
 		key->type->free_preparse(&prep);
-error:
 	return ret;
 }
 EXPORT_SYMBOL(key_update);

[PATCH 6/9] KEYS: Trusted: Use key preparsing

[David Howells]

Make use of key preparsing in trusted keys so that quota size determination
can take place prior to keyring locking when a key is being added.

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

 security/keys/trusted.c |  190 ++++++++++++++++++++++-------------------------
 1 file changed, 90 insertions(+), 100 deletions(-)

diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index e13fcf7636f7..ac444e3cfaa7 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -859,52 +859,22 @@ static int datablob_parse(char *datablob, struct trusted_key_payload *p,
 	return ret;
 }
 
-static struct trusted_key_options *trusted_options_alloc(void)
-{
-	struct trusted_key_options *options;
-
-	options = kzalloc(sizeof *options, GFP_KERNEL);
-	if (options) {
-		/* set any non-zero defaults */
-		options->keytype = SRK_keytype;
-		options->keyhandle = SRKHANDLE;
-	}
-	return options;
-}
-
-static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
-{
-	struct trusted_key_payload *p = NULL;
-	int ret;
-
-	ret = key_payload_reserve(key, sizeof *p);
-	if (ret < 0)
-		return p;
-	p = kzalloc(sizeof *p, GFP_KERNEL);
-	if (p)
-		p->migratable = 1; /* migratable by default */
-	return p;
-}
-
 /*
- * trusted_instantiate - create a new trusted key
+ * trusted_preparse - Preparse data for an trusted key
  *
- * Unseal an existing trusted blob or, for a new key, get a
- * random key, then seal and create a trusted key-type key,
- * adding it to the specified keyring.
+ * Decrypt an existing encrypted datablob or create a new encrypted key
+ * based on a kernel random number.
  *
  * On success, return 0. Otherwise return errno.
  */
-static int trusted_instantiate(struct key *key,
-			       struct key_preparsed_payload *prep)
+static int trusted_preparse(struct key_preparsed_payload *prep)
 {
 	struct trusted_key_payload *payload = NULL;
 	struct trusted_key_options *options = NULL;
 	size_t datalen = prep->datalen;
 	char *datablob;
-	int ret = 0;
-	int key_cmd;
-	size_t key_len;
+	long key_cmd;
+	int ret = -ENOMEM;
 
 	if (datalen <= 0 || datalen > 32767 || !prep->data)
 		return -EINVAL;
@@ -915,16 +885,20 @@ static int trusted_instantiate(struct key *key,
 	memcpy(datablob, prep->data, datalen);
 	datablob[datalen] = '\0';
 
-	options = trusted_options_alloc();
-	if (!options) {
-		ret = -ENOMEM;
+	payload = kzalloc(sizeof(*payload), GFP_KERNEL);
+	if (!payload)
 		goto out;
-	}
-	payload = trusted_payload_alloc(key);
-	if (!payload) {
-		ret = -ENOMEM;
+	prep->payload[0] = payload;
+
+	options = kzalloc(sizeof(*options), GFP_KERNEL);
+	if (!options)
 		goto out;
-	}
+	prep->type_data[0] = options;
+
+	/* set any non-zero defaults */
+	payload->migratable = 1; /* migratable by default */
+	options->keytype = SRK_keytype;
+	options->keyhandle = SRKHANDLE;
 
 	key_cmd = datablob_parse(datablob, payload, options);
 	if (key_cmd < 0) {
@@ -932,42 +906,81 @@ static int trusted_instantiate(struct key *key,
 		goto out;
 	}
 
+	prep->type_data[1] = (void *)key_cmd;
+
 	dump_payload(payload);
 	dump_options(options);
+out:
+	kfree(datablob);
+	return ret;
+}
+
+static void trusted_free_preparse(struct key_preparsed_payload *prep)
+{
+	struct trusted_key_payload *p = prep->payload[0];
+	struct trusted_key_options *o = prep->type_data[0];
+
+	if (p) {
+		memset(p->key, 0, p->key_len);
+		kfree(p);
+	}
+	kfree(o);
+}
+
+/*
+ * trusted_instantiate - create a new trusted key
+ *
+ * Unseal an existing trusted blob or, for a new key, get a
+ * random key, then seal and create a trusted key-type key,
+ * adding it to the specified keyring.
+ *
+ * On success, return 0. Otherwise return errno.
+ */
+static int trusted_instantiate(struct key *key,
+			       struct key_preparsed_payload *prep)
+{
+	struct trusted_key_payload *payload = prep->payload[0];
+	struct trusted_key_options *options = prep->type_data[0];
+	long key_cmd = (unsigned long)prep->type_data[1];
+	int ret;
+	size_t key_len;
 
 	switch (key_cmd) {
 	case Opt_load:
 		ret = key_unseal(payload, options);
 		dump_payload(payload);
 		dump_options(options);
-		if (ret < 0)
+		if (ret < 0) {
 			pr_info("trusted_key: key_unseal failed (%d)\n", ret);
+			return ret;
+		}
 		break;
 	case Opt_new:
 		key_len = payload->key_len;
 		ret = tpm_get_random(TPM_ANY_NUM, payload->key, key_len);
 		if (ret != key_len) {
 			pr_info("trusted_key: key_create failed (%d)\n", ret);
-			goto out;
+			return ret;
 		}
 		ret = key_seal(payload, options);
-		if (ret < 0)
+		if (ret < 0) {
 			pr_info("trusted_key: key_seal failed (%d)\n", ret);
+			return ret;
+		}
 		break;
 	default:
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
-	if (!ret && options->pcrlock)
+
+	if (options->pcrlock) {
 		ret = pcrlock(options->pcrlock);
-out:
-	kfree(datablob);
-	kfree(options);
-	if (!ret)
-		rcu_assign_keypointer(key, payload);
-	else
-		kfree(payload);
-	return ret;
+		if (ret < 0)
+			return ret;
+	}
+
+	rcu_assign_keypointer(key, prep->payload[0]);
+	prep->payload[0] = NULL;
+	return 0;
 }
 
 static void trusted_rcu_free(struct rcu_head *rcu)
@@ -985,39 +998,17 @@ static void trusted_rcu_free(struct rcu_head *rcu)
 static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 {
 	struct trusted_key_payload *p = key->payload.data;
-	struct trusted_key_payload *new_p;
-	struct trusted_key_options *new_o;
-	size_t datalen = prep->datalen;
-	char *datablob;
+	struct trusted_key_payload *new_p = prep->payload[0];
+	struct trusted_key_options *new_o = prep->type_data[0];
+	long key_cmd = (unsigned long)prep->type_data[1];
 	int ret = 0;
 
 	if (!p->migratable)
 		return -EPERM;
-	if (datalen <= 0 || datalen > 32767 || !prep->data)
-		return -EINVAL;
 
-	datablob = kmalloc(datalen + 1, GFP_KERNEL);
-	if (!datablob)
-		return -ENOMEM;
-	new_o = trusted_options_alloc();
-	if (!new_o) {
-		ret = -ENOMEM;
-		goto out;
-	}
-	new_p = trusted_payload_alloc(key);
-	if (!new_p) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (key_cmd != Opt_update)
+		return -EINVAL;
 
-	memcpy(datablob, prep->data, datalen);
-	datablob[datalen] = '\0';
-	ret = datablob_parse(datablob, new_p, new_o);
-	if (ret != Opt_update) {
-		ret = -EINVAL;
-		kfree(new_p);
-		goto out;
-	}
 	/* copy old key values, and reseal with new pcrs */
 	new_p->migratable = p->migratable;
 	new_p->key_len = p->key_len;
@@ -1028,23 +1019,19 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 	ret = key_seal(new_p, new_o);
 	if (ret < 0) {
 		pr_info("trusted_key: key_seal failed (%d)\n", ret);
-		kfree(new_p);
-		goto out;
+		return ret;
 	}
 	if (new_o->pcrlock) {
 		ret = pcrlock(new_o->pcrlock);
 		if (ret < 0) {
 			pr_info("trusted_key: pcrlock failed (%d)\n", ret);
-			kfree(new_p);
-			goto out;
+			return ret;
 		}
 	}
 	rcu_assign_keypointer(key, new_p);
 	call_rcu(&p->rcu, trusted_rcu_free);
-out:
-	kfree(datablob);
-	kfree(new_o);
-	return ret;
+	prep->payload[0] = NULL;
+	return 0;
 }
 
 /*
@@ -1093,13 +1080,16 @@ static void trusted_destroy(struct key *key)
 }
 
 struct key_type key_type_trusted = {
-	.name = "trusted",
-	.instantiate = trusted_instantiate,
-	.update = trusted_update,
-	.match = user_match,
-	.destroy = trusted_destroy,
-	.describe = user_describe,
-	.read = trusted_read,
+	.name		= "trusted",
+	.def_datalen	= sizeof(struct trusted_key_payload),
+	.preparse	= trusted_preparse,
+	.free_preparse	= trusted_free_preparse,
+	.instantiate	= trusted_instantiate,
+	.update		= trusted_update,
+	.match		= user_match,
+	.destroy	= trusted_destroy,
+	.describe	= user_describe,
+	.read		= trusted_read,
 };
 
 EXPORT_SYMBOL_GPL(key_type_trusted);

Re: [PATCH 6/9] KEYS: Trusted: Use key preparsing

[Mimi Zohar]

On Mon, 2013-11-04 at 16:23 +0000, David Howells wrote:
> Make use of key preparsing in trusted keys so that quota size determination
> can take place prior to keyring locking when a key is being added.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Cc'ing Dave Safford.

> ---
> 
>  security/keys/trusted.c |  190 ++++++++++++++++++++++-------------------------
>  1 file changed, 90 insertions(+), 100 deletions(-)
> 
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index e13fcf7636f7..ac444e3cfaa7 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -859,52 +859,22 @@ static int datablob_parse(char *datablob, struct trusted_key_payload *p,
>  	return ret;
>  }
> 
> -static struct trusted_key_options *trusted_options_alloc(void)
> -{
> -	struct trusted_key_options *options;
> -
> -	options = kzalloc(sizeof *options, GFP_KERNEL);
> -	if (options) {
> -		/* set any non-zero defaults */
> -		options->keytype = SRK_keytype;
> -		options->keyhandle = SRKHANDLE;
> -	}
> -	return options;
> -}
> -
> -static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
> -{
> -	struct trusted_key_payload *p = NULL;
> -	int ret;
> -
> -	ret = key_payload_reserve(key, sizeof *p);
> -	if (ret < 0)
> -		return p;
> -	p = kzalloc(sizeof *p, GFP_KERNEL);
> -	if (p)
> -		p->migratable = 1; /* migratable by default */
> -	return p;
> -}
> -
>  /*
> - * trusted_instantiate - create a new trusted key
> + * trusted_preparse - Preparse data for an trusted key
>   *
> - * Unseal an existing trusted blob or, for a new key, get a
> - * random key, then seal and create a trusted key-type key,
> - * adding it to the specified keyring.
> + * Decrypt an existing encrypted datablob or create a new encrypted key
> + * based on a kernel random number.

Not sure why the change in terminology from unsealing/trusted blob to
decrypt/encrypted blob.

>   * On success, return 0. Otherwise return errno.
>   */
> -static int trusted_instantiate(struct key *key,
> -			       struct key_preparsed_payload *prep)
> +static int trusted_preparse(struct key_preparsed_payload *prep)
>  {
>  	struct trusted_key_payload *payload = NULL;
>  	struct trusted_key_options *options = NULL;
>  	size_t datalen = prep->datalen;
>  	char *datablob;
> -	int ret = 0;
> -	int key_cmd;
> -	size_t key_len;
> +	long key_cmd;
> +	int ret = -ENOMEM;
> 
>  	if (datalen <= 0 || datalen > 32767 || !prep->data)
>  		return -EINVAL;
> @@ -915,16 +885,20 @@ static int trusted_instantiate(struct key *key,
>  	memcpy(datablob, prep->data, datalen);
>  	datablob[datalen] = '\0';
> 
> -	options = trusted_options_alloc();
> -	if (!options) {
> -		ret = -ENOMEM;
> +	payload = kzalloc(sizeof(*payload), GFP_KERNEL);
> +	if (!payload)
>  		goto out;
> -	}
> -	payload = trusted_payload_alloc(key);
> -	if (!payload) {
> -		ret = -ENOMEM;
> +	prep->payload[0] = payload;
> +
> +	options = kzalloc(sizeof(*options), GFP_KERNEL);
> +	if (!options)
>  		goto out;
> -	}
> +	prep->type_data[0] = options;
> +
> +	/* set any non-zero defaults */
> +	payload->migratable = 1; /* migratable by default */
> +	options->keytype = SRK_keytype;
> +	options->keyhandle = SRKHANDLE;
> 
>  	key_cmd = datablob_parse(datablob, payload, options);
>  	if (key_cmd < 0) {
> @@ -932,42 +906,81 @@ static int trusted_instantiate(struct key *key,
>  		goto out;
>  	}
> 
> +	prep->type_data[1] = (void *)key_cmd;
> +
>  	dump_payload(payload);
>  	dump_options(options);
> +out:
> +	kfree(datablob);
> +	return ret;

trusted_preparse() always fails, since ret is initialized to -ENOMEM and
never set.

Mimi

> +}
> +
> +static void trusted_free_preparse(struct key_preparsed_payload *prep)
> +{
> +	struct trusted_key_payload *p = prep->payload[0];
> +	struct trusted_key_options *o = prep->type_data[0];
> +
> +	if (p) {
> +		memset(p->key, 0, p->key_len);
> +		kfree(p);
> +	}
> +	kfree(o);
> +}
> +
> +/*
> + * trusted_instantiate - create a new trusted key
> + *
> + * Unseal an existing trusted blob or, for a new key, get a
> + * random key, then seal and create a trusted key-type key,
> + * adding it to the specified keyring.
> + *
> + * On success, return 0. Otherwise return errno.
> + */
> +static int trusted_instantiate(struct key *key,
> +			       struct key_preparsed_payload *prep)
> +{
> +	struct trusted_key_payload *payload = prep->payload[0];
> +	struct trusted_key_options *options = prep->type_data[0];
> +	long key_cmd = (unsigned long)prep->type_data[1];
> +	int ret;
> +	size_t key_len;
> 
>  	switch (key_cmd) {
>  	case Opt_load:
>  		ret = key_unseal(payload, options);
>  		dump_payload(payload);
>  		dump_options(options);
> -		if (ret < 0)
> +		if (ret < 0) {
>  			pr_info("trusted_key: key_unseal failed (%d)\n", ret);
> +			return ret;
> +		}
>  		break;
>  	case Opt_new:
>  		key_len = payload->key_len;
>  		ret = tpm_get_random(TPM_ANY_NUM, payload->key, key_len);
>  		if (ret != key_len) {
>  			pr_info("trusted_key: key_create failed (%d)\n", ret);
> -			goto out;
> +			return ret;
>  		}
>  		ret = key_seal(payload, options);
> -		if (ret < 0)
> +		if (ret < 0) {
>  			pr_info("trusted_key: key_seal failed (%d)\n", ret);
> +			return ret;
> +		}
>  		break;
>  	default:
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>  	}
> -	if (!ret && options->pcrlock)
> +
> +	if (options->pcrlock) {
>  		ret = pcrlock(options->pcrlock);
> -out:
> -	kfree(datablob);
> -	kfree(options);
> -	if (!ret)
> -		rcu_assign_keypointer(key, payload);
> -	else
> -		kfree(payload);
> -	return ret;
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	rcu_assign_keypointer(key, prep->payload[0]);
> +	prep->payload[0] = NULL;
> +	return 0;
>  }
> 
>  static void trusted_rcu_free(struct rcu_head *rcu)
> @@ -985,39 +998,17 @@ static void trusted_rcu_free(struct rcu_head *rcu)
>  static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
>  {
>  	struct trusted_key_payload *p = key->payload.data;
> -	struct trusted_key_payload *new_p;
> -	struct trusted_key_options *new_o;
> -	size_t datalen = prep->datalen;
> -	char *datablob;
> +	struct trusted_key_payload *new_p = prep->payload[0];
> +	struct trusted_key_options *new_o = prep->type_data[0];
> +	long key_cmd = (unsigned long)prep->type_data[1];
>  	int ret = 0;
> 
>  	if (!p->migratable)
>  		return -EPERM;
> -	if (datalen <= 0 || datalen > 32767 || !prep->data)
> -		return -EINVAL;
> 
> -	datablob = kmalloc(datalen + 1, GFP_KERNEL);
> -	if (!datablob)
> -		return -ENOMEM;
> -	new_o = trusted_options_alloc();
> -	if (!new_o) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -	new_p = trusted_payload_alloc(key);
> -	if (!new_p) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> +	if (key_cmd != Opt_update)
> +		return -EINVAL;
> 
> -	memcpy(datablob, prep->data, datalen);
> -	datablob[datalen] = '\0';
> -	ret = datablob_parse(datablob, new_p, new_o);
> -	if (ret != Opt_update) {
> -		ret = -EINVAL;
> -		kfree(new_p);
> -		goto out;
> -	}
>  	/* copy old key values, and reseal with new pcrs */
>  	new_p->migratable = p->migratable;
>  	new_p->key_len = p->key_len;
> @@ -1028,23 +1019,19 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
>  	ret = key_seal(new_p, new_o);
>  	if (ret < 0) {
>  		pr_info("trusted_key: key_seal failed (%d)\n", ret);
> -		kfree(new_p);
> -		goto out;
> +		return ret;
>  	}
>  	if (new_o->pcrlock) {
>  		ret = pcrlock(new_o->pcrlock);
>  		if (ret < 0) {
>  			pr_info("trusted_key: pcrlock failed (%d)\n", ret);
> -			kfree(new_p);
> -			goto out;
> +			return ret;
>  		}
>  	}
>  	rcu_assign_keypointer(key, new_p);
>  	call_rcu(&p->rcu, trusted_rcu_free);
> -out:
> -	kfree(datablob);
> -	kfree(new_o);
> -	return ret;
> +	prep->payload[0] = NULL;
> +	return 0;
>  }
> 
>  /*
> @@ -1093,13 +1080,16 @@ static void trusted_destroy(struct key *key)
>  }
> 
>  struct key_type key_type_trusted = {
> -	.name = "trusted",
> -	.instantiate = trusted_instantiate,
> -	.update = trusted_update,
> -	.match = user_match,
> -	.destroy = trusted_destroy,
> -	.describe = user_describe,
> -	.read = trusted_read,
> +	.name		= "trusted",
> +	.def_datalen	= sizeof(struct trusted_key_payload),
> +	.preparse	= trusted_preparse,
> +	.free_preparse	= trusted_free_preparse,
> +	.instantiate	= trusted_instantiate,
> +	.update		= trusted_update,
> +	.match		= user_match,
> +	.destroy	= trusted_destroy,
> +	.describe	= user_describe,
> +	.read		= trusted_read,
>  };
> 
>  EXPORT_SYMBOL_GPL(key_type_trusted);
> 
> --
> 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
> 

Re: [PATCH 6/9] KEYS: Trusted: Use key preparsing

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> > - * trusted_instantiate - create a new trusted key
> > + * trusted_preparse - Preparse data for an trusted key
> >   *
> > - * Unseal an existing trusted blob or, for a new key, get a
> > - * random key, then seal and create a trusted key-type key,
> > - * adding it to the specified keyring.
> > + * Decrypt an existing encrypted datablob or create a new encrypted key
> > + * based on a kernel random number.
> 
> Not sure why the change in terminology from unsealing/trusted blob to
> decrypt/encrypted blob.

Because the "Unseal an existing trusted blob ..." bit is still attached to the
trusted_instantiate() function.  However, even what I wrote is still not a
good description of the trusted_preparse(), so I'll reduce it to just:

	/*
	 * trusted_preparse - Preparse the payload data for an trusted key
	 *
	 * On success, return 0. Otherwise return errno.
	 */

> trusted_preparse() always fails, since ret is initialized to -ENOMEM and
> never set.

Fixed.

Thanks,
David

[PATCH 7/9] KEYS: Add a keyctl function to alter/control a key in type-dependent way

[David Howells]

Add a function to permit a key to be altered or controlled in a type-dependent
way.  This is given text strings as its command and argument parameters and is
permitted to return a string to a maximum buffer size (including NUL):

	long keyctl_control(key_serial_t keyid,
			    const char *command,
			    char *reply_buffer,
			    size_t reply_size);

The caller must have WRITE permission on a key to be able to perform this
function.  The type is not required to implement this, but if it does, it must
perform its own locking against other 'writes' using the key semaphore.

The command string must begin with the type name and a space so that the method
can check that the command is from its expected set (it is permitted, however,
to honour commands from another type's set).  This should be followed by the
command name and then, optionally, another space and whatever arguments are
required.  The command can be up to 1MB in size including the NUL terminator.

The reply buffer is optional and can be up to 1MB in size.  The actual size of
the reply will be returned and, if necessary, the reply will be truncated to
reply_size.

This can be invoked from the keyctl command in userspace.  One example would be
to use this to change the master key used by an encrypted key:

	keyctl control 1234 "encrypted change-master-key 6789"

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

 Documentation/security/keys.txt |   34 +++++++++++++
 include/linux/key-type.h        |    6 ++
 include/uapi/linux/keyctl.h     |    1 
 security/keys/compat.c          |    6 ++
 security/keys/internal.h        |    2 +
 security/keys/keyctl.c          |  104 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 153 insertions(+)

diff --git a/Documentation/security/keys.txt b/Documentation/security/keys.txt
index 3bbec087fc5f..a5792bcb64b2 100644
--- a/Documentation/security/keys.txt
+++ b/Documentation/security/keys.txt
@@ -1331,6 +1331,40 @@ The structure has a number of fields, some of which are mandatory:
      	 The authorisation key.
 
 
+ (*) long (*control)(struct key *key, char *command, char *reply,
+		     size_t reply_size);
+
+     This method is optional.  If provided, keyctl_control() can be invoked
+     from userspace to perform some type-specific operation upon the key.  If
+     the method is not implemented then error EOPNOTSUPP will be returned.
+
+     The method must provide its own locking against other 'writes' using the
+     key semaphore.
+
+     The command argument will point to a NUL-terminated string up to 1MB in
+     size, including the NUL terminator.  The method may modify the command
+     buffer (eg. with strsep()).
+
+     The command should begin with the type name and a space so that the method
+     can check that the command is from its expected set.  It is permitted,
+     however, to honour commands from another type's set.  This should be
+     followed by the command name and then, optionally, another space and
+     whatever arguments are required.
+
+     The reply buffer will be NULL if userspace didn't ask for a reply.
+     Otherwise it will be a kernel-space buffer the size of which was specified
+     by userspace (max 1MB).  The actual size of the reply should be returned
+     (and can be larger than reply_size).  The caller will copy back the
+     contents of the reply buffer up to reply_size.
+
+     An example might be:
+
+	encrypted change-master-key <key-id>
+
+     The return value is the reply size, 0 (if no reply) or a negative error
+     code.
+
+
 ============================
 REQUEST-KEY CALLBACK SERVICE
 ============================
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 44792ee649de..610669c780f7 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -129,6 +129,12 @@ struct key_type {
 	 */
 	request_key_actor_t request_key;
 
+	/* Control or alter a key (optional)
+	 * - The command string can be modified (eg. with strsep()).
+	 */
+	long (*control)(struct key *key, char *command,
+			char *reply, size_t reply_size);
+
 	/* internal fields */
 	struct list_head	link;		/* link in types list */
 	struct lock_class_key	lock_class;	/* key->sem lock class */
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 840cb990abe2..9687b9c726b2 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -57,5 +57,6 @@
 #define KEYCTL_INSTANTIATE_IOV		20	/* instantiate a partially constructed key */
 #define KEYCTL_INVALIDATE		21	/* invalidate a key */
 #define KEYCTL_GET_PERSISTENT		22	/* get a user's persistent keyring */
+#define KEYCTL_CONTROL			23	/* control/alter a key */
 
 #endif /*  _LINUX_KEYCTL_H */
diff --git a/security/keys/compat.c b/security/keys/compat.c
index bbd32c729dbb..78d82d86471f 100644
--- a/security/keys/compat.c
+++ b/security/keys/compat.c
@@ -141,6 +141,12 @@ asmlinkage long compat_sys_keyctl(u32 option,
 	case KEYCTL_GET_PERSISTENT:
 		return keyctl_get_persistent(arg2, arg3);
 
+	case KEYCTL_CONTROL:
+		return keyctl_control_key(arg2,
+					  compat_ptr(arg3),
+					  compat_ptr(arg4),
+					  arg5);
+
 	default:
 		return -EOPNOTSUPP;
 	}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 80b2aac4f50c..4a6944374b0c 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -264,6 +264,8 @@ static inline long keyctl_get_persistent(uid_t uid, key_serial_t destring)
 	return -EOPNOTSUPP;
 }
 #endif
+extern long keyctl_control_key(key_serial_t, const char __user *,
+			       char __user *, size_t);
 
 /*
  * Debugging key validation
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index cee72ce64222..46fe18fe6d16 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1565,6 +1565,104 @@ error_keyring:
 }
 
 /*
+ * Control or alter a key in a type-dependent way.
+ *
+ * The key must grant the caller Write permission and the key type must support
+ * the control op for this to work.
+ *
+ * If successful, the full reply size will be returned.  If the key type does
+ * not support the control op, then -EOPNOTSUPP will be returned.
+ */
+long keyctl_control_key(key_serial_t id,
+			const char __user *_command,
+			char __user *_reply_buffer,
+			size_t reply_size)
+{
+	struct key *key;
+	key_ref_t key_ref;
+	char *command, *reply_buffer = NULL;
+	long cmd_size, ret;
+
+	ret = -EINVAL;
+	if (!_command || reply_size > 1024 * 1024)
+		goto error;
+
+	cmd_size = strnlen_user(_command, 1024 * 1024);
+	if (cmd_size < 0) {
+		ret = cmd_size;
+		goto error;
+	}
+
+	if (cmd_size > 1024 * 1024 - 1)
+		goto error;
+
+	command = vmalloc(cmd_size + 1);
+	if (!command) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	if (copy_from_user(command, _command, cmd_size) != 0) {
+		ret = -EFAULT;
+		goto error_cmd;
+	}
+	if (command[0] == '\0')
+		goto error_cmd;
+
+	if (_reply_buffer) {
+		if (reply_size <= 0)
+			goto error_cmd;
+		if (!access_ok(VERIFY_WRITE, _reply_buffer, reply_size)) {
+			ret = -EFAULT;
+			goto error_cmd;
+		}
+		reply_buffer = vmalloc(reply_size);
+		if (!reply_buffer)
+			goto error_cmd;
+	}
+
+	/* find the target key (which must be writable) */
+	key_ref = lookup_user_key(id, 0, KEY_WRITE);
+	if (IS_ERR(key_ref)) {
+		ret = PTR_ERR(key_ref);
+		goto error_reply;
+	}
+	key = key_ref_to_ptr(key_ref);
+
+	/* call the control function if available */
+	ret = -EOPNOTSUPP;
+	if (!key->type->control)
+		goto error_key;
+
+	ret = key->type->control(key, command, reply_buffer, reply_size);
+	if (ret < 0)
+		goto error_key;
+
+	/* Return the reply.  It's possible that the available reply would have
+	 * exceeded the buffer size, so we return the ideal size but truncate
+	 * if we would otherwise overrun the buffer.
+	 */
+	if (reply_buffer) {
+		if (ret < reply_size)
+			reply_size = ret;
+		if (reply_size > 0 &&
+		    copy_to_user(_reply_buffer, reply_buffer, reply_size) != 0) {
+			ret = -EFAULT;
+			goto error_key;
+		}
+	}
+
+error_key:
+	key_put(key);
+error_reply:
+	vfree(reply_buffer);
+error_cmd:
+	vfree(command);
+error:
+	return ret;
+}
+
+/*
  * The key control system call
  */
 SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
@@ -1670,6 +1768,12 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case KEYCTL_GET_PERSISTENT:
 		return keyctl_get_persistent((uid_t)arg2, (key_serial_t)arg3);
 
+	case KEYCTL_CONTROL:
+		return keyctl_control_key((key_serial_t)arg2,
+					  (const char __user *)arg3,
+					  (char __user *)arg4,
+					  (size_t)arg5);
+
 	default:
 		return -EOPNOTSUPP;
 	}

[PATCH 8/9] KEYS: Implement keyctl control for encrypted keys

[David Howells]

Implement "keyctl control" for encrypted keys rather than trying to do this
with the update method (which won't function correctly when add_key() tries to
update an existing key).

Provide a command to change the master key:

	keyctl control <keyid> "encrypted change-master-key <m-key-id>"

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

 security/keys/encrypted-keys/encrypted.c |   56 ++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 9e1e005c7596..f9e7b808fa47 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -884,6 +884,61 @@ out:
 }
 
 /*
+ * encrypted_control - control an encrypted key in a type-specific way
+ */
+static long encrypted_control(struct key *key, char *command,
+			      char *reply, size_t reply_size)
+{
+	static const char expected_command[] = "encrypted change-master-key ";
+	struct encrypted_key_payload *epayload, *new_epayload;
+	char *new_master_desc = NULL;
+	const char *format = NULL;
+	size_t datalen;
+	int ret;
+
+	if (memcmp(command, expected_command, sizeof(expected_command) - 1) != 0)
+		return -EINVAL;
+	command += sizeof(expected_command) - 1;
+	datalen = strlen(command);
+
+	if (datalen <= 0 || datalen > 32767)
+		return -EINVAL;
+
+	ret = datablob_parse(command, &format, &new_master_desc, NULL, NULL);
+	if (ret < 0)
+		return ret;
+
+	down_write(&key->sem);
+	epayload = rcu_dereference_protected(key->payload.rcudata, &key->sem);
+
+	ret = valid_master_desc(new_master_desc, epayload->master_desc);
+	if (ret < 0) {
+		up_write(&key->sem);
+		return ret;
+	}
+
+	new_epayload = encrypted_key_alloc(key, epayload->format,
+					   new_master_desc, epayload->datalen);
+	if (IS_ERR(new_epayload)) {
+		up_write(&key->sem);
+		return PTR_ERR(new_epayload);
+	}
+
+	__ekey_init(new_epayload, epayload->format, new_master_desc,
+		    epayload->datalen);
+
+	memcpy(new_epayload->iv, epayload->iv, ivsize);
+	memcpy(new_epayload->payload_data, epayload->payload_data,
+	       epayload->payload_datalen);
+
+	rcu_assign_keypointer(key, new_epayload);
+
+	up_write(&key->sem);
+	call_rcu(&epayload->rcu, encrypted_rcu_free);
+	return 0;
+}
+
+/*
  * encrypted_read - format and copy the encrypted data to userspace
  *
  * The resulting datablob format is:
@@ -974,6 +1029,7 @@ struct key_type key_type_encrypted = {
 	.destroy = encrypted_destroy,
 	.describe = user_describe,
 	.read = encrypted_read,
+	.control = encrypted_control,
 };
 EXPORT_SYMBOL_GPL(key_type_encrypted);
 

[PATCH 9/9] KEYS: Fix encrypted key type update method

[David Howells]

The encrypted key type was using the update method to change the master key
used to encrypt a key without passing in all the requisite parameters to
completely replace the key contents (it was taking some parameters from the
old key contents).  Unfortunately, this has a number of problems:

 (1) Update is conceptually meant to be the same as add_key() except that it
     replaces the contents of the nominated key completely rather than creating
     a new key.

 (2) add_key() can call ->update() if it detects that the key exists.  This can
     cause the operation to fail if the caller embedded the wrong command in
     the payload when they called it.  The caller cannot know what the right
     command is without someway to lock the keyring.

 (3) keyctl_update() and add_key() can thus race with adding, linking,
     requesting and unlinking a key.

The best way to fix this is to offload this operation to keyctl_control() and
make encrypted_update() just replace the contents of a key entirely (or maybe
just not permit updates if this would be a problem).

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

 security/keys/encrypted-keys/encrypted.c |  101 ++++++++++++++----------------
 1 file changed, 47 insertions(+), 54 deletions(-)

diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index f9e7b808fa47..c9e4fa5b1e2c 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -595,7 +595,7 @@ out:
 }
 
 /* Allocate memory for decrypted key and datablob. */
-static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
+static struct encrypted_key_payload *encrypted_key_alloc(size_t *_quotalen,
 							 const char *format,
 							 const char *master_desc,
 							 const char *datalen)
@@ -632,10 +632,7 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
 	datablob_len = format_len + 1 + strlen(master_desc) + 1
 	    + strlen(datalen) + 1 + ivsize + 1 + encrypted_datalen;
 
-	ret = key_payload_reserve(key, payload_datalen + datablob_len
-				  + HASH_SIZE + 1);
-	if (ret < 0)
-		return ERR_PTR(ret);
+	*_quotalen = payload_datalen + datablob_len + HASH_SIZE + 1;
 
 	epayload = kzalloc(sizeof(*epayload) + payload_datalen +
 			   datablob_len + HASH_SIZE + 1, GFP_KERNEL);
@@ -773,8 +770,7 @@ static int encrypted_init(struct encrypted_key_payload *epayload,
  *
  * On success, return 0. Otherwise return errno.
  */
-static int encrypted_instantiate(struct key *key,
-				 struct key_preparsed_payload *prep)
+static int encrypted_preparse(struct key_preparsed_payload *prep)
 {
 	struct encrypted_key_payload *epayload = NULL;
 	char *datablob = NULL;
@@ -798,25 +794,55 @@ static int encrypted_instantiate(struct key *key,
 	if (ret < 0)
 		goto out;
 
-	epayload = encrypted_key_alloc(key, format, master_desc,
+	epayload = encrypted_key_alloc(&prep->quotalen, format, master_desc,
 				       decrypted_datalen);
 	if (IS_ERR(epayload)) {
 		ret = PTR_ERR(epayload);
 		goto out;
 	}
-	ret = encrypted_init(epayload, key->description, format, master_desc,
+	ret = encrypted_init(epayload, prep->description, format, master_desc,
 			     decrypted_datalen, hex_encoded_iv);
 	if (ret < 0) {
 		kfree(epayload);
 		goto out;
 	}
 
-	rcu_assign_keypointer(key, epayload);
+	prep->payload[0] = epayload;
+
 out:
 	kfree(datablob);
 	return ret;
 }
 
+/*
+ * encrypted_free_preparse - Clean up preparse data for an encrypted key
+ */
+static void encrypted_free_preparse(struct key_preparsed_payload *prep)
+{
+	struct encrypted_key_payload *epayload = prep->payload[0];
+
+	if (epayload) {
+		memset(epayload->decrypted_data, 0,
+		       epayload->decrypted_datalen);
+		kfree(epayload);
+	}
+}
+
+static int encrypted_instantiate(struct key *key,
+				 struct key_preparsed_payload *prep)
+{
+	struct encrypted_key_payload *epayload = prep->payload[0];
+	int ret;
+
+	ret = key_payload_reserve(key, prep->quotalen);
+	if (ret < 0)
+		return ret;
+
+	rcu_assign_keypointer(key, epayload);
+	prep->payload[0] = NULL;
+	return 0;
+}
+
 static void encrypted_rcu_free(struct rcu_head *rcu)
 {
 	struct encrypted_key_payload *epayload;
@@ -837,50 +863,15 @@ static void encrypted_rcu_free(struct rcu_head *rcu)
  */
 static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 {
-	struct encrypted_key_payload *epayload = key->payload.data;
-	struct encrypted_key_payload *new_epayload;
-	char *buf;
-	char *new_master_desc = NULL;
-	const char *format = NULL;
-	size_t datalen = prep->datalen;
-	int ret = 0;
-
-	if (datalen <= 0 || datalen > 32767 || !prep->data)
-		return -EINVAL;
+	struct encrypted_key_payload *old;
+	struct encrypted_key_payload *new = prep->payload[0];
 
-	buf = kmalloc(datalen + 1, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	buf[datalen] = 0;
-	memcpy(buf, prep->data, datalen);
-	ret = datablob_parse(buf, &format, &new_master_desc, NULL, NULL);
-	if (ret < 0)
-		goto out;
+	old = rcu_dereference_protected(key->payload.rcudata, &key->sem);
 
-	ret = valid_master_desc(new_master_desc, epayload->master_desc);
-	if (ret < 0)
-		goto out;
-
-	new_epayload = encrypted_key_alloc(key, epayload->format,
-					   new_master_desc, epayload->datalen);
-	if (IS_ERR(new_epayload)) {
-		ret = PTR_ERR(new_epayload);
-		goto out;
-	}
-
-	__ekey_init(new_epayload, epayload->format, new_master_desc,
-		    epayload->datalen);
-
-	memcpy(new_epayload->iv, epayload->iv, ivsize);
-	memcpy(new_epayload->payload_data, epayload->payload_data,
-	       epayload->payload_datalen);
-
-	rcu_assign_keypointer(key, new_epayload);
-	call_rcu(&epayload->rcu, encrypted_rcu_free);
-out:
-	kfree(buf);
-	return ret;
+	rcu_assign_keypointer(key, new);
+	prep->payload[0] = NULL;
+	call_rcu(&old->rcu, encrypted_rcu_free);
+	return 0;
 }
 
 /*
@@ -893,7 +884,7 @@ static long encrypted_control(struct key *key, char *command,
 	struct encrypted_key_payload *epayload, *new_epayload;
 	char *new_master_desc = NULL;
 	const char *format = NULL;
-	size_t datalen;
+	size_t datalen, quotalen;
 	int ret;
 
 	if (memcmp(command, expected_command, sizeof(expected_command) - 1) != 0)
@@ -917,7 +908,7 @@ static long encrypted_control(struct key *key, char *command,
 		return ret;
 	}
 
-	new_epayload = encrypted_key_alloc(key, epayload->format,
+	new_epayload = encrypted_key_alloc(&quotalen, epayload->format,
 					   new_master_desc, epayload->datalen);
 	if (IS_ERR(new_epayload)) {
 		up_write(&key->sem);
@@ -1023,6 +1014,8 @@ static void encrypted_destroy(struct key *key)
 
 struct key_type key_type_encrypted = {
 	.name = "encrypted",
+	.preparse = encrypted_preparse,
+	.free_preparse = encrypted_free_preparse,
 	.instantiate = encrypted_instantiate,
 	.update = encrypted_update,
 	.match = user_match,

Re: [PATCH 9/9] KEYS: Fix encrypted key type update method

[Mimi Zohar]

On Mon, 2013-11-04 at 16:23 +0000, David Howells wrote:
> The encrypted key type was using the update method to change the master key
> used to encrypt a key without passing in all the requisite parameters to
> completely replace the key contents (it was taking some parameters from the
> old key contents).  Unfortunately, this has a number of problems:
> 
>  (1) Update is conceptually meant to be the same as add_key() except that it
>      replaces the contents of the nominated key completely rather than creating
>      a new key.
> 
>  (2) add_key() can call ->update() if it detects that the key exists.  This can
>      cause the operation to fail if the caller embedded the wrong command in
>      the payload when they called it.  The caller cannot know what the right
>      command is without someway to lock the keyring.
> 
>  (3) keyctl_update() and add_key() can thus race with adding, linking,
>      requesting and unlinking a key.
> 
> The best way to fix this is to offload this operation to keyctl_control() and
> make encrypted_update() just replace the contents of a key entirely (or maybe
> just not permit updates if this would be a problem).
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

The code looks good, other than breaking the existing userspace/kernel
API, but I haven't tested it yet.  Is there a keyutils git repo with a
version of keyctl that supports the control option?

A couple of minor comments:
- type size_t is unsigned, no need to verify that it is negative.
- missing Documentation/security/keys-trusted-encrypted.txt updates
- the encrypted_preparse() comment still says 'encrypted_instantiate'

thanks,

Mimi

> ---
> 
>  security/keys/encrypted-keys/encrypted.c |  101 ++++++++++++++----------------
>  1 file changed, 47 insertions(+), 54 deletions(-)
> 
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index f9e7b808fa47..c9e4fa5b1e2c 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -595,7 +595,7 @@ out:
>  }
> 
>  /* Allocate memory for decrypted key and datablob. */
> -static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
> +static struct encrypted_key_payload *encrypted_key_alloc(size_t *_quotalen,
>  							 const char *format,
>  							 const char *master_desc,
>  							 const char *datalen)
> @@ -632,10 +632,7 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
>  	datablob_len = format_len + 1 + strlen(master_desc) + 1
>  	    + strlen(datalen) + 1 + ivsize + 1 + encrypted_datalen;
> 
> -	ret = key_payload_reserve(key, payload_datalen + datablob_len
> -				  + HASH_SIZE + 1);
> -	if (ret < 0)
> -		return ERR_PTR(ret);
> +	*_quotalen = payload_datalen + datablob_len + HASH_SIZE + 1;
> 
>  	epayload = kzalloc(sizeof(*epayload) + payload_datalen +
>  			   datablob_len + HASH_SIZE + 1, GFP_KERNEL);
> @@ -773,8 +770,7 @@ static int encrypted_init(struct encrypted_key_payload *epayload,
>   *
>   * On success, return 0. Otherwise return errno.
>   */
> -static int encrypted_instantiate(struct key *key,
> -				 struct key_preparsed_payload *prep)
> +static int encrypted_preparse(struct key_preparsed_payload *prep)

>  {
>  	struct encrypted_key_payload *epayload = NULL;
>  	char *datablob = NULL;
> @@ -798,25 +794,55 @@ static int encrypted_instantiate(struct key *key,
>  	if (ret < 0)
>  		goto out;
> 
> -	epayload = encrypted_key_alloc(key, format, master_desc,
> +	epayload = encrypted_key_alloc(&prep->quotalen, format, master_desc,
>  				       decrypted_datalen);
>  	if (IS_ERR(epayload)) {
>  		ret = PTR_ERR(epayload);
>  		goto out;
>  	}
> -	ret = encrypted_init(epayload, key->description, format, master_desc,
> +	ret = encrypted_init(epayload, prep->description, format, master_desc,
>  			     decrypted_datalen, hex_encoded_iv);
>  	if (ret < 0) {
>  		kfree(epayload);
>  		goto out;
>  	}
> 
> -	rcu_assign_keypointer(key, epayload);
> +	prep->payload[0] = epayload;
> +
>  out:
>  	kfree(datablob);
>  	return ret;
>  }
> 
> +/*
> + * encrypted_free_preparse - Clean up preparse data for an encrypted key
> + */
> +static void encrypted_free_preparse(struct key_preparsed_payload *prep)
> +{
> +	struct encrypted_key_payload *epayload = prep->payload[0];
> +
> +	if (epayload) {
> +		memset(epayload->decrypted_data, 0,
> +		       epayload->decrypted_datalen);
> +		kfree(epayload);
> +	}
> +}
> +
> +static int encrypted_instantiate(struct key *key,
> +				 struct key_preparsed_payload *prep)
> +{
> +	struct encrypted_key_payload *epayload = prep->payload[0];
> +	int ret;
> +
> +	ret = key_payload_reserve(key, prep->quotalen);
> +	if (ret < 0)
> +		return ret;
> +
> +	rcu_assign_keypointer(key, epayload);
> +	prep->payload[0] = NULL;
> +	return 0;
> +}
> +
>  static void encrypted_rcu_free(struct rcu_head *rcu)
>  {
>  	struct encrypted_key_payload *epayload;
> @@ -837,50 +863,15 @@ static void encrypted_rcu_free(struct rcu_head *rcu)
>   */
>  static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
>  {
> -	struct encrypted_key_payload *epayload = key->payload.data;
> -	struct encrypted_key_payload *new_epayload;
> -	char *buf;
> -	char *new_master_desc = NULL;
> -	const char *format = NULL;
> -	size_t datalen = prep->datalen;
> -	int ret = 0;
> -
> -	if (datalen <= 0 || datalen > 32767 || !prep->data)
> -		return -EINVAL;
> +	struct encrypted_key_payload *old;
> +	struct encrypted_key_payload *new = prep->payload[0];
> 
> -	buf = kmalloc(datalen + 1, GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> -
> -	buf[datalen] = 0;
> -	memcpy(buf, prep->data, datalen);
> -	ret = datablob_parse(buf, &format, &new_master_desc, NULL, NULL);
> -	if (ret < 0)
> -		goto out;
> +	old = rcu_dereference_protected(key->payload.rcudata, &key->sem);
> 
> -	ret = valid_master_desc(new_master_desc, epayload->master_desc);
> -	if (ret < 0)
> -		goto out;
> -
> -	new_epayload = encrypted_key_alloc(key, epayload->format,
> -					   new_master_desc, epayload->datalen);
> -	if (IS_ERR(new_epayload)) {
> -		ret = PTR_ERR(new_epayload);
> -		goto out;
> -	}
> -
> -	__ekey_init(new_epayload, epayload->format, new_master_desc,
> -		    epayload->datalen);
> -
> -	memcpy(new_epayload->iv, epayload->iv, ivsize);
> -	memcpy(new_epayload->payload_data, epayload->payload_data,
> -	       epayload->payload_datalen);
> -
> -	rcu_assign_keypointer(key, new_epayload);
> -	call_rcu(&epayload->rcu, encrypted_rcu_free);
> -out:
> -	kfree(buf);
> -	return ret;
> +	rcu_assign_keypointer(key, new);
> +	prep->payload[0] = NULL;
> +	call_rcu(&old->rcu, encrypted_rcu_free);
> +	return 0;
>  }
> 
>  /*
> @@ -893,7 +884,7 @@ static long encrypted_control(struct key *key, char *command,
>  	struct encrypted_key_payload *epayload, *new_epayload;
>  	char *new_master_desc = NULL;
>  	const char *format = NULL;
> -	size_t datalen;
> +	size_t datalen, quotalen;
>  	int ret;
> 
>  	if (memcmp(command, expected_command, sizeof(expected_command) - 1) != 0)
> @@ -917,7 +908,7 @@ static long encrypted_control(struct key *key, char *command,
>  		return ret;
>  	}
> 
> -	new_epayload = encrypted_key_alloc(key, epayload->format,
> +	new_epayload = encrypted_key_alloc(&quotalen, epayload->format,
>  					   new_master_desc, epayload->datalen);
>  	if (IS_ERR(new_epayload)) {
>  		up_write(&key->sem);
> @@ -1023,6 +1014,8 @@ static void encrypted_destroy(struct key *key)
> 
>  struct key_type key_type_encrypted = {
>  	.name = "encrypted",
> +	.preparse = encrypted_preparse,
> +	.free_preparse = encrypted_free_preparse,
>  	.instantiate = encrypted_instantiate,
>  	.update = encrypted_update,
>  	.match = user_match,
> 
> --
> 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
> 

Re: [PATCH 9/9] KEYS: Fix encrypted key type update method

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> Is there a keyutils git repo with a version of keyctl that supports the
> control option?

http://git.kernel.org/cgit/linux/kernel/git/dhowells/keyutils.git/log/?h=development

> - type size_t is unsigned, no need to verify that it is negative.

It doesn't hurt either...

> - missing Documentation/security/keys-trusted-encrypted.txt updates

Fixed (see diff below), but I suspect trusted_update() also needs scrutiny.

> - the encrypted_preparse() comment still says 'encrypted_instantiate'

Fixed.

David
---
diff --git a/Documentation/security/keys-trusted-encrypted.txt b/Documentation/security/keys-trusted-encrypted.txt
index e105ae97a4f5..78794adf445d 100644
--- a/Documentation/security/keys-trusted-encrypted.txt
+++ b/Documentation/security/keys-trusted-encrypted.txt
@@ -61,7 +61,7 @@ Usage:
     keyctl add encrypted name "new [format] key-type:master-key-name keylen"
         ring
     keyctl add encrypted name "load hex_blob" ring
-    keyctl update keyid "update key-type:master-key-name"
+    keyctl control keyid encrypted change-master-key "key-type:master-key-name"
 
 format:= 'default | ecryptfs'
 key-type:= 'trusted' | 'user'

Re: [PATCH 9/9] KEYS: Fix encrypted key type update method

[Mimi Zohar]

On Thu, 2013-11-14 at 17:59 +0000, David Howells wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > Is there a keyutils git repo with a version of keyctl that supports the
> > control option?
> 
> http://git.kernel.org/cgit/linux/kernel/git/dhowells/keyutils.git/log/?h=development

Thanks!

> > - type size_t is unsigned, no need to verify that it is negative.
> 
> It doesn't hurt either...
> 
> > - missing Documentation/security/keys-trusted-encrypted.txt updates
> 
> Fixed (see diff below), but I suspect trusted_update() also needs scrutiny.
> 
> > - the encrypted_preparse() comment still says 'encrypted_instantiate'
> 
> Fixed.
> 
> David
> ---
> diff --git a/Documentation/security/keys-trusted-encrypted.txt b/Documentation/security/keys-trusted-encrypted.txt
> index e105ae97a4f5..78794adf445d 100644
> --- a/Documentation/security/keys-trusted-encrypted.txt
> +++ b/Documentation/security/keys-trusted-encrypted.txt
> @@ -61,7 +61,7 @@ Usage:
>      keyctl add encrypted name "new [format] key-type:master-key-name keylen"
>          ring
>      keyctl add encrypted name "load hex_blob" ring
> -    keyctl update keyid "update key-type:master-key-name"
> +    keyctl control keyid encrypted change-master-key "key-type:master-key-name"
> 
>  format:= 'default | ecryptfs'
>  key-type:= 'trusted' | 'user'

The command has remained the same as before: "update key-type:master-key-name".

diff --git a/Documentation/security/keys-trusted-encrypted.txt b/Documentation/security/keys-trusted-encrypted.txt
index e105ae9..6610be4 100644
--- a/Documentation/security/keys-trusted-encrypted.txt
+++ b/Documentation/security/keys-trusted-encrypted.txt
@@ -61,12 +61,12 @@ Usage:
     keyctl add encrypted name "new [format] key-type:master-key-name keylen"
         ring
     keyctl add encrypted name "load hex_blob" ring
-    keyctl update keyid "update key-type:master-key-name"
+    keyctl control <keyid> encrypted change-master-key \
+		"update key-type:master-key-name"
 
 format:= 'default | ecryptfs'
 key-type:= 'trusted' | 'user'
 
-
 Examples of trusted and encrypted key usage:
 
 Create and save a trusted key named "kmk" of length 32 bytes:
@@ -153,6 +153,12 @@ Load an encrypted key "evm" from saved blob:
     82dbbc55be2a44616e4959430436dc4f2a7a9659aa60bb4652aeb2120f149ed197c564e0
     24717c64 5972dcb82ab2dde83376d82b2e3c09ffc
 
+Encrypt the "evm" key with a new master key kmk-new:
+
+    $ keyctl add trusted kmk-new "new 32" @u
+    $ keyctl control 831684262 encrypted change-master-key \
+	"update trusted:kmk-new"
+
 Other uses for trusted and encrypted keys, such as for disk and file encryption
 are anticipated.  In particular the new format 'ecryptfs' has been defined in
 in order to use encrypted keys to mount an eCryptfs filesystem.  More details


thanks,

Mimi

Re: [PATCH 9/9] KEYS: Fix encrypted key type update method

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> +    keyctl control <keyid> encrypted change-master-key \
> +		"update key-type:master-key-name"

Why include the word "update" in the argument?  Isn't that implicit in the
command name?

David

Re: [PATCH 9/9] KEYS: Fix encrypted key type update method

[Mimi Zohar]

On Sun, 2013-11-17 at 09:17 +0000, David Howells wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > +    keyctl control <keyid> encrypted change-master-key \
> > +		"update key-type:master-key-name"
> 
> Why include the word "update" in the argument?  Isn't that implicit in the
> command name?

Agreed, it's redundant. encrypted_control() currently strips off the
"encrypted change-master-key" string and then calls datablob_parse(),
which still expects a key_cmd (eg. new, load, or update).  Instead, we
could pass "change-master-key" to datablob_parse() as the key_cmd and
make the necessary changes.

thanks,

Mimi

Re: [RFC][PATCH 0/9] encrypted keys & key control op

[Dmitry Kasatkin]

Hello David,

I will be looking to patches today...

- Dmitry

On 04/11/13 18:22, David Howells wrote:
> Hi Mimi, Dmitry,
>
> Here's a series of patches, the last three of which attempt to fix up a
> problem with encrypted keys update method.  The preceding patches are fixes or
> are preparatory for other changes that I want to put underneath.
>
> I really want to make all key types use ->preparse() to avoid a deadlock in
> the garbage collector with quota recycling, but encrypted_update() requires
> access to the old key - which you can't have in ->preparse() because the
> keyring isn't locked (and this lock is the root of the gc deadlock anyway).
>
> Further, the existence of encrypted_update() means that add_key() will
> sometimes get things wrong with encrypted keys (add_key() will call ->update()
> if a matching key already exists rather than creating a new key).  But you
> can't pre-search for the existence of a key and mould the payload accordingly
> because that means you can race against both add_key() and keyctl_unlink().
>
> My solution is to add a new keyctl function that allows the caller to perform
> a type-specific operation on a key:
>
> 	long keyctl_control(key_serial_t key_id,
> 			    const char *command,
> 			    char *reply_buffer,
> 			    size_t reply_size);
>
> This would then take a NUL-terminated string indicating the command and
> arguments and potentially return a reply up to the buffer length.
>
> For instance:
>
> 	keyctl_control(1234, "encrypted change-master-key fred's key", NULL, 0);
>
> or, from the shell:
>
> 	keyctl control 1234 "encrypted change-master-key fred's key"
>
> (I think that requiring the command string to be prefixed with the expected
> key type is probably a good idea).
>
> The control op could also be used for other things like pushing a key into a
> TPM.
>
> What do you think?
>
> David
> ---
> David Howells (9):
>       KEYS: The RSA public key algorithm needs to select MPILIB
>       KEYS: Provide a generic instantiation function
>       KEYS: struct key_preparsed_payload should have two payload pointers
>       KEYS: Allow expiry time to be set when preparsing a key
>       KEYS: Call ->free_preparse() even after ->preparse() returns an error
>       KEYS: Trusted: Use key preparsing
>       KEYS: Add a keyctl function to alter/control a key in type-dependent way
>       KEYS: Implement keyctl control for encrypted keys
>       KEYS: Fix encrypted key type update method
>
>
>  Documentation/security/keys.txt          |   48 +++++++-
>  crypto/asymmetric_keys/Kconfig           |    1 
>  crypto/asymmetric_keys/asymmetric_type.c |   27 ----
>  crypto/asymmetric_keys/x509_public_key.c |    2 
>  include/linux/key-type.h                 |   11 ++
>  include/uapi/linux/keyctl.h              |    1 
>  security/keys/compat.c                   |    6 +
>  security/keys/encrypted-keys/encrypted.c |  111 +++++++++++++-----
>  security/keys/internal.h                 |    2 
>  security/keys/key.c                      |   49 +++++++-
>  security/keys/keyctl.c                   |  104 ++++++++++++++++
>  security/keys/trusted.c                  |  190 ++++++++++++++----------------
>  12 files changed, 385 insertions(+), 167 deletions(-)
>
>

Re: [RFC][PATCH 0/9] encrypted keys & key control op

[David Howells]

Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:

> I will be looking to patches today...

Excellent, thanks!

David

Re: [RFC][PATCH 0/9] encrypted keys & key control op

[Mimi Zohar]

On Mon, 2013-11-04 at 16:22 +0000, David Howells wrote:
> Hi Mimi, Dmitry,
> 
> Here's a series of patches, the last three of which attempt to fix up a
> problem with encrypted keys update method.  The preceding patches are fixes or
> are preparatory for other changes that I want to put underneath.
> 
> I really want to make all key types use ->preparse() to avoid a deadlock in
> the garbage collector with quota recycling, but encrypted_update() requires
> access to the old key - which you can't have in ->preparse() because the
> keyring isn't locked (and this lock is the root of the gc deadlock anyway).

Ok.

> Further, the existence of encrypted_update() means that add_key() will
> sometimes get things wrong with encrypted keys (add_key() will call ->update()
> if a matching key already exists rather than creating a new key).

I see.  The key_type structure defines a number of methods,
including .instantiate and .update.  I would have thought that
only .update would be allowed to update an existing key.  Instantiating
a new key should create a new key or fail, not update a key.  I'm sure
there is/was a good reason for add_key() to do both.

>   But you
> can't pre-search for the existence of a key and mould the payload accordingly
> because that means you can race against both add_key() and keyctl_unlink().

Would this still be the case, if you differentiated between
instantiating and updating a key?

> My solution is to add a new keyctl function that allows the caller to perform
> a type-specific operation on a key:
> 
> 	long keyctl_control(key_serial_t key_id,
> 			    const char *command,
> 			    char *reply_buffer,
> 			    size_t reply_size);

> This would then take a NUL-terminated string indicating the command and
> arguments and potentially return a reply up to the buffer length.

What is the usecase scenario that would require a reply_buffer?

> For instance:
> 
> 	keyctl_control(1234, "encrypted change-master-key fred's key", NULL, 0);
> 
> or, from the shell:
> 
> 	keyctl control 1234 "encrypted change-master-key fred's key"
> 
> (I think that requiring the command string to be prefixed with the expected
> key type is probably a good idea).

Agreed.  

Mimi

> The control op could also be used for other things like pushing a key into a
> TPM.

> What do you think?
> 
> David
> ---
> David Howells (9):
>       KEYS: The RSA public key algorithm needs to select MPILIB
>       KEYS: Provide a generic instantiation function
>       KEYS: struct key_preparsed_payload should have two payload pointers
>       KEYS: Allow expiry time to be set when preparsing a key
>       KEYS: Call ->free_preparse() even after ->preparse() returns an error
>       KEYS: Trusted: Use key preparsing
>       KEYS: Add a keyctl function to alter/control a key in type-dependent way
>       KEYS: Implement keyctl control for encrypted keys
>       KEYS: Fix encrypted key type update method
> 
> 
>  Documentation/security/keys.txt          |   48 +++++++-
>  crypto/asymmetric_keys/Kconfig           |    1 
>  crypto/asymmetric_keys/asymmetric_type.c |   27 ----
>  crypto/asymmetric_keys/x509_public_key.c |    2 
>  include/linux/key-type.h                 |   11 ++
>  include/uapi/linux/keyctl.h              |    1 
>  security/keys/compat.c                   |    6 +
>  security/keys/encrypted-keys/encrypted.c |  111 +++++++++++++-----
>  security/keys/internal.h                 |    2 
>  security/keys/key.c                      |   49 +++++++-
>  security/keys/keyctl.c                   |  104 ++++++++++++++++
>  security/keys/trusted.c                  |  190 ++++++++++++++----------------
>  12 files changed, 385 insertions(+), 167 deletions(-)
> 

Re: [RFC][PATCH 0/9] encrypted keys & key control op

[Mimi Zohar]

On Mon, 2013-11-04 at 16:22 +0000, David Howells wrote:
> 
> The control op could also be used for other things like pushing a key
> into a TPM.
> 
> What do you think?

Trusted keys already creates a symmetric key based on the TPM RNG. 
What type of key would I be interested in pushing to the TPM?   What
usecase scenario would this solve?

thanks,

Mimi

Re: [RFC][PATCH 0/9] encrypted keys & key control op

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> > Further, the existence of encrypted_update() means that add_key() will
> > sometimes get things wrong with encrypted keys (add_key() will call
> > ->update() if a matching key already exists rather than creating a new
> > key).
> 
> I see.  The key_type structure defines a number of methods,
> including .instantiate and .update.  I would have thought that
> only .update would be allowed to update an existing key.

That is correct.  ->instantiate() is only called for a new key and ->update()
for an existing key.

> Instantiating a new key should create a new key or fail, not update a key.

That is subjective and depends on how you want your interface to work.  If you
look upon it as the "key struct" is a box with some content and the content is
switchable, then does it matter?

And, yes, ->update() should probably have been called something like replace
or reinstantiate.  The benefits of hindsight...

> I'm sure there is/was a good reason for add_key() to do both.

Yes.  No race.

> > But you can't pre-search for the existence of a key and mould the payload
> > accordingly because that means you can race against both add_key() and
> > keyctl_unlink().
> 
> Would this still be the case, if you differentiated between
> instantiating and updating a key?

Yes.  Imagine, you try to add a key and it gets rejected because the key
already exists.  You then try to update the existing key, but that gets
rejected because someone unlinked the key in the meantime.  So you try and add
it again, but this now fails because someone added a new key.  Repeat.

Or add_key() could immediately displace a key someone else just added, leaving
them with a key ID that disappeared as soon as it was returned due to an
add/add race.

The right behaviour can be argued in different ways.

> > My solution is to add a new keyctl function that allows the caller to
> > perform a type-specific operation on a key:
> > 
> > 	long keyctl_control(key_serial_t key_id,
> > 			    const char *command,
> > 			    char *reply_buffer,
> > 			    size_t reply_size);
> 
> > This would then take a NUL-terminated string indicating the command and
> > arguments and potentially return a reply up to the buffer length.
> 
> What is the usecase scenario that would require a reply_buffer?

I don't want to have to add a keyctl_control2() down the line that has a
reply_buffer if this one doesn't when someone finds a use it if it's easy
enough and small enough to provide the facility here.  This is aimed at being
a general interface rather than being specifically for encrypted keys.

David

Re: [RFC][PATCH 0/9] encrypted keys & key control op

[Mimi Zohar]

On Mon, 2013-11-11 at 22:34 +0000, David Howells wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > > Further, the existence of encrypted_update() means that add_key() will
> > > sometimes get things wrong with encrypted keys (add_key() will call
> > > ->update() if a matching key already exists rather than creating a new
> > > key).
> > 
> > I see.  The key_type structure defines a number of methods,
> > including .instantiate and .update.  I would have thought that
> > only .update would be allowed to update an existing key.
> 
> That is correct.  ->instantiate() is only called for a new key and ->update()
> for an existing key.
> 
> > Instantiating a new key should create a new key or fail, not update a key.
> 
> That is subjective and depends on how you want your interface to work.  If you
> look upon it as the "key struct" is a box with some content and the content is
> switchable, then does it matter?

I think it does.  (Example below)

> And, yes, ->update() should probably have been called something like replace
> or reinstantiate.  The benefits of hindsight...

I don't have a problem with the existing name.

> > I'm sure there is/was a good reason for add_key() to do both.
> 
> Yes.  No race.
> 
> > > But you can't pre-search for the existence of a key and mould the payload
> > > accordingly because that means you can race against both add_key() and
> > > keyctl_unlink().
> > 
> > Would this still be the case, if you differentiated between
> > instantiating and updating a key?
> 
> Yes.  Imagine, you try to add a key and it gets rejected because the key
> already exists.  You then try to update the existing key, but that gets
> rejected because someone unlinked the key in the meantime.  So you try and add
> it again, but this now fails because someone added a new key.  Repeat.

A counter example would be two processes, having nothing to do with each
other, attempt to a create a key with the same name.  Instead of each
process getting its own key, they land up sharing the same key.
Not only are they sharing the same key, but neither process knows that
there is another process using the same key.  I would think this is a
bigger problem.

Failing to create/update a key, at least to me, seems safer than having
two apps trying to create a key with same name, but instead land up
using the same key.

> Or add_key() could immediately displace a key someone else just added, leaving
> them with a key ID that disappeared as soon as it was returned due to an
> add/add race.

This is a separate issue.  If a key/keyring exists, a new key/keyring,
with the same name, should not be created replacing the existing
key/keyring.  It should simply fail.  (Removing a key/keyring first,
before creating a key/keyring of the same name, is different.)

Mimi

> The right behaviour can be argued in different ways.

> > > My solution is to add a new keyctl function that allows the caller to
> > > perform a type-specific operation on a key:
> > > 
> > > 	long keyctl_control(key_serial_t key_id,
> > > 			    const char *command,
> > > 			    char *reply_buffer,
> > > 			    size_t reply_size);
> > 
> > > This would then take a NUL-terminated string indicating the command and
> > > arguments and potentially return a reply up to the buffer length.
> > 
> > What is the usecase scenario that would require a reply_buffer?
> 
> I don't want to have to add a keyctl_control2() down the line that has a
> reply_buffer if this one doesn't when someone finds a use it if it's easy
> enough and small enough to provide the facility here.  This is aimed at being
> a general interface rather than being specifically for encrypted keys.
> 
> David
> 

Re: [RFC][PATCH 0/9] encrypted keys & key control op

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> > > I'm sure there is/was a good reason for add_key() to do both.
> > 
> > Yes.  No race.
> > 
> > > > But you can't pre-search for the existence of a key and mould the
> > > > payload accordingly because that means you can race against both
> > > > add_key() and keyctl_unlink().
> > > 
> > > Would this still be the case, if you differentiated between
> > > instantiating and updating a key?
> > 
> > Yes.  Imagine, you try to add a key and it gets rejected because the key
> > already exists.  You then try to update the existing key, but that gets
> > rejected because someone unlinked the key in the meantime.  So you try and
> > add it again, but this now fails because someone added a new key.  Repeat.
> 
> A counter example would be two processes, having nothing to do with each
> other, attempt to a create a key with the same name.  Instead of each
> process getting its own key, they land up sharing the same key.
> Not only are they sharing the same key, but neither process knows that
> there is another process using the same key.  I would think this is a
> bigger problem.
> 
> Failing to create/update a key, at least to me, seems safer than having
> two apps trying to create a key with same name, but instead land up
> using the same key.

Yes.  Two keys of the same type with the same description should be able to
substitute for one another and should be able to fulfil the same roll.  Safety
should not be an issue.

> > Or add_key() could immediately displace a key someone else just added,
> > leaving them with a key ID that disappeared as soon as it was returned due
> > to an add/add race.
> 
> This is a separate issue.  If a key/keyring exists, a new key/keyring,
> with the same name, should not be created replacing the existing
> key/keyring.  It should simply fail.  (Removing a key/keyring first,
> before creating a key/keyring of the same name, is different.)

If you have a key type that's not "updateable" then you'd have to unlink it
before trying to add a new one.  This would give you a gap in time where the
key does not exist.

So, no, creating a new key with the same name *should* atomically displace an
old one if it exists - if it doesn't update it instead.  Note that keys have an
"under construction" concept so that the core can create a partially formed key
and then instantiate it at its leisure whilst using it to block those that
would like to use it.

David

Re: [RFC][PATCH 0/9] encrypted keys & key control op

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> > The control op could also be used for other things like pushing a key
> > into a TPM.
> > 
> > What do you think?
> 
> Trusted keys already creates a symmetric key based on the TPM RNG. 
> What type of key would I be interested in pushing to the TPM?   What
> usecase scenario would this solve?

Dmitry mentioned something along these lines when I talked to him in
Edinburgh.  Anyway, it was just an example suggestion.

David