All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-18 23:45 ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-18 23:45 UTC (permalink / raw)
  To: jarkko.sakkinen
  Cc: linux-nvdimm, James Bottomley, Roberto Sassu, linux-kernel,
	Mimi Zohar, David Howells, keyrings

Rather than fail initialization of the trusted.ko module, arrange for
the module to load, but rely on trusted_instantiate() to fail
trusted-key operations.

Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: James Bottomley <jejb@linux.ibm.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/trusted.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index bcc9c6ead7fd..d959597a688e 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -45,6 +45,13 @@ struct sdesc {
 static struct crypto_shash *hashalg;
 static struct crypto_shash *hmacalg;
 
+static struct device *chip_dev(struct tpm_chip *chip)
+{
+	if (chip)
+		return &chip->dev;
+	return NULL;
+}
+
 static struct sdesc *init_sdesc(struct crypto_shash *alg)
 {
 	struct sdesc *sdesc;
@@ -1224,6 +1231,14 @@ static int __init init_digests(void)
 	int ret;
 	int i;
 
+	/*
+	 * Hardware tpm operations are disabled, but allow the software
+	 * module to initialize, and depend on trusted_instantiate() to
+	 * fail any attempts to access the missing hardware.
+	 */
+	if (!chip)
+		return 0;
+
 	ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
 	if (ret < 0)
 		return ret;
@@ -1246,8 +1261,6 @@ static int __init init_trusted(void)
 	int ret;
 
 	chip = tpm_default_chip();
-	if (!chip)
-		return -ENOENT;
 	ret = init_digests();
 	if (ret < 0)
 		goto err_put;
@@ -1263,13 +1276,13 @@ static int __init init_trusted(void)
 err_free:
 	kfree(digests);
 err_put:
-	put_device(&chip->dev);
+	put_device(chip_dev(chip));
 	return ret;
 }
 
 static void __exit cleanup_trusted(void)
 {
-	put_device(&chip->dev);
+	put_device(chip_dev(chip));
 	kfree(digests);
 	trusted_shash_release();
 	unregister_key_type(&key_type_trusted);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-18 23:45 ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-18 23:45 UTC (permalink / raw)
  To: jarkko.sakkinen
  Cc: linux-nvdimm, James Bottomley, Roberto Sassu, linux-kernel,
	Mimi Zohar, David Howells, keyrings

Rather than fail initialization of the trusted.ko module, arrange for
the module to load, but rely on trusted_instantiate() to fail
trusted-key operations.

Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: James Bottomley <jejb@linux.ibm.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/trusted.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index bcc9c6ead7fd..d959597a688e 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -45,6 +45,13 @@ struct sdesc {
 static struct crypto_shash *hashalg;
 static struct crypto_shash *hmacalg;
 
+static struct device *chip_dev(struct tpm_chip *chip)
+{
+	if (chip)
+		return &chip->dev;
+	return NULL;
+}
+
 static struct sdesc *init_sdesc(struct crypto_shash *alg)
 {
 	struct sdesc *sdesc;
@@ -1224,6 +1231,14 @@ static int __init init_digests(void)
 	int ret;
 	int i;
 
+	/*
+	 * Hardware tpm operations are disabled, but allow the software
+	 * module to initialize, and depend on trusted_instantiate() to
+	 * fail any attempts to access the missing hardware.
+	 */
+	if (!chip)
+		return 0;
+
 	ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
 	if (ret < 0)
 		return ret;
@@ -1246,8 +1261,6 @@ static int __init init_trusted(void)
 	int ret;
 
 	chip = tpm_default_chip();
-	if (!chip)
-		return -ENOENT;
 	ret = init_digests();
 	if (ret < 0)
 		goto err_put;
@@ -1263,13 +1276,13 @@ static int __init init_trusted(void)
 err_free:
 	kfree(digests);
 err_put:
-	put_device(&chip->dev);
+	put_device(chip_dev(chip));
 	return ret;
 }
 
 static void __exit cleanup_trusted(void)
 {
-	put_device(&chip->dev);
+	put_device(chip_dev(chip));
 	kfree(digests);
 	trusted_shash_release();
 	unregister_key_type(&key_type_trusted);

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-18 23:45 ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-18 23:45 UTC (permalink / raw)
  To: jarkko.sakkinen
  Cc: Roberto Sassu, James Bottomley, Mimi Zohar, David Howells,
	keyrings, linux-nvdimm, linux-kernel

Rather than fail initialization of the trusted.ko module, arrange for
the module to load, but rely on trusted_instantiate() to fail
trusted-key operations.

Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: James Bottomley <jejb@linux.ibm.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/trusted.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index bcc9c6ead7fd..d959597a688e 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -45,6 +45,13 @@ struct sdesc {
 static struct crypto_shash *hashalg;
 static struct crypto_shash *hmacalg;
 
+static struct device *chip_dev(struct tpm_chip *chip)
+{
+	if (chip)
+		return &chip->dev;
+	return NULL;
+}
+
 static struct sdesc *init_sdesc(struct crypto_shash *alg)
 {
 	struct sdesc *sdesc;
@@ -1224,6 +1231,14 @@ static int __init init_digests(void)
 	int ret;
 	int i;
 
+	/*
+	 * Hardware tpm operations are disabled, but allow the software
+	 * module to initialize, and depend on trusted_instantiate() to
+	 * fail any attempts to access the missing hardware.
+	 */
+	if (!chip)
+		return 0;
+
 	ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
 	if (ret < 0)
 		return ret;
@@ -1246,8 +1261,6 @@ static int __init init_trusted(void)
 	int ret;
 
 	chip = tpm_default_chip();
-	if (!chip)
-		return -ENOENT;
 	ret = init_digests();
 	if (ret < 0)
 		goto err_put;
@@ -1263,13 +1276,13 @@ static int __init init_trusted(void)
 err_free:
 	kfree(digests);
 err_put:
-	put_device(&chip->dev);
+	put_device(chip_dev(chip));
 	return ret;
 }
 
 static void __exit cleanup_trusted(void)
 {
-	put_device(&chip->dev);
+	put_device(chip_dev(chip));
 	kfree(digests);
 	trusted_shash_release();
 	unregister_key_type(&key_type_trusted);


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
       [not found] ` <155295271345.1945351.6465460744078693578.stgit-p8uTFz9XbKj2zm6wflaqv1nYeNYlB/vhral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  2019-03-19  0:24     ` James Bottomley
@ 2019-03-19  0:24     ` James Bottomley
  0 siblings, 0 replies; 50+ messages in thread
From: James Bottomley @ 2019-03-19  0:24 UTC (permalink / raw)
  To: Dan Williams, jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Roberto Sassu,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mimi Zohar, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA

On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> Rather than fail initialization of the trusted.ko module, arrange for
> the module to load, but rely on trusted_instantiate() to fail
> trusted-key operations.

What actual problem is this fixing?  To me it would seem like an
enhancement to make the trusted module fail at load time if there's no
TPM rather than waiting until first use to find out it can never work. 
Is there some piece of user code that depends on the successful
insertion of trusted.ko?

James

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19  0:24     ` James Bottomley
  0 siblings, 0 replies; 50+ messages in thread
From: James Bottomley @ 2019-03-19  0:24 UTC (permalink / raw)
  To: Dan Williams, jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Roberto Sassu,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mimi Zohar, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA

On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> Rather than fail initialization of the trusted.ko module, arrange for
> the module to load, but rely on trusted_instantiate() to fail
> trusted-key operations.

What actual problem is this fixing?  To me it would seem like an
enhancement to make the trusted module fail at load time if there's no
TPM rather than waiting until first use to find out it can never work. 
Is there some piece of user code that depends on the successful
insertion of trusted.ko?

James

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19  0:24     ` James Bottomley
  0 siblings, 0 replies; 50+ messages in thread
From: James Bottomley @ 2019-03-19  0:24 UTC (permalink / raw)
  To: Dan Williams, jarkko.sakkinen
  Cc: Roberto Sassu, Mimi Zohar, David Howells, keyrings, linux-nvdimm,
	linux-kernel

On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> Rather than fail initialization of the trusted.ko module, arrange for
> the module to load, but rely on trusted_instantiate() to fail
> trusted-key operations.

What actual problem is this fixing?  To me it would seem like an
enhancement to make the trusted module fail at load time if there's no
TPM rather than waiting until first use to find out it can never work. 
Is there some piece of user code that depends on the successful
insertion of trusted.ko?

James


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-19  0:24     ` James Bottomley
  (?)
@ 2019-03-19  0:30       ` Dan Williams
  -1 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-19  0:30 UTC (permalink / raw)
  To: James Bottomley
  Cc: linux-nvdimm, Roberto Sassu, Linux Kernel Mailing List,
	Mimi Zohar, David Howells, keyrings, Jarkko Sakkinen

On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > Rather than fail initialization of the trusted.ko module, arrange for
> > the module to load, but rely on trusted_instantiate() to fail
> > trusted-key operations.
>
> What actual problem is this fixing?  To me it would seem like an
> enhancement to make the trusted module fail at load time if there's no
> TPM rather than waiting until first use to find out it can never work.
> Is there some piece of user code that depends on the successful
> insertion of trusted.ko?

The module dependency chain relies on it. If that can be broken that
would also be an acceptable fix.

I found this through the following dependency chain: libnvdimm.ko ->
encrypted_keys.ko -> trusted.ko.

"key_type_trusted" is the symbol that encrypted_keys needs regardless
of whether the tpm is present.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19  0:30       ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-19  0:30 UTC (permalink / raw)
  To: James Bottomley
  Cc: linux-nvdimm, Roberto Sassu, Linux Kernel Mailing List,
	Mimi Zohar, David Howells, keyrings, Jarkko Sakkinen

On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > Rather than fail initialization of the trusted.ko module, arrange for
> > the module to load, but rely on trusted_instantiate() to fail
> > trusted-key operations.
>
> What actual problem is this fixing?  To me it would seem like an
> enhancement to make the trusted module fail at load time if there's no
> TPM rather than waiting until first use to find out it can never work.
> Is there some piece of user code that depends on the successful
> insertion of trusted.ko?

The module dependency chain relies on it. If that can be broken that
would also be an acceptable fix.

I found this through the following dependency chain: libnvdimm.ko ->
encrypted_keys.ko -> trusted.ko.

"key_type_trusted" is the symbol that encrypted_keys needs regardless
of whether the tpm is present.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19  0:30       ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-19  0:30 UTC (permalink / raw)
  To: James Bottomley
  Cc: Jarkko Sakkinen, linux-nvdimm, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > Rather than fail initialization of the trusted.ko module, arrange for
> > the module to load, but rely on trusted_instantiate() to fail
> > trusted-key operations.
>
> What actual problem is this fixing?  To me it would seem like an
> enhancement to make the trusted module fail at load time if there's no
> TPM rather than waiting until first use to find out it can never work.
> Is there some piece of user code that depends on the successful
> insertion of trusted.ko?

The module dependency chain relies on it. If that can be broken that
would also be an acceptable fix.

I found this through the following dependency chain: libnvdimm.ko ->
encrypted_keys.ko -> trusted.ko.

"key_type_trusted" is the symbol that encrypted_keys needs regardless
of whether the tpm is present.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
       [not found]       ` <CAA9_cmcOD2zPaaNbkYAaH5DRDRAebPkW+hwPA0zPKY4kU8R-rg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2019-03-19  0:56           ` James Bottomley
@ 2019-03-19  0:56           ` James Bottomley
  1 sibling, 0 replies; 50+ messages in thread
From: James Bottomley @ 2019-03-19  0:56 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA, Jarkko Sakkinen

On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb-tEXmvtCZX7AybS5Ee8rs3A@public.gmane.org>
> wrote:
> > 
> > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > Rather than fail initialization of the trusted.ko module, arrange
> > > for the module to load, but rely on trusted_instantiate() to fail
> > > trusted-key operations.
> > 
> > What actual problem is this fixing?  To me it would seem like an
> > enhancement to make the trusted module fail at load time if there's
> > no TPM rather than waiting until first use to find out it can never
> > work. Is there some piece of user code that depends on the
> > successful insertion of trusted.ko?
> 
> The module dependency chain relies on it. If that can be broken that
> would also be an acceptable fix.
> 
> I found this through the following dependency chain: libnvdimm.ko ->
> encrypted_keys.ko -> trusted.ko.
> 
> "key_type_trusted" is the symbol that encrypted_keys needs regardless
> of whether the tpm is present.

That's a nasty dependency caused by every key type module exporting a
symbol for its key type.  It really seems that key types should be
looked up by name not symbol to prevent more of these dependency issues
from spreading.  Something like this (untested and definitely won't
work without doing an EXPORT_SYMBOL on key_type_lookup).

If it does look acceptable we can also disentangle the nasty module
dependencies in the encrypted key code around masterkey which alone
should be a huge improvement because that code is too hacky to live.

James

---
diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
index dc3d18cae642..b98416f091e2 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
@@ -32,8 +33,14 @@ struct key *request_trusted_key(const char *trusted_desc,
 {
 	struct trusted_key_payload *tpayload;
 	struct key *tkey;
+	struct key_type *type;
 
-	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;

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19  0:56           ` James Bottomley
  0 siblings, 0 replies; 50+ messages in thread
From: James Bottomley @ 2019-03-19  0:56 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA, Jarkko Sakkinen

On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com>
> wrote:
> > 
> > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > Rather than fail initialization of the trusted.ko module, arrange
> > > for the module to load, but rely on trusted_instantiate() to fail
> > > trusted-key operations.
> > 
> > What actual problem is this fixing?  To me it would seem like an
> > enhancement to make the trusted module fail at load time if there's
> > no TPM rather than waiting until first use to find out it can never
> > work. Is there some piece of user code that depends on the
> > successful insertion of trusted.ko?
> 
> The module dependency chain relies on it. If that can be broken that
> would also be an acceptable fix.
> 
> I found this through the following dependency chain: libnvdimm.ko ->
> encrypted_keys.ko -> trusted.ko.
> 
> "key_type_trusted" is the symbol that encrypted_keys needs regardless
> of whether the tpm is present.

That's a nasty dependency caused by every key type module exporting a
symbol for its key type.  It really seems that key types should be
looked up by name not symbol to prevent more of these dependency issues
from spreading.  Something like this (untested and definitely won't
work without doing an EXPORT_SYMBOL on key_type_lookup).

If it does look acceptable we can also disentangle the nasty module
dependencies in the encrypted key code around masterkey which alone
should be a huge improvement because that code is too hacky to live.

James

---
diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
index dc3d18cae642..b98416f091e2 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
@@ -32,8 +33,14 @@ struct key *request_trusted_key(const char *trusted_desc,
 {
 	struct trusted_key_payload *tpayload;
 	struct key *tkey;
+	struct key_type *type;
 
-	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;
 

^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19  0:56           ` James Bottomley
  0 siblings, 0 replies; 50+ messages in thread
From: James Bottomley @ 2019-03-19  0:56 UTC (permalink / raw)
  To: Dan Williams
  Cc: Jarkko Sakkinen, linux-nvdimm, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com>
> wrote:
> > 
> > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > Rather than fail initialization of the trusted.ko module, arrange
> > > for the module to load, but rely on trusted_instantiate() to fail
> > > trusted-key operations.
> > 
> > What actual problem is this fixing?  To me it would seem like an
> > enhancement to make the trusted module fail at load time if there's
> > no TPM rather than waiting until first use to find out it can never
> > work. Is there some piece of user code that depends on the
> > successful insertion of trusted.ko?
> 
> The module dependency chain relies on it. If that can be broken that
> would also be an acceptable fix.
> 
> I found this through the following dependency chain: libnvdimm.ko ->
> encrypted_keys.ko -> trusted.ko.
> 
> "key_type_trusted" is the symbol that encrypted_keys needs regardless
> of whether the tpm is present.

That's a nasty dependency caused by every key type module exporting a
symbol for its key type.  It really seems that key types should be
looked up by name not symbol to prevent more of these dependency issues
from spreading.  Something like this (untested and definitely won't
work without doing an EXPORT_SYMBOL on key_type_lookup).

If it does look acceptable we can also disentangle the nasty module
dependencies in the encrypted key code around masterkey which alone
should be a huge improvement because that code is too hacky to live.

James

---
diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
index dc3d18cae642..b98416f091e2 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
@@ -32,8 +33,14 @@ struct key *request_trusted_key(const char *trusted_desc,
 {
 	struct trusted_key_payload *tpayload;
 	struct key *tkey;
+	struct key_type *type;
 
-	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;
 


^ permalink raw reply related	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-19  0:56           ` James Bottomley
  (?)
@ 2019-03-19  1:34             ` Dan Williams
  -1 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-19  1:34 UTC (permalink / raw)
  To: James Bottomley
  Cc: linux-nvdimm, Roberto Sassu, Linux Kernel Mailing List,
	Mimi Zohar, David Howells, keyrings, Jarkko Sakkinen

On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com>
> > wrote:
> > >
> > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > Rather than fail initialization of the trusted.ko module, arrange
> > > > for the module to load, but rely on trusted_instantiate() to fail
> > > > trusted-key operations.
> > >
> > > What actual problem is this fixing?  To me it would seem like an
> > > enhancement to make the trusted module fail at load time if there's
> > > no TPM rather than waiting until first use to find out it can never
> > > work. Is there some piece of user code that depends on the
> > > successful insertion of trusted.ko?
> >
> > The module dependency chain relies on it. If that can be broken that
> > would also be an acceptable fix.
> >
> > I found this through the following dependency chain: libnvdimm.ko ->
> > encrypted_keys.ko -> trusted.ko.
> >
> > "key_type_trusted" is the symbol that encrypted_keys needs regardless
> > of whether the tpm is present.
>
> That's a nasty dependency caused by every key type module exporting a
> symbol for its key type.  It really seems that key types should be
> looked up by name not symbol to prevent more of these dependency issues
> from spreading.  Something like this (untested and definitely won't
> work without doing an EXPORT_SYMBOL on key_type_lookup).
>
> If it does look acceptable we can also disentangle the nasty module
> dependencies in the encrypted key code around masterkey which alone
> should be a huge improvement because that code is too hacky to live.

Looks good to me. I fired it up with the export added and also included a:

    MODULE_SOFTDEP("pre: trusted");

...to encourage trusted.ko to be ready, if possible, for the lookup.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19  1:34             ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-19  1:34 UTC (permalink / raw)
  To: James Bottomley
  Cc: linux-nvdimm, Roberto Sassu, Linux Kernel Mailing List,
	Mimi Zohar, David Howells, keyrings, Jarkko Sakkinen

On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com>
> > wrote:
> > >
> > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > Rather than fail initialization of the trusted.ko module, arrange
> > > > for the module to load, but rely on trusted_instantiate() to fail
> > > > trusted-key operations.
> > >
> > > What actual problem is this fixing?  To me it would seem like an
> > > enhancement to make the trusted module fail at load time if there's
> > > no TPM rather than waiting until first use to find out it can never
> > > work. Is there some piece of user code that depends on the
> > > successful insertion of trusted.ko?
> >
> > The module dependency chain relies on it. If that can be broken that
> > would also be an acceptable fix.
> >
> > I found this through the following dependency chain: libnvdimm.ko ->
> > encrypted_keys.ko -> trusted.ko.
> >
> > "key_type_trusted" is the symbol that encrypted_keys needs regardless
> > of whether the tpm is present.
>
> That's a nasty dependency caused by every key type module exporting a
> symbol for its key type.  It really seems that key types should be
> looked up by name not symbol to prevent more of these dependency issues
> from spreading.  Something like this (untested and definitely won't
> work without doing an EXPORT_SYMBOL on key_type_lookup).
>
> If it does look acceptable we can also disentangle the nasty module
> dependencies in the encrypted key code around masterkey which alone
> should be a huge improvement because that code is too hacky to live.

Looks good to me. I fired it up with the export added and also included a:

    MODULE_SOFTDEP("pre: trusted");

...to encourage trusted.ko to be ready, if possible, for the lookup.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19  1:34             ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-19  1:34 UTC (permalink / raw)
  To: James Bottomley
  Cc: Jarkko Sakkinen, linux-nvdimm, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com>
> > wrote:
> > >
> > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > Rather than fail initialization of the trusted.ko module, arrange
> > > > for the module to load, but rely on trusted_instantiate() to fail
> > > > trusted-key operations.
> > >
> > > What actual problem is this fixing?  To me it would seem like an
> > > enhancement to make the trusted module fail at load time if there's
> > > no TPM rather than waiting until first use to find out it can never
> > > work. Is there some piece of user code that depends on the
> > > successful insertion of trusted.ko?
> >
> > The module dependency chain relies on it. If that can be broken that
> > would also be an acceptable fix.
> >
> > I found this through the following dependency chain: libnvdimm.ko ->
> > encrypted_keys.ko -> trusted.ko.
> >
> > "key_type_trusted" is the symbol that encrypted_keys needs regardless
> > of whether the tpm is present.
>
> That's a nasty dependency caused by every key type module exporting a
> symbol for its key type.  It really seems that key types should be
> looked up by name not symbol to prevent more of these dependency issues
> from spreading.  Something like this (untested and definitely won't
> work without doing an EXPORT_SYMBOL on key_type_lookup).
>
> If it does look acceptable we can also disentangle the nasty module
> dependencies in the encrypted key code around masterkey which alone
> should be a huge improvement because that code is too hacky to live.

Looks good to me. I fired it up with the export added and also included a:

    MODULE_SOFTDEP("pre: trusted");

...to encourage trusted.ko to be ready, if possible, for the lookup.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
       [not found]       ` <CAA9_cmcOD2zPaaNbkYAaH5DRDRAebPkW+hwPA0zPKY4kU8R-rg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2019-03-19  0:56           ` James Bottomley
@ 2019-03-19 22:56           ` Mimi Zohar
  1 sibling, 0 replies; 50+ messages in thread
From: Mimi Zohar @ 2019-03-19 22:56 UTC (permalink / raw)
  To: Dan Williams, James Bottomley
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Roberto Sassu,
	Linux Kernel Mailing List, Jarkko Sakkinen, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA

Hi Dan,

On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:

Sorry for the late reply.

> On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com> wrote:
> >
> > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > Rather than fail initialization of the trusted.ko module, arrange for
> > > the module to load, but rely on trusted_instantiate() to fail
> > > trusted-key operations.
> >
> > What actual problem is this fixing?  To me it would seem like an
> > enhancement to make the trusted module fail at load time if there's no
> > TPM rather than waiting until first use to find out it can never work.
> > Is there some piece of user code that depends on the successful
> > insertion of trusted.ko?
> 
> The module dependency chain relies on it. If that can be broken that
> would also be an acceptable fix.
> 
> I found this through the following dependency chain: libnvdimm.ko ->
> encrypted_keys.ko -> trusted.ko.
> 
> "key_type_trusted" is the symbol that encrypted_keys needs regardless
> of whether the tpm is present.

Commit 982e617a313b ("encrypted-keys: remove trusted-keys dependency")
removed the dependency on trusted keys.  masterkey_trusted.c should
only be included if "CONFIG_TRUSTED_KEYS" is enabled.  Is
CONFIG_TRUSTED_KEYS enabled?

Mimi

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19 22:56           ` Mimi Zohar
  0 siblings, 0 replies; 50+ messages in thread
From: Mimi Zohar @ 2019-03-19 22:56 UTC (permalink / raw)
  To: Dan Williams, James Bottomley
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Roberto Sassu,
	Linux Kernel Mailing List, Jarkko Sakkinen, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA

Hi Dan,

On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:

Sorry for the late reply.

> On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com> wrote:
> >
> > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > Rather than fail initialization of the trusted.ko module, arrange for
> > > the module to load, but rely on trusted_instantiate() to fail
> > > trusted-key operations.
> >
> > What actual problem is this fixing?  To me it would seem like an
> > enhancement to make the trusted module fail at load time if there's no
> > TPM rather than waiting until first use to find out it can never work.
> > Is there some piece of user code that depends on the successful
> > insertion of trusted.ko?
> 
> The module dependency chain relies on it. If that can be broken that
> would also be an acceptable fix.
> 
> I found this through the following dependency chain: libnvdimm.ko ->
> encrypted_keys.ko -> trusted.ko.
> 
> "key_type_trusted" is the symbol that encrypted_keys needs regardless
> of whether the tpm is present.

Commit 982e617a313b ("encrypted-keys: remove trusted-keys dependency")
removed the dependency on trusted keys.  masterkey_trusted.c should
only be included if "CONFIG_TRUSTED_KEYS" is enabled.  Is
CONFIG_TRUSTED_KEYS enabled?

Mimi

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19 22:56           ` Mimi Zohar
  0 siblings, 0 replies; 50+ messages in thread
From: Mimi Zohar @ 2019-03-19 22:56 UTC (permalink / raw)
  To: Dan Williams, James Bottomley
  Cc: Jarkko Sakkinen, linux-nvdimm, Roberto Sassu,
	Linux Kernel Mailing List, David Howells, keyrings

Hi Dan,

On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:

Sorry for the late reply.

> On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com> wrote:
> >
> > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > Rather than fail initialization of the trusted.ko module, arrange for
> > > the module to load, but rely on trusted_instantiate() to fail
> > > trusted-key operations.
> >
> > What actual problem is this fixing?  To me it would seem like an
> > enhancement to make the trusted module fail at load time if there's no
> > TPM rather than waiting until first use to find out it can never work.
> > Is there some piece of user code that depends on the successful
> > insertion of trusted.ko?
> 
> The module dependency chain relies on it. If that can be broken that
> would also be an acceptable fix.
> 
> I found this through the following dependency chain: libnvdimm.ko ->
> encrypted_keys.ko -> trusted.ko.
> 
> "key_type_trusted" is the symbol that encrypted_keys needs regardless
> of whether the tpm is present.

Commit 982e617a313b ("encrypted-keys: remove trusted-keys dependency")
removed the dependency on trusted keys.  masterkey_trusted.c should
only be included if "CONFIG_TRUSTED_KEYS" is enabled.  Is
CONFIG_TRUSTED_KEYS enabled?

Mimi


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-19 22:56           ` Mimi Zohar
  (?)
@ 2019-03-19 23:01             ` Dan Williams
  -1 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-19 23:01 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-nvdimm, James Bottomley, Roberto Sassu,
	Linux Kernel Mailing List, Jarkko Sakkinen, David Howells,
	keyrings

On Tue, Mar 19, 2019 at 3:56 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Dan,
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
>
> Sorry for the late reply.
>
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com> wrote:
> > >
> > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > the module to load, but rely on trusted_instantiate() to fail
> > > > trusted-key operations.
> > >
> > > What actual problem is this fixing?  To me it would seem like an
> > > enhancement to make the trusted module fail at load time if there's no
> > > TPM rather than waiting until first use to find out it can never work.
> > > Is there some piece of user code that depends on the successful
> > > insertion of trusted.ko?
> >
> > The module dependency chain relies on it. If that can be broken that
> > would also be an acceptable fix.
> >
> > I found this through the following dependency chain: libnvdimm.ko ->
> > encrypted_keys.ko -> trusted.ko.
> >
> > "key_type_trusted" is the symbol that encrypted_keys needs regardless
> > of whether the tpm is present.
>
> Commit 982e617a313b ("encrypted-keys: remove trusted-keys dependency")
> removed the dependency on trusted keys.  masterkey_trusted.c should
> only be included if "CONFIG_TRUSTED_KEYS" is enabled.  Is
> CONFIG_TRUSTED_KEYS enabled?

Yes, TRUSTED_KEYS is enabled, the module is built/available, and tries
to load when encrypted_keys.ko loads. The problem is that it fails to
load due an error returned from init_trusted(). The error is new for
v5.1. So, instead of requiring the module dependencies to resolve
successfully, and init_trusted() to return 0, the proposal is to just
lookup the key types by name.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19 23:01             ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-19 23:01 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-nvdimm, James Bottomley, Roberto Sassu,
	Linux Kernel Mailing List, Jarkko Sakkinen, David Howells,
	keyrings

On Tue, Mar 19, 2019 at 3:56 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Dan,
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
>
> Sorry for the late reply.
>
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com> wrote:
> > >
> > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > the module to load, but rely on trusted_instantiate() to fail
> > > > trusted-key operations.
> > >
> > > What actual problem is this fixing?  To me it would seem like an
> > > enhancement to make the trusted module fail at load time if there's no
> > > TPM rather than waiting until first use to find out it can never work.
> > > Is there some piece of user code that depends on the successful
> > > insertion of trusted.ko?
> >
> > The module dependency chain relies on it. If that can be broken that
> > would also be an acceptable fix.
> >
> > I found this through the following dependency chain: libnvdimm.ko ->
> > encrypted_keys.ko -> trusted.ko.
> >
> > "key_type_trusted" is the symbol that encrypted_keys needs regardless
> > of whether the tpm is present.
>
> Commit 982e617a313b ("encrypted-keys: remove trusted-keys dependency")
> removed the dependency on trusted keys.  masterkey_trusted.c should
> only be included if "CONFIG_TRUSTED_KEYS" is enabled.  Is
> CONFIG_TRUSTED_KEYS enabled?

Yes, TRUSTED_KEYS is enabled, the module is built/available, and tries
to load when encrypted_keys.ko loads. The problem is that it fails to
load due an error returned from init_trusted(). The error is new for
v5.1. So, instead of requiring the module dependencies to resolve
successfully, and init_trusted() to return 0, the proposal is to just
lookup the key types by name.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-19 23:01             ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-19 23:01 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: James Bottomley, Jarkko Sakkinen, linux-nvdimm, Roberto Sassu,
	Linux Kernel Mailing List, David Howells, keyrings

On Tue, Mar 19, 2019 at 3:56 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Dan,
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
>
> Sorry for the late reply.
>
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com> wrote:
> > >
> > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > the module to load, but rely on trusted_instantiate() to fail
> > > > trusted-key operations.
> > >
> > > What actual problem is this fixing?  To me it would seem like an
> > > enhancement to make the trusted module fail at load time if there's no
> > > TPM rather than waiting until first use to find out it can never work.
> > > Is there some piece of user code that depends on the successful
> > > insertion of trusted.ko?
> >
> > The module dependency chain relies on it. If that can be broken that
> > would also be an acceptable fix.
> >
> > I found this through the following dependency chain: libnvdimm.ko ->
> > encrypted_keys.ko -> trusted.ko.
> >
> > "key_type_trusted" is the symbol that encrypted_keys needs regardless
> > of whether the tpm is present.
>
> Commit 982e617a313b ("encrypted-keys: remove trusted-keys dependency")
> removed the dependency on trusted keys.  masterkey_trusted.c should
> only be included if "CONFIG_TRUSTED_KEYS" is enabled.  Is
> CONFIG_TRUSTED_KEYS enabled?

Yes, TRUSTED_KEYS is enabled, the module is built/available, and tries
to load when encrypted_keys.ko loads. The problem is that it fails to
load due an error returned from init_trusted(). The error is new for
v5.1. So, instead of requiring the module dependencies to resolve
successfully, and init_trusted() to return 0, the proposal is to just
lookup the key types by name.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-19  0:56           ` James Bottomley
  (?)
@ 2019-03-20  1:55             ` Dan Williams
  -1 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-20  1:55 UTC (permalink / raw)
  To: James Bottomley
  Cc: linux-nvdimm, Roberto Sassu, Linux Kernel Mailing List,
	Mimi Zohar, David Howells, keyrings, Jarkko Sakkinen

On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com>
> > wrote:
> > >
> > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > Rather than fail initialization of the trusted.ko module, arrange
> > > > for the module to load, but rely on trusted_instantiate() to fail
> > > > trusted-key operations.
> > >
> > > What actual problem is this fixing?  To me it would seem like an
> > > enhancement to make the trusted module fail at load time if there's
> > > no TPM rather than waiting until first use to find out it can never
> > > work. Is there some piece of user code that depends on the
> > > successful insertion of trusted.ko?
> >
> > The module dependency chain relies on it. If that can be broken that
> > would also be an acceptable fix.
> >
> > I found this through the following dependency chain: libnvdimm.ko ->
> > encrypted_keys.ko -> trusted.ko.
> >
> > "key_type_trusted" is the symbol that encrypted_keys needs regardless
> > of whether the tpm is present.
>
> That's a nasty dependency caused by every key type module exporting a
> symbol for its key type.  It really seems that key types should be
> looked up by name not symbol to prevent more of these dependency issues
> from spreading.  Something like this (untested and definitely won't
> work without doing an EXPORT_SYMBOL on key_type_lookup).
>
> If it does look acceptable we can also disentangle the nasty module
> dependencies in the encrypted key code around masterkey which alone
> should be a huge improvement because that code is too hacky to live.
>
> James
>
> ---
> diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
> index dc3d18cae642..b98416f091e2 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
> @@ -32,8 +33,14 @@ struct key *request_trusted_key(const char *trusted_desc,
>  {
>         struct trusted_key_payload *tpayload;
>         struct key *tkey;
> +       struct key_type *type;
>
> -       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;


This falls over with the need to pin the module while any key that
needs service from the hosting key_type operations might be live in
the system.

I could hang a "struct module *" off of the key_type so the host
module can be pinned, but that requires teaching all consumers of the
key_type module lifetime. Not impossible, but I think too big for a
fix, and I'd rather go with this local fixup to drop the dependency on
tpm_default_chip() successfully enumerating a TPM.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-20  1:55             ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-20  1:55 UTC (permalink / raw)
  To: James Bottomley
  Cc: linux-nvdimm, Roberto Sassu, Linux Kernel Mailing List,
	Mimi Zohar, David Howells, keyrings, Jarkko Sakkinen

On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com>
> > wrote:
> > >
> > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > Rather than fail initialization of the trusted.ko module, arrange
> > > > for the module to load, but rely on trusted_instantiate() to fail
> > > > trusted-key operations.
> > >
> > > What actual problem is this fixing?  To me it would seem like an
> > > enhancement to make the trusted module fail at load time if there's
> > > no TPM rather than waiting until first use to find out it can never
> > > work. Is there some piece of user code that depends on the
> > > successful insertion of trusted.ko?
> >
> > The module dependency chain relies on it. If that can be broken that
> > would also be an acceptable fix.
> >
> > I found this through the following dependency chain: libnvdimm.ko ->
> > encrypted_keys.ko -> trusted.ko.
> >
> > "key_type_trusted" is the symbol that encrypted_keys needs regardless
> > of whether the tpm is present.
>
> That's a nasty dependency caused by every key type module exporting a
> symbol for its key type.  It really seems that key types should be
> looked up by name not symbol to prevent more of these dependency issues
> from spreading.  Something like this (untested and definitely won't
> work without doing an EXPORT_SYMBOL on key_type_lookup).
>
> If it does look acceptable we can also disentangle the nasty module
> dependencies in the encrypted key code around masterkey which alone
> should be a huge improvement because that code is too hacky to live.
>
> James
>
> ---
> diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
> index dc3d18cae642..b98416f091e2 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
> @@ -32,8 +33,14 @@ struct key *request_trusted_key(const char *trusted_desc,
>  {
>         struct trusted_key_payload *tpayload;
>         struct key *tkey;
> +       struct key_type *type;
>
> -       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;


This falls over with the need to pin the module while any key that
needs service from the hosting key_type operations might be live in
the system.

I could hang a "struct module *" off of the key_type so the host
module can be pinned, but that requires teaching all consumers of the
key_type module lifetime. Not impossible, but I think too big for a
fix, and I'd rather go with this local fixup to drop the dependency on
tpm_default_chip() successfully enumerating a TPM.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-20  1:55             ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-20  1:55 UTC (permalink / raw)
  To: James Bottomley
  Cc: Jarkko Sakkinen, linux-nvdimm, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <jejb@linux.ibm.com>
> > wrote:
> > >
> > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > Rather than fail initialization of the trusted.ko module, arrange
> > > > for the module to load, but rely on trusted_instantiate() to fail
> > > > trusted-key operations.
> > >
> > > What actual problem is this fixing?  To me it would seem like an
> > > enhancement to make the trusted module fail at load time if there's
> > > no TPM rather than waiting until first use to find out it can never
> > > work. Is there some piece of user code that depends on the
> > > successful insertion of trusted.ko?
> >
> > The module dependency chain relies on it. If that can be broken that
> > would also be an acceptable fix.
> >
> > I found this through the following dependency chain: libnvdimm.ko ->
> > encrypted_keys.ko -> trusted.ko.
> >
> > "key_type_trusted" is the symbol that encrypted_keys needs regardless
> > of whether the tpm is present.
>
> That's a nasty dependency caused by every key type module exporting a
> symbol for its key type.  It really seems that key types should be
> looked up by name not symbol to prevent more of these dependency issues
> from spreading.  Something like this (untested and definitely won't
> work without doing an EXPORT_SYMBOL on key_type_lookup).
>
> If it does look acceptable we can also disentangle the nasty module
> dependencies in the encrypted key code around masterkey which alone
> should be a huge improvement because that code is too hacky to live.
>
> James
>
> ---
> diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c
> index dc3d18cae642..b98416f091e2 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
> @@ -32,8 +33,14 @@ struct key *request_trusted_key(const char *trusted_desc,
>  {
>         struct trusted_key_payload *tpayload;
>         struct key *tkey;
> +       struct key_type *type;
>
> -       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;


This falls over with the need to pin the module while any key that
needs service from the hosting key_type operations might be live in
the system.

I could hang a "struct module *" off of the key_type so the host
module can be pinned, but that requires teaching all consumers of the
key_type module lifetime. Not impossible, but I think too big for a
fix, and I'd rather go with this local fixup to drop the dependency on
tpm_default_chip() successfully enumerating a TPM.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
       [not found]             ` <CAPcyv4hXKQcdLnKG6rPNOJr2wjq31uCCr+16S5Yu6S5A_5UGrA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2019-03-20  2:19                 ` James Bottomley
@ 2019-03-20  2:19                 ` James Bottomley
  0 siblings, 0 replies; 50+ messages in thread
From: James Bottomley @ 2019-03-20  2:19 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA, Jarkko Sakkinen

On Tue, 2019-03-19 at 18:55 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb-tEXmvtCZX7AybS5Ee8rs3A@public.gmane.org>
> wrote:
> > 
> > On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley
> > > <jejb-tEXmvtCZX7AybS5Ee8rs3A@public.gmane.org> wrote:
> > > > 
> > > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > > Rather than fail initialization of the trusted.ko module,
> > > > > arrange for the module to load, but rely on
> > > > > trusted_instantiate() to fail trusted-key operations.
> > > > 
> > > > What actual problem is this fixing?  To me it would seem like
> > > > an enhancement to make the trusted module fail at load time if
> > > > there's no TPM rather than waiting until first use to find out
> > > > it can never work. Is there some piece of user code that
> > > > depends on the successful insertion of trusted.ko?
> > > 
> > > The module dependency chain relies on it. If that can be broken
> > > that would also be an acceptable fix.
> > > 
> > > I found this through the following dependency chain: libnvdimm.ko
> > > -> encrypted_keys.ko -> trusted.ko.
> > > 
> > > "key_type_trusted" is the symbol that encrypted_keys needs
> > > regardless of whether the tpm is present.
> > 
> > That's a nasty dependency caused by every key type module exporting
> > a symbol for its key type.  It really seems that key types should
> > be looked up by name not symbol to prevent more of these dependency
> > issues from spreading.  Something like this (untested and
> > definitely won't work without doing an EXPORT_SYMBOL on
> > key_type_lookup).
> > 
> > If it does look acceptable we can also disentangle the nasty module
> > dependencies in the encrypted key code around masterkey which alone
> > should be a huge improvement because that code is too hacky to
> > live.
> > 
> > James
> > 
> > ---
> > diff --git a/security/keys/encrypted-keys/masterkey_trusted.c
> > b/security/keys/encrypted-keys/masterkey_trusted.c
> > index dc3d18cae642..b98416f091e2 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
> > @@ -32,8 +33,14 @@ struct key *request_trusted_key(const char
> > *trusted_desc,
> >  {
> >         struct trusted_key_payload *tpayload;
> >         struct key *tkey;
> > +       struct key_type *type;
> > 
> > -       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;
> 
> 
> This falls over with the need to pin the module while any key that
> needs service from the hosting key_type operations might be live in
> the system.
> 
> I could hang a "struct module *" off of the key_type so the host
> module can be pinned, but that requires teaching all consumers of the
> key_type module lifetime. Not impossible, but I think too big for a
> fix, and I'd rather go with this local fixup to drop the dependency
> on tpm_default_chip() successfully enumerating a TPM.

Heh, well this proved to be a can of worms and no mistake. 
Unfortunately all of this does need fixing otherwise the keyctl syscall
has exactly the same problem.  But I think I agree it's getting way out
of scope for the bug you found.

James

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-20  2:19                 ` James Bottomley
  0 siblings, 0 replies; 50+ messages in thread
From: James Bottomley @ 2019-03-20  2:19 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA, Jarkko Sakkinen

On Tue, 2019-03-19 at 18:55 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb@linux.ibm.com>
> wrote:
> > 
> > On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley
> > > <jejb@linux.ibm.com> wrote:
> > > > 
> > > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > > Rather than fail initialization of the trusted.ko module,
> > > > > arrange for the module to load, but rely on
> > > > > trusted_instantiate() to fail trusted-key operations.
> > > > 
> > > > What actual problem is this fixing?  To me it would seem like
> > > > an enhancement to make the trusted module fail at load time if
> > > > there's no TPM rather than waiting until first use to find out
> > > > it can never work. Is there some piece of user code that
> > > > depends on the successful insertion of trusted.ko?
> > > 
> > > The module dependency chain relies on it. If that can be broken
> > > that would also be an acceptable fix.
> > > 
> > > I found this through the following dependency chain: libnvdimm.ko
> > > -> encrypted_keys.ko -> trusted.ko.
> > > 
> > > "key_type_trusted" is the symbol that encrypted_keys needs
> > > regardless of whether the tpm is present.
> > 
> > That's a nasty dependency caused by every key type module exporting
> > a symbol for its key type.  It really seems that key types should
> > be looked up by name not symbol to prevent more of these dependency
> > issues from spreading.  Something like this (untested and
> > definitely won't work without doing an EXPORT_SYMBOL on
> > key_type_lookup).
> > 
> > If it does look acceptable we can also disentangle the nasty module
> > dependencies in the encrypted key code around masterkey which alone
> > should be a huge improvement because that code is too hacky to
> > live.
> > 
> > James
> > 
> > ---
> > diff --git a/security/keys/encrypted-keys/masterkey_trusted.c
> > b/security/keys/encrypted-keys/masterkey_trusted.c
> > index dc3d18cae642..b98416f091e2 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
> > @@ -32,8 +33,14 @@ struct key *request_trusted_key(const char
> > *trusted_desc,
> >  {
> >         struct trusted_key_payload *tpayload;
> >         struct key *tkey;
> > +       struct key_type *type;
> > 
> > -       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;
> 
> 
> This falls over with the need to pin the module while any key that
> needs service from the hosting key_type operations might be live in
> the system.
> 
> I could hang a "struct module *" off of the key_type so the host
> module can be pinned, but that requires teaching all consumers of the
> key_type module lifetime. Not impossible, but I think too big for a
> fix, and I'd rather go with this local fixup to drop the dependency
> on tpm_default_chip() successfully enumerating a TPM.

Heh, well this proved to be a can of worms and no mistake. 
Unfortunately all of this does need fixing otherwise the keyctl syscall
has exactly the same problem.  But I think I agree it's getting way out
of scope for the bug you found.

James

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-20  2:19                 ` James Bottomley
  0 siblings, 0 replies; 50+ messages in thread
From: James Bottomley @ 2019-03-20  2:19 UTC (permalink / raw)
  To: Dan Williams
  Cc: Jarkko Sakkinen, linux-nvdimm, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Tue, 2019-03-19 at 18:55 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb@linux.ibm.com>
> wrote:
> > 
> > On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley
> > > <jejb@linux.ibm.com> wrote:
> > > > 
> > > > On Mon, 2019-03-18 at 16:45 -0700, Dan Williams wrote:
> > > > > Rather than fail initialization of the trusted.ko module,
> > > > > arrange for the module to load, but rely on
> > > > > trusted_instantiate() to fail trusted-key operations.
> > > > 
> > > > What actual problem is this fixing?  To me it would seem like
> > > > an enhancement to make the trusted module fail at load time if
> > > > there's no TPM rather than waiting until first use to find out
> > > > it can never work. Is there some piece of user code that
> > > > depends on the successful insertion of trusted.ko?
> > > 
> > > The module dependency chain relies on it. If that can be broken
> > > that would also be an acceptable fix.
> > > 
> > > I found this through the following dependency chain: libnvdimm.ko
> > > -> encrypted_keys.ko -> trusted.ko.
> > > 
> > > "key_type_trusted" is the symbol that encrypted_keys needs
> > > regardless of whether the tpm is present.
> > 
> > That's a nasty dependency caused by every key type module exporting
> > a symbol for its key type.  It really seems that key types should
> > be looked up by name not symbol to prevent more of these dependency
> > issues from spreading.  Something like this (untested and
> > definitely won't work without doing an EXPORT_SYMBOL on
> > key_type_lookup).
> > 
> > If it does look acceptable we can also disentangle the nasty module
> > dependencies in the encrypted key code around masterkey which alone
> > should be a huge improvement because that code is too hacky to
> > live.
> > 
> > James
> > 
> > ---
> > diff --git a/security/keys/encrypted-keys/masterkey_trusted.c
> > b/security/keys/encrypted-keys/masterkey_trusted.c
> > index dc3d18cae642..b98416f091e2 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
> > @@ -32,8 +33,14 @@ struct key *request_trusted_key(const char
> > *trusted_desc,
> >  {
> >         struct trusted_key_payload *tpayload;
> >         struct key *tkey;
> > +       struct key_type *type;
> > 
> > -       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;
> 
> 
> This falls over with the need to pin the module while any key that
> needs service from the hosting key_type operations might be live in
> the system.
> 
> I could hang a "struct module *" off of the key_type so the host
> module can be pinned, but that requires teaching all consumers of the
> key_type module lifetime. Not impossible, but I think too big for a
> fix, and I'd rather go with this local fixup to drop the dependency
> on tpm_default_chip() successfully enumerating a TPM.

Heh, well this proved to be a can of worms and no mistake. 
Unfortunately all of this does need fixing otherwise the keyctl syscall
has exactly the same problem.  But I think I agree it's getting way out
of scope for the bug you found.

James


^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-18 23:45 ` Dan Williams
  (?)
@ 2019-03-21 13:54   ` Jarkko Sakkinen
  -1 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-21 13:54 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm, James Bottomley, Roberto Sassu, linux-kernel,
	Mimi Zohar, David Howells, keyrings

On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> Rather than fail initialization of the trusted.ko module, arrange for
> the module to load, but rely on trusted_instantiate() to fail
> trusted-key operations.
> 
> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Cc: James Bottomley <jejb@linux.ibm.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>

It should check for chip in each function that uses TPM now that
the code does not rely on default chip. Otherwise, the semantics
are kind of inconsistent.

/Jarkko
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-21 13:54   ` Jarkko Sakkinen
  0 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-21 13:54 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-nvdimm, James Bottomley, Roberto Sassu, linux-kernel,
	Mimi Zohar, David Howells, keyrings

On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> Rather than fail initialization of the trusted.ko module, arrange for
> the module to load, but rely on trusted_instantiate() to fail
> trusted-key operations.
> 
> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Cc: James Bottomley <jejb@linux.ibm.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>

It should check for chip in each function that uses TPM now that
the code does not rely on default chip. Otherwise, the semantics
are kind of inconsistent.

/Jarkko

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-21 13:54   ` Jarkko Sakkinen
  0 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-21 13:54 UTC (permalink / raw)
  To: Dan Williams
  Cc: Roberto Sassu, James Bottomley, Mimi Zohar, David Howells,
	keyrings, linux-nvdimm, linux-kernel

On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> Rather than fail initialization of the trusted.ko module, arrange for
> the module to load, but rely on trusted_instantiate() to fail
> trusted-key operations.
> 
> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Cc: James Bottomley <jejb@linux.ibm.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>

It should check for chip in each function that uses TPM now that
the code does not rely on default chip. Otherwise, the semantics
are kind of inconsistent.

/Jarkko

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-21 13:54   ` Jarkko Sakkinen
  (?)
@ 2019-03-21 14:26     ` Roberto Sassu
  -1 siblings, 0 replies; 50+ messages in thread
From: Roberto Sassu @ 2019-03-21 14:26 UTC (permalink / raw)
  To: Jarkko Sakkinen, Dan Williams
  Cc: James Bottomley, Mimi Zohar, David Howells, keyrings,
	linux-nvdimm, linux-kernel

On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
>> Rather than fail initialization of the trusted.ko module, arrange for
>> the module to load, but rely on trusted_instantiate() to fail
>> trusted-key operations.
>>
>> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
>> Cc: Roberto Sassu <roberto.sassu@huawei.com>
>> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> Cc: James Bottomley <jejb@linux.ibm.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>
> 
> It should check for chip in each function that uses TPM now that
> the code does not rely on default chip. Otherwise, the semantics
> are kind of inconsistent.

If no other TPM function can be used before a successful key
instantiate, checking for a chip only in trusted_instantiate() seems
sufficient. Then, the same chip will be used by all TPM functions until
module unloading, since we incremented the reference count.

I would suggest to move the tpm_default_chip() and init_digests() calls
to trusted_instantiate() to restore the old behavior of init_trusted().

trusted_instantiate() should look like:
---
if (!chip) {
	chip = tpm_default_chip();
	if (!chip)
		return -ENODEV;
}

if (!digests) {
	ret = init_digests();
	if (ret < 0)
		return ret;
}
---

Roberto


> /Jarkko
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-21 14:26     ` Roberto Sassu
  0 siblings, 0 replies; 50+ messages in thread
From: Roberto Sassu @ 2019-03-21 14:26 UTC (permalink / raw)
  To: Jarkko Sakkinen, Dan Williams
  Cc: James Bottomley, Mimi Zohar, David Howells, keyrings,
	linux-nvdimm, linux-kernel

On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
>> Rather than fail initialization of the trusted.ko module, arrange for
>> the module to load, but rely on trusted_instantiate() to fail
>> trusted-key operations.
>>
>> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
>> Cc: Roberto Sassu <roberto.sassu@huawei.com>
>> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> Cc: James Bottomley <jejb@linux.ibm.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>
> 
> It should check for chip in each function that uses TPM now that
> the code does not rely on default chip. Otherwise, the semantics
> are kind of inconsistent.

If no other TPM function can be used before a successful key
instantiate, checking for a chip only in trusted_instantiate() seems
sufficient. Then, the same chip will be used by all TPM functions until
module unloading, since we incremented the reference count.

I would suggest to move the tpm_default_chip() and init_digests() calls
to trusted_instantiate() to restore the old behavior of init_trusted().

trusted_instantiate() should look like:
---
if (!chip) {
	chip = tpm_default_chip();
	if (!chip)
		return -ENODEV;
}

if (!digests) {
	ret = init_digests();
	if (ret < 0)
		return ret;
}
---

Roberto


> /Jarkko
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-21 14:26     ` Roberto Sassu
  0 siblings, 0 replies; 50+ messages in thread
From: Roberto Sassu @ 2019-03-21 14:26 UTC (permalink / raw)
  To: Jarkko Sakkinen, Dan Williams
  Cc: James Bottomley, Mimi Zohar, David Howells, keyrings,
	linux-nvdimm, linux-kernel

On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
>> Rather than fail initialization of the trusted.ko module, arrange for
>> the module to load, but rely on trusted_instantiate() to fail
>> trusted-key operations.
>>
>> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
>> Cc: Roberto Sassu <roberto.sassu@huawei.com>
>> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> Cc: James Bottomley <jejb@linux.ibm.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>
> 
> It should check for chip in each function that uses TPM now that
> the code does not rely on default chip. Otherwise, the semantics
> are kind of inconsistent.

If no other TPM function can be used before a successful key
instantiate, checking for a chip only in trusted_instantiate() seems
sufficient. Then, the same chip will be used by all TPM functions until
module unloading, since we incremented the reference count.

I would suggest to move the tpm_default_chip() and init_digests() calls
to trusted_instantiate() to restore the old behavior of init_trusted().

trusted_instantiate() should look like:
---
if (!chip) {
	chip = tpm_default_chip();
	if (!chip)
		return -ENODEV;
}

if (!digests) {
	ret = init_digests();
	if (ret < 0)
		return ret;
}
---

Roberto


> /Jarkko
> 

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-21 14:26     ` Roberto Sassu
  (?)
@ 2019-03-21 16:30       ` Dan Williams
  -1 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-21 16:30 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: linux-nvdimm, James Bottomley, Linux Kernel Mailing List,
	Mimi Zohar, David Howells, keyrings, Jarkko Sakkinen

On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> >> Rather than fail initialization of the trusted.ko module, arrange for
> >> the module to load, but rely on trusted_instantiate() to fail
> >> trusted-key operations.
> >>
> >> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> >> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> >> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >> Cc: James Bottomley <jejb@linux.ibm.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>
> >
> > It should check for chip in each function that uses TPM now that
> > the code does not rely on default chip. Otherwise, the semantics
> > are kind of inconsistent.
>
> If no other TPM function can be used before a successful key
> instantiate, checking for a chip only in trusted_instantiate() seems
> sufficient. Then, the same chip will be used by all TPM functions until
> module unloading, since we incremented the reference count.
>
> I would suggest to move the tpm_default_chip() and init_digests() calls
> to trusted_instantiate() to restore the old behavior of init_trusted().
>
> trusted_instantiate() should look like:
> ---
> if (!chip) {
>         chip = tpm_default_chip();
>         if (!chip)
>                 return -ENODEV;
> }
>
> if (!digests) {
>         ret = init_digests();
>         if (ret < 0)
>                 return ret;
> }

This patch already achieves that because tpm_find_get_ops() will fail
and cause tpm_is_tpm2() to return NULL.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-21 16:30       ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-21 16:30 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: linux-nvdimm, James Bottomley, Linux Kernel Mailing List,
	Mimi Zohar, David Howells, keyrings, Jarkko Sakkinen

On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> >> Rather than fail initialization of the trusted.ko module, arrange for
> >> the module to load, but rely on trusted_instantiate() to fail
> >> trusted-key operations.
> >>
> >> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> >> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> >> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >> Cc: James Bottomley <jejb@linux.ibm.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>
> >
> > It should check for chip in each function that uses TPM now that
> > the code does not rely on default chip. Otherwise, the semantics
> > are kind of inconsistent.
>
> If no other TPM function can be used before a successful key
> instantiate, checking for a chip only in trusted_instantiate() seems
> sufficient. Then, the same chip will be used by all TPM functions until
> module unloading, since we incremented the reference count.
>
> I would suggest to move the tpm_default_chip() and init_digests() calls
> to trusted_instantiate() to restore the old behavior of init_trusted().
>
> trusted_instantiate() should look like:
> ---
> if (!chip) {
>         chip = tpm_default_chip();
>         if (!chip)
>                 return -ENODEV;
> }
>
> if (!digests) {
>         ret = init_digests();
>         if (ret < 0)
>                 return ret;
> }

This patch already achieves that because tpm_find_get_ops() will fail
and cause tpm_is_tpm2() to return NULL.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-21 16:30       ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-21 16:30 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: Jarkko Sakkinen, James Bottomley, Mimi Zohar, David Howells,
	keyrings, linux-nvdimm, Linux Kernel Mailing List

On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> >> Rather than fail initialization of the trusted.ko module, arrange for
> >> the module to load, but rely on trusted_instantiate() to fail
> >> trusted-key operations.
> >>
> >> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> >> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> >> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >> Cc: James Bottomley <jejb@linux.ibm.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>
> >
> > It should check for chip in each function that uses TPM now that
> > the code does not rely on default chip. Otherwise, the semantics
> > are kind of inconsistent.
>
> If no other TPM function can be used before a successful key
> instantiate, checking for a chip only in trusted_instantiate() seems
> sufficient. Then, the same chip will be used by all TPM functions until
> module unloading, since we incremented the reference count.
>
> I would suggest to move the tpm_default_chip() and init_digests() calls
> to trusted_instantiate() to restore the old behavior of init_trusted().
>
> trusted_instantiate() should look like:
> ---
> if (!chip) {
>         chip = tpm_default_chip();
>         if (!chip)
>                 return -ENODEV;
> }
>
> if (!digests) {
>         ret = init_digests();
>         if (ret < 0)
>                 return ret;
> }

This patch already achieves that because tpm_find_get_ops() will fail
and cause tpm_is_tpm2() to return NULL.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
       [not found]       ` <CAPcyv4gc_KWedC12bTcX24KwjKiBCF=yvouRNzCbJKaLrgJdDg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2019-03-21 17:45           ` Roberto Sassu
@ 2019-03-21 17:45           ` Roberto Sassu
  0 siblings, 0 replies; 50+ messages in thread
From: Roberto Sassu @ 2019-03-21 17:45 UTC (permalink / raw)
  To: Dan Williams, Jarkko Sakkinen
  Cc: Silviu Vlasceanu, linux-nvdimm, James Bottomley,
	Linux Kernel Mailing List, Mimi Zohar, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA

On 3/21/2019 5:30 PM, Dan Williams wrote:
> On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
>>
>> On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
>>> On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
>>>> Rather than fail initialization of the trusted.ko module, arrange for
>>>> the module to load, but rely on trusted_instantiate() to fail
>>>> trusted-key operations.
>>>>
>>>> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
>>>> Cc: Roberto Sassu <roberto.sassu-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>> Cc: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>> Cc: James Bottomley <jejb-tEXmvtCZX7AybS5Ee8rs3A@public.gmane.org>
>>>> Cc: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>>> Cc: Mimi Zohar <zohar-tEXmvtCZX7AybS5Ee8rs3A@public.gmane.org>
>>>> Cc: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>> Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>
>>> It should check for chip in each function that uses TPM now that
>>> the code does not rely on default chip. Otherwise, the semantics
>>> are kind of inconsistent.
>>
>> If no other TPM function can be used before a successful key
>> instantiate, checking for a chip only in trusted_instantiate() seems
>> sufficient. Then, the same chip will be used by all TPM functions until
>> module unloading, since we incremented the reference count.
>>
>> I would suggest to move the tpm_default_chip() and init_digests() calls
>> to trusted_instantiate() to restore the old behavior of init_trusted().
>>
>> trusted_instantiate() should look like:
>> ---
>> if (!chip) {
>>          chip = tpm_default_chip();
>>          if (!chip)
>>                  return -ENODEV;
>> }
>>
>> if (!digests) {
>>          ret = init_digests();
>>          if (ret < 0)
>>                  return ret;
>> }
> 
> This patch already achieves that because tpm_find_get_ops() will fail
> and cause tpm_is_tpm2() to return NULL.

In addition, the changes I proposed would allow users to create trusted
keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
CONFIG_TCG_TPM=m is a valid configuration.

Jarkko, Dan's patch seems sufficient to fix the issue. He could include
the changes I proposed in his patch. What is your opinion?

Thanks

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-21 17:45           ` Roberto Sassu
  0 siblings, 0 replies; 50+ messages in thread
From: Roberto Sassu @ 2019-03-21 17:45 UTC (permalink / raw)
  To: Dan Williams, Jarkko Sakkinen
  Cc: Silviu Vlasceanu, linux-nvdimm, James Bottomley,
	Linux Kernel Mailing List, Mimi Zohar, David Howells,
	keyrings-u79uwXL29TY76Z2rM5mHXA

On 3/21/2019 5:30 PM, Dan Williams wrote:
> On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>>
>> On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
>>> On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
>>>> Rather than fail initialization of the trusted.ko module, arrange for
>>>> the module to load, but rely on trusted_instantiate() to fail
>>>> trusted-key operations.
>>>>
>>>> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
>>>> Cc: Roberto Sassu <roberto.sassu@huawei.com>
>>>> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>> Cc: James Bottomley <jejb@linux.ibm.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>
>>>
>>> It should check for chip in each function that uses TPM now that
>>> the code does not rely on default chip. Otherwise, the semantics
>>> are kind of inconsistent.
>>
>> If no other TPM function can be used before a successful key
>> instantiate, checking for a chip only in trusted_instantiate() seems
>> sufficient. Then, the same chip will be used by all TPM functions until
>> module unloading, since we incremented the reference count.
>>
>> I would suggest to move the tpm_default_chip() and init_digests() calls
>> to trusted_instantiate() to restore the old behavior of init_trusted().
>>
>> trusted_instantiate() should look like:
>> ---
>> if (!chip) {
>>          chip = tpm_default_chip();
>>          if (!chip)
>>                  return -ENODEV;
>> }
>>
>> if (!digests) {
>>          ret = init_digests();
>>          if (ret < 0)
>>                  return ret;
>> }
> 
> This patch already achieves that because tpm_find_get_ops() will fail
> and cause tpm_is_tpm2() to return NULL.

In addition, the changes I proposed would allow users to create trusted
keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
CONFIG_TCG_TPM=m is a valid configuration.

Jarkko, Dan's patch seems sufficient to fix the issue. He could include
the changes I proposed in his patch. What is your opinion?

Thanks

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-21 17:45           ` Roberto Sassu
  0 siblings, 0 replies; 50+ messages in thread
From: Roberto Sassu @ 2019-03-21 17:45 UTC (permalink / raw)
  To: Dan Williams, Jarkko Sakkinen
  Cc: James Bottomley, Mimi Zohar, David Howells, keyrings,
	linux-nvdimm, Linux Kernel Mailing List, Silviu Vlasceanu

On 3/21/2019 5:30 PM, Dan Williams wrote:
> On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>>
>> On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
>>> On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
>>>> Rather than fail initialization of the trusted.ko module, arrange for
>>>> the module to load, but rely on trusted_instantiate() to fail
>>>> trusted-key operations.
>>>>
>>>> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
>>>> Cc: Roberto Sassu <roberto.sassu@huawei.com>
>>>> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>> Cc: James Bottomley <jejb@linux.ibm.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>
>>>
>>> It should check for chip in each function that uses TPM now that
>>> the code does not rely on default chip. Otherwise, the semantics
>>> are kind of inconsistent.
>>
>> If no other TPM function can be used before a successful key
>> instantiate, checking for a chip only in trusted_instantiate() seems
>> sufficient. Then, the same chip will be used by all TPM functions until
>> module unloading, since we incremented the reference count.
>>
>> I would suggest to move the tpm_default_chip() and init_digests() calls
>> to trusted_instantiate() to restore the old behavior of init_trusted().
>>
>> trusted_instantiate() should look like:
>> ---
>> if (!chip) {
>>          chip = tpm_default_chip();
>>          if (!chip)
>>                  return -ENODEV;
>> }
>>
>> if (!digests) {
>>          ret = init_digests();
>>          if (ret < 0)
>>                  return ret;
>> }
> 
> This patch already achieves that because tpm_find_get_ops() will fail
> and cause tpm_is_tpm2() to return NULL.

In addition, the changes I proposed would allow users to create trusted
keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
CONFIG_TCG_TPM=m is a valid configuration.

Jarkko, Dan's patch seems sufficient to fix the issue. He could include
the changes I proposed in his patch. What is your opinion?

Thanks

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-21 17:45           ` Roberto Sassu
@ 2019-03-22 10:12             ` Jarkko Sakkinen
  -1 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-22 10:12 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: Dan Williams, James Bottomley, Mimi Zohar, David Howells,
	keyrings, linux-nvdimm, Linux Kernel Mailing List,
	Silviu Vlasceanu

On Thu, Mar 21, 2019 at 06:45:47PM +0100, Roberto Sassu wrote:
> On 3/21/2019 5:30 PM, Dan Williams wrote:
> > On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> > > 
> > > On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > > > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> > > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > > the module to load, but rely on trusted_instantiate() to fail
> > > > > trusted-key operations.
> > > > > 
> > > > > Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > Cc: James Bottomley <jejb@linux.ibm.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>
> > > > 
> > > > It should check for chip in each function that uses TPM now that
> > > > the code does not rely on default chip. Otherwise, the semantics
> > > > are kind of inconsistent.
> > > 
> > > If no other TPM function can be used before a successful key
> > > instantiate, checking for a chip only in trusted_instantiate() seems
> > > sufficient. Then, the same chip will be used by all TPM functions until
> > > module unloading, since we incremented the reference count.
> > > 
> > > I would suggest to move the tpm_default_chip() and init_digests() calls
> > > to trusted_instantiate() to restore the old behavior of init_trusted().
> > > 
> > > trusted_instantiate() should look like:
> > > ---
> > > if (!chip) {
> > >          chip = tpm_default_chip();
> > >          if (!chip)
> > >                  return -ENODEV;
> > > }
> > > 
> > > if (!digests) {
> > >          ret = init_digests();
> > >          if (ret < 0)
> > >                  return ret;
> > > }
> > 
> > This patch already achieves that because tpm_find_get_ops() will fail
> > and cause tpm_is_tpm2() to return NULL.
> 
> In addition, the changes I proposed would allow users to create trusted
> keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
> CONFIG_TCG_TPM=m is a valid configuration.
> 
> Jarkko, Dan's patch seems sufficient to fix the issue. He could include
> the changes I proposed in his patch. What is your opinion?

Agreed.

/Jarkko

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-22 10:12             ` Jarkko Sakkinen
  0 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-22 10:12 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: Dan Williams, James Bottomley, Mimi Zohar, David Howells,
	keyrings, linux-nvdimm, Linux Kernel Mailing List,
	Silviu Vlasceanu

On Thu, Mar 21, 2019 at 06:45:47PM +0100, Roberto Sassu wrote:
> On 3/21/2019 5:30 PM, Dan Williams wrote:
> > On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> > > 
> > > On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > > > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> > > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > > the module to load, but rely on trusted_instantiate() to fail
> > > > > trusted-key operations.
> > > > > 
> > > > > Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > Cc: James Bottomley <jejb@linux.ibm.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>
> > > > 
> > > > It should check for chip in each function that uses TPM now that
> > > > the code does not rely on default chip. Otherwise, the semantics
> > > > are kind of inconsistent.
> > > 
> > > If no other TPM function can be used before a successful key
> > > instantiate, checking for a chip only in trusted_instantiate() seems
> > > sufficient. Then, the same chip will be used by all TPM functions until
> > > module unloading, since we incremented the reference count.
> > > 
> > > I would suggest to move the tpm_default_chip() and init_digests() calls
> > > to trusted_instantiate() to restore the old behavior of init_trusted().
> > > 
> > > trusted_instantiate() should look like:
> > > ---
> > > if (!chip) {
> > >          chip = tpm_default_chip();
> > >          if (!chip)
> > >                  return -ENODEV;
> > > }
> > > 
> > > if (!digests) {
> > >          ret = init_digests();
> > >          if (ret < 0)
> > >                  return ret;
> > > }
> > 
> > This patch already achieves that because tpm_find_get_ops() will fail
> > and cause tpm_is_tpm2() to return NULL.
> 
> In addition, the changes I proposed would allow users to create trusted
> keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
> CONFIG_TCG_TPM=m is a valid configuration.
> 
> Jarkko, Dan's patch seems sufficient to fix the issue. He could include
> the changes I proposed in his patch. What is your opinion?

Agreed.

/Jarkko

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-22 10:12             ` Jarkko Sakkinen
  (?)
@ 2019-03-22 15:24               ` Dan Williams
  -1 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-22 15:24 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Silviu Vlasceanu, linux-nvdimm, James Bottomley, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Fri, Mar 22, 2019 at 3:12 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Mar 21, 2019 at 06:45:47PM +0100, Roberto Sassu wrote:
> > On 3/21/2019 5:30 PM, Dan Williams wrote:
> > > On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> > > >
> > > > On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > > > > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> > > > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > > > the module to load, but rely on trusted_instantiate() to fail
> > > > > > trusted-key operations.
> > > > > >
> > > > > > Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> > > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > Cc: James Bottomley <jejb@linux.ibm.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>
> > > > >
> > > > > It should check for chip in each function that uses TPM now that
> > > > > the code does not rely on default chip. Otherwise, the semantics
> > > > > are kind of inconsistent.
> > > >
> > > > If no other TPM function can be used before a successful key
> > > > instantiate, checking for a chip only in trusted_instantiate() seems
> > > > sufficient. Then, the same chip will be used by all TPM functions until
> > > > module unloading, since we incremented the reference count.
> > > >
> > > > I would suggest to move the tpm_default_chip() and init_digests() calls
> > > > to trusted_instantiate() to restore the old behavior of init_trusted().
> > > >
> > > > trusted_instantiate() should look like:
> > > > ---
> > > > if (!chip) {
> > > >          chip = tpm_default_chip();
> > > >          if (!chip)
> > > >                  return -ENODEV;
> > > > }
> > > >
> > > > if (!digests) {
> > > >          ret = init_digests();
> > > >          if (ret < 0)
> > > >                  return ret;
> > > > }
> > >
> > > This patch already achieves that because tpm_find_get_ops() will fail
> > > and cause tpm_is_tpm2() to return NULL.
> >
> > In addition, the changes I proposed would allow users to create trusted
> > keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
> > CONFIG_TCG_TPM=m is a valid configuration.
> >
> > Jarkko, Dan's patch seems sufficient to fix the issue. He could include
> > the changes I proposed in his patch. What is your opinion?
>
> Agreed.

What changes?

Robert, please feel free to re-author the proposed patch however you
see fit, I just want whatever will get libnvdimm operational again in
the shortest amount of time.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-22 15:24               ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-22 15:24 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Silviu Vlasceanu, linux-nvdimm, James Bottomley, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Fri, Mar 22, 2019 at 3:12 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Mar 21, 2019 at 06:45:47PM +0100, Roberto Sassu wrote:
> > On 3/21/2019 5:30 PM, Dan Williams wrote:
> > > On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> > > >
> > > > On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > > > > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> > > > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > > > the module to load, but rely on trusted_instantiate() to fail
> > > > > > trusted-key operations.
> > > > > >
> > > > > > Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> > > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > Cc: James Bottomley <jejb@linux.ibm.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>
> > > > >
> > > > > It should check for chip in each function that uses TPM now that
> > > > > the code does not rely on default chip. Otherwise, the semantics
> > > > > are kind of inconsistent.
> > > >
> > > > If no other TPM function can be used before a successful key
> > > > instantiate, checking for a chip only in trusted_instantiate() seems
> > > > sufficient. Then, the same chip will be used by all TPM functions until
> > > > module unloading, since we incremented the reference count.
> > > >
> > > > I would suggest to move the tpm_default_chip() and init_digests() calls
> > > > to trusted_instantiate() to restore the old behavior of init_trusted().
> > > >
> > > > trusted_instantiate() should look like:
> > > > ---
> > > > if (!chip) {
> > > >          chip = tpm_default_chip();
> > > >          if (!chip)
> > > >                  return -ENODEV;
> > > > }
> > > >
> > > > if (!digests) {
> > > >          ret = init_digests();
> > > >          if (ret < 0)
> > > >                  return ret;
> > > > }
> > >
> > > This patch already achieves that because tpm_find_get_ops() will fail
> > > and cause tpm_is_tpm2() to return NULL.
> >
> > In addition, the changes I proposed would allow users to create trusted
> > keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
> > CONFIG_TCG_TPM=m is a valid configuration.
> >
> > Jarkko, Dan's patch seems sufficient to fix the issue. He could include
> > the changes I proposed in his patch. What is your opinion?
>
> Agreed.

What changes?

Robert, please feel free to re-author the proposed patch however you
see fit, I just want whatever will get libnvdimm operational again in
the shortest amount of time.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-22 15:24               ` Dan Williams
  0 siblings, 0 replies; 50+ messages in thread
From: Dan Williams @ 2019-03-22 15:24 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Roberto Sassu, James Bottomley, Mimi Zohar, David Howells,
	keyrings, linux-nvdimm, Linux Kernel Mailing List,
	Silviu Vlasceanu

On Fri, Mar 22, 2019 at 3:12 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Mar 21, 2019 at 06:45:47PM +0100, Roberto Sassu wrote:
> > On 3/21/2019 5:30 PM, Dan Williams wrote:
> > > On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> > > >
> > > > On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > > > > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> > > > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > > > the module to load, but rely on trusted_instantiate() to fail
> > > > > > trusted-key operations.
> > > > > >
> > > > > > Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> > > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > Cc: James Bottomley <jejb@linux.ibm.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>
> > > > >
> > > > > It should check for chip in each function that uses TPM now that
> > > > > the code does not rely on default chip. Otherwise, the semantics
> > > > > are kind of inconsistent.
> > > >
> > > > If no other TPM function can be used before a successful key
> > > > instantiate, checking for a chip only in trusted_instantiate() seems
> > > > sufficient. Then, the same chip will be used by all TPM functions until
> > > > module unloading, since we incremented the reference count.
> > > >
> > > > I would suggest to move the tpm_default_chip() and init_digests() calls
> > > > to trusted_instantiate() to restore the old behavior of init_trusted().
> > > >
> > > > trusted_instantiate() should look like:
> > > > ---
> > > > if (!chip) {
> > > >          chip = tpm_default_chip();
> > > >          if (!chip)
> > > >                  return -ENODEV;
> > > > }
> > > >
> > > > if (!digests) {
> > > >          ret = init_digests();
> > > >          if (ret < 0)
> > > >                  return ret;
> > > > }
> > >
> > > This patch already achieves that because tpm_find_get_ops() will fail
> > > and cause tpm_is_tpm2() to return NULL.
> >
> > In addition, the changes I proposed would allow users to create trusted
> > keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
> > CONFIG_TCG_TPM=m is a valid configuration.
> >
> > Jarkko, Dan's patch seems sufficient to fix the issue. He could include
> > the changes I proposed in his patch. What is your opinion?
>
> Agreed.

What changes?

Robert, please feel free to re-author the proposed patch however you
see fit, I just want whatever will get libnvdimm operational again in
the shortest amount of time.

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-22 15:24               ` Dan Williams
  (?)
@ 2019-03-25 14:12                 ` Jarkko Sakkinen
  -1 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-25 14:12 UTC (permalink / raw)
  To: Dan Williams
  Cc: Silviu Vlasceanu, linux-nvdimm, James Bottomley, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Fri, Mar 22, 2019 at 08:24:01AM -0700, Dan Williams wrote:
> On Fri, Mar 22, 2019 at 3:12 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 06:45:47PM +0100, Roberto Sassu wrote:
> > > On 3/21/2019 5:30 PM, Dan Williams wrote:
> > > > On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> > > > >
> > > > > On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > > > > > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> > > > > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > > > > the module to load, but rely on trusted_instantiate() to fail
> > > > > > > trusted-key operations.
> > > > > > >
> > > > > > > Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> > > > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > > Cc: James Bottomley <jejb@linux.ibm.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>
> > > > > >
> > > > > > It should check for chip in each function that uses TPM now that
> > > > > > the code does not rely on default chip. Otherwise, the semantics
> > > > > > are kind of inconsistent.
> > > > >
> > > > > If no other TPM function can be used before a successful key
> > > > > instantiate, checking for a chip only in trusted_instantiate() seems
> > > > > sufficient. Then, the same chip will be used by all TPM functions until
> > > > > module unloading, since we incremented the reference count.
> > > > >
> > > > > I would suggest to move the tpm_default_chip() and init_digests() calls
> > > > > to trusted_instantiate() to restore the old behavior of init_trusted().
> > > > >
> > > > > trusted_instantiate() should look like:
> > > > > ---
> > > > > if (!chip) {
> > > > >          chip = tpm_default_chip();
> > > > >          if (!chip)
> > > > >                  return -ENODEV;
> > > > > }
> > > > >
> > > > > if (!digests) {
> > > > >          ret = init_digests();
> > > > >          if (ret < 0)
> > > > >                  return ret;
> > > > > }
> > > >
> > > > This patch already achieves that because tpm_find_get_ops() will fail
> > > > and cause tpm_is_tpm2() to return NULL.
> > >
> > > In addition, the changes I proposed would allow users to create trusted
> > > keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
> > > CONFIG_TCG_TPM=m is a valid configuration.
> > >
> > > Jarkko, Dan's patch seems sufficient to fix the issue. He could include
> > > the changes I proposed in his patch. What is your opinion?
> >
> > Agreed.
> 
> What changes?
> 
> Robert, please feel free to re-author the proposed patch however you
> see fit, I just want whatever will get libnvdimm operational again in
> the shortest amount of time.

I've decided go with a patch of least innovation i.e. one that simply reverts
the old behavior. Sending patch soon.

/Jarkko
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-25 14:12                 ` Jarkko Sakkinen
  0 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-25 14:12 UTC (permalink / raw)
  To: Dan Williams
  Cc: Silviu Vlasceanu, linux-nvdimm, James Bottomley, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Fri, Mar 22, 2019 at 08:24:01AM -0700, Dan Williams wrote:
> On Fri, Mar 22, 2019 at 3:12 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 06:45:47PM +0100, Roberto Sassu wrote:
> > > On 3/21/2019 5:30 PM, Dan Williams wrote:
> > > > On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> > > > >
> > > > > On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > > > > > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> > > > > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > > > > the module to load, but rely on trusted_instantiate() to fail
> > > > > > > trusted-key operations.
> > > > > > >
> > > > > > > Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> > > > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > > Cc: James Bottomley <jejb@linux.ibm.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>
> > > > > >
> > > > > > It should check for chip in each function that uses TPM now that
> > > > > > the code does not rely on default chip. Otherwise, the semantics
> > > > > > are kind of inconsistent.
> > > > >
> > > > > If no other TPM function can be used before a successful key
> > > > > instantiate, checking for a chip only in trusted_instantiate() seems
> > > > > sufficient. Then, the same chip will be used by all TPM functions until
> > > > > module unloading, since we incremented the reference count.
> > > > >
> > > > > I would suggest to move the tpm_default_chip() and init_digests() calls
> > > > > to trusted_instantiate() to restore the old behavior of init_trusted().
> > > > >
> > > > > trusted_instantiate() should look like:
> > > > > ---
> > > > > if (!chip) {
> > > > >          chip = tpm_default_chip();
> > > > >          if (!chip)
> > > > >                  return -ENODEV;
> > > > > }
> > > > >
> > > > > if (!digests) {
> > > > >          ret = init_digests();
> > > > >          if (ret < 0)
> > > > >                  return ret;
> > > > > }
> > > >
> > > > This patch already achieves that because tpm_find_get_ops() will fail
> > > > and cause tpm_is_tpm2() to return NULL.
> > >
> > > In addition, the changes I proposed would allow users to create trusted
> > > keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
> > > CONFIG_TCG_TPM=m is a valid configuration.
> > >
> > > Jarkko, Dan's patch seems sufficient to fix the issue. He could include
> > > the changes I proposed in his patch. What is your opinion?
> >
> > Agreed.
> 
> What changes?
> 
> Robert, please feel free to re-author the proposed patch however you
> see fit, I just want whatever will get libnvdimm operational again in
> the shortest amount of time.

I've decided go with a patch of least innovation i.e. one that simply reverts
the old behavior. Sending patch soon.

/Jarkko

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-25 14:12                 ` Jarkko Sakkinen
  0 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-25 14:12 UTC (permalink / raw)
  To: Dan Williams
  Cc: Roberto Sassu, James Bottomley, Mimi Zohar, David Howells,
	keyrings, linux-nvdimm, Linux Kernel Mailing List,
	Silviu Vlasceanu

On Fri, Mar 22, 2019 at 08:24:01AM -0700, Dan Williams wrote:
> On Fri, Mar 22, 2019 at 3:12 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Thu, Mar 21, 2019 at 06:45:47PM +0100, Roberto Sassu wrote:
> > > On 3/21/2019 5:30 PM, Dan Williams wrote:
> > > > On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
> > > > >
> > > > > On 3/21/2019 2:54 PM, Jarkko Sakkinen wrote:
> > > > > > On Mon, Mar 18, 2019 at 04:45:13PM -0700, Dan Williams wrote:
> > > > > > > Rather than fail initialization of the trusted.ko module, arrange for
> > > > > > > the module to load, but rely on trusted_instantiate() to fail
> > > > > > > trusted-key operations.
> > > > > > >
> > > > > > > Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> > > > > > > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > > Cc: James Bottomley <jejb@linux.ibm.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>
> > > > > >
> > > > > > It should check for chip in each function that uses TPM now that
> > > > > > the code does not rely on default chip. Otherwise, the semantics
> > > > > > are kind of inconsistent.
> > > > >
> > > > > If no other TPM function can be used before a successful key
> > > > > instantiate, checking for a chip only in trusted_instantiate() seems
> > > > > sufficient. Then, the same chip will be used by all TPM functions until
> > > > > module unloading, since we incremented the reference count.
> > > > >
> > > > > I would suggest to move the tpm_default_chip() and init_digests() calls
> > > > > to trusted_instantiate() to restore the old behavior of init_trusted().
> > > > >
> > > > > trusted_instantiate() should look like:
> > > > > ---
> > > > > if (!chip) {
> > > > >          chip = tpm_default_chip();
> > > > >          if (!chip)
> > > > >                  return -ENODEV;
> > > > > }
> > > > >
> > > > > if (!digests) {
> > > > >          ret = init_digests();
> > > > >          if (ret < 0)
> > > > >                  return ret;
> > > > > }
> > > >
> > > > This patch already achieves that because tpm_find_get_ops() will fail
> > > > and cause tpm_is_tpm2() to return NULL.
> > >
> > > In addition, the changes I proposed would allow users to create trusted
> > > keys if a TPM is added later. CONFIG_TRUSTED_KEYS=y and
> > > CONFIG_TCG_TPM=m is a valid configuration.
> > >
> > > Jarkko, Dan's patch seems sufficient to fix the issue. He could include
> > > the changes I proposed in his patch. What is your opinion?
> >
> > Agreed.
> 
> What changes?
> 
> Robert, please feel free to re-author the proposed patch however you
> see fit, I just want whatever will get libnvdimm operational again in
> the shortest amount of time.

I've decided go with a patch of least innovation i.e. one that simply reverts
the old behavior. Sending patch soon.

/Jarkko

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
  2019-03-25 14:12                 ` Jarkko Sakkinen
  (?)
@ 2019-03-25 14:50                   ` Jarkko Sakkinen
  -1 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-25 14:50 UTC (permalink / raw)
  To: Dan Williams
  Cc: Silviu Vlasceanu, linux-nvdimm, James Bottomley, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Mon, Mar 25, 2019 at 04:12:23PM +0200, Jarkko Sakkinen wrote:
> > Robert, please feel free to re-author the proposed patch however you
> > see fit, I just want whatever will get libnvdimm operational again in
> > the shortest amount of time.
> 
> I've decided go with a patch of least innovation i.e. one that simply reverts
> the old behavior. Sending patch soon.

I.e. https://www.lkml.org/lkml/2019/3/25/674

I think fixing the actual dependency issue would be unorthodox at this
point of release cycle as adding dependencies was not the root cause
for the bug even if existing somewhat unorthodox were. Better to just
the problem by reverting the semantics near the same as in v5.0 and
take this dependency discussion "offline" for the release work...

/Jarkko
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-25 14:50                   ` Jarkko Sakkinen
  0 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-25 14:50 UTC (permalink / raw)
  To: Dan Williams
  Cc: Silviu Vlasceanu, linux-nvdimm, James Bottomley, Roberto Sassu,
	Linux Kernel Mailing List, Mimi Zohar, David Howells, keyrings

On Mon, Mar 25, 2019 at 04:12:23PM +0200, Jarkko Sakkinen wrote:
> > Robert, please feel free to re-author the proposed patch however you
> > see fit, I just want whatever will get libnvdimm operational again in
> > the shortest amount of time.
> 
> I've decided go with a patch of least innovation i.e. one that simply reverts
> the old behavior. Sending patch soon.

I.e. https://www.lkml.org/lkml/2019/3/25/674

I think fixing the actual dependency issue would be unorthodox at this
point of release cycle as adding dependencies was not the root cause
for the bug even if existing somewhat unorthodox were. Better to just
the problem by reverting the semantics near the same as in v5.0 and
take this dependency discussion "offline" for the release work...

/Jarkko

^ permalink raw reply	[flat|nested] 50+ messages in thread

* Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM
@ 2019-03-25 14:50                   ` Jarkko Sakkinen
  0 siblings, 0 replies; 50+ messages in thread
From: Jarkko Sakkinen @ 2019-03-25 14:50 UTC (permalink / raw)
  To: Dan Williams
  Cc: Roberto Sassu, James Bottomley, Mimi Zohar, David Howells,
	keyrings, linux-nvdimm, Linux Kernel Mailing List,
	Silviu Vlasceanu

On Mon, Mar 25, 2019 at 04:12:23PM +0200, Jarkko Sakkinen wrote:
> > Robert, please feel free to re-author the proposed patch however you
> > see fit, I just want whatever will get libnvdimm operational again in
> > the shortest amount of time.
> 
> I've decided go with a patch of least innovation i.e. one that simply reverts
> the old behavior. Sending patch soon.

I.e. https://www.lkml.org/lkml/2019/3/25/674

I think fixing the actual dependency issue would be unorthodox at this
point of release cycle as adding dependencies was not the root cause
for the bug even if existing somewhat unorthodox were. Better to just
the problem by reverting the semantics near the same as in v5.0 and
take this dependency discussion "offline" for the release work...

/Jarkko

^ permalink raw reply	[flat|nested] 50+ messages in thread

end of thread, other threads:[~2019-03-25 14:50 UTC | newest]

Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-18 23:45 [PATCH] security/keys/trusted: Allow operation without hardware TPM Dan Williams
2019-03-18 23:45 ` Dan Williams
2019-03-18 23:45 ` Dan Williams
     [not found] ` <155295271345.1945351.6465460744078693578.stgit-p8uTFz9XbKj2zm6wflaqv1nYeNYlB/vhral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2019-03-19  0:24   ` James Bottomley
2019-03-19  0:24     ` James Bottomley
2019-03-19  0:24     ` James Bottomley
2019-03-19  0:30     ` Dan Williams
2019-03-19  0:30       ` Dan Williams
2019-03-19  0:30       ` Dan Williams
     [not found]       ` <CAA9_cmcOD2zPaaNbkYAaH5DRDRAebPkW+hwPA0zPKY4kU8R-rg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-19  0:56         ` James Bottomley
2019-03-19  0:56           ` James Bottomley
2019-03-19  0:56           ` James Bottomley
2019-03-19  1:34           ` Dan Williams
2019-03-19  1:34             ` Dan Williams
2019-03-19  1:34             ` Dan Williams
2019-03-20  1:55           ` Dan Williams
2019-03-20  1:55             ` Dan Williams
2019-03-20  1:55             ` Dan Williams
     [not found]             ` <CAPcyv4hXKQcdLnKG6rPNOJr2wjq31uCCr+16S5Yu6S5A_5UGrA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-20  2:19               ` James Bottomley
2019-03-20  2:19                 ` James Bottomley
2019-03-20  2:19                 ` James Bottomley
2019-03-19 22:56         ` Mimi Zohar
2019-03-19 22:56           ` Mimi Zohar
2019-03-19 22:56           ` Mimi Zohar
2019-03-19 23:01           ` Dan Williams
2019-03-19 23:01             ` Dan Williams
2019-03-19 23:01             ` Dan Williams
2019-03-21 13:54 ` Jarkko Sakkinen
2019-03-21 13:54   ` Jarkko Sakkinen
2019-03-21 13:54   ` Jarkko Sakkinen
2019-03-21 14:26   ` Roberto Sassu
2019-03-21 14:26     ` Roberto Sassu
2019-03-21 14:26     ` Roberto Sassu
2019-03-21 16:30     ` Dan Williams
2019-03-21 16:30       ` Dan Williams
2019-03-21 16:30       ` Dan Williams
     [not found]       ` <CAPcyv4gc_KWedC12bTcX24KwjKiBCF=yvouRNzCbJKaLrgJdDg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-21 17:45         ` Roberto Sassu
2019-03-21 17:45           ` Roberto Sassu
2019-03-21 17:45           ` Roberto Sassu
2019-03-22 10:12           ` Jarkko Sakkinen
2019-03-22 10:12             ` Jarkko Sakkinen
2019-03-22 15:24             ` Dan Williams
2019-03-22 15:24               ` Dan Williams
2019-03-22 15:24               ` Dan Williams
2019-03-25 14:12               ` Jarkko Sakkinen
2019-03-25 14:12                 ` Jarkko Sakkinen
2019-03-25 14:12                 ` Jarkko Sakkinen
2019-03-25 14:50                 ` Jarkko Sakkinen
2019-03-25 14:50                   ` Jarkko Sakkinen
2019-03-25 14:50                   ` Jarkko Sakkinen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.