[PATCH 0/6] security/keys/encrypted: Break module dependency chain

[PATCH 0/6] security/keys/encrypted: Break module dependency chain

[Dan Williams]

With v5.1-rc1 all the nvdimm sub-system regression tests started failing
because the libnvdimm module failed to load in the qemu-kvm test
environment.  Critically that environment does not have a TPM. Commit
240730437deb "KEYS: trusted: explicitly use tpm_chip structure..."
started to require a TPM to be present for the trusted.ko module to load
where there was no requirement for that before.

Rather than undo the "fail if no hardware" behavior James points out
that the module dependencies can be broken by looking up the key-type by
name. Remove the dependencies on the "key_type_trusted" and
"key_type_encrypted" symbol exports, and clean up other boilerplate that
supported those exports in different configurations.

---

Dan Williams (6):
      security/keys/encrypted: Allow operation without trusted.ko
      security/keys/encrypted: Clean up request_trusted_key()
      libnvdimm/security: Drop direct dependency on key_type_encrypted
      security/keys/ecryptfs: Drop direct dependency on key_type_encrypted
      security/integrity/evm: Drop direct dependency on key_type_encrypted
      security/keys/encrypted: Drop export of key_type_encrypted


 drivers/nvdimm/security.c                        |   11 ++++-
 fs/ecryptfs/ecryptfs_kernel.h                    |   22 -----------
 fs/ecryptfs/keystore.c                           |   12 ++++++
 include/keys/encrypted-type.h                    |    2 -
 include/linux/key.h                              |    1 
 security/integrity/evm/evm_crypto.c              |    9 ++++
 security/keys/encrypted-keys/Makefile            |    3 -
 security/keys/encrypted-keys/encrypted.c         |   35 ++++++++++++++++-
 security/keys/encrypted-keys/encrypted.h         |   12 ------
 security/keys/encrypted-keys/masterkey_trusted.c |   46 ----------------------
 security/keys/internal.h                         |    2 -
 security/keys/key.c                              |    1 
 12 files changed, 65 insertions(+), 91 deletions(-)
 delete mode 100644 security/keys/encrypted-keys/masterkey_trusted.c

[PATCH 1/6] security/keys/encrypted: Allow operation without trusted.ko

[Dan Williams]

The trusted.ko module may fail to load. In the common case this failure
is simply due to the platform missing a TPM. Teach the encrypted_keys
implementation to lookup the key type by name rather than having a
module dependency.

Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
Suggested-by: James Bottomley <jejb@linux.ibm.com>
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 security/keys/encrypted-keys/masterkey_trusted.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
index dc3d18cae642..7560aea6438d 100644
--- a/security/keys/encrypted-keys/masterkey_trusted.c
+++ b/security/keys/encrypted-keys/masterkey_trusted.c
@@ -19,6 +19,7 @@
 #include <keys/trusted-type.h>
 #include <keys/encrypted-type.h>
 #include "encrypted.h"
+#include "../internal.h"
 
 /*
  * request_trusted_key - request the trusted key
@@ -31,9 +32,15 @@ struct key *request_trusted_key(const char *trusted_desc,
 				const u8 **master_key, size_t *master_keylen)
 {
 	struct trusted_key_payload *tpayload;
+	struct key_type *type;
 	struct key *tkey;
 
-	tkey = request_key(&key_type_trusted, trusted_desc, NULL);
+	type = key_type_lookup("trusted");
+	if (IS_ERR(type)) {
+		tkey = (struct key *)type;
+		goto error;
+	}
+	tkey = request_key(type, trusted_desc, NULL);
 	if (IS_ERR(tkey))
 		goto error;
 
@@ -44,3 +51,5 @@ struct key *request_trusted_key(const char *trusted_desc,
 error:
 	return tkey;
 }
+
+MODULE_SOFTDEP("pre: trusted");

[PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Dan Williams]

Now that the trusted key type is looked up by name rather than direct
symbol there is no need to play games with detecting the build
configuration. Make request_trusted_key() a static facility internal to
the encrypted-keys implementation.

Suggested-by: James Bottomley <jejb@linux.ibm.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/key.h                              |    1 
 security/keys/encrypted-keys/Makefile            |    3 -
 security/keys/encrypted-keys/encrypted.c         |   32 +++++++++++++
 security/keys/encrypted-keys/encrypted.h         |   12 -----
 security/keys/encrypted-keys/masterkey_trusted.c |   55 ----------------------
 security/keys/internal.h                         |    2 -
 security/keys/key.c                              |    1 
 7 files changed, 34 insertions(+), 72 deletions(-)
 delete mode 100644 security/keys/encrypted-keys/masterkey_trusted.c

diff --git a/include/linux/key.h b/include/linux/key.h
index 7099985e35a9..e7bfd037d26f 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -270,6 +270,7 @@ static inline void key_ref_put(key_ref_t key_ref)
 	key_put(key_ref_to_ptr(key_ref));
 }
 
+extern struct key_type *key_type_lookup(const char *type);
 extern struct key *request_key(struct key_type *type,
 			       const char *description,
 			       const char *callout_info);
diff --git a/security/keys/encrypted-keys/Makefile b/security/keys/encrypted-keys/Makefile
index 7a44dce6f69d..d42487bb3d8a 100644
--- a/security/keys/encrypted-keys/Makefile
+++ b/security/keys/encrypted-keys/Makefile
@@ -6,6 +6,3 @@
 obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys.o
 
 encrypted-keys-y := encrypted.o ecryptfs_format.o
-masterkey-$(CONFIG_TRUSTED_KEYS) := masterkey_trusted.o
-masterkey-$(CONFIG_TRUSTED_KEYS)-$(CONFIG_ENCRYPTED_KEYS) := masterkey_trusted.o
-encrypted-keys-y += $(masterkey-y) $(masterkey-m-m)
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 347108f660a1..06925d3b30c9 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -423,6 +423,37 @@ static struct skcipher_request *init_skcipher_req(const u8 *key,
 	return req;
 }
 
+/*
+ * request_trusted_key - request the trusted key
+ *
+ * Trusted keys are sealed to PCRs and other metadata. Although userspace
+ * manages both trusted/encrypted key-types, like the encrypted key type
+ * data, trusted key type data is not visible decrypted from userspace.
+ */
+static struct key *request_trusted_key(const char *trusted_desc,
+				const u8 **master_key, size_t *master_keylen)
+{
+	struct trusted_key_payload *tpayload;
+	struct key_type *type;
+	struct key *tkey;
+
+	type = key_type_lookup("trusted");
+	if (IS_ERR(type)) {
+		tkey = (struct key *)type;
+		goto error;
+	}
+	tkey = request_key(type, trusted_desc, NULL);
+	if (IS_ERR(tkey))
+		goto error;
+
+	down_read(&tkey->sem);
+	tpayload = tkey->payload.data[0];
+	*master_key = tpayload->key;
+	*master_keylen = tpayload->key_len;
+error:
+	return tkey;
+}
+
 static struct key *request_master_key(struct encrypted_key_payload *epayload,
 				      const u8 **master_key, size_t *master_keylen)
 {
@@ -1025,3 +1056,4 @@ late_initcall(init_encrypted);
 module_exit(cleanup_encrypted);
 
 MODULE_LICENSE("GPL");
+MODULE_SOFTDEP("pre: trusted");
diff --git a/security/keys/encrypted-keys/encrypted.h b/security/keys/encrypted-keys/encrypted.h
index 1809995db452..0ae67824a24a 100644
--- a/security/keys/encrypted-keys/encrypted.h
+++ b/security/keys/encrypted-keys/encrypted.h
@@ -3,18 +3,6 @@
 #define __ENCRYPTED_KEY_H
 
 #define ENCRYPTED_DEBUG 0
-#if defined(CONFIG_TRUSTED_KEYS) || \
-  (defined(CONFIG_TRUSTED_KEYS_MODULE) && defined(CONFIG_ENCRYPTED_KEYS_MODULE))
-extern struct key *request_trusted_key(const char *trusted_desc,
-				       const u8 **master_key, size_t *master_keylen);
-#else
-static inline struct key *request_trusted_key(const char *trusted_desc,
-					      const u8 **master_key,
-					      size_t *master_keylen)
-{
-	return ERR_PTR(-EOPNOTSUPP);
-}
-#endif
 
 #if ENCRYPTED_DEBUG
 static inline void dump_master_key(const u8 *master_key, size_t master_keylen)
diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
deleted file mode 100644
index 7560aea6438d..000000000000
--- a/security/keys/encrypted-keys/masterkey_trusted.c
+++ /dev/null
@@ -1,55 +0,0 @@
-/*
- * Copyright (C) 2010 IBM Corporation
- * Copyright (C) 2010 Politecnico di Torino, Italy
- *                    TORSEC group -- http://security.polito.it
- *
- * Authors:
- * Mimi Zohar <zohar@us.ibm.com>
- * Roberto Sassu <roberto.sassu@polito.it>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, version 2 of the License.
- *
- * See Documentation/security/keys/trusted-encrypted.rst
- */
-
-#include <linux/uaccess.h>
-#include <linux/err.h>
-#include <keys/trusted-type.h>
-#include <keys/encrypted-type.h>
-#include "encrypted.h"
-#include "../internal.h"
-
-/*
- * request_trusted_key - request the trusted key
- *
- * Trusted keys are sealed to PCRs and other metadata. Although userspace
- * manages both trusted/encrypted key-types, like the encrypted key type
- * data, trusted key type data is not visible decrypted from userspace.
- */
-struct key *request_trusted_key(const char *trusted_desc,
-				const u8 **master_key, size_t *master_keylen)
-{
-	struct trusted_key_payload *tpayload;
-	struct key_type *type;
-	struct key *tkey;
-
-	type = key_type_lookup("trusted");
-	if (IS_ERR(type)) {
-		tkey = (struct key *)type;
-		goto error;
-	}
-	tkey = request_key(type, trusted_desc, NULL);
-	if (IS_ERR(tkey))
-		goto error;
-
-	down_read(&tkey->sem);
-	tpayload = tkey->payload.data[0];
-	*master_key = tpayload->key;
-	*master_keylen = tpayload->key_len;
-error:
-	return tkey;
-}
-
-MODULE_SOFTDEP("pre: trusted");
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 8f533c81aa8d..ea2eb78459bf 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -89,8 +89,6 @@ extern spinlock_t key_serial_lock;
 extern struct mutex key_construction_mutex;
 extern wait_queue_head_t request_key_conswq;
 
-
-extern struct key_type *key_type_lookup(const char *type);
 extern void key_type_put(struct key_type *ktype);
 
 extern int __key_link_begin(struct key *keyring,
diff --git a/security/keys/key.c b/security/keys/key.c
index 696f1c092c50..9045b62afb04 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type)
 found_kernel_type:
 	return ktype;
 }
+EXPORT_SYMBOL_GPL(key_type_lookup);
 
 void key_set_timeout(struct key *key, unsigned timeout)
 {

[PATCH 3/6] libnvdimm/security: Drop direct dependency on key_type_encrypted

[Dan Williams]

Lookup the key type by name and protect libnvdimm from encrypted_keys.ko
module load failures.

Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvdimm/security.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index f8bb746a549f..7f9e412f743a 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -48,12 +48,17 @@ static void nvdimm_put_key(struct key *key)
 static struct key *nvdimm_request_key(struct nvdimm *nvdimm)
 {
 	struct key *key = NULL;
+	struct key_type *type;
 	static const char NVDIMM_PREFIX[] = "nvdimm:";
 	char desc[NVDIMM_KEY_DESC_LEN + sizeof(NVDIMM_PREFIX)];
 	struct device *dev = &nvdimm->dev;
 
 	sprintf(desc, "%s%s", NVDIMM_PREFIX, nvdimm->dimm_id);
-	key = request_key(&key_type_encrypted, desc, "");
+	type = key_type_lookup("encrypted");
+	if (IS_ERR(type))
+		return (struct key *) type;
+
+	key = request_key(type, desc, "");
 	if (IS_ERR(key)) {
 		if (PTR_ERR(key) == -ENOKEY)
 			dev_dbg(dev, "request_key() found no key\n");
@@ -88,7 +93,7 @@ static struct key *nvdimm_lookup_user_key(struct nvdimm *nvdimm,
 		return NULL;
 
 	key = key_ref_to_ptr(keyref);
-	if (key->type != &key_type_encrypted) {
+	if (strcmp(key->type->name, "encrypted") != 0) {
 		key_put(key);
 		return NULL;
 	}
@@ -452,3 +457,5 @@ void nvdimm_security_overwrite_query(struct work_struct *work)
 	__nvdimm_security_overwrite_query(nvdimm);
 	nvdimm_bus_unlock(&nvdimm->dev);
 }
+
+MODULE_SOFTDEP("pre: encrypted_keys");

[PATCH 4/6] security/keys/ecryptfs: Drop direct dependency on key_type_encrypted

[Dan Williams]

Lookup the key type by name and protect ecryptfs from encrypted_keys.ko
module load failures, and cleanup the configuration dependencies on the
definition of the ecryptfs_get_encrypted_key() helper.

Cc: Tyler Hicks <tyhicks@canonical.com>
Cc: <ecryptfs@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 fs/ecryptfs/ecryptfs_kernel.h |   22 +---------------------
 fs/ecryptfs/keystore.c        |   12 ++++++++++++
 2 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index e74cb2a0b299..3106d23d95f0 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -87,13 +87,12 @@ struct ecryptfs_page_crypt_context {
 	} param;
 };
 
-#if defined(CONFIG_ENCRYPTED_KEYS) || defined(CONFIG_ENCRYPTED_KEYS_MODULE)
 static inline struct ecryptfs_auth_tok *
 ecryptfs_get_encrypted_key_payload_data(struct key *key)
 {
 	struct encrypted_key_payload *payload;
 
-	if (key->type != &key_type_encrypted)
+	if (strcmp(key->type->name, "encrypted") != 0)
 		return NULL;
 
 	payload = key->payload.data[0];
@@ -103,25 +102,6 @@ ecryptfs_get_encrypted_key_payload_data(struct key *key)
 	return (struct ecryptfs_auth_tok *)payload->payload_data;
 }
 
-static inline struct key *ecryptfs_get_encrypted_key(char *sig)
-{
-	return request_key(&key_type_encrypted, sig, NULL);
-}
-
-#else
-static inline struct ecryptfs_auth_tok *
-ecryptfs_get_encrypted_key_payload_data(struct key *key)
-{
-	return NULL;
-}
-
-static inline struct key *ecryptfs_get_encrypted_key(char *sig)
-{
-	return ERR_PTR(-ENOKEY);
-}
-
-#endif /* CONFIG_ENCRYPTED_KEYS */
-
 static inline struct ecryptfs_auth_tok *
 ecryptfs_get_key_payload_data(struct key *key)
 {
diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
index e74fe84d0886..52a01dd57f4a 100644
--- a/fs/ecryptfs/keystore.c
+++ b/fs/ecryptfs/keystore.c
@@ -1619,6 +1619,17 @@ parse_tag_11_packet(unsigned char *data, unsigned char *contents,
 	return rc;
 }
 
+static struct key *ecryptfs_get_encrypted_key(char *sig)
+{
+	struct key_type *type;
+	struct key *key;
+
+	type = key_type_lookup("encrypted");
+	if (IS_ERR(type))
+		return (struct key *) type;
+	return request_key(type, sig, NULL);
+}
+
 int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
 				      struct ecryptfs_auth_tok **auth_tok,
 				      char *sig)
@@ -2542,3 +2553,4 @@ ecryptfs_add_global_auth_tok(struct ecryptfs_mount_crypt_stat *mount_crypt_stat,
 	return 0;
 }
 
+MODULE_SOFTDEP("pre: encrypted_keys");

[PATCH 5/6] security/integrity/evm: Drop direct dependency on key_type_encrypted

[Dan Williams]

Lookup the key type by name and protect evm from encrypted_keys.ko
module load failures.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: <linux-integrity@vger.kernel.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 security/integrity/evm/evm_crypto.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index c37d08118af5..5c65c3aef427 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -354,10 +354,15 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
 int evm_init_key(void)
 {
 	struct key *evm_key;
+	struct key_type *type;
 	struct encrypted_key_payload *ekp;
 	int rc;
 
-	evm_key = request_key(&key_type_encrypted, EVMKEY, NULL);
+	type = key_type_lookup("encrypted");
+	if (IS_ERR(type))
+		return PTR_ERR(type);
+
+	evm_key = request_key(type, EVMKEY, NULL);
 	if (IS_ERR(evm_key))
 		return -ENOENT;
 
@@ -372,3 +377,5 @@ int evm_init_key(void)
 	key_put(evm_key);
 	return rc;
 }
+
+MODULE_SOFTDEP("pre: encrypted_keys");

[PATCH 6/6] security/keys/encrypted: Drop export of key_type_encrypted

[Dan Williams]

Now that all users lookup the key type by name, drop the export and the
direct module dependency.

Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/keys/encrypted-type.h            |    2 --
 security/keys/encrypted-keys/encrypted.c |    3 +--
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/keys/encrypted-type.h b/include/keys/encrypted-type.h
index 1d4541370a64..dd509835b4a4 100644
--- a/include/keys/encrypted-type.h
+++ b/include/keys/encrypted-type.h
@@ -33,6 +33,4 @@ struct encrypted_key_payload {
 	u8 payload_data[0];	/* payload data + datablob + hmac */
 };
 
-extern struct key_type key_type_encrypted;
-
 #endif /* _KEYS_ENCRYPTED_TYPE_H */
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 06925d3b30c9..c3999d5e2a19 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -1012,7 +1012,7 @@ static void encrypted_destroy(struct key *key)
 	kzfree(key->payload.data[0]);
 }
 
-struct key_type key_type_encrypted = {
+static struct key_type key_type_encrypted = {
 	.name = "encrypted",
 	.instantiate = encrypted_instantiate,
 	.update = encrypted_update,
@@ -1020,7 +1020,6 @@ struct key_type key_type_encrypted = {
 	.describe = user_describe,
 	.read = encrypted_read,
 };
-EXPORT_SYMBOL_GPL(key_type_encrypted);
 
 static int __init init_encrypted(void)
 {

Re: [PATCH 3/6] libnvdimm/security: Drop direct dependency on key_type_encrypted

[Dave Jiang]

On 3/18/19 11:06 PM, Dan Williams wrote:
> Lookup the key type by name and protect libnvdimm from encrypted_keys.ko
> module load failures.
> 
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/nvdimm/security.c |   11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index f8bb746a549f..7f9e412f743a 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -48,12 +48,17 @@ static void nvdimm_put_key(struct key *key)
>  static struct key *nvdimm_request_key(struct nvdimm *nvdimm)
>  {
>  	struct key *key = NULL;
> +	struct key_type *type;
>  	static const char NVDIMM_PREFIX[] = "nvdimm:";
>  	char desc[NVDIMM_KEY_DESC_LEN + sizeof(NVDIMM_PREFIX)];
>  	struct device *dev = &nvdimm->dev;
>  
>  	sprintf(desc, "%s%s", NVDIMM_PREFIX, nvdimm->dimm_id);
> -	key = request_key(&key_type_encrypted, desc, "");
> +	type = key_type_lookup("encrypted");
> +	if (IS_ERR(type))
> +		return (struct key *) type;
> +
> +	key = request_key(type, desc, "");
>  	if (IS_ERR(key)) {
>  		if (PTR_ERR(key) == -ENOKEY)
>  			dev_dbg(dev, "request_key() found no key\n");
> @@ -88,7 +93,7 @@ static struct key *nvdimm_lookup_user_key(struct nvdimm *nvdimm,
>  		return NULL;
>  
>  	key = key_ref_to_ptr(keyref);
> -	if (key->type != &key_type_encrypted) {
> +	if (strcmp(key->type->name, "encrypted") != 0) {
>  		key_put(key);
>  		return NULL;
>  	}
> @@ -452,3 +457,5 @@ void nvdimm_security_overwrite_query(struct work_struct *work)
>  	__nvdimm_security_overwrite_query(nvdimm);
>  	nvdimm_bus_unlock(&nvdimm->dev);
>  }
> +
> +MODULE_SOFTDEP("pre: encrypted_keys");
> 

Re: [PATCH 0/6] security/keys/encrypted: Break module dependency chain

[Dan Williams]

On Mon, Mar 18, 2019 at 11:18 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> With v5.1-rc1 all the nvdimm sub-system regression tests started failing
> because the libnvdimm module failed to load in the qemu-kvm test
> environment.  Critically that environment does not have a TPM. Commit
> 240730437deb "KEYS: trusted: explicitly use tpm_chip structure..."
> started to require a TPM to be present for the trusted.ko module to load
> where there was no requirement for that before.
>
> Rather than undo the "fail if no hardware" behavior James points out
> that the module dependencies can be broken by looking up the key-type by
> name. Remove the dependencies on the "key_type_trusted" and
> "key_type_encrypted" symbol exports, and clean up other boilerplate that
> supported those exports in different configurations.

Any feedback? Was hoping to get at least patch1 in the queue for
v5.1-rc2 since this effectively disables the nvdimm driver on typical
configurations. Jarkko, would you be willing to merge it since the
regression came through your tree?

> Dan Williams (6):
>       security/keys/encrypted: Allow operation without trusted.ko
>       security/keys/encrypted: Clean up request_trusted_key()
>       libnvdimm/security: Drop direct dependency on key_type_encrypted
>       security/keys/ecryptfs: Drop direct dependency on key_type_encrypted
>       security/integrity/evm: Drop direct dependency on key_type_encrypted
>       security/keys/encrypted: Drop export of key_type_encrypted
>
>
>  drivers/nvdimm/security.c                        |   11 ++++-
>  fs/ecryptfs/ecryptfs_kernel.h                    |   22 -----------
>  fs/ecryptfs/keystore.c                           |   12 ++++++
>  include/keys/encrypted-type.h                    |    2 -
>  include/linux/key.h                              |    1
>  security/integrity/evm/evm_crypto.c              |    9 ++++
>  security/keys/encrypted-keys/Makefile            |    3 -
>  security/keys/encrypted-keys/encrypted.c         |   35 ++++++++++++++++-
>  security/keys/encrypted-keys/encrypted.h         |   12 ------
>  security/keys/encrypted-keys/masterkey_trusted.c |   46 ----------------------
>  security/keys/internal.h                         |    2 -
>  security/keys/key.c                              |    1
>  12 files changed, 65 insertions(+), 91 deletions(-)
>  delete mode 100644 security/keys/encrypted-keys/masterkey_trusted.c

Re: [PATCH 0/6] security/keys/encrypted: Break module dependency chain

[James Bottomley]

On Tue, 2019-03-19 at 14:01 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 11:18 PM Dan Williams <dan.j.williams@intel.c
> om> wrote:
> > 
> > With v5.1-rc1 all the nvdimm sub-system regression tests started
> > failing because the libnvdimm module failed to load in the qemu-kvm 
> > test environment.  Critically that environment does not have a TPM.
> > Commit 240730437deb "KEYS: trusted: explicitly use tpm_chip
> > structure..." started to require a TPM to be present for the
> > trusted.ko module to load where there was no requirement for that
> > before.
> > 
> > Rather than undo the "fail if no hardware" behavior James points
> > out that the module dependencies can be broken by looking up the
> > key-type by name. Remove the dependencies on the "key_type_trusted"
> > and "key_type_encrypted" symbol exports, and clean up other
> > boilerplate that supported those exports in different
> > configurations.
> 
> Any feedback? Was hoping to get at least patch1 in the queue for
> v5.1-rc2 since this effectively disables the nvdimm driver on typical
> configurations. Jarkko, would you be willing to merge it since the
> regression came through your tree?

The reason I sent out the RFC was to see if the people who actually
wrote the code had any reasons they needed the current way of doing
things preserving.

I think your series looks fine except you need to export
key_type_lookup (patch 2) before you use it (patch 1) to preserve
bisectability of builds.

James

Re: [PATCH 0/6] security/keys/encrypted: Break module dependency chain

[Dan Williams]

On Tue, Mar 19, 2019 at 2:09 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Tue, 2019-03-19 at 14:01 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 11:18 PM Dan Williams <dan.j.williams@intel.c
> > om> wrote:
> > >
> > > With v5.1-rc1 all the nvdimm sub-system regression tests started
> > > failing because the libnvdimm module failed to load in the qemu-kvm
> > > test environment.  Critically that environment does not have a TPM.
> > > Commit 240730437deb "KEYS: trusted: explicitly use tpm_chip
> > > structure..." started to require a TPM to be present for the
> > > trusted.ko module to load where there was no requirement for that
> > > before.
> > >
> > > Rather than undo the "fail if no hardware" behavior James points
> > > out that the module dependencies can be broken by looking up the
> > > key-type by name. Remove the dependencies on the "key_type_trusted"
> > > and "key_type_encrypted" symbol exports, and clean up other
> > > boilerplate that supported those exports in different
> > > configurations.
> >
> > Any feedback? Was hoping to get at least patch1 in the queue for
> > v5.1-rc2 since this effectively disables the nvdimm driver on typical
> > configurations. Jarkko, would you be willing to merge it since the
> > regression came through your tree?
>
> The reason I sent out the RFC was to see if the people who actually
> wrote the code had any reasons they needed the current way of doing
> things preserving.
>
> I think your series looks fine except you need to export
> key_type_lookup (patch 2) before you use it (patch 1) to preserve
> bisectability of builds.

Good catch, I fumbled that around. I'll respin and let 0day chew on it
before sending out another version.

Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Mimi Zohar]

On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote:

< snip >

> +/*
> + * request_trusted_key - request the trusted key
> + *
> + * Trusted keys are sealed to PCRs and other metadata. Although userspace
> + * manages both trusted/encrypted key-types, like the encrypted key type
> + * data, trusted key type data is not visible decrypted from userspace.
> + */
> +static struct key *request_trusted_key(const char *trusted_desc,
> +				const u8 **master_key, size_t *master_keylen)
> +{
> +	struct trusted_key_payload *tpayload;
> +	struct key_type *type;
> +	struct key *tkey;
> +
> +	type = key_type_lookup("trusted");

The associated key_type_put() will need to be called.

> +	if (IS_ERR(type)) {
> +		tkey = (struct key *)type;
> +		goto error;
> +	}
> +	tkey = request_key(type, trusted_desc, NULL);
> +	if (IS_ERR(tkey))
> +		goto error;
> +
> +	down_read(&tkey->sem);
> +	tpayload = tkey->payload.data[0];
> +	*master_key = tpayload->key;
> +	*master_keylen = tpayload->key_len;
> +error:
> +	return tkey;
> +}
> +



> diff --git a/security/keys/key.c b/security/keys/key.c
> index 696f1c092c50..9045b62afb04 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type)
>  found_kernel_type:
>  	return ktype;
>  }
> +EXPORT_SYMBOL_GPL(key_type_lookup);

Only the kernel is calling key_type_lookup().  Why does
key_type_lookup() need to be exported?

Mimi

>  
>  void key_set_timeout(struct key *key, unsigned timeout)
>  {
> 

Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Dan Williams]

On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote:
>
> < snip >
>
> > +/*
> > + * request_trusted_key - request the trusted key
> > + *
> > + * Trusted keys are sealed to PCRs and other metadata. Although userspace
> > + * manages both trusted/encrypted key-types, like the encrypted key type
> > + * data, trusted key type data is not visible decrypted from userspace.
> > + */
> > +static struct key *request_trusted_key(const char *trusted_desc,
> > +                             const u8 **master_key, size_t *master_keylen)
> > +{
> > +     struct trusted_key_payload *tpayload;
> > +     struct key_type *type;
> > +     struct key *tkey;
> > +
> > +     type = key_type_lookup("trusted");
>
> The associated key_type_put() will need to be called.

Yes.

>
> > +     if (IS_ERR(type)) {
> > +             tkey = (struct key *)type;
> > +             goto error;
> > +     }
> > +     tkey = request_key(type, trusted_desc, NULL);
> > +     if (IS_ERR(tkey))
> > +             goto error;
> > +
> > +     down_read(&tkey->sem);
> > +     tpayload = tkey->payload.data[0];
> > +     *master_key = tpayload->key;
> > +     *master_keylen = tpayload->key_len;
> > +error:
> > +     return tkey;
> > +}
> > +
>
>
>
> > diff --git a/security/keys/key.c b/security/keys/key.c
> > index 696f1c092c50..9045b62afb04 100644
> > --- a/security/keys/key.c
> > +++ b/security/keys/key.c
> > @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type)
> >  found_kernel_type:
> >       return ktype;
> >  }
> > +EXPORT_SYMBOL_GPL(key_type_lookup);

This needs to be moved to patch1.

> Only the kernel is calling key_type_lookup().  Why does
> key_type_lookup() need to be exported?

This patch series adds several new callers outside of keys-subsystem
core that need this export, the first one being encrypted-keys itself
in patch1.

drivers/nvdimm/security.c:57:   type = key_type_lookup("encrypted");
fs/ecryptfs/keystore.c:1627:    type = key_type_lookup("encrypted");
security/integrity/evm/evm_crypto.c:361:        type =
key_type_lookup("encrypted");
security/keys/encrypted-keys/encrypted.c:440:   type =
key_type_lookup("trusted");

Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Mimi Zohar]

On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote:
> 
> > > diff --git a/security/keys/key.c b/security/keys/key.c
> > > index 696f1c092c50..9045b62afb04 100644
> > > --- a/security/keys/key.c
> > > +++ b/security/keys/key.c
> > > @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type)
> > >  found_kernel_type:
> > >       return ktype;
> > >  }
> > > +EXPORT_SYMBOL_GPL(key_type_lookup);
> 
> This needs to be moved to patch1.
> 
> > Only the kernel is calling key_type_lookup().  Why does
> > key_type_lookup() need to be exported?
> 
> This patch series adds several new callers outside of keys-subsystem
> core that need this export, the first one being encrypted-keys itself
> in patch1.

It's needed, because they could be compiled as kernel modules, not
builtin (eg. EVM).

Mimi

> 
> drivers/nvdimm/security.c:57:   type = key_type_lookup("encrypted");
> fs/ecryptfs/keystore.c:1627:    type = key_type_lookup("encrypted");
> security/integrity/evm/evm_crypto.c:361:        type =
> key_type_lookup("encrypted");
> security/keys/encrypted-keys/encrypted.c:440:   type =
> key_type_lookup("trusted");
> 

Re: [PATCH 0/6] security/keys/encrypted: Break module dependency chain

[Mimi Zohar]

On Tue, 2019-03-19 at 14:08 -0700, James Bottomley wrote:
> On Tue, 2019-03-19 at 14:01 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 11:18 PM Dan Williams <dan.j.williams@intel.c
> > om> wrote:
> > > 
> > > With v5.1-rc1 all the nvdimm sub-system regression tests started
> > > failing because the libnvdimm module failed to load in the qemu-kvm 
> > > test environment.  Critically that environment does not have a TPM.
> > > Commit 240730437deb "KEYS: trusted: explicitly use tpm_chip
> > > structure..." started to require a TPM to be present for the
> > > trusted.ko module to load where there was no requirement for that
> > > before.
> > > 
> > > Rather than undo the "fail if no hardware" behavior James points
> > > out that the module dependencies can be broken by looking up the
> > > key-type by name. Remove the dependencies on the "key_type_trusted"
> > > and "key_type_encrypted" symbol exports, and clean up other
> > > boilerplate that supported those exports in different
> > > configurations.
> > 
> > Any feedback? Was hoping to get at least patch1 in the queue for
> > v5.1-rc2 since this effectively disables the nvdimm driver on typical
> > configurations. Jarkko, would you be willing to merge it since the
> > regression came through your tree?
> 
> The reason I sent out the RFC was to see if the people who actually
> wrote the code had any reasons they needed the current way of doing
> things preserving.

No, it looks fine.  The error return codes will change, but I don't
think that is a problem.

Mimi

> 
> I think your series looks fine except you need to export
> key_type_lookup (patch 2) before you use it (patch 1) to preserve
> bisectability of builds.
> 
> James

Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Dan Williams]

On Tue, Mar 19, 2019 at 6:11 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote:
> >
> > > > diff --git a/security/keys/key.c b/security/keys/key.c
> > > > index 696f1c092c50..9045b62afb04 100644
> > > > --- a/security/keys/key.c
> > > > +++ b/security/keys/key.c
> > > > @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type)
> > > >  found_kernel_type:
> > > >       return ktype;
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(key_type_lookup);
> >
> > This needs to be moved to patch1.
> >
> > > Only the kernel is calling key_type_lookup().  Why does
> > > key_type_lookup() need to be exported?
> >
> > This patch series adds several new callers outside of keys-subsystem
> > core that need this export, the first one being encrypted-keys itself
> > in patch1.
>
> It's needed, because they could be compiled as kernel modules, not
> builtin (eg. EVM).
>

Right, but now I realize a problem. The side effect of having direct
module dependencies to the key_type_{encrypted,trusted} symbols is
that module reference counting is handled automatically.

So, I need to respin this with some explicit try_module_get() and
module_put() added otherwise the encrypted_keys facility can be
removed while active keys are instantiated.

> > drivers/nvdimm/security.c:57:   type = key_type_lookup("encrypted");
> > fs/ecryptfs/keystore.c:1627:    type = key_type_lookup("encrypted");
> > security/integrity/evm/evm_crypto.c:361:        type =
> > key_type_lookup("encrypted");
> > security/keys/encrypted-keys/encrypted.c:440:   type =
> > key_type_lookup("trusted");
> >
>

Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Dan Williams]

On Tue, Mar 19, 2019 at 6:34 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Tue, Mar 19, 2019 at 6:11 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote:
> > >
> > > > > diff --git a/security/keys/key.c b/security/keys/key.c
> > > > > index 696f1c092c50..9045b62afb04 100644
> > > > > --- a/security/keys/key.c
> > > > > +++ b/security/keys/key.c
> > > > > @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type)
> > > > >  found_kernel_type:
> > > > >       return ktype;
> > > > >  }
> > > > > +EXPORT_SYMBOL_GPL(key_type_lookup);
> > >
> > > This needs to be moved to patch1.
> > >
> > > > Only the kernel is calling key_type_lookup().  Why does
> > > > key_type_lookup() need to be exported?
> > >
> > > This patch series adds several new callers outside of keys-subsystem
> > > core that need this export, the first one being encrypted-keys itself
> > > in patch1.
> >
> > It's needed, because they could be compiled as kernel modules, not
> > builtin (eg. EVM).
> >
>
> Right, but now I realize a problem. The side effect of having direct
> module dependencies to the key_type_{encrypted,trusted} symbols is
> that module reference counting is handled automatically.
>
> So, I need to respin this with some explicit try_module_get() and
> module_put() added otherwise the encrypted_keys facility can be
> removed while active keys are instantiated.

...and now I'm wondering if this refactoring is getting too big and
should wait for v5.2. In the meantime apply my small fix for v5.1

https://patchwork.kernel.org/patch/10858649/

Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Mimi Zohar]

On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote:
> >
> > < snip >
> >
> > > +/*
> > > + * request_trusted_key - request the trusted key
> > > + *
> > > + * Trusted keys are sealed to PCRs and other metadata. Although userspace
> > > + * manages both trusted/encrypted key-types, like the encrypted key type
> > > + * data, trusted key type data is not visible decrypted from userspace.
> > > + */
> > > +static struct key *request_trusted_key(const char *trusted_desc,
> > > +                             const u8 **master_key, size_t *master_keylen)
> > > +{
> > > +     struct trusted_key_payload *tpayload;
> > > +     struct key_type *type;
> > > +     struct key *tkey;
> > > +
> > > +     type = key_type_lookup("trusted");
> >
> > The associated key_type_put() will need to be called.
> 
> Yes.

I don't know if defining a key_type_lookup() wrapper, perhaps named
is_key_type_available(), would help.  Both key_type_lookup() and
key_type_put() would be called.  The existing code could then remain
the same.

Mimi

> 
> >
> > > +     if (IS_ERR(type)) {
> > > +             tkey = (struct key *)type;
> > > +             goto error;
> > > +     }
> > > +     tkey = request_key(type, trusted_desc, NULL);
> > > +     if (IS_ERR(tkey))
> > > +             goto error;
> > > +
> > > +     down_read(&tkey->sem);
> > > +     tpayload = tkey->payload.data[0];
> > > +     *master_key = tpayload->key;
> > > +     *master_keylen = tpayload->key_len;
> > > +error:
> > > +     return tkey;
> > > +}
> > > +
> >

Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Dan Williams]

On Tue, Mar 19, 2019 at 7:36 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >
> > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote:
> > >
> > > < snip >
> > >
> > > > +/*
> > > > + * request_trusted_key - request the trusted key
> > > > + *
> > > > + * Trusted keys are sealed to PCRs and other metadata. Although userspace
> > > > + * manages both trusted/encrypted key-types, like the encrypted key type
> > > > + * data, trusted key type data is not visible decrypted from userspace.
> > > > + */
> > > > +static struct key *request_trusted_key(const char *trusted_desc,
> > > > +                             const u8 **master_key, size_t *master_keylen)
> > > > +{
> > > > +     struct trusted_key_payload *tpayload;
> > > > +     struct key_type *type;
> > > > +     struct key *tkey;
> > > > +
> > > > +     type = key_type_lookup("trusted");
> > >
> > > The associated key_type_put() will need to be called.
> >
> > Yes.
>
> I don't know if defining a key_type_lookup() wrapper, perhaps named
> is_key_type_available(), would help.  Both key_type_lookup() and
> key_type_put() would be called.  The existing code could then remain
> the same.
>

Maybe, but something still needs to pin the hosting module. I think
this means that the first call to key_type->instantiate() pins the
hosting module, and the ->destroy() of the last key for the key_type
unpins the module. It does mean that the ->destroy() method is no
longer optional.

Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Mimi Zohar]

On Tue, 2019-03-19 at 22:48 -0700, Dan Williams wrote:
> On Tue, Mar 19, 2019 at 7:36 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > >
> > > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote:
> > > >
> > > > < snip >
> > > >
> > > > > +/*
> > > > > + * request_trusted_key - request the trusted key
> > > > > + *
> > > > > + * Trusted keys are sealed to PCRs and other metadata. Although userspace
> > > > > + * manages both trusted/encrypted key-types, like the encrypted key type
> > > > > + * data, trusted key type data is not visible decrypted from userspace.
> > > > > + */
> > > > > +static struct key *request_trusted_key(const char *trusted_desc,
> > > > > +                             const u8 **master_key, size_t *master_keylen)
> > > > > +{
> > > > > +     struct trusted_key_payload *tpayload;
> > > > > +     struct key_type *type;
> > > > > +     struct key *tkey;
> > > > > +
> > > > > +     type = key_type_lookup("trusted");
> > > >
> > > > The associated key_type_put() will need to be called.
> > >
> > > Yes.
> >
> > I don't know if defining a key_type_lookup() wrapper, perhaps named
> > is_key_type_available(), would help.  Both key_type_lookup() and
> > key_type_put() would be called.  The existing code could then remain
> > the same.
> >
> 
> Maybe, but something still needs to pin the hosting module. I think
> this means that the first call to key_type->instantiate() pins the
> hosting module, and the ->destroy() of the last key for the key_type
> unpins the module. It does mean that the ->destroy() method is no
> longer optional.

This sounds like it isn't a new problem.  Both issues need to be
addressed, but I think we should differentiate between them and
address them separately.

In terms of the original nvdimm encrypted/trusted key problem, the
above suggestion requires the least amount of change.  For v5.2, I
would replace it with the full updated patch set.

Mimi

Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()

[Dan Williams]

On Wed, Mar 20, 2019 at 5:07 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Tue, 2019-03-19 at 22:48 -0700, Dan Williams wrote:
> > On Tue, Mar 19, 2019 at 7:36 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >
> > > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > >
> > > > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote:
> > > > >
> > > > > < snip >
> > > > >
> > > > > > +/*
> > > > > > + * request_trusted_key - request the trusted key
> > > > > > + *
> > > > > > + * Trusted keys are sealed to PCRs and other metadata. Although userspace
> > > > > > + * manages both trusted/encrypted key-types, like the encrypted key type
> > > > > > + * data, trusted key type data is not visible decrypted from userspace.
> > > > > > + */
> > > > > > +static struct key *request_trusted_key(const char *trusted_desc,
> > > > > > +                             const u8 **master_key, size_t *master_keylen)
> > > > > > +{
> > > > > > +     struct trusted_key_payload *tpayload;
> > > > > > +     struct key_type *type;
> > > > > > +     struct key *tkey;
> > > > > > +
> > > > > > +     type = key_type_lookup("trusted");
> > > > >
> > > > > The associated key_type_put() will need to be called.
> > > >
> > > > Yes.
> > >
> > > I don't know if defining a key_type_lookup() wrapper, perhaps named
> > > is_key_type_available(), would help.  Both key_type_lookup() and
> > > key_type_put() would be called.  The existing code could then remain
> > > the same.
> > >
> >
> > Maybe, but something still needs to pin the hosting module. I think
> > this means that the first call to key_type->instantiate() pins the
> > hosting module, and the ->destroy() of the last key for the key_type
> > unpins the module. It does mean that the ->destroy() method is no
> > longer optional.
>
> This sounds like it isn't a new problem.  Both issues need to be
> addressed, but I think we should differentiate between them and
> address them separately.
>
> In terms of the original nvdimm encrypted/trusted key problem, the
> above suggestion requires the least amount of change.  For v5.2, I
> would replace it with the full updated patch set.

I believe smallest amount of change is this single patch:

https://patchwork.kernel.org/patch/10858649/

Re: [PATCH 0/6] security/keys/encrypted: Break module dependency chain

[Jarkko Sakkinen]

On Tue, Mar 19, 2019 at 02:01:44PM -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 11:18 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > With v5.1-rc1 all the nvdimm sub-system regression tests started failing
> > because the libnvdimm module failed to load in the qemu-kvm test
> > environment.  Critically that environment does not have a TPM. Commit
> > 240730437deb "KEYS: trusted: explicitly use tpm_chip structure..."
> > started to require a TPM to be present for the trusted.ko module to load
> > where there was no requirement for that before.
> >
> > Rather than undo the "fail if no hardware" behavior James points out
> > that the module dependencies can be broken by looking up the key-type by
> > name. Remove the dependencies on the "key_type_trusted" and
> > "key_type_encrypted" symbol exports, and clean up other boilerplate that
> > supported those exports in different configurations.
> 
> Any feedback? Was hoping to get at least patch1 in the queue for
> v5.1-rc2 since this effectively disables the nvdimm driver on typical
> configurations. Jarkko, would you be willing to merge it since the
> regression came through your tree?

Yes, of course. The feedback has been extremely passive because I've
been sick leave for the early week :-)

Before I'm merging this I'm just thinking that would it be better
idea to merge a patch for trusted.c that reverts the old behavior
with cc to stable and fixes tags as I said in my earlier response.

It would less intrusive for stable kernels. Lets quickly sort out
the best strategy before merging.

/Jarkko

Re: [PATCH 0/6] security/keys/encrypted: Break module dependency chain

[Jarkko Sakkinen]

On Thu, Mar 21, 2019 at 03:45:49PM +0200, Jarkko Sakkinen wrote:
> On Tue, Mar 19, 2019 at 02:01:44PM -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 11:18 PM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > With v5.1-rc1 all the nvdimm sub-system regression tests started failing
> > > because the libnvdimm module failed to load in the qemu-kvm test
> > > environment.  Critically that environment does not have a TPM. Commit
> > > 240730437deb "KEYS: trusted: explicitly use tpm_chip structure..."
> > > started to require a TPM to be present for the trusted.ko module to load
> > > where there was no requirement for that before.
> > >
> > > Rather than undo the "fail if no hardware" behavior James points out
> > > that the module dependencies can be broken by looking up the key-type by
> > > name. Remove the dependencies on the "key_type_trusted" and
> > > "key_type_encrypted" symbol exports, and clean up other boilerplate that
> > > supported those exports in different configurations.
> > 
> > Any feedback? Was hoping to get at least patch1 in the queue for
> > v5.1-rc2 since this effectively disables the nvdimm driver on typical
> > configurations. Jarkko, would you be willing to merge it since the
> > regression came through your tree?
> 
> Yes, of course. The feedback has been extremely passive because I've
> been sick leave for the early week :-)
> 
> Before I'm merging this I'm just thinking that would it be better
> idea to merge a patch for trusted.c that reverts the old behavior
> with cc to stable and fixes tags as I said in my earlier response.
> 
> It would less intrusive for stable kernels. Lets quickly sort out
> the best strategy before merging.

I.e. the way I see the situation:

1. Reverting the old behavior in the sense that missing TPM does
   not prevent init of trusted.ko should be done right now.
2. Your patch could be definitely merged but not as a bug fix.
3. At some point we could consider failing the init of trusted.ko
   if TPM is missing because that is kind of senseful anyway with
   better testing now that we understand the dependency context
   better.