Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM

[James Bottomley]

On Tue, 2019-03-19 at 18:55 -0700, Dan Williams wrote:
> On Mon, Mar 18, 2019 at 5:56 PM James Bottomley <jejb@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx> 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