security/keys/trusted: Allow operation without hardware TPM

Commit Message

Dan Williams March 18, 2019, 11:45 p.m. UTC

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 <[e-mail]>
Cc: Jarkko Sakkinen <[e-mail]>
Cc: James Bottomley <[e-mail]>
Cc: Jarkko Sakkinen <[e-mail]>
Cc: Mimi Zohar <[e-mail]>
Cc: David Howells <[e-mail]>
Signed-off-by: Dan Williams <[e-mail]>
---
 security/keys/trusted.c |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Comments

James Bottomley March 19, 2019, 12:24 a.m. UTC | #1

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

Dan Williams March 19, 2019, 12:30 a.m. UTC | #2

On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <[e-mail]> 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.

James Bottomley March 19, 2019, 12:56 a.m. UTC | #3

On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <[e-mail]>
> 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;

Dan Williams March 19, 2019, 1:34 a.m. UTC | #4

On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <[e-mail]> wrote:
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <[e-mail]>
> > 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.

Mimi Zohar March 19, 2019, 10:56 p.m. UTC | #5

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 <[e-mail]> 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

Dan Williams March 19, 2019, 11:01 p.m. UTC | #6

On Tue, Mar 19, 2019 at 3:56 PM Mimi Zohar <[e-mail]> 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 <[e-mail]> 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.

Dan Williams March 20, 2019, 1:55 a.m. UTC | #7

On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <[e-mail]> wrote:
>
> On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley <[e-mail]>
> > 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.

James Bottomley March 20, 2019, 2:19 a.m. UTC | #8

On Tue, 2019-03-19 at 18:55 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <[e-mail]>
> wrote:
> > 
> > On Mon, 2019-03-18 at 17:30 -0700, Dan Williams wrote:
> > > On Mon, Mar 18, 2019 at 5:24 PM James Bottomley
> > > <[e-mail]> 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

Jarkko Sakkinen March 21, 2019, 1:54 p.m. UTC | #9

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 <[e-mail]>
> Cc: Jarkko Sakkinen <[e-mail]>
> Cc: James Bottomley <[e-mail]>
> Cc: Jarkko Sakkinen <[e-mail]>
> Cc: Mimi Zohar <[e-mail]>
> Cc: David Howells <[e-mail]>
> Signed-off-by: Dan Williams <[e-mail]>

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

Roberto Sassu March 21, 2019, 2:26 p.m. UTC | #10

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 <[e-mail]>
>> Cc: Jarkko Sakkinen <[e-mail]>
>> Cc: James Bottomley <[e-mail]>
>> Cc: Jarkko Sakkinen <[e-mail]>
>> Cc: Mimi Zohar <[e-mail]>
>> Cc: David Howells <[e-mail]>
>> Signed-off-by: Dan Williams <[e-mail]>
> 
> 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
>

Dan Williams March 21, 2019, 4:30 p.m. UTC | #11

On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <[e-mail]> 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 <[e-mail]>
> >> Cc: Jarkko Sakkinen <[e-mail]>
> >> Cc: James Bottomley <[e-mail]>
> >> Cc: Jarkko Sakkinen <[e-mail]>
> >> Cc: Mimi Zohar <[e-mail]>
> >> Cc: David Howells <[e-mail]>
> >> Signed-off-by: Dan Williams <[e-mail]>
> >
> > 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.

Roberto Sassu March 21, 2019, 5:45 p.m. UTC | #12

On 3/21/2019 5:30 PM, Dan Williams wrote:
> On Thu, Mar 21, 2019 at 7:27 AM Roberto Sassu <[e-mail]> 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 <[e-mail]>
>>>> Cc: Jarkko Sakkinen <[e-mail]>
>>>> Cc: James Bottomley <[e-mail]>
>>>> Cc: Jarkko Sakkinen <[e-mail]>
>>>> Cc: Mimi Zohar <[e-mail]>
>>>> Cc: David Howells <[e-mail]>
>>>> Signed-off-by: Dan Williams <[e-mail]>
>>>
>>> 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

Jarkko Sakkinen March 22, 2019, 10:12 a.m. UTC | #13

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 <[e-mail]> 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 <[e-mail]>
> > > > > Cc: Jarkko Sakkinen <[e-mail]>
> > > > > Cc: James Bottomley <[e-mail]>
> > > > > Cc: Jarkko Sakkinen <[e-mail]>
> > > > > Cc: Mimi Zohar <[e-mail]>
> > > > > Cc: David Howells <[e-mail]>
> > > > > Signed-off-by: Dan Williams <[e-mail]>
> > > > 
> > > > 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

Dan Williams March 22, 2019, 3:24 p.m. UTC | #14

On Fri, Mar 22, 2019 at 3:12 AM Jarkko Sakkinen
<[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > > > > Cc: Jarkko Sakkinen <[e-mail]>
> > > > > > Cc: James Bottomley <[e-mail]>
> > > > > > Cc: Jarkko Sakkinen <[e-mail]>
> > > > > > Cc: Mimi Zohar <[e-mail]>
> > > > > > Cc: David Howells <[e-mail]>
> > > > > > Signed-off-by: Dan Williams <[e-mail]>
> > > > >
> > > > > 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.

Jarkko Sakkinen March 25, 2019, 2:12 p.m. UTC | #15

On Fri, Mar 22, 2019 at 08:24:01AM -0700, Dan Williams wrote:
> On Fri, Mar 22, 2019 at 3:12 AM Jarkko Sakkinen
> <[e-mail]> 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 <[e-mail]> 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 <[e-mail]>
> > > > > > > Cc: Jarkko Sakkinen <[e-mail]>
> > > > > > > Cc: James Bottomley <[e-mail]>
> > > > > > > Cc: Jarkko Sakkinen <[e-mail]>
> > > > > > > Cc: Mimi Zohar <[e-mail]>
> > > > > > > Cc: David Howells <[e-mail]>
> > > > > > > Signed-off-by: Dan Williams <[e-mail]>
> > > > > >
> > > > > > 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

Jarkko Sakkinen March 25, 2019, 2:50 p.m. UTC | #16

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

Patch

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);