[PATCH v7 0/4] Introduce TEE based Trusted Keys support

[PATCH v7 0/4] Introduce TEE based Trusted Keys support

[Sumit Garg]

Add support for TEE based trusted keys where TEE provides the functionality
to seal and unseal trusted keys using hardware unique key. Also, this is
an alternative in case platform doesn't possess a TPM device.

This patch-set has been tested with OP-TEE based early TA which is already
merged in upstream [1].

[1] https://github.com/OP-TEE/optee_os/commit/f86ab8e7e0de869dfa25ca05a37ee070d7e5b86b

Changes in v7:
1. Added a trusted.source module parameter in order to enforce user's
   choice in case a particular platform posses both TPM and TEE.
2. Refine commit description for patch #1.

Changes in v6:
1. Revert back to dynamic detection of trust source.
2. Drop author mention from trusted_core.c and trusted_tpm1.c files.
3. Rebased to latest tpmdd/master.

Changes in v5:
1. Drop dynamic detection of trust source and use compile time flags
   instead.
2. Rename trusted_common.c -> trusted_core.c.
3. Rename callback: cleanup() -> exit().
4. Drop "tk" acronym.
5. Other misc. comments.
6. Added review tags for patch #3 and #4.

Changes in v4:
1. Pushed independent TEE features separately:
  - Part of recent TEE PR: https://lkml.org/lkml/2020/5/4/1062
2. Updated trusted-encrypted doc with TEE as a new trust source.
3. Rebased onto latest tpmdd/master.

Changes in v3:
1. Update patch #2 to support registration of multiple kernel pages.
2. Incoporate dependency patch #4 in this patch-set:
   https://patchwork.kernel.org/patch/11091435/

Changes in v2:
1. Add reviewed-by tags for patch #1 and #2.
2. Incorporate comments from Jens for patch #3.
3. Switch to use generic trusted keys framework.

Sumit Garg (4):
  KEYS: trusted: Add generic trusted keys framework
  KEYS: trusted: Introduce TEE based Trusted Keys
  doc: trusted-encrypted: updates with TEE as a new trust source
  MAINTAINERS: Add entry for TEE based Trusted Keys

 Documentation/security/keys/trusted-encrypted.rst | 203 ++++++++++---
 MAINTAINERS                                       |   8 +
 include/keys/trusted-type.h                       |  47 +++
 include/keys/trusted_tee.h                        |  55 ++++
 include/keys/trusted_tpm.h                        |  17 +-
 security/keys/trusted-keys/Makefile               |   2 +
 security/keys/trusted-keys/trusted_core.c         | 334 +++++++++++++++++++++
 security/keys/trusted-keys/trusted_tee.c          | 278 ++++++++++++++++++
 security/keys/trusted-keys/trusted_tpm1.c         | 336 ++++------------------
 9 files changed, 953 insertions(+), 327 deletions(-)
 create mode 100644 include/keys/trusted_tee.h
 create mode 100644 security/keys/trusted-keys/trusted_core.c
 create mode 100644 security/keys/trusted-keys/trusted_tee.c

-- 
2.7.4

[PATCH v7 1/4] KEYS: trusted: Add generic trusted keys framework

[Sumit Garg]

Current trusted keys framework is tightly coupled to use TPM device as
an underlying implementation which makes it difficult for implementations
like Trusted Execution Environment (TEE) etc. to provide trusted keys
support in case platform doesn't posses a TPM device.

Add a generic trusted keys framework where underlying implementations
can be easily plugged in. Create struct trusted_key_ops to achieve this,
which contains necessary functions of a backend.

Also, add a module parameter in order to select a particular trust source
in case a platform support multiple trust sources.

Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 include/keys/trusted-type.h               |  47 +++++
 include/keys/trusted_tpm.h                |  17 +-
 security/keys/trusted-keys/Makefile       |   1 +
 security/keys/trusted-keys/trusted_core.c | 330 +++++++++++++++++++++++++++++
 security/keys/trusted-keys/trusted_tpm1.c | 336 +++++-------------------------
 5 files changed, 436 insertions(+), 295 deletions(-)
 create mode 100644 security/keys/trusted-keys/trusted_core.c

diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index a94c03a..a566451 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -40,6 +40,53 @@ struct trusted_key_options {
 	uint32_t policyhandle;
 };
 
+struct trusted_key_ops {
+	/*
+	 * flag to indicate if trusted key implementation supports migration
+	 * or not.
+	 */
+	unsigned char migratable;
+
+	/* Initialize key interface. */
+	int (*init)(void);
+
+	/* Seal a key. */
+	int (*seal)(struct trusted_key_payload *p, char *datablob);
+
+	/* Unseal a key. */
+	int (*unseal)(struct trusted_key_payload *p, char *datablob);
+
+	/* Get a randomized key. */
+	int (*get_random)(unsigned char *key, size_t key_len);
+
+	/* Exit key interface. */
+	void (*exit)(void);
+};
+
+struct trusted_key_source {
+	char *name;
+	struct trusted_key_ops *ops;
+};
+
 extern struct key_type key_type_trusted;
 
+#define TRUSTED_DEBUG 0
+
+#if TRUSTED_DEBUG
+static inline void dump_payload(struct trusted_key_payload *p)
+{
+	pr_info("trusted_key: key_len %d\n", p->key_len);
+	print_hex_dump(KERN_INFO, "key ", DUMP_PREFIX_NONE,
+		       16, 1, p->key, p->key_len, 0);
+	pr_info("trusted_key: bloblen %d\n", p->blob_len);
+	print_hex_dump(KERN_INFO, "blob ", DUMP_PREFIX_NONE,
+		       16, 1, p->blob, p->blob_len, 0);
+	pr_info("trusted_key: migratable %d\n", p->migratable);
+}
+#else
+static inline void dump_payload(struct trusted_key_payload *p)
+{
+}
+#endif
+
 #endif /* _KEYS_TRUSTED_TYPE_H */
diff --git a/include/keys/trusted_tpm.h b/include/keys/trusted_tpm.h
index a56d8e1..fb3280a 100644
--- a/include/keys/trusted_tpm.h
+++ b/include/keys/trusted_tpm.h
@@ -16,6 +16,8 @@
 #define LOAD32N(buffer, offset)	(*(uint32_t *)&buffer[offset])
 #define LOAD16(buffer, offset)	(ntohs(*(uint16_t *)&buffer[offset]))
 
+extern struct trusted_key_ops tpm_trusted_key_ops;
+
 struct osapsess {
 	uint32_t handle;
 	unsigned char secret[SHA1_DIGEST_SIZE];
@@ -60,17 +62,6 @@ static inline void dump_options(struct trusted_key_options *o)
 		       16, 1, o->pcrinfo, o->pcrinfo_len, 0);
 }
 
-static inline void dump_payload(struct trusted_key_payload *p)
-{
-	pr_info("trusted_key: key_len %d\n", p->key_len);
-	print_hex_dump(KERN_INFO, "key ", DUMP_PREFIX_NONE,
-		       16, 1, p->key, p->key_len, 0);
-	pr_info("trusted_key: bloblen %d\n", p->blob_len);
-	print_hex_dump(KERN_INFO, "blob ", DUMP_PREFIX_NONE,
-		       16, 1, p->blob, p->blob_len, 0);
-	pr_info("trusted_key: migratable %d\n", p->migratable);
-}
-
 static inline void dump_sess(struct osapsess *s)
 {
 	print_hex_dump(KERN_INFO, "trusted-key: handle ", DUMP_PREFIX_NONE,
@@ -96,10 +87,6 @@ static inline void dump_options(struct trusted_key_options *o)
 {
 }
 
-static inline void dump_payload(struct trusted_key_payload *p)
-{
-}
-
 static inline void dump_sess(struct osapsess *s)
 {
 }
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
index 7b73ceb..49e3bcf 100644
--- a/security/keys/trusted-keys/Makefile
+++ b/security/keys/trusted-keys/Makefile
@@ -4,5 +4,6 @@
 #
 
 obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
+trusted-y += trusted_core.o
 trusted-y += trusted_tpm1.o
 trusted-y += trusted_tpm2.o
diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
new file mode 100644
index 0000000..71a5e27
--- /dev/null
+++ b/security/keys/trusted-keys/trusted_core.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2010 IBM Corporation
+ * Copyright (c) 2019-2020, Linaro Limited
+ *
+ * See Documentation/security/keys/trusted-encrypted.rst
+ */
+
+#include <keys/user-type.h>
+#include <keys/trusted-type.h>
+#include <keys/trusted_tpm.h>
+#include <linux/capability.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/key-type.h>
+#include <linux/module.h>
+#include <linux/parser.h>
+#include <linux/rcupdate.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/uaccess.h>
+
+static char *trusted_key_source;
+module_param_named(source, trusted_key_source, charp, 0);
+MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
+
+static struct trusted_key_source trusted_key_sources[] = {
+#if defined(CONFIG_TCG_TPM)
+	{ "tpm", &tpm_trusted_key_ops },
+#endif
+};
+static struct trusted_key_ops *trusted_key_ops;
+
+enum {
+	Opt_err,
+	Opt_new, Opt_load, Opt_update,
+};
+
+static const match_table_t key_tokens = {
+	{Opt_new, "new"},
+	{Opt_load, "load"},
+	{Opt_update, "update"},
+	{Opt_err, NULL}
+};
+
+/*
+ * datablob_parse - parse the keyctl data and fill in the
+ *                  payload structure
+ *
+ * On success returns 0, otherwise -EINVAL.
+ */
+static int datablob_parse(char *datablob, struct trusted_key_payload *p)
+{
+	substring_t args[MAX_OPT_ARGS];
+	long keylen;
+	int ret = -EINVAL;
+	int key_cmd;
+	char *c;
+
+	/* main command */
+	c = strsep(&datablob, " \t");
+	if (!c)
+		return -EINVAL;
+	key_cmd = match_token(c, key_tokens, args);
+	switch (key_cmd) {
+	case Opt_new:
+		/* first argument is key size */
+		c = strsep(&datablob, " \t");
+		if (!c)
+			return -EINVAL;
+		ret = kstrtol(c, 10, &keylen);
+		if (ret < 0 || keylen < MIN_KEY_SIZE || keylen > MAX_KEY_SIZE)
+			return -EINVAL;
+		p->key_len = keylen;
+		ret = Opt_new;
+		break;
+	case Opt_load:
+		/* first argument is sealed blob */
+		c = strsep(&datablob, " \t");
+		if (!c)
+			return -EINVAL;
+		p->blob_len = strlen(c) / 2;
+		if (p->blob_len > MAX_BLOB_SIZE)
+			return -EINVAL;
+		ret = hex2bin(p->blob, c, p->blob_len);
+		if (ret < 0)
+			return -EINVAL;
+		ret = Opt_load;
+		break;
+	case Opt_update:
+		ret = Opt_update;
+		break;
+	case Opt_err:
+		return -EINVAL;
+	}
+	return ret;
+}
+
+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);
+
+	p->migratable = trusted_key_ops->migratable;
+
+	return p;
+}
+
+/*
+ * 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 = NULL;
+	size_t datalen = prep->datalen;
+	char *datablob;
+	int ret = 0;
+	int key_cmd;
+	size_t key_len;
+
+	if (datalen <= 0 || datalen > 32767 || !prep->data)
+		return -EINVAL;
+
+	datablob = kmalloc(datalen + 1, GFP_KERNEL);
+	if (!datablob)
+		return -ENOMEM;
+	memcpy(datablob, prep->data, datalen);
+	datablob[datalen] = '\0';
+
+	payload = trusted_payload_alloc(key);
+	if (!payload) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	key_cmd = datablob_parse(datablob, payload);
+	if (key_cmd < 0) {
+		ret = key_cmd;
+		goto out;
+	}
+
+	dump_payload(payload);
+
+	switch (key_cmd) {
+	case Opt_load:
+		ret = trusted_key_ops->unseal(payload, datablob);
+		dump_payload(payload);
+		if (ret < 0)
+			pr_info("trusted_key: key_unseal failed (%d)\n", ret);
+		break;
+	case Opt_new:
+		key_len = payload->key_len;
+		ret = trusted_key_ops->get_random(payload->key, key_len);
+		if (ret != key_len) {
+			pr_info("trusted_key: key_create failed (%d)\n", ret);
+			goto out;
+		}
+
+		ret = trusted_key_ops->seal(payload, datablob);
+		if (ret < 0)
+			pr_info("trusted_key: key_seal failed (%d)\n", ret);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+out:
+	kfree_sensitive(datablob);
+	if (!ret)
+		rcu_assign_keypointer(key, payload);
+	else
+		kfree_sensitive(payload);
+	return ret;
+}
+
+static void trusted_rcu_free(struct rcu_head *rcu)
+{
+	struct trusted_key_payload *p;
+
+	p = container_of(rcu, struct trusted_key_payload, rcu);
+	kfree_sensitive(p);
+}
+
+/*
+ * trusted_update - reseal an existing key with new PCR values
+ */
+static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
+{
+	struct trusted_key_payload *p;
+	struct trusted_key_payload *new_p;
+	size_t datalen = prep->datalen;
+	char *datablob;
+	int ret = 0;
+
+	if (key_is_negative(key))
+		return -ENOKEY;
+	p = key->payload.data[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_p = trusted_payload_alloc(key);
+	if (!new_p) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	memcpy(datablob, prep->data, datalen);
+	datablob[datalen] = '\0';
+	ret = datablob_parse(datablob, new_p);
+	if (ret != Opt_update) {
+		ret = -EINVAL;
+		kfree_sensitive(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;
+	memcpy(new_p->key, p->key, p->key_len);
+	dump_payload(p);
+	dump_payload(new_p);
+
+	ret = trusted_key_ops->seal(new_p, datablob);
+	if (ret < 0) {
+		pr_info("trusted_key: key_seal failed (%d)\n", ret);
+		kfree_sensitive(new_p);
+		goto out;
+	}
+
+	rcu_assign_keypointer(key, new_p);
+	call_rcu(&p->rcu, trusted_rcu_free);
+out:
+	kfree_sensitive(datablob);
+	return ret;
+}
+
+/*
+ * trusted_read - copy the sealed blob data to userspace in hex.
+ * On success, return to userspace the trusted key datablob size.
+ */
+static long trusted_read(const struct key *key, char *buffer,
+			 size_t buflen)
+{
+	const struct trusted_key_payload *p;
+	char *bufp;
+	int i;
+
+	p = dereference_key_locked(key);
+	if (!p)
+		return -EINVAL;
+
+	if (buffer && buflen >= 2 * p->blob_len) {
+		bufp = buffer;
+		for (i = 0; i < p->blob_len; i++)
+			bufp = hex_byte_pack(bufp, p->blob[i]);
+	}
+	return 2 * p->blob_len;
+}
+
+/*
+ * trusted_destroy - clear and free the key's payload
+ */
+static void trusted_destroy(struct key *key)
+{
+	kfree_sensitive(key->payload.data[0]);
+}
+
+struct key_type key_type_trusted = {
+	.name = "trusted",
+	.instantiate = trusted_instantiate,
+	.update = trusted_update,
+	.destroy = trusted_destroy,
+	.describe = user_describe,
+	.read = trusted_read,
+};
+EXPORT_SYMBOL_GPL(key_type_trusted);
+
+static int __init init_trusted(void)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < ARRAY_SIZE(trusted_key_sources); i++) {
+		if (trusted_key_source &&
+		    strncmp(trusted_key_source, trusted_key_sources[i].name,
+			    strlen(trusted_key_sources[i].name)))
+			continue;
+
+		trusted_key_ops = trusted_key_sources[i].ops;
+
+		ret = trusted_key_ops->init();
+		if (!ret)
+			break;
+	}
+
+	/*
+	 * encrypted_keys.ko depends on successful load of this module even if
+	 * trusted key implementation is not found.
+	 */
+	if (ret == -ENODEV)
+		return 0;
+
+	return ret;
+}
+
+static void __exit cleanup_trusted(void)
+{
+	trusted_key_ops->exit();
+}
+
+late_initcall(init_trusted);
+module_exit(cleanup_trusted);
+
+MODULE_LICENSE("GPL");
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index b9fe02e..1c5df77 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -1,29 +1,22 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (C) 2010 IBM Corporation
- *
- * Author:
- * David Safford <safford@us.ibm.com>
+ * Copyright (c) 2019-2020, Linaro Limited
  *
  * See Documentation/security/keys/trusted-encrypted.rst
  */
 
 #include <crypto/hash_info.h>
-#include <linux/uaccess.h>
-#include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/parser.h>
 #include <linux/string.h>
 #include <linux/err.h>
-#include <keys/user-type.h>
 #include <keys/trusted-type.h>
 #include <linux/key-type.h>
-#include <linux/rcupdate.h>
 #include <linux/crypto.h>
 #include <crypto/hash.h>
 #include <crypto/sha.h>
-#include <linux/capability.h>
 #include <linux/tpm.h>
 #include <linux/tpm_command.h>
 
@@ -703,7 +696,6 @@ static int key_unseal(struct trusted_key_payload *p,
 
 enum {
 	Opt_err,
-	Opt_new, Opt_load, Opt_update,
 	Opt_keyhandle, Opt_keyauth, Opt_blobauth,
 	Opt_pcrinfo, Opt_pcrlock, Opt_migratable,
 	Opt_hash,
@@ -712,9 +704,6 @@ enum {
 };
 
 static const match_table_t key_tokens = {
-	{Opt_new, "new"},
-	{Opt_load, "load"},
-	{Opt_update, "update"},
 	{Opt_keyhandle, "keyhandle=%s"},
 	{Opt_keyauth, "keyauth=%s"},
 	{Opt_blobauth, "blobauth=%s"},
@@ -841,71 +830,6 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
 	return 0;
 }
 
-/*
- * datablob_parse - parse the keyctl data and fill in the
- * 		    payload and options structures
- *
- * On success returns 0, otherwise -EINVAL.
- */
-static int datablob_parse(char *datablob, struct trusted_key_payload *p,
-			  struct trusted_key_options *o)
-{
-	substring_t args[MAX_OPT_ARGS];
-	long keylen;
-	int ret = -EINVAL;
-	int key_cmd;
-	char *c;
-
-	/* main command */
-	c = strsep(&datablob, " \t");
-	if (!c)
-		return -EINVAL;
-	key_cmd = match_token(c, key_tokens, args);
-	switch (key_cmd) {
-	case Opt_new:
-		/* first argument is key size */
-		c = strsep(&datablob, " \t");
-		if (!c)
-			return -EINVAL;
-		ret = kstrtol(c, 10, &keylen);
-		if (ret < 0 || keylen < MIN_KEY_SIZE || keylen > MAX_KEY_SIZE)
-			return -EINVAL;
-		p->key_len = keylen;
-		ret = getoptions(datablob, p, o);
-		if (ret < 0)
-			return ret;
-		ret = Opt_new;
-		break;
-	case Opt_load:
-		/* first argument is sealed blob */
-		c = strsep(&datablob, " \t");
-		if (!c)
-			return -EINVAL;
-		p->blob_len = strlen(c) / 2;
-		if (p->blob_len > MAX_BLOB_SIZE)
-			return -EINVAL;
-		ret = hex2bin(p->blob, c, p->blob_len);
-		if (ret < 0)
-			return -EINVAL;
-		ret = getoptions(datablob, p, o);
-		if (ret < 0)
-			return ret;
-		ret = Opt_load;
-		break;
-	case Opt_update:
-		/* all arguments are options */
-		ret = getoptions(datablob, p, o);
-		if (ret < 0)
-			return ret;
-		ret = Opt_update;
-		break;
-	case Opt_err:
-		return -EINVAL;
-		break;
-	}
-	return ret;
-}
-
 static struct trusted_key_options *trusted_options_alloc(void)
 {
 	struct trusted_key_options *options;
@@ -926,248 +850,99 @@ static struct trusted_key_options *trusted_options_alloc(void)
 	return options;
 }
 
-static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
+static int tpm_trusted_seal(struct trusted_key_payload *p, char *datablob)
 {
-	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
- *
- * 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 = NULL;
 	struct trusted_key_options *options = NULL;
-	size_t datalen = prep->datalen;
-	char *datablob;
 	int ret = 0;
-	int key_cmd;
-	size_t key_len;
 	int tpm2;
 
 	tpm2 = tpm_is_tpm2(chip);
 	if (tpm2 < 0)
 		return tpm2;
 
-	if (datalen <= 0 || datalen > 32767 || !prep->data)
-		return -EINVAL;
-
-	datablob = kmalloc(datalen + 1, GFP_KERNEL);
-	if (!datablob)
-		return -ENOMEM;
-	memcpy(datablob, prep->data, datalen);
-	datablob[datalen] = '\0';
-
 	options = trusted_options_alloc();
-	if (!options) {
-		ret = -ENOMEM;
-		goto out;
-	}
-	payload = trusted_payload_alloc(key);
-	if (!payload) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!options)
+		return -ENOMEM;
 
-	key_cmd = datablob_parse(datablob, payload, options);
-	if (key_cmd < 0) {
-		ret = key_cmd;
+	ret = getoptions(datablob, p, options);
+	if (ret < 0)
 		goto out;
-	}
+	dump_options(options);
 
 	if (!options->keyhandle) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	dump_payload(payload);
-	dump_options(options);
+	if (tpm2)
+		ret = tpm2_seal_trusted(chip, p, options);
+	else
+		ret = key_seal(p, options);
+	if (ret < 0) {
+		pr_info("tpm_trusted_key: key_seal failed (%d)\n", ret);
+		goto out;
+	}
 
-	switch (key_cmd) {
-	case Opt_load:
-		if (tpm2)
-			ret = tpm2_unseal_trusted(chip, payload, options);
-		else
-			ret = key_unseal(payload, options);
-		dump_payload(payload);
-		dump_options(options);
-		if (ret < 0)
-			pr_info("trusted_key: key_unseal failed (%d)\n", ret);
-		break;
-	case Opt_new:
-		key_len = payload->key_len;
-		ret = tpm_get_random(chip, payload->key, key_len);
-		if (ret != key_len) {
-			pr_info("trusted_key: key_create failed (%d)\n", ret);
+	if (options->pcrlock) {
+		ret = pcrlock(options->pcrlock);
+		if (ret < 0) {
+			pr_info("tpm_trusted_key: pcrlock failed (%d)\n", ret);
 			goto out;
 		}
-		if (tpm2)
-			ret = tpm2_seal_trusted(chip, payload, options);
-		else
-			ret = key_seal(payload, options);
-		if (ret < 0)
-			pr_info("trusted_key: key_seal failed (%d)\n", ret);
-		break;
-	default:
-		ret = -EINVAL;
-		goto out;
 	}
-	if (!ret && options->pcrlock)
-		ret = pcrlock(options->pcrlock);
 out:
-	kfree_sensitive(datablob);
 	kfree_sensitive(options);
-	if (!ret)
-		rcu_assign_keypointer(key, payload);
-	else
-		kfree_sensitive(payload);
 	return ret;
 }
 
-static void trusted_rcu_free(struct rcu_head *rcu)
-{
-	struct trusted_key_payload *p;
-
-	p = container_of(rcu, struct trusted_key_payload, rcu);
-	kfree_sensitive(p);
-}
-
-/*
- * trusted_update - reseal an existing key with new PCR values
- */
-static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
+static int tpm_trusted_unseal(struct trusted_key_payload *p, char *datablob)
 {
-	struct trusted_key_payload *p;
-	struct trusted_key_payload *new_p;
-	struct trusted_key_options *new_o;
-	size_t datalen = prep->datalen;
-	char *datablob;
+	struct trusted_key_options *options = NULL;
 	int ret = 0;
+	int tpm2;
 
-	if (key_is_negative(key))
-		return -ENOKEY;
-	p = key->payload.data[0];
-	if (!p->migratable)
-		return -EPERM;
-	if (datalen <= 0 || datalen > 32767 || !prep->data)
-		return -EINVAL;
+	tpm2 = tpm_is_tpm2(chip);
+	if (tpm2 < 0)
+		return tpm2;
 
-	datablob = kmalloc(datalen + 1, GFP_KERNEL);
-	if (!datablob)
+	options = trusted_options_alloc();
+	if (!options)
 		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;
-	}
 
-	memcpy(datablob, prep->data, datalen);
-	datablob[datalen] = '\0';
-	ret = datablob_parse(datablob, new_p, new_o);
-	if (ret != Opt_update) {
-		ret = -EINVAL;
-		kfree_sensitive(new_p);
+	ret = getoptions(datablob, p, options);
+	if (ret < 0)
 		goto out;
-	}
+	dump_options(options);
 
-	if (!new_o->keyhandle) {
+	if (!options->keyhandle) {
 		ret = -EINVAL;
-		kfree_sensitive(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;
-	memcpy(new_p->key, p->key, p->key_len);
-	dump_payload(p);
-	dump_payload(new_p);
+	if (tpm2)
+		ret = tpm2_unseal_trusted(chip, p, options);
+	else
+		ret = key_unseal(p, options);
+	if (ret < 0)
+		pr_info("tpm_trusted_key: key_unseal failed (%d)\n", ret);
 
-	ret = key_seal(new_p, new_o);
-	if (ret < 0) {
-		pr_info("trusted_key: key_seal failed (%d)\n", ret);
-		kfree_sensitive(new_p);
-		goto out;
-	}
-	if (new_o->pcrlock) {
-		ret = pcrlock(new_o->pcrlock);
+	if (options->pcrlock) {
+		ret = pcrlock(options->pcrlock);
 		if (ret < 0) {
-			pr_info("trusted_key: pcrlock failed (%d)\n", ret);
-			kfree_sensitive(new_p);
+			pr_info("tpm_trusted_key: pcrlock failed (%d)\n", ret);
 			goto out;
 		}
 	}
-	rcu_assign_keypointer(key, new_p);
-	call_rcu(&p->rcu, trusted_rcu_free);
 out:
-	kfree_sensitive(datablob);
-	kfree_sensitive(new_o);
+	kfree_sensitive(options);
 	return ret;
 }
 
-/*
- * trusted_read - copy the sealed blob data to userspace in hex.
- * On success, return to userspace the trusted key datablob size.
- */
-static long trusted_read(const struct key *key, char *buffer,
-			 size_t buflen)
-{
-	const struct trusted_key_payload *p;
-	char *bufp;
-	int i;
-
-	p = dereference_key_locked(key);
-	if (!p)
-		return -EINVAL;
-
-	if (buffer && buflen >= 2 * p->blob_len) {
-		bufp = buffer;
-		for (i = 0; i < p->blob_len; i++)
-			bufp = hex_byte_pack(bufp, p->blob[i]);
-	}
-	return 2 * p->blob_len;
-}
-
-/*
- * trusted_destroy - clear and free the key's payload
- */
-static void trusted_destroy(struct key *key)
+static int tpm_trusted_get_random(unsigned char *key, size_t key_len)
 {
-	kfree_sensitive(key->payload.data[0]);
+	return tpm_get_random(chip, key, key_len);
 }
 
-struct key_type key_type_trusted = {
-	.name = "trusted",
-	.instantiate = trusted_instantiate,
-	.update = trusted_update,
-	.destroy = trusted_destroy,
-	.describe = user_describe,
-	.read = trusted_read,
-};
-
-EXPORT_SYMBOL_GPL(key_type_trusted);
-
 static void trusted_shash_release(void)
 {
 	if (hashalg)
@@ -1182,14 +957,14 @@ static int __init trusted_shash_alloc(void)
 
 	hmacalg = crypto_alloc_shash(hmac_alg, 0, 0);
 	if (IS_ERR(hmacalg)) {
-		pr_info("trusted_key: could not allocate crypto %s\n",
+		pr_info("tpm_trusted_key: could not allocate crypto %s\n",
 			hmac_alg);
 		return PTR_ERR(hmacalg);
 	}
 
 	hashalg = crypto_alloc_shash(hash_alg, 0, 0);
 	if (IS_ERR(hashalg)) {
-		pr_info("trusted_key: could not allocate crypto %s\n",
+		pr_info("tpm_trusted_key: could not allocate crypto %s\n",
 			hash_alg);
 		ret = PTR_ERR(hashalg);
 		goto hashalg_fail;
@@ -1217,16 +992,13 @@ static int __init init_digests(void)
 	return 0;
 }
 
-static int __init init_trusted(void)
+static int __init init_tpm_trusted(void)
 {
 	int ret;
 
-	/* encrypted_keys.ko depends on successful load of this module even if
-	 * TPM is not used.
-	 */
 	chip = tpm_default_chip();
 	if (!chip)
-		return 0;
+		return -ENODEV;
 
 	ret = init_digests();
 	if (ret < 0)
@@ -1247,7 +1019,7 @@ static int __init init_trusted(void)
 	return ret;
 }
 
-static void __exit cleanup_trusted(void)
+static void __exit exit_tpm_trusted(void)
 {
 	if (chip) {
 		put_device(&chip->dev);
@@ -1257,7 +1029,11 @@ static void __exit cleanup_trusted(void)
 	}
 }
 
-late_initcall(init_trusted);
-module_exit(cleanup_trusted);
-
-MODULE_LICENSE("GPL");
+struct trusted_key_ops tpm_trusted_key_ops = {
+	.migratable = 1, /* migratable by default */
+	.init = init_tpm_trusted,
+	.seal = tpm_trusted_seal,
+	.unseal = tpm_trusted_unseal,
+	.get_random = tpm_trusted_get_random,
+	.exit = exit_tpm_trusted,
+};
-- 
2.7.4

[PATCH v7 2/4] KEYS: trusted: Introduce TEE based Trusted Keys

[Sumit Garg]

Add support for TEE based trusted keys where TEE provides the functionality
to seal and unseal trusted keys using hardware unique key.

Refer to Documentation/tee.txt for detailed information about TEE.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 include/keys/trusted_tee.h                |  55 ++++++
 security/keys/trusted-keys/Makefile       |   1 +
 security/keys/trusted-keys/trusted_core.c |   4 +
 security/keys/trusted-keys/trusted_tee.c  | 278 ++++++++++++++++++++++++++++++
 4 files changed, 338 insertions(+)
 create mode 100644 include/keys/trusted_tee.h
 create mode 100644 security/keys/trusted-keys/trusted_tee.c

diff --git a/include/keys/trusted_tee.h b/include/keys/trusted_tee.h
new file mode 100644
index 0000000..2e2bb15
--- /dev/null
+++ b/include/keys/trusted_tee.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019-2020 Linaro Ltd.
+ *
+ * Author:
+ * Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#ifndef __TEE_TRUSTED_KEY_H
+#define __TEE_TRUSTED_KEY_H
+
+#include <linux/tee_drv.h>
+
+#define DRIVER_NAME "tee-trusted-key"
+
+/*
+ * Get random data for symmetric key
+ *
+ * [out]     memref[0]        Random data
+ */
+#define TA_CMD_GET_RANDOM	0x0
+
+/*
+ * Seal trusted key using hardware unique key
+ *
+ * [in]      memref[0]        Plain key
+ * [out]     memref[1]        Sealed key datablob
+ */
+#define TA_CMD_SEAL		0x1
+
+/*
+ * Unseal trusted key using hardware unique key
+ *
+ * [in]      memref[0]        Sealed key datablob
+ * [out]     memref[1]        Plain key
+ */
+#define TA_CMD_UNSEAL		0x2
+
+/**
+ * struct trusted_key_private - TEE Trusted key private data
+ * @dev:		TEE based Trusted key device.
+ * @ctx:		TEE context handler.
+ * @session_id:		Trusted key TA session identifier.
+ * @shm_pool:		Memory pool shared with TEE device.
+ */
+struct trusted_key_private {
+	struct device *dev;
+	struct tee_context *ctx;
+	u32 session_id;
+	struct tee_shm *shm_pool;
+};
+
+extern struct trusted_key_ops tee_trusted_key_ops;
+
+#endif
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
index 49e3bcf..012dd78 100644
--- a/security/keys/trusted-keys/Makefile
+++ b/security/keys/trusted-keys/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
 trusted-y += trusted_core.o
 trusted-y += trusted_tpm1.o
 trusted-y += trusted_tpm2.o
+trusted-y += trusted_tee.o
diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
index 71a5e27..74a3d80 100644
--- a/security/keys/trusted-keys/trusted_core.c
+++ b/security/keys/trusted-keys/trusted_core.c
@@ -8,6 +8,7 @@
 
 #include <keys/user-type.h>
 #include <keys/trusted-type.h>
+#include <keys/trusted_tee.h>
 #include <keys/trusted_tpm.h>
 #include <linux/capability.h>
 #include <linux/err.h>
@@ -28,6 +29,9 @@ static struct trusted_key_source trusted_key_sources[] = {
 #if defined(CONFIG_TCG_TPM)
 	{ "tpm", &tpm_trusted_key_ops },
 #endif
+#if defined(CONFIG_TEE)
+	{ "tee", &tee_trusted_key_ops },
+#endif
 };
 static struct trusted_key_ops *trusted_key_ops;
 
diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
new file mode 100644
index 0000000..b414d52
--- /dev/null
+++ b/security/keys/trusted-keys/trusted_tee.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019-2020 Linaro Ltd.
+ *
+ * Author:
+ * Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#include <linux/err.h>
+#include <linux/key-type.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/uuid.h>
+
+#include <keys/trusted-type.h>
+#include <keys/trusted_tee.h>
+
+static struct trusted_key_private pvt_data;
+
+/*
+ * Have the TEE seal(encrypt) the symmetric key
+ */
+static int tee_trusted_seal(struct trusted_key_payload *p, char *datablob)
+{
+	int ret = 0;
+	struct tee_ioctl_invoke_arg inv_arg;
+	struct tee_param param[4];
+	struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
+
+	memset(&inv_arg, 0, sizeof(inv_arg));
+	memset(&param, 0, sizeof(param));
+
+	reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
+				      p->key_len, TEE_SHM_DMA_BUF |
+				      TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm_in)) {
+		dev_err(pvt_data.dev, "key shm register failed\n");
+		return PTR_ERR(reg_shm_in);
+	}
+
+	reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
+				       sizeof(p->blob), TEE_SHM_DMA_BUF |
+				       TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm_out)) {
+		dev_err(pvt_data.dev, "blob shm register failed\n");
+		ret = PTR_ERR(reg_shm_out);
+		goto out;
+	}
+
+	inv_arg.func = TA_CMD_SEAL;
+	inv_arg.session = pvt_data.session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+	param[0].u.memref.shm = reg_shm_in;
+	param[0].u.memref.size = p->key_len;
+	param[0].u.memref.shm_offs = 0;
+	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[1].u.memref.shm = reg_shm_out;
+	param[1].u.memref.size = sizeof(p->blob);
+	param[1].u.memref.shm_offs = 0;
+
+	ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(pvt_data.dev, "TA_CMD_SEAL invoke err: %x\n",
+			inv_arg.ret);
+		ret = -EFAULT;
+	} else {
+		p->blob_len = param[1].u.memref.size;
+	}
+
+out:
+	if (reg_shm_out)
+		tee_shm_free(reg_shm_out);
+	if (reg_shm_in)
+		tee_shm_free(reg_shm_in);
+
+	return ret;
+}
+
+/*
+ * Have the TEE unseal(decrypt) the symmetric key
+ */
+static int tee_trusted_unseal(struct trusted_key_payload *p, char *datablob)
+{
+	int ret = 0;
+	struct tee_ioctl_invoke_arg inv_arg;
+	struct tee_param param[4];
+	struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
+
+	memset(&inv_arg, 0, sizeof(inv_arg));
+	memset(&param, 0, sizeof(param));
+
+	reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
+				      p->blob_len, TEE_SHM_DMA_BUF |
+				      TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm_in)) {
+		dev_err(pvt_data.dev, "blob shm register failed\n");
+		return PTR_ERR(reg_shm_in);
+	}
+
+	reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
+				       sizeof(p->key), TEE_SHM_DMA_BUF |
+				       TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm_out)) {
+		dev_err(pvt_data.dev, "key shm register failed\n");
+		ret = PTR_ERR(reg_shm_out);
+		goto out;
+	}
+
+	inv_arg.func = TA_CMD_UNSEAL;
+	inv_arg.session = pvt_data.session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
+	param[0].u.memref.shm = reg_shm_in;
+	param[0].u.memref.size = p->blob_len;
+	param[0].u.memref.shm_offs = 0;
+	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[1].u.memref.shm = reg_shm_out;
+	param[1].u.memref.size = sizeof(p->key);
+	param[1].u.memref.shm_offs = 0;
+
+	ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(pvt_data.dev, "TA_CMD_UNSEAL invoke err: %x\n",
+			inv_arg.ret);
+		ret = -EFAULT;
+	} else {
+		p->key_len = param[1].u.memref.size;
+	}
+
+out:
+	if (reg_shm_out)
+		tee_shm_free(reg_shm_out);
+	if (reg_shm_in)
+		tee_shm_free(reg_shm_in);
+
+	return ret;
+}
+
+/*
+ * Have the TEE generate random symmetric key
+ */
+static int tee_trusted_get_random(unsigned char *key, size_t key_len)
+{
+	int ret = 0;
+	struct tee_ioctl_invoke_arg inv_arg;
+	struct tee_param param[4];
+	struct tee_shm *reg_shm = NULL;
+
+	memset(&inv_arg, 0, sizeof(inv_arg));
+	memset(&param, 0, sizeof(param));
+
+	reg_shm = tee_shm_register(pvt_data.ctx, (unsigned long)key, key_len,
+				   TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED);
+	if (IS_ERR(reg_shm)) {
+		dev_err(pvt_data.dev, "key shm register failed\n");
+		return PTR_ERR(reg_shm);
+	}
+
+	inv_arg.func = TA_CMD_GET_RANDOM;
+	inv_arg.session = pvt_data.session_id;
+	inv_arg.num_params = 4;
+
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
+	param[0].u.memref.shm = reg_shm;
+	param[0].u.memref.size = key_len;
+	param[0].u.memref.shm_offs = 0;
+
+	ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
+	if ((ret < 0) || (inv_arg.ret != 0)) {
+		dev_err(pvt_data.dev, "TA_CMD_GET_RANDOM invoke err: %x\n",
+			inv_arg.ret);
+		ret = -EFAULT;
+	} else {
+		ret = param[0].u.memref.size;
+	}
+
+	tee_shm_free(reg_shm);
+
+	return ret;
+}
+
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	if (ver->impl_id == TEE_IMPL_ID_OPTEE)
+		return 1;
+	else
+		return 0;
+}
+
+static int trusted_key_probe(struct device *dev)
+{
+	struct tee_client_device *rng_device = to_tee_client_device(dev);
+	int ret = 0, err = -ENODEV;
+	struct tee_ioctl_open_session_arg sess_arg;
+
+	memset(&sess_arg, 0, sizeof(sess_arg));
+
+	pvt_data.ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
+					       NULL);
+	if (IS_ERR(pvt_data.ctx))
+		return -ENODEV;
+
+	memcpy(sess_arg.uuid, rng_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
+	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
+	sess_arg.num_params = 0;
+
+	ret = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL);
+	if ((ret < 0) || (sess_arg.ret != 0)) {
+		dev_err(dev, "tee_client_open_session failed, err: %x\n",
+			sess_arg.ret);
+		err = -EINVAL;
+		goto out_ctx;
+	}
+	pvt_data.session_id = sess_arg.session;
+
+	ret = register_key_type(&key_type_trusted);
+	if (ret < 0)
+		goto out_sess;
+
+	pvt_data.dev = dev;
+
+	return 0;
+
+out_sess:
+	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+out_ctx:
+	tee_client_close_context(pvt_data.ctx);
+
+	return err;
+}
+
+static int trusted_key_remove(struct device *dev)
+{
+	unregister_key_type(&key_type_trusted);
+	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+	tee_client_close_context(pvt_data.ctx);
+
+	return 0;
+}
+
+static const struct tee_client_device_id trusted_key_id_table[] = {
+	{UUID_INIT(0xf04a0fe7, 0x1f5d, 0x4b9b,
+		   0xab, 0xf7, 0x61, 0x9b, 0x85, 0xb4, 0xce, 0x8c)},
+	{}
+};
+MODULE_DEVICE_TABLE(tee, trusted_key_id_table);
+
+static struct tee_client_driver trusted_key_driver = {
+	.id_table	= trusted_key_id_table,
+	.driver		= {
+		.name		= DRIVER_NAME,
+		.bus		= &tee_bus_type,
+		.probe		= trusted_key_probe,
+		.remove		= trusted_key_remove,
+	},
+};
+
+static int __init init_tee_trusted(void)
+{
+	return driver_register(&trusted_key_driver.driver);
+}
+
+static void __exit exit_tee_trusted(void)
+{
+	driver_unregister(&trusted_key_driver.driver);
+}
+
+struct trusted_key_ops tee_trusted_key_ops = {
+	.migratable = 0, /* non-migratable */
+	.init = init_tee_trusted,
+	.seal = tee_trusted_seal,
+	.unseal = tee_trusted_unseal,
+	.get_random = tee_trusted_get_random,
+	.exit = exit_tee_trusted,
+};
-- 
2.7.4

[PATCH v7 3/4] doc: trusted-encrypted: updates with TEE as a new trust source

[Sumit Garg]

Update documentation for Trusted and Encrypted Keys with TEE as a new
trust source. Following is brief description of updates:

- Add a section to demostrate a list of supported devices along with
  their security properties/guarantees.
- Add a key generation section.
- Updates for usage section including differences specific to a trust
  source.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 Documentation/security/keys/trusted-encrypted.rst | 203 ++++++++++++++++++----
 1 file changed, 171 insertions(+), 32 deletions(-)

diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
index 9483a74..a355045 100644
--- a/Documentation/security/keys/trusted-encrypted.rst
+++ b/Documentation/security/keys/trusted-encrypted.rst
@@ -6,30 +6,161 @@ Trusted and Encrypted Keys are two new key types added to the existing kernel
 key ring service.  Both of these new types are variable length symmetric keys,
 and in both cases all keys are created in the kernel, and user space sees,
 stores, and loads only encrypted blobs.  Trusted Keys require the availability
-of a Trusted Platform Module (TPM) chip for greater security, while Encrypted
-Keys can be used on any system.  All user level blobs, are displayed and loaded
-in hex ascii for convenience, and are integrity verified.
+of a Trust Source for greater security, while Encrypted Keys can be used on any
+system. All user level blobs, are displayed and loaded in hex ascii for
+convenience, and are integrity verified.
 
-Trusted Keys use a TPM both to generate and to seal the keys.  Keys are sealed
-under a 2048 bit RSA key in the TPM, and optionally sealed to specified PCR
-(integrity measurement) values, and only unsealed by the TPM, if PCRs and blob
-integrity verifications match.  A loaded Trusted Key can be updated with new
-(future) PCR values, so keys are easily migrated to new pcr values, such as
-when the kernel and initramfs are updated.  The same key can have many saved
-blobs under different PCR values, so multiple boots are easily supported.
 
-TPM 1.2
--------
+Trust Source
+============
 
-By default, trusted keys are sealed under the SRK, which has the default
-authorization value (20 zeros).  This can be set at takeownership time with the
-trouser's utility: "tpm_takeownership -u -z".
+Trust Source provides the source of security for the Trusted Keys, on which
+basis Trusted Keys establishes a Trust model with its user. A Trust Source could
+differ from one system to another depending on its security requirements. It
+could be either an off-chip device or an on-chip device. Following section
+demostrates a list of supported devices along with their security properties/
+guarantees:
 
-TPM 2.0
--------
+  *  Root of trust for storage
 
-The user must first create a storage key and make it persistent, so the key is
-available after reboot. This can be done using the following commands.
+     (1) TPM (Trusted Platform Module: hardware device)
+
+         Rooted to Storage Root Key (SRK) which never leaves the TPM that
+         provides crypto operation to establish root of trust for storage.
+
+     (2) TEE (Trusted Execution Environment: OP-TEE based on Arm TrustZone)
+
+         Rooted to Hardware Unique Key (HUK) which is generally burnt in on-chip
+         fuses and is accessible to TEE only.
+
+  *  Execution isolation
+
+     (1) TPM
+
+         Fixed set of operations running in isolated execution environment.
+
+     (2) TEE
+
+         Customizable set of operations running in isolated execution
+         environment verified via Secure/Trusted boot process.
+
+  * Optional binding to platform integrity state
+
+     (1) TPM
+
+         Keys can be optionally sealed to specified PCR (integrity measurement)
+         values, and only unsealed by the TPM, if PCRs and blob integrity
+         verifications match. A loaded Trusted Key can be updated with new
+         (future) PCR values, so keys are easily migrated to new PCR values,
+         such as when the kernel and initramfs are updated. The same key can
+         have many saved blobs under different PCR values, so multiple boots are
+         easily supported.
+
+     (2) TEE
+
+         Relies on Secure/Trusted boot process for platform integrity. It can
+         be extended with TEE based measured boot process.
+
+  *  On-chip versus off-chip
+
+     (1) TPM
+
+         Off-chip device connected via serial bus (like I2C, SPI etc.) exposing
+         physical access which represents an attack surface that can be
+         mitigated via tamper detection.
+
+     (2) TEE
+
+         On-chip functionality, immune to this attack surface.
+
+  *  Memory attacks (DRAM based like attaching a bus monitor etc.)
+
+     (1) TPM
+
+         Immune to these attacks as it doesn’t make use of system DRAM.
+
+     (2) TEE
+
+         An implementation based on TrustZone protected DRAM is susceptible to
+         such attacks. In order to mitigate these attacks one needs to rely on
+         on-chip secure RAM to store secrets or have the entire TEE
+         implementation based on on-chip secure RAM. An alternative mitigation
+         would be to use encrypted DRAM.
+
+  *  Side-channel attacks (cache, memory, CPU or time based)
+
+     (1) TPM
+
+         Immune to side-channel attacks as its resources are isolated from the
+         main OS.
+
+     (2) TEE
+
+         A careful implementation is required to mitigate against these attacks
+         for resources which are shared (eg. shared memory) with the main OS.
+         Cache and CPU based side-channel attacks can be mitigated via
+         invalidating caches and CPU registers during context switch to and from
+         the secure world.
+         To mitigate against time based attacks, one needs to have time
+         invariant implementations (like crypto algorithms etc.).
+
+  *  Resistance to physical attacks (power analysis, electromagnetic emanation,
+     probes etc.)
+
+     (1) TPM
+
+         Provides limited protection utilizing tamper resistance.
+
+     (2) TEE
+
+         Provides no protection by itself, relies on the underlying platform for
+         features such as tamper resistance.
+
+
+Key Generation
+==============
+
+Trusted Keys
+------------
+
+New keys are created from trust source generated random numbers, and are
+encrypted/decrypted using trust source storage root key.
+
+  *  TPM (hardware device) based RNG
+
+     Strength of random numbers may vary from one device manufacturer to
+     another.
+
+  *  TEE (OP-TEE based on Arm TrustZone) based RNG
+
+     RNG is customizable as per platform needs. It can either be direct output
+     from platform specific hardware RNG or a software based Fortuna CSPRNG
+     which can be seeded via multiple entropy sources.
+
+Encrypted Keys
+--------------
+
+Encrypted keys do not depend on a trust source, and are faster, as they use AES
+for encryption/decryption. New keys are created from kernel generated random
+numbers, and are encrypted/decrypted using a specified ‘master’ key. The
+‘master’ key can either be a trusted-key or user-key type. The main disadvantage
+of encrypted keys is that if they are not rooted in a trusted key, they are only
+as secure as the user key encrypting them. The master user key should therefore
+be loaded in as secure a way as possible, preferably early in boot.
+
+
+Usage
+=====
+
+Trusted Keys usage: TPM
+-----------------------
+
+TPM 1.2: By default, trusted keys are sealed under the SRK, which has the
+default authorization value (20 zeros).  This can be set at takeownership time
+with the TrouSerS utility: "tpm_takeownership -u -z".
+
+TPM 2.0: The user must first create a storage key and make it persistent, so the
+key is available after reboot. This can be done using the following commands.
 
 With the IBM TSS 2 stack::
 
@@ -79,14 +210,21 @@ TPM_STORED_DATA format.  The key length for new keys are always in bytes.
 Trusted Keys can be 32 - 128 bytes (256 - 1024 bits), the upper limit is to fit
 within the 2048 bit SRK (RSA) keylength, with all necessary structure/padding.
 
-Encrypted keys do not depend on a TPM, and are faster, as they use AES for
-encryption/decryption.  New keys are created from kernel generated random
-numbers, and are encrypted/decrypted using a specified 'master' key.  The
-'master' key can either be a trusted-key or user-key type.  The main
-disadvantage of encrypted keys is that if they are not rooted in a trusted key,
-they are only as secure as the user key encrypting them.  The master user key
-should therefore be loaded in as secure a way as possible, preferably early in
-boot.
+Trusted Keys usage: TEE
+-----------------------
+
+Usage::
+
+    keyctl add trusted name "new keylen" ring
+    keyctl add trusted name "load hex_blob" ring
+    keyctl print keyid
+
+"keyctl print" returns an ascii hex copy of the sealed key, which is in format
+specific to TEE device implementation.  The key length for new keys are always
+in bytes. Trusted Keys can be 32 - 128 bytes (256 - 1024 bits).
+
+Encrypted Keys usage
+--------------------
 
 The decrypted portion of encrypted keys can contain either a simple symmetric
 key or a more complex structure. The format of the more complex structure is
@@ -104,8 +242,8 @@ Where::
 	format:= 'default | ecryptfs | enc32'
 	key-type:= 'trusted' | 'user'
 
-
 Examples of trusted and encrypted key usage:
+--------------------------------------------
 
 Create and save a trusted key named "kmk" of length 32 bytes.
 
@@ -151,7 +289,7 @@ Load a trusted key from the saved blob::
     f1f8fff03ad0acb083725535636addb08d73dedb9832da198081e5deae84bfaf0409c22b
     e4a8aea2b607ec96931e6f4d4fe563ba
 
-Reseal a trusted key under new pcr values::
+Reseal (TPM specific) a trusted key under new PCR values::
 
     $ keyctl update 268728824 "update pcrinfo=`cat pcr.blob`"
     $ keyctl print 268728824
@@ -165,11 +303,12 @@ Reseal a trusted key under new pcr values::
     7ef6a24defe4846104209bf0c3eced7fa1a672ed5b125fc9d8cd88b476a658a4434644ef
     df8ae9a178e9f83ba9f08d10fa47e4226b98b0702f06b3b8
 
+
 The initial consumer of trusted keys is EVM, which at boot time needs a high
-quality symmetric key for HMAC protection of file metadata.  The use of a
+quality symmetric key for HMAC protection of file metadata. The use of a
 trusted key provides strong guarantees that the EVM key has not been
-compromised by a user level problem, and when sealed to specific boot PCR
-values, protects against boot and offline attacks.  Create and save an
+compromised by a user level problem, and when sealed to a platform integrity
+state, protects against boot and offline attacks. Create and save an
 encrypted key "evm" using the above trusted key "kmk":
 
 option 1: omitting 'format'::
-- 
2.7.4

[PATCH v7 4/4] MAINTAINERS: Add entry for TEE based Trusted Keys

[Sumit Garg]

Add MAINTAINERS entry for TEE based Trusted Keys framework.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 48aff80..eb3d889 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9663,6 +9663,14 @@ F:	include/keys/trusted-type.h
 F:	include/keys/trusted_tpm.h
 F:	security/keys/trusted-keys/
 
+KEYS-TRUSTED-TEE
+M:	Sumit Garg <sumit.garg@linaro.org>
+L:	linux-integrity@vger.kernel.org
+L:	keyrings@vger.kernel.org
+S:	Supported
+F:	include/keys/trusted_tee.h
+F:	security/keys/trusted-keys/trusted_tee.c
+
 KEYS/KEYRINGS
 M:	David Howells <dhowells@redhat.com>
 M:	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
-- 
2.7.4

Re: [PATCH v7 1/4] KEYS: trusted: Add generic trusted keys framework

[Jarkko Sakkinen]

On Wed, Oct 07, 2020 at 03:37:45PM +0530, Sumit Garg wrote:
> Current trusted keys framework is tightly coupled to use TPM device as
> an underlying implementation which makes it difficult for implementations
> like Trusted Execution Environment (TEE) etc. to provide trusted keys
> support in case platform doesn't posses a TPM device.
> 
> Add a generic trusted keys framework where underlying implementations
> can be easily plugged in. Create struct trusted_key_ops to achieve this,
> which contains necessary functions of a backend.
> 
> Also, add a module parameter in order to select a particular trust source
> in case a platform support multiple trust sources.
> 
> Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

This is exactly kind of place where I think static_call() should be
taken into use, which is a v5.10 feature [1]. For background and
context, I'd read [2].

The other thing that I see that does not make much else than additional
complexity, is trusted_tpm.ko. We can do with one trusted.ko.

I'd also *guess* that the static_call() mechanism does not work accross
module boundaries.

[1] https://lore.kernel.org/lkml/20201012155542.GA3557765@gmail.com/
[2] https://lwn.net/Articles/815908/

/Jarkko

Re: [PATCH v7 2/4] KEYS: trusted: Introduce TEE based Trusted Keys

[Jarkko Sakkinen]

On Wed, Oct 07, 2020 at 03:37:46PM +0530, Sumit Garg wrote:
> Add support for TEE based trusted keys where TEE provides the functionality
> to seal and unseal trusted keys using hardware unique key.
> 
> Refer to Documentation/tee.txt for detailed information about TEE.
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  include/keys/trusted_tee.h                |  55 ++++++
>  security/keys/trusted-keys/Makefile       |   1 +
>  security/keys/trusted-keys/trusted_core.c |   4 +
>  security/keys/trusted-keys/trusted_tee.c  | 278 ++++++++++++++++++++++++++++++
>  4 files changed, 338 insertions(+)
>  create mode 100644 include/keys/trusted_tee.h
>  create mode 100644 security/keys/trusted-keys/trusted_tee.c
> 
> diff --git a/include/keys/trusted_tee.h b/include/keys/trusted_tee.h
> new file mode 100644
> index 0000000..2e2bb15
> --- /dev/null
> +++ b/include/keys/trusted_tee.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019-2020 Linaro Ltd.
> + *
> + * Author:
> + * Sumit Garg <sumit.garg@linaro.org>
> + */
> +
> +#ifndef __TEE_TRUSTED_KEY_H
> +#define __TEE_TRUSTED_KEY_H
> +
> +#include <linux/tee_drv.h>
> +
> +#define DRIVER_NAME "tee-trusted-key"
> +
> +/*
> + * Get random data for symmetric key
> + *
> + * [out]     memref[0]        Random data
> + */
> +#define TA_CMD_GET_RANDOM	0x0
> +
> +/*
> + * Seal trusted key using hardware unique key
> + *
> + * [in]      memref[0]        Plain key
> + * [out]     memref[1]        Sealed key datablob
> + */
> +#define TA_CMD_SEAL		0x1
> +
> +/*
> + * Unseal trusted key using hardware unique key
> + *
> + * [in]      memref[0]        Sealed key datablob
> + * [out]     memref[1]        Plain key
> + */
> +#define TA_CMD_UNSEAL		0x2
> +
> +/**
> + * struct trusted_key_private - TEE Trusted key private data
> + * @dev:		TEE based Trusted key device.
> + * @ctx:		TEE context handler.
> + * @session_id:		Trusted key TA session identifier.
> + * @shm_pool:		Memory pool shared with TEE device.
> + */
> +struct trusted_key_private {
> +	struct device *dev;
> +	struct tee_context *ctx;
> +	u32 session_id;
> +	struct tee_shm *shm_pool;
> +};
> +
> +extern struct trusted_key_ops tee_trusted_key_ops;
> +
> +#endif
> diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
> index 49e3bcf..012dd78 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
>  trusted-y += trusted_core.o
>  trusted-y += trusted_tpm1.o
>  trusted-y += trusted_tpm2.o
> +trusted-y += trusted_tee.o
> diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> index 71a5e27..74a3d80 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -8,6 +8,7 @@
>  
>  #include <keys/user-type.h>
>  #include <keys/trusted-type.h>
> +#include <keys/trusted_tee.h>
>  #include <keys/trusted_tpm.h>
>  #include <linux/capability.h>
>  #include <linux/err.h>
> @@ -28,6 +29,9 @@ static struct trusted_key_source trusted_key_sources[] = {
>  #if defined(CONFIG_TCG_TPM)
>  	{ "tpm", &tpm_trusted_key_ops },
>  #endif
> +#if defined(CONFIG_TEE)
> +	{ "tee", &tee_trusted_key_ops },
> +#endif
>  };
>  static struct trusted_key_ops *trusted_key_ops;
>  
> diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> new file mode 100644
> index 0000000..b414d52
> --- /dev/null
> +++ b/security/keys/trusted-keys/trusted_tee.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019-2020 Linaro Ltd.
> + *
> + * Author:
> + * Sumit Garg <sumit.garg@linaro.org>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/key-type.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/uuid.h>
> +
> +#include <keys/trusted-type.h>
> +#include <keys/trusted_tee.h>
> +
> +static struct trusted_key_private pvt_data;
> +
> +/*
> + * Have the TEE seal(encrypt) the symmetric key
> + */
> +static int tee_trusted_seal(struct trusted_key_payload *p, char *datablob)

Use trusted_tee_* prefix.

> +{
> +	int ret = 0;

"int ret;"

It is never used uninitialized.

> +	struct tee_ioctl_invoke_arg inv_arg;
> +	struct tee_param param[4];
> +	struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> +
> +	memset(&inv_arg, 0, sizeof(inv_arg));
> +	memset(&param, 0, sizeof(param));
> +
> +	reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
> +				      p->key_len, TEE_SHM_DMA_BUF |
> +				      TEE_SHM_KERNEL_MAPPED);
> +	if (IS_ERR(reg_shm_in)) {
> +		dev_err(pvt_data.dev, "key shm register failed\n");
> +		return PTR_ERR(reg_shm_in);
> +	}
> +
> +	reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
> +				       sizeof(p->blob), TEE_SHM_DMA_BUF |
> +				       TEE_SHM_KERNEL_MAPPED);
> +	if (IS_ERR(reg_shm_out)) {
> +		dev_err(pvt_data.dev, "blob shm register failed\n");
> +		ret = PTR_ERR(reg_shm_out);
> +		goto out;
> +	}
> +
> +	inv_arg.func = TA_CMD_SEAL;
> +	inv_arg.session = pvt_data.session_id;
> +	inv_arg.num_params = 4;
> +
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> +	param[0].u.memref.shm = reg_shm_in;
> +	param[0].u.memref.size = p->key_len;
> +	param[0].u.memref.shm_offs = 0;
> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> +	param[1].u.memref.shm = reg_shm_out;
> +	param[1].u.memref.size = sizeof(p->blob);
> +	param[1].u.memref.shm_offs = 0;
> +
> +	ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> +	if ((ret < 0) || (inv_arg.ret != 0)) {
> +		dev_err(pvt_data.dev, "TA_CMD_SEAL invoke err: %x\n",
> +			inv_arg.ret);
> +		ret = -EFAULT;
> +	} else {
> +		p->blob_len = param[1].u.memref.size;
> +	}
> +
> +out:
> +	if (reg_shm_out)
> +		tee_shm_free(reg_shm_out);
> +	if (reg_shm_in)
> +		tee_shm_free(reg_shm_in);
> +
> +	return ret;
> +}
> +
> +/*
> + * Have the TEE unseal(decrypt) the symmetric key
> + */
> +static int tee_trusted_unseal(struct trusted_key_payload *p, char *datablob)
> +{
> +	int ret = 0;

Ditto.

> +	struct tee_ioctl_invoke_arg inv_arg;
> +	struct tee_param param[4];
> +	struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> +
> +	memset(&inv_arg, 0, sizeof(inv_arg));
> +	memset(&param, 0, sizeof(param));
> +
> +	reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
> +				      p->blob_len, TEE_SHM_DMA_BUF |
> +				      TEE_SHM_KERNEL_MAPPED);
> +	if (IS_ERR(reg_shm_in)) {
> +		dev_err(pvt_data.dev, "blob shm register failed\n");
> +		return PTR_ERR(reg_shm_in);
> +	}
> +
> +	reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
> +				       sizeof(p->key), TEE_SHM_DMA_BUF |
> +				       TEE_SHM_KERNEL_MAPPED);
> +	if (IS_ERR(reg_shm_out)) {
> +		dev_err(pvt_data.dev, "key shm register failed\n");
> +		ret = PTR_ERR(reg_shm_out);
> +		goto out;
> +	}
> +
> +	inv_arg.func = TA_CMD_UNSEAL;
> +	inv_arg.session = pvt_data.session_id;
> +	inv_arg.num_params = 4;
> +
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> +	param[0].u.memref.shm = reg_shm_in;
> +	param[0].u.memref.size = p->blob_len;
> +	param[0].u.memref.shm_offs = 0;
> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> +	param[1].u.memref.shm = reg_shm_out;
> +	param[1].u.memref.size = sizeof(p->key);
> +	param[1].u.memref.shm_offs = 0;
> +
> +	ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> +	if ((ret < 0) || (inv_arg.ret != 0)) {
> +		dev_err(pvt_data.dev, "TA_CMD_UNSEAL invoke err: %x\n",
> +			inv_arg.ret);
> +		ret = -EFAULT;
> +	} else {
> +		p->key_len = param[1].u.memref.size;
> +	}
> +
> +out:
> +	if (reg_shm_out)
> +		tee_shm_free(reg_shm_out);
> +	if (reg_shm_in)
> +		tee_shm_free(reg_shm_in);
> +
> +	return ret;
> +}
> +
> +/*
> + * Have the TEE generate random symmetric key
> + */
> +static int tee_trusted_get_random(unsigned char *key, size_t key_len)
> +{
> +	int ret = 0;

Ditto.

> +	struct tee_ioctl_invoke_arg inv_arg;
> +	struct tee_param param[4];
> +	struct tee_shm *reg_shm = NULL;
> +
> +	memset(&inv_arg, 0, sizeof(inv_arg));
> +	memset(&param, 0, sizeof(param));
> +
> +	reg_shm = tee_shm_register(pvt_data.ctx, (unsigned long)key, key_len,
> +				   TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED);
> +	if (IS_ERR(reg_shm)) {
> +		dev_err(pvt_data.dev, "key shm register failed\n");
> +		return PTR_ERR(reg_shm);
> +	}
> +
> +	inv_arg.func = TA_CMD_GET_RANDOM;
> +	inv_arg.session = pvt_data.session_id;
> +	inv_arg.num_params = 4;
> +
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> +	param[0].u.memref.shm = reg_shm;
> +	param[0].u.memref.size = key_len;
> +	param[0].u.memref.shm_offs = 0;
> +
> +	ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> +	if ((ret < 0) || (inv_arg.ret != 0)) {
> +		dev_err(pvt_data.dev, "TA_CMD_GET_RANDOM invoke err: %x\n",
> +			inv_arg.ret);
> +		ret = -EFAULT;
> +	} else {
> +		ret = param[0].u.memref.size;
> +	}
> +
> +	tee_shm_free(reg_shm);
> +
> +	return ret;
> +}
> +
> +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> +	if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> +		return 1;
> +	else
> +		return 0;
> +}
> +
> +static int trusted_key_probe(struct device *dev)
> +{
> +	struct tee_client_device *rng_device = to_tee_client_device(dev);
> +	int ret = 0, err = -ENODEV;
> +	struct tee_ioctl_open_session_arg sess_arg;

Ditto. I'm not sure why you need both 'ret' and 'err'.

> +
> +	memset(&sess_arg, 0, sizeof(sess_arg));
> +
> +	pvt_data.ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
> +					       NULL);
> +	if (IS_ERR(pvt_data.ctx))
> +		return -ENODEV;
> +
> +	memcpy(sess_arg.uuid, rng_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
> +	sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> +	sess_arg.num_params = 0;
> +
> +	ret = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL);
> +	if ((ret < 0) || (sess_arg.ret != 0)) {
> +		dev_err(dev, "tee_client_open_session failed, err: %x\n",
> +			sess_arg.ret);
> +		err = -EINVAL;

Couldn't you just overwrite 'ret'?

> +		goto out_ctx;
> +	}
> +	pvt_data.session_id = sess_arg.session;
> +
> +	ret = register_key_type(&key_type_trusted);
> +	if (ret < 0)
> +		goto out_sess;
> +
> +	pvt_data.dev = dev;
> +
> +	return 0;
> +
> +out_sess:
> +	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
> +out_ctx:
> +	tee_client_close_context(pvt_data.ctx);
> +
> +	return err;
> +}
> +
> +static int trusted_key_remove(struct device *dev)
> +{
> +	unregister_key_type(&key_type_trusted);
> +	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
> +	tee_client_close_context(pvt_data.ctx);
> +
> +	return 0;
> +}
> +
> +static const struct tee_client_device_id trusted_key_id_table[] = {
> +	{UUID_INIT(0xf04a0fe7, 0x1f5d, 0x4b9b,
> +		   0xab, 0xf7, 0x61, 0x9b, 0x85, 0xb4, 0xce, 0x8c)},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(tee, trusted_key_id_table);
> +
> +static struct tee_client_driver trusted_key_driver = {
> +	.id_table	= trusted_key_id_table,
> +	.driver		= {
> +		.name		= DRIVER_NAME,
> +		.bus		= &tee_bus_type,
> +		.probe		= trusted_key_probe,
> +		.remove		= trusted_key_remove,
> +	},
> +};
> +
> +static int __init init_tee_trusted(void)
> +{
> +	return driver_register(&trusted_key_driver.driver);
> +}
> +
> +static void __exit exit_tee_trusted(void)
> +{
> +	driver_unregister(&trusted_key_driver.driver);
> +}
> +
> +struct trusted_key_ops tee_trusted_key_ops = {
> +	.migratable = 0, /* non-migratable */
> +	.init = init_tee_trusted,
> +	.seal = tee_trusted_seal,
> +	.unseal = tee_trusted_unseal,
> +	.get_random = tee_trusted_get_random,
> +	.exit = exit_tee_trusted,
> +};
> -- 
> 2.7.4
> 

/Jarkko

Re: [PATCH v7 4/4] MAINTAINERS: Add entry for TEE based Trusted Keys

[Jarkko Sakkinen]

On Wed, Oct 07, 2020 at 03:37:48PM +0530, Sumit Garg wrote:
> Add MAINTAINERS entry for TEE based Trusted Keys framework.
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 48aff80..eb3d889 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9663,6 +9663,14 @@ F:	include/keys/trusted-type.h
>  F:	include/keys/trusted_tpm.h
>  F:	security/keys/trusted-keys/
>  
> +KEYS-TRUSTED-TEE
> +M:	Sumit Garg <sumit.garg@linaro.org>
> +L:	linux-integrity@vger.kernel.org
> +L:	keyrings@vger.kernel.org
> +S:	Supported
> +F:	include/keys/trusted_tee.h
> +F:	security/keys/trusted-keys/trusted_tee.c
> +
>  KEYS/KEYRINGS
>  M:	David Howells <dhowells@redhat.com>
>  M:	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> -- 
> 2.7.4

I'm sorry but I think I have changed my mind on this. This has been
spinning for a while and sometimes conclusions change over the time.

I don't think that we really need a separate subsystem tag. I'd be for a
new M-entry or R-entry to the existing subsystem tag. It's essential to
have ack from someone with ARM and TEE knowledge but this way too heavy
for the purpose.

I also see it the most manageable if the trusted keys PR's come from a
single source.

/Jarkko

Re: [PATCH v7 1/4] KEYS: trusted: Add generic trusted keys framework

[Sumit Garg]

On Tue, 13 Oct 2020 at 07:13, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Wed, Oct 07, 2020 at 03:37:45PM +0530, Sumit Garg wrote:
> > Current trusted keys framework is tightly coupled to use TPM device as
> > an underlying implementation which makes it difficult for implementations
> > like Trusted Execution Environment (TEE) etc. to provide trusted keys
> > support in case platform doesn't posses a TPM device.
> >
> > Add a generic trusted keys framework where underlying implementations
> > can be easily plugged in. Create struct trusted_key_ops to achieve this,
> > which contains necessary functions of a backend.
> >
> > Also, add a module parameter in order to select a particular trust source
> > in case a platform support multiple trust sources.
> >
> > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>
> This is exactly kind of place where I think static_call() should be
> taken into use, which is a v5.10 feature [1]. For background and
> context, I'd read [2].

This looks like an interesting feature. But I am not sure about the
real benefits that it will provide in case of trusted keys. If we are
looking at it performance wise then I think the gain will be
negligible when compared with slow TPM communication interface (eg.
SPI, I2C) or when compared with context switching involved in TEE.

Also, it requires arch specific support too which currently seems to
be limited to x86 only.

>
> The other thing that I see that does not make much else than additional
> complexity, is trusted_tpm.ko. We can do with one trusted.ko.
>

Current implementation only builds a single trusted.ko module. There
isn't any trusted_tpm.ko.

-Sumit

> I'd also *guess* that the static_call() mechanism does not work accross
> module boundaries.
>
> [1] https://lore.kernel.org/lkml/20201012155542.GA3557765@gmail.com/
> [2] https://lwn.net/Articles/815908/
>
> /Jarkko

Re: [PATCH v7 2/4] KEYS: trusted: Introduce TEE based Trusted Keys

[Sumit Garg]

On Tue, 13 Oct 2020 at 07:22, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Wed, Oct 07, 2020 at 03:37:46PM +0530, Sumit Garg wrote:
> > Add support for TEE based trusted keys where TEE provides the functionality
> > to seal and unseal trusted keys using hardware unique key.
> >
> > Refer to Documentation/tee.txt for detailed information about TEE.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  include/keys/trusted_tee.h                |  55 ++++++
> >  security/keys/trusted-keys/Makefile       |   1 +
> >  security/keys/trusted-keys/trusted_core.c |   4 +
> >  security/keys/trusted-keys/trusted_tee.c  | 278 ++++++++++++++++++++++++++++++
> >  4 files changed, 338 insertions(+)
> >  create mode 100644 include/keys/trusted_tee.h
> >  create mode 100644 security/keys/trusted-keys/trusted_tee.c
> >
> > diff --git a/include/keys/trusted_tee.h b/include/keys/trusted_tee.h
> > new file mode 100644
> > index 0000000..2e2bb15
> > --- /dev/null
> > +++ b/include/keys/trusted_tee.h
> > @@ -0,0 +1,55 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2019-2020 Linaro Ltd.
> > + *
> > + * Author:
> > + * Sumit Garg <sumit.garg@linaro.org>
> > + */
> > +
> > +#ifndef __TEE_TRUSTED_KEY_H
> > +#define __TEE_TRUSTED_KEY_H
> > +
> > +#include <linux/tee_drv.h>
> > +
> > +#define DRIVER_NAME "tee-trusted-key"
> > +
> > +/*
> > + * Get random data for symmetric key
> > + *
> > + * [out]     memref[0]        Random data
> > + */
> > +#define TA_CMD_GET_RANDOM    0x0
> > +
> > +/*
> > + * Seal trusted key using hardware unique key
> > + *
> > + * [in]      memref[0]        Plain key
> > + * [out]     memref[1]        Sealed key datablob
> > + */
> > +#define TA_CMD_SEAL          0x1
> > +
> > +/*
> > + * Unseal trusted key using hardware unique key
> > + *
> > + * [in]      memref[0]        Sealed key datablob
> > + * [out]     memref[1]        Plain key
> > + */
> > +#define TA_CMD_UNSEAL                0x2
> > +
> > +/**
> > + * struct trusted_key_private - TEE Trusted key private data
> > + * @dev:             TEE based Trusted key device.
> > + * @ctx:             TEE context handler.
> > + * @session_id:              Trusted key TA session identifier.
> > + * @shm_pool:                Memory pool shared with TEE device.
> > + */
> > +struct trusted_key_private {
> > +     struct device *dev;
> > +     struct tee_context *ctx;
> > +     u32 session_id;
> > +     struct tee_shm *shm_pool;
> > +};
> > +
> > +extern struct trusted_key_ops tee_trusted_key_ops;
> > +
> > +#endif
> > diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
> > index 49e3bcf..012dd78 100644
> > --- a/security/keys/trusted-keys/Makefile
> > +++ b/security/keys/trusted-keys/Makefile
> > @@ -7,3 +7,4 @@ obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
> >  trusted-y += trusted_core.o
> >  trusted-y += trusted_tpm1.o
> >  trusted-y += trusted_tpm2.o
> > +trusted-y += trusted_tee.o
> > diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> > index 71a5e27..74a3d80 100644
> > --- a/security/keys/trusted-keys/trusted_core.c
> > +++ b/security/keys/trusted-keys/trusted_core.c
> > @@ -8,6 +8,7 @@
> >
> >  #include <keys/user-type.h>
> >  #include <keys/trusted-type.h>
> > +#include <keys/trusted_tee.h>
> >  #include <keys/trusted_tpm.h>
> >  #include <linux/capability.h>
> >  #include <linux/err.h>
> > @@ -28,6 +29,9 @@ static struct trusted_key_source trusted_key_sources[] = {
> >  #if defined(CONFIG_TCG_TPM)
> >       { "tpm", &tpm_trusted_key_ops },
> >  #endif
> > +#if defined(CONFIG_TEE)
> > +     { "tee", &tee_trusted_key_ops },
> > +#endif
> >  };
> >  static struct trusted_key_ops *trusted_key_ops;
> >
> > diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
> > new file mode 100644
> > index 0000000..b414d52
> > --- /dev/null
> > +++ b/security/keys/trusted-keys/trusted_tee.c
> > @@ -0,0 +1,278 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019-2020 Linaro Ltd.
> > + *
> > + * Author:
> > + * Sumit Garg <sumit.garg@linaro.org>
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/key-type.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/uuid.h>
> > +
> > +#include <keys/trusted-type.h>
> > +#include <keys/trusted_tee.h>
> > +
> > +static struct trusted_key_private pvt_data;
> > +
> > +/*
> > + * Have the TEE seal(encrypt) the symmetric key
> > + */
> > +static int tee_trusted_seal(struct trusted_key_payload *p, char *datablob)
>
> Use trusted_tee_* prefix.
>

Okay, so will also switch to trusted_tpm_* prefix in patch #1 too.

> > +{
> > +     int ret = 0;
>
> "int ret;"
>
> It is never used uninitialized.
>

Ack.

> > +     struct tee_ioctl_invoke_arg inv_arg;
> > +     struct tee_param param[4];
> > +     struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > +
> > +     memset(&inv_arg, 0, sizeof(inv_arg));
> > +     memset(&param, 0, sizeof(param));
> > +
> > +     reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
> > +                                   p->key_len, TEE_SHM_DMA_BUF |
> > +                                   TEE_SHM_KERNEL_MAPPED);
> > +     if (IS_ERR(reg_shm_in)) {
> > +             dev_err(pvt_data.dev, "key shm register failed\n");
> > +             return PTR_ERR(reg_shm_in);
> > +     }
> > +
> > +     reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
> > +                                    sizeof(p->blob), TEE_SHM_DMA_BUF |
> > +                                    TEE_SHM_KERNEL_MAPPED);
> > +     if (IS_ERR(reg_shm_out)) {
> > +             dev_err(pvt_data.dev, "blob shm register failed\n");
> > +             ret = PTR_ERR(reg_shm_out);
> > +             goto out;
> > +     }
> > +
> > +     inv_arg.func = TA_CMD_SEAL;
> > +     inv_arg.session = pvt_data.session_id;
> > +     inv_arg.num_params = 4;
> > +
> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > +     param[0].u.memref.shm = reg_shm_in;
> > +     param[0].u.memref.size = p->key_len;
> > +     param[0].u.memref.shm_offs = 0;
> > +     param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > +     param[1].u.memref.shm = reg_shm_out;
> > +     param[1].u.memref.size = sizeof(p->blob);
> > +     param[1].u.memref.shm_offs = 0;
> > +
> > +     ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > +     if ((ret < 0) || (inv_arg.ret != 0)) {
> > +             dev_err(pvt_data.dev, "TA_CMD_SEAL invoke err: %x\n",
> > +                     inv_arg.ret);
> > +             ret = -EFAULT;
> > +     } else {
> > +             p->blob_len = param[1].u.memref.size;
> > +     }
> > +
> > +out:
> > +     if (reg_shm_out)
> > +             tee_shm_free(reg_shm_out);
> > +     if (reg_shm_in)
> > +             tee_shm_free(reg_shm_in);
> > +
> > +     return ret;
> > +}
> > +
> > +/*
> > + * Have the TEE unseal(decrypt) the symmetric key
> > + */
> > +static int tee_trusted_unseal(struct trusted_key_payload *p, char *datablob)
> > +{
> > +     int ret = 0;
>
> Ditto.
>

Ack.

> > +     struct tee_ioctl_invoke_arg inv_arg;
> > +     struct tee_param param[4];
> > +     struct tee_shm *reg_shm_in = NULL, *reg_shm_out = NULL;
> > +
> > +     memset(&inv_arg, 0, sizeof(inv_arg));
> > +     memset(&param, 0, sizeof(param));
> > +
> > +     reg_shm_in = tee_shm_register(pvt_data.ctx, (unsigned long)p->blob,
> > +                                   p->blob_len, TEE_SHM_DMA_BUF |
> > +                                   TEE_SHM_KERNEL_MAPPED);
> > +     if (IS_ERR(reg_shm_in)) {
> > +             dev_err(pvt_data.dev, "blob shm register failed\n");
> > +             return PTR_ERR(reg_shm_in);
> > +     }
> > +
> > +     reg_shm_out = tee_shm_register(pvt_data.ctx, (unsigned long)p->key,
> > +                                    sizeof(p->key), TEE_SHM_DMA_BUF |
> > +                                    TEE_SHM_KERNEL_MAPPED);
> > +     if (IS_ERR(reg_shm_out)) {
> > +             dev_err(pvt_data.dev, "key shm register failed\n");
> > +             ret = PTR_ERR(reg_shm_out);
> > +             goto out;
> > +     }
> > +
> > +     inv_arg.func = TA_CMD_UNSEAL;
> > +     inv_arg.session = pvt_data.session_id;
> > +     inv_arg.num_params = 4;
> > +
> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT;
> > +     param[0].u.memref.shm = reg_shm_in;
> > +     param[0].u.memref.size = p->blob_len;
> > +     param[0].u.memref.shm_offs = 0;
> > +     param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > +     param[1].u.memref.shm = reg_shm_out;
> > +     param[1].u.memref.size = sizeof(p->key);
> > +     param[1].u.memref.shm_offs = 0;
> > +
> > +     ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > +     if ((ret < 0) || (inv_arg.ret != 0)) {
> > +             dev_err(pvt_data.dev, "TA_CMD_UNSEAL invoke err: %x\n",
> > +                     inv_arg.ret);
> > +             ret = -EFAULT;
> > +     } else {
> > +             p->key_len = param[1].u.memref.size;
> > +     }
> > +
> > +out:
> > +     if (reg_shm_out)
> > +             tee_shm_free(reg_shm_out);
> > +     if (reg_shm_in)
> > +             tee_shm_free(reg_shm_in);
> > +
> > +     return ret;
> > +}
> > +
> > +/*
> > + * Have the TEE generate random symmetric key
> > + */
> > +static int tee_trusted_get_random(unsigned char *key, size_t key_len)
> > +{
> > +     int ret = 0;
>
> Ditto.
>

Ack.

> > +     struct tee_ioctl_invoke_arg inv_arg;
> > +     struct tee_param param[4];
> > +     struct tee_shm *reg_shm = NULL;
> > +
> > +     memset(&inv_arg, 0, sizeof(inv_arg));
> > +     memset(&param, 0, sizeof(param));
> > +
> > +     reg_shm = tee_shm_register(pvt_data.ctx, (unsigned long)key, key_len,
> > +                                TEE_SHM_DMA_BUF | TEE_SHM_KERNEL_MAPPED);
> > +     if (IS_ERR(reg_shm)) {
> > +             dev_err(pvt_data.dev, "key shm register failed\n");
> > +             return PTR_ERR(reg_shm);
> > +     }
> > +
> > +     inv_arg.func = TA_CMD_GET_RANDOM;
> > +     inv_arg.session = pvt_data.session_id;
> > +     inv_arg.num_params = 4;
> > +
> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT;
> > +     param[0].u.memref.shm = reg_shm;
> > +     param[0].u.memref.size = key_len;
> > +     param[0].u.memref.shm_offs = 0;
> > +
> > +     ret = tee_client_invoke_func(pvt_data.ctx, &inv_arg, param);
> > +     if ((ret < 0) || (inv_arg.ret != 0)) {
> > +             dev_err(pvt_data.dev, "TA_CMD_GET_RANDOM invoke err: %x\n",
> > +                     inv_arg.ret);
> > +             ret = -EFAULT;
> > +     } else {
> > +             ret = param[0].u.memref.size;
> > +     }
> > +
> > +     tee_shm_free(reg_shm);
> > +
> > +     return ret;
> > +}
> > +
> > +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > +{
> > +     if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > +             return 1;
> > +     else
> > +             return 0;
> > +}
> > +
> > +static int trusted_key_probe(struct device *dev)
> > +{
> > +     struct tee_client_device *rng_device = to_tee_client_device(dev);
> > +     int ret = 0, err = -ENODEV;
> > +     struct tee_ioctl_open_session_arg sess_arg;
>
> Ditto. I'm not sure why you need both 'ret' and 'err'.
>

Okay, will use 'ret' only.

> > +
> > +     memset(&sess_arg, 0, sizeof(sess_arg));
> > +
> > +     pvt_data.ctx = tee_client_open_context(NULL, optee_ctx_match, NULL,
> > +                                            NULL);
> > +     if (IS_ERR(pvt_data.ctx))
> > +             return -ENODEV;
> > +
> > +     memcpy(sess_arg.uuid, rng_device->id.uuid.b, TEE_IOCTL_UUID_LEN);
> > +     sess_arg.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL;
> > +     sess_arg.num_params = 0;
> > +
> > +     ret = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL);
> > +     if ((ret < 0) || (sess_arg.ret != 0)) {
> > +             dev_err(dev, "tee_client_open_session failed, err: %x\n",
> > +                     sess_arg.ret);
> > +             err = -EINVAL;
>
> Couldn't you just overwrite 'ret'?
>

Ack.

-Sumit

> > +             goto out_ctx;
> > +     }
> > +     pvt_data.session_id = sess_arg.session;
> > +
> > +     ret = register_key_type(&key_type_trusted);
> > +     if (ret < 0)
> > +             goto out_sess;
> > +
> > +     pvt_data.dev = dev;
> > +
> > +     return 0;
> > +
> > +out_sess:
> > +     tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
> > +out_ctx:
> > +     tee_client_close_context(pvt_data.ctx);
> > +
> > +     return err;
> > +}
> > +
> > +static int trusted_key_remove(struct device *dev)
> > +{
> > +     unregister_key_type(&key_type_trusted);
> > +     tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
> > +     tee_client_close_context(pvt_data.ctx);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct tee_client_device_id trusted_key_id_table[] = {
> > +     {UUID_INIT(0xf04a0fe7, 0x1f5d, 0x4b9b,
> > +                0xab, 0xf7, 0x61, 0x9b, 0x85, 0xb4, 0xce, 0x8c)},
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(tee, trusted_key_id_table);
> > +
> > +static struct tee_client_driver trusted_key_driver = {
> > +     .id_table       = trusted_key_id_table,
> > +     .driver         = {
> > +             .name           = DRIVER_NAME,
> > +             .bus            = &tee_bus_type,
> > +             .probe          = trusted_key_probe,
> > +             .remove         = trusted_key_remove,
> > +     },
> > +};
> > +
> > +static int __init init_tee_trusted(void)
> > +{
> > +     return driver_register(&trusted_key_driver.driver);
> > +}
> > +
> > +static void __exit exit_tee_trusted(void)
> > +{
> > +     driver_unregister(&trusted_key_driver.driver);
> > +}
> > +
> > +struct trusted_key_ops tee_trusted_key_ops = {
> > +     .migratable = 0, /* non-migratable */
> > +     .init = init_tee_trusted,
> > +     .seal = tee_trusted_seal,
> > +     .unseal = tee_trusted_unseal,
> > +     .get_random = tee_trusted_get_random,
> > +     .exit = exit_tee_trusted,
> > +};
> > --
> > 2.7.4
> >
>
> /Jarkko

Re: [PATCH v7 4/4] MAINTAINERS: Add entry for TEE based Trusted Keys

[Sumit Garg]

On Tue, 13 Oct 2020 at 07:52, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Wed, Oct 07, 2020 at 03:37:48PM +0530, Sumit Garg wrote:
> > Add MAINTAINERS entry for TEE based Trusted Keys framework.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  MAINTAINERS | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 48aff80..eb3d889 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9663,6 +9663,14 @@ F:     include/keys/trusted-type.h
> >  F:   include/keys/trusted_tpm.h
> >  F:   security/keys/trusted-keys/
> >
> > +KEYS-TRUSTED-TEE
> > +M:   Sumit Garg <sumit.garg@linaro.org>
> > +L:   linux-integrity@vger.kernel.org
> > +L:   keyrings@vger.kernel.org
> > +S:   Supported
> > +F:   include/keys/trusted_tee.h
> > +F:   security/keys/trusted-keys/trusted_tee.c
> > +
> >  KEYS/KEYRINGS
> >  M:   David Howells <dhowells@redhat.com>
> >  M:   Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > --
> > 2.7.4
>
> I'm sorry but I think I have changed my mind on this. This has been
> spinning for a while and sometimes conclusions change over the time.
>
> I don't think that we really need a separate subsystem tag.

I don't see it as a separate subsystem but rather a kind of underlying
trust source (TEE) driver plugged into existing trusted keys
subsystem. We could relate it to the RNG subsystem as well where there
is a subsystem maintainer and specific driver maintainers.

IMO, having a dedicated entry like this brings clarity in maintenance
and in future we may have more trust sources like this added where
everyone may not have access to all the trust sources to test.

> I'd be for a
> new M-entry or R-entry to the existing subsystem tag. It's essential to
> have ack from someone with ARM and TEE knowledge but this way too heavy
> for the purpose.

If you still think otherwise then I am fine with a new M-entry for
existing trusted keys subsystem as well.

>
> I also see it the most manageable if the trusted keys PR's come from a
> single source.

I echo here with you to have a single source for trusted keys PR's
irrespective of whether we go with a separate trust source entry or
update existing subsystem entry.

-Sumit

>
> /Jarkko

Re: [PATCH v7 1/4] KEYS: trusted: Add generic trusted keys framework

[Jarkko Sakkinen]

On Tue, Oct 13, 2020 at 04:23:36PM +0530, Sumit Garg wrote:
> On Tue, 13 Oct 2020 at 07:13, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Wed, Oct 07, 2020 at 03:37:45PM +0530, Sumit Garg wrote:
> > > Current trusted keys framework is tightly coupled to use TPM device as
> > > an underlying implementation which makes it difficult for implementations
> > > like Trusted Execution Environment (TEE) etc. to provide trusted keys
> > > support in case platform doesn't posses a TPM device.
> > >
> > > Add a generic trusted keys framework where underlying implementations
> > > can be easily plugged in. Create struct trusted_key_ops to achieve this,
> > > which contains necessary functions of a backend.
> > >
> > > Also, add a module parameter in order to select a particular trust source
> > > in case a platform support multiple trust sources.
> > >
> > > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >
> > This is exactly kind of place where I think static_call() should be
> > taken into use, which is a v5.10 feature [1]. For background and
> > context, I'd read [2].
> 
> This looks like an interesting feature. But I am not sure about the
> real benefits that it will provide in case of trusted keys. If we are
> looking at it performance wise then I think the gain will be
> negligible when compared with slow TPM communication interface (eg.
> SPI, I2C) or when compared with context switching involved in TEE.
> 
> Also, it requires arch specific support too which currently seems to
> be limited to x86 only.

Please, do not purposely add indirect calls, unless you  must. Here it's
not a must.

static_call() is the correct kernel idiom to define what you are doing
in this patch. arch's will catch up.

> > The other thing that I see that does not make much else than additional
> > complexity, is trusted_tpm.ko. We can do with one trusted.ko.
> >
> 
> Current implementation only builds a single trusted.ko module. There
> isn't any trusted_tpm.ko.
> -Sumit

You're right, I'm sorry. I misread this:

-static void __exit cleanup_trusted(void)
+static void __exit exit_tpm_trusted(void)
 {
 	if (chip) {
 		put_device(&chip->dev);
@@ -1257,7 +1029,11 @@  static void __exit cleanup_trusted(void)
 	}
 }
 
-late_initcall(init_trusted);
-module_exit(cleanup_trusted);
-
-MODULE_LICENSE("GPL");
+struct trusted_key_ops tpm_trusted_key_ops = {
+	.migratable = 1, /* migratable by default */
+	.init = init_tpm_trusted,
+	.seal = tpm_trusted_seal,
+	.unseal = tpm_trusted_unseal,
+	.get_random = tpm_trusted_get_random,
+	.exit = exit_tpm_trusted,
+};

Please remove "__init" and  "__exit" for the functions as they are used
as fields as members of a struct that has neither life span. That messed
up my head.

Please use a single convention for the function names. It would
be optimal to prefix with the subsystem name because that makes easier
to use tracing tools:  trusted_tpm_<callback name> would work.

/Jarkko

Re: [PATCH v7 4/4] MAINTAINERS: Add entry for TEE based Trusted Keys

[Jarkko Sakkinen]

On Tue, Oct 13, 2020 at 04:58:47PM +0530, Sumit Garg wrote:
> On Tue, 13 Oct 2020 at 07:52, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Wed, Oct 07, 2020 at 03:37:48PM +0530, Sumit Garg wrote:
> > > Add MAINTAINERS entry for TEE based Trusted Keys framework.
> > >
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > >  MAINTAINERS | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 48aff80..eb3d889 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -9663,6 +9663,14 @@ F:     include/keys/trusted-type.h
> > >  F:   include/keys/trusted_tpm.h
> > >  F:   security/keys/trusted-keys/
> > >
> > > +KEYS-TRUSTED-TEE
> > > +M:   Sumit Garg <sumit.garg@linaro.org>
> > > +L:   linux-integrity@vger.kernel.org
> > > +L:   keyrings@vger.kernel.org
> > > +S:   Supported
> > > +F:   include/keys/trusted_tee.h
> > > +F:   security/keys/trusted-keys/trusted_tee.c
> > > +
> > >  KEYS/KEYRINGS
> > >  M:   David Howells <dhowells@redhat.com>
> > >  M:   Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > --
> > > 2.7.4
> >
> > I'm sorry but I think I have changed my mind on this. This has been
> > spinning for a while and sometimes conclusions change over the time.
> >
> > I don't think that we really need a separate subsystem tag.
> 
> I don't see it as a separate subsystem but rather a kind of underlying
> trust source (TEE) driver plugged into existing trusted keys
> subsystem. We could relate it to the RNG subsystem as well where there
> is a subsystem maintainer and specific driver maintainers.
> 
> IMO, having a dedicated entry like this brings clarity in maintenance
> and in future we may have more trust sources like this added where
> everyone may not have access to all the trust sources to test.

More entries pointing to the exact same stuff does not necessarily mean
clarity in my books.

> > I'd be for a
> > new M-entry or R-entry to the existing subsystem tag. It's essential to
> > have ack from someone with ARM and TEE knowledge but this way too heavy
> > for the purpose.
> 
> If you still think otherwise then I am fine with a new M-entry for
> existing trusted keys subsystem as well.

Adding a M-entry does makes sense because trusted keys backends can be
based on various technologies and standard. It's a different in that
sense than lets say a TPM hardware driver.

> > I also see it the most manageable if the trusted keys PR's come from a
> > single source.
> 
> I echo here with you to have a single source for trusted keys PR's
> irrespective of whether we go with a separate trust source entry or
> update existing subsystem entry.
> 
> -Sumit

And I echo that oviously if there is someone to say the final ack about
TEE, I will require that as the minimum to ever pick any of those
changes :-)

I would resolve this with just the M-entry, and we can *later on*
restructure, if there is a need for that. These things are not sealed
to stone.

/Jarkko

Re: [PATCH v7 1/4] KEYS: trusted: Add generic trusted keys framework

[Sumit Garg]

On Tue, 13 Oct 2020 at 17:29, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Tue, Oct 13, 2020 at 04:23:36PM +0530, Sumit Garg wrote:
> > On Tue, 13 Oct 2020 at 07:13, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Wed, Oct 07, 2020 at 03:37:45PM +0530, Sumit Garg wrote:
> > > > Current trusted keys framework is tightly coupled to use TPM device as
> > > > an underlying implementation which makes it difficult for implementations
> > > > like Trusted Execution Environment (TEE) etc. to provide trusted keys
> > > > support in case platform doesn't posses a TPM device.
> > > >
> > > > Add a generic trusted keys framework where underlying implementations
> > > > can be easily plugged in. Create struct trusted_key_ops to achieve this,
> > > > which contains necessary functions of a backend.
> > > >
> > > > Also, add a module parameter in order to select a particular trust source
> > > > in case a platform support multiple trust sources.
> > > >
> > > > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > >
> > > This is exactly kind of place where I think static_call() should be
> > > taken into use, which is a v5.10 feature [1]. For background and
> > > context, I'd read [2].
> >
> > This looks like an interesting feature. But I am not sure about the
> > real benefits that it will provide in case of trusted keys. If we are
> > looking at it performance wise then I think the gain will be
> > negligible when compared with slow TPM communication interface (eg.
> > SPI, I2C) or when compared with context switching involved in TEE.
> >
> > Also, it requires arch specific support too which currently seems to
> > be limited to x86 only.
>
> Please, do not purposely add indirect calls, unless you  must. Here it's
> not a must.
>
> static_call() is the correct kernel idiom to define what you are doing
> in this patch. arch's will catch up.

Okay, fair enough. I will try to use it instead.

>
> > > The other thing that I see that does not make much else than additional
> > > complexity, is trusted_tpm.ko. We can do with one trusted.ko.
> > >
> >
> > Current implementation only builds a single trusted.ko module. There
> > isn't any trusted_tpm.ko.
> > -Sumit
>
> You're right, I'm sorry. I misread this:
>
> -static void __exit cleanup_trusted(void)
> +static void __exit exit_tpm_trusted(void)
>  {
>         if (chip) {
>                 put_device(&chip->dev);
> @@ -1257,7 +1029,11 @@  static void __exit cleanup_trusted(void)
>         }
>  }
>
> -late_initcall(init_trusted);
> -module_exit(cleanup_trusted);
> -
> -MODULE_LICENSE("GPL");
> +struct trusted_key_ops tpm_trusted_key_ops = {
> +       .migratable = 1, /* migratable by default */
> +       .init = init_tpm_trusted,
> +       .seal = tpm_trusted_seal,
> +       .unseal = tpm_trusted_unseal,
> +       .get_random = tpm_trusted_get_random,
> +       .exit = exit_tpm_trusted,
> +};
>
> Please remove "__init" and  "__exit" for the functions as they are used
> as fields as members of a struct that has neither life span. That messed
> up my head.

Okay.

>
> Please use a single convention for the function names. It would
> be optimal to prefix with the subsystem name because that makes easier
> to use tracing tools:  trusted_tpm_<callback name> would work.
>

Okay.

-Sumit

> /Jarkko

Re: [PATCH v7 4/4] MAINTAINERS: Add entry for TEE based Trusted Keys

[Sumit Garg]

On Tue, 13 Oct 2020 at 19:10, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Tue, Oct 13, 2020 at 04:58:47PM +0530, Sumit Garg wrote:
> > On Tue, 13 Oct 2020 at 07:52, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Wed, Oct 07, 2020 at 03:37:48PM +0530, Sumit Garg wrote:
> > > > Add MAINTAINERS entry for TEE based Trusted Keys framework.
> > > >
> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > ---
> > > >  MAINTAINERS | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 48aff80..eb3d889 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -9663,6 +9663,14 @@ F:     include/keys/trusted-type.h
> > > >  F:   include/keys/trusted_tpm.h
> > > >  F:   security/keys/trusted-keys/
> > > >
> > > > +KEYS-TRUSTED-TEE
> > > > +M:   Sumit Garg <sumit.garg@linaro.org>
> > > > +L:   linux-integrity@vger.kernel.org
> > > > +L:   keyrings@vger.kernel.org
> > > > +S:   Supported
> > > > +F:   include/keys/trusted_tee.h
> > > > +F:   security/keys/trusted-keys/trusted_tee.c
> > > > +
> > > >  KEYS/KEYRINGS
> > > >  M:   David Howells <dhowells@redhat.com>
> > > >  M:   Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > --
> > > > 2.7.4
> > >
> > > I'm sorry but I think I have changed my mind on this. This has been
> > > spinning for a while and sometimes conclusions change over the time.
> > >
> > > I don't think that we really need a separate subsystem tag.
> >
> > I don't see it as a separate subsystem but rather a kind of underlying
> > trust source (TEE) driver plugged into existing trusted keys
> > subsystem. We could relate it to the RNG subsystem as well where there
> > is a subsystem maintainer and specific driver maintainers.
> >
> > IMO, having a dedicated entry like this brings clarity in maintenance
> > and in future we may have more trust sources like this added where
> > everyone may not have access to all the trust sources to test.
>
> More entries pointing to the exact same stuff does not necessarily mean
> clarity in my books.
>
> > > I'd be for a
> > > new M-entry or R-entry to the existing subsystem tag. It's essential to
> > > have ack from someone with ARM and TEE knowledge but this way too heavy
> > > for the purpose.
> >
> > If you still think otherwise then I am fine with a new M-entry for
> > existing trusted keys subsystem as well.
>
> Adding a M-entry does makes sense because trusted keys backends can be
> based on various technologies and standard. It's a different in that
> sense than lets say a TPM hardware driver.
>
> > > I also see it the most manageable if the trusted keys PR's come from a
> > > single source.
> >
> > I echo here with you to have a single source for trusted keys PR's
> > irrespective of whether we go with a separate trust source entry or
> > update existing subsystem entry.
> >
> > -Sumit
>
> And I echo that oviously if there is someone to say the final ack about
> TEE, I will require that as the minimum to ever pick any of those
> changes :-)
>
> I would resolve this with just the M-entry, and we can *later on*
> restructure, if there is a need for that. These things are not sealed
> to stone.

Okay, will add a M-entry for existing trusted keys subsystem.

-Sumit

>
> /Jarkko

Re: [PATCH v7 1/4] KEYS: trusted: Add generic trusted keys framework

[Mimi Zohar]

On Wed, 2020-10-07 at 15:37 +0530, Sumit Garg wrote:

> +/*
> + * trusted_destroy - clear and free the key's payload
> + */
> +static void trusted_destroy(struct key *key)
> +{
> +	kfree_sensitive(key->payload.data[0]);
> +}
> +
> +struct key_type key_type_trusted = {
> +	.name = "trusted",
> +	.instantiate = trusted_instantiate,
> +	.update = trusted_update,
> +	.destroy = trusted_destroy,
> +	.describe = user_describe,
> +	.read = trusted_read,
> +};
> +EXPORT_SYMBOL_GPL(key_type_trusted);
> +
> +static int __init init_trusted(void)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(trusted_key_sources); i++) {
> +		if (trusted_key_source &&
> +		    strncmp(trusted_key_source, trusted_key_sources[i].name,
> +			    strlen(trusted_key_sources[i].name)))
> +			continue;
> +
> +		trusted_key_ops = trusted_key_sources[i].ops;
> +
> +		ret = trusted_key_ops->init();
> +		if (!ret)
> +			break;
> +	}

In the case when the module paramater isn't specified and both TPM and
TEE are enabled, trusted_key_ops is set to the last source initialized.
After patch 2/4, the last trusted source initialized is TEE.  If the
intention is to limit it to either TPM or TEE, then trusted_key_ops
should have a default value, which could be overwritten at runtime. 
That would address Luke Hind's concerns of making the decision at
compile time.

trusted_key_ops should be defined as __ro_after_init, like is currently
done for other LSM structures.

> +
> +	/*
> +	 * encrypted_keys.ko depends on successful load of this module even if
> +	 * trusted key implementation is not found.
> +	 */
> +	if (ret == -ENODEV)
> +		return 0;
> +
> +	return ret;
> +}
> +
> +static void __exit cleanup_trusted(void)
> +{
> +	trusted_key_ops->exit();

If the intention is really to support both TPM and TEE trusted keys at
the same time, as James suggested, then the same "for" loop as in
init_trusted() is needed here and probably elsewhere.

thanks,

Mimi

Re: [PATCH v7 1/4] KEYS: trusted: Add generic trusted keys framework

[Sumit Garg]

Thanks Mimi for your comments.

On Wed, 21 Oct 2020 at 08:51, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2020-10-07 at 15:37 +0530, Sumit Garg wrote:
>
> > +/*
> > + * trusted_destroy - clear and free the key's payload
> > + */
> > +static void trusted_destroy(struct key *key)
> > +{
> > +     kfree_sensitive(key->payload.data[0]);
> > +}
> > +
> > +struct key_type key_type_trusted = {
> > +     .name = "trusted",
> > +     .instantiate = trusted_instantiate,
> > +     .update = trusted_update,
> > +     .destroy = trusted_destroy,
> > +     .describe = user_describe,
> > +     .read = trusted_read,
> > +};
> > +EXPORT_SYMBOL_GPL(key_type_trusted);
> > +
> > +static int __init init_trusted(void)
> > +{
> > +     int i, ret = 0;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(trusted_key_sources); i++) {
> > +             if (trusted_key_source &&
> > +                 strncmp(trusted_key_source, trusted_key_sources[i].name,
> > +                         strlen(trusted_key_sources[i].name)))
> > +                     continue;
> > +
> > +             trusted_key_ops = trusted_key_sources[i].ops;
> > +
> > +             ret = trusted_key_ops->init();
> > +             if (!ret)
> > +                     break;
> > +     }
>
> In the case when the module paramater isn't specified and both TPM and
> TEE are enabled, trusted_key_ops is set to the last source initialized.

I guess there is some misunderstanding. Here it's only a single trust
source (TPM *or* TEE) is initialized and only that trust source would
be active at runtime. And trusted_key_ops would be initialized to the
first trust source whose initialization is successful (see check: "if
(!ret)").

> After patch 2/4, the last trusted source initialized is TEE.  If the
> intention is to limit it to either TPM or TEE, then trusted_key_ops
> should have a default value, which could be overwritten at runtime.
> That would address Luke Hind's concerns of making the decision at
> compile time.

I think traversing the trust source list with the initial value being
TPM would be default value.

>
> trusted_key_ops should be defined as __ro_after_init, like is currently
> done for other LSM structures.

Sure, will do.

>
> > +
> > +     /*
> > +      * encrypted_keys.ko depends on successful load of this module even if
> > +      * trusted key implementation is not found.
> > +      */
> > +     if (ret == -ENODEV)
> > +             return 0;
> > +
> > +     return ret;
> > +}
> > +
> > +static void __exit cleanup_trusted(void)
> > +{
> > +     trusted_key_ops->exit();
>
> If the intention is really to support both TPM and TEE trusted keys at
> the same time, as James suggested, then the same "for" loop as in
> init_trusted() is needed here and probably elsewhere.

Current intention is to only support a single trust source (TPM or
TEE) at runtime. But in future if there are use-cases then framework
can be extended to support multiple trust sources at runtime as well.

-Sumit

>
> thanks,
>
> Mimi
>

Re: [PATCH v7 1/4] KEYS: trusted: Add generic trusted keys framework

[Mimi Zohar]

On Wed, 2020-10-21 at 11:16 +0530, Sumit Garg wrote:
> Thanks Mimi for your comments.
> 
> On Wed, 21 Oct 2020 at 08:51, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Wed, 2020-10-07 at 15:37 +0530, Sumit Garg wrote:
> >
> > > +/*
> > > + * trusted_destroy - clear and free the key's payload
> > > + */
> > > +static void trusted_destroy(struct key *key)
> > > +{
> > > +     kfree_sensitive(key->payload.data[0]);
> > > +}
> > > +
> > > +struct key_type key_type_trusted = {
> > > +     .name = "trusted",
> > > +     .instantiate = trusted_instantiate,
> > > +     .update = trusted_update,
> > > +     .destroy = trusted_destroy,
> > > +     .describe = user_describe,
> > > +     .read = trusted_read,
> > > +};
> > > +EXPORT_SYMBOL_GPL(key_type_trusted);
> > > +
> > > +static int __init init_trusted(void)
> > > +{
> > > +     int i, ret = 0;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(trusted_key_sources); i++) {
> > > +             if (trusted_key_source &&
> > > +                 strncmp(trusted_key_source, trusted_key_sources[i].name,
> > > +                         strlen(trusted_key_sources[i].name)))
> > > +                     continue;
> > > +
> > > +             trusted_key_ops = trusted_key_sources[i].ops;
> > > +
> > > +             ret = trusted_key_ops->init();
> > > +             if (!ret)
> > > +                     break;
> > > +     }
> >
> > In the case when the module paramater isn't specified and both TPM and
> > TEE are enabled, trusted_key_ops is set to the last source initialized.
> 
> I guess there is some misunderstanding. Here it's only a single trust
> source (TPM *or* TEE) is initialized and only that trust source would
> be active at runtime. And trusted_key_ops would be initialized to the
> first trust source whose initialization is successful (see check: "if
> (!ret)").

My mistake.

> 
> > After patch 2/4, the last trusted source initialized is TEE.  If the
> > intention is to limit it to either TPM or TEE, then trusted_key_ops
> > should have a default value, which could be overwritten at runtime.
> > That would address Luke Hind's concerns of making the decision at
> > compile time.
> 
> I think traversing the trust source list with the initial value being
> TPM would be default value.

Agreed
> 
> >
> > trusted_key_ops should be defined as __ro_after_init, like is currently
> > done for other LSM structures.
> 
> Sure, will do.

Thanks
> 
> >
> > > +
> > > +     /*
> > > +      * encrypted_keys.ko depends on successful load of this module even if
> > > +      * trusted key implementation is not found.
> > > +      */
> > > +     if (ret == -ENODEV)
> > > +             return 0;
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static void __exit cleanup_trusted(void)
> > > +{
> > > +     trusted_key_ops->exit();
> >
> > If the intention is really to support both TPM and TEE trusted keys at
> > the same time, as James suggested, then the same "for" loop as in
> > init_trusted() is needed here and probably elsewhere.
> 
> Current intention is to only support a single trust source (TPM or
> TEE) at runtime. But in future if there are use-cases then framework
> can be extended to support multiple trust sources at runtime as well.

Ok, the last sentence of the patch description, "Also, add a module
parameter in order to select a particular trust source in case a
platform support multiple trust sources.", needs to be expanded to:
- indicate only one trust source at a time is supported
- indicate the default, if the module_param is not specified

I would also change the word from "add" to "define".   The new "source"
module parameter needs to be added to the admin-guide/kernel-parameters 
documentation.

thanks,

Mimi   

Re: [PATCH v7 1/4] KEYS: trusted: Add generic trusted keys framework

[Sumit Garg]

On Wed, 21 Oct 2020 at 17:55, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2020-10-21 at 11:16 +0530, Sumit Garg wrote:
> > Thanks Mimi for your comments.
> >
> > On Wed, 21 Oct 2020 at 08:51, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >
> > > On Wed, 2020-10-07 at 15:37 +0530, Sumit Garg wrote:
> > >
> > > > +/*
> > > > + * trusted_destroy - clear and free the key's payload
> > > > + */
> > > > +static void trusted_destroy(struct key *key)
> > > > +{
> > > > +     kfree_sensitive(key->payload.data[0]);
> > > > +}
> > > > +
> > > > +struct key_type key_type_trusted = {
> > > > +     .name = "trusted",
> > > > +     .instantiate = trusted_instantiate,
> > > > +     .update = trusted_update,
> > > > +     .destroy = trusted_destroy,
> > > > +     .describe = user_describe,
> > > > +     .read = trusted_read,
> > > > +};
> > > > +EXPORT_SYMBOL_GPL(key_type_trusted);
> > > > +
> > > > +static int __init init_trusted(void)
> > > > +{
> > > > +     int i, ret = 0;
> > > > +
> > > > +     for (i = 0; i < ARRAY_SIZE(trusted_key_sources); i++) {
> > > > +             if (trusted_key_source &&
> > > > +                 strncmp(trusted_key_source, trusted_key_sources[i].name,
> > > > +                         strlen(trusted_key_sources[i].name)))
> > > > +                     continue;
> > > > +
> > > > +             trusted_key_ops = trusted_key_sources[i].ops;
> > > > +
> > > > +             ret = trusted_key_ops->init();
> > > > +             if (!ret)
> > > > +                     break;
> > > > +     }
> > >
> > > In the case when the module paramater isn't specified and both TPM and
> > > TEE are enabled, trusted_key_ops is set to the last source initialized.
> >
> > I guess there is some misunderstanding. Here it's only a single trust
> > source (TPM *or* TEE) is initialized and only that trust source would
> > be active at runtime. And trusted_key_ops would be initialized to the
> > first trust source whose initialization is successful (see check: "if
> > (!ret)").
>
> My mistake.
>
> >
> > > After patch 2/4, the last trusted source initialized is TEE.  If the
> > > intention is to limit it to either TPM or TEE, then trusted_key_ops
> > > should have a default value, which could be overwritten at runtime.
> > > That would address Luke Hind's concerns of making the decision at
> > > compile time.
> >
> > I think traversing the trust source list with the initial value being
> > TPM would be default value.
>
> Agreed
> >
> > >
> > > trusted_key_ops should be defined as __ro_after_init, like is currently
> > > done for other LSM structures.
> >
> > Sure, will do.
>
> Thanks
> >
> > >
> > > > +
> > > > +     /*
> > > > +      * encrypted_keys.ko depends on successful load of this module even if
> > > > +      * trusted key implementation is not found.
> > > > +      */
> > > > +     if (ret == -ENODEV)
> > > > +             return 0;
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +static void __exit cleanup_trusted(void)
> > > > +{
> > > > +     trusted_key_ops->exit();
> > >
> > > If the intention is really to support both TPM and TEE trusted keys at
> > > the same time, as James suggested, then the same "for" loop as in
> > > init_trusted() is needed here and probably elsewhere.
> >
> > Current intention is to only support a single trust source (TPM or
> > TEE) at runtime. But in future if there are use-cases then framework
> > can be extended to support multiple trust sources at runtime as well.
>
> Ok, the last sentence of the patch description, "Also, add a module
> parameter in order to select a particular trust source in case a
> platform support multiple trust sources.", needs to be expanded to:
> - indicate only one trust source at a time is supported
> - indicate the default, if the module_param is not specified
>

Sure, I will expand that.

> I would also change the word from "add" to "define".

Ack.

>   The new "source"
> module parameter needs to be added to the admin-guide/kernel-parameters
> documentation.

Okay, will update documentation as well.

-Sumit

>
> thanks,
>
> Mimi
>
>