lkml.org 
[lkml]   [2019]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] security/keys/trusted: Allow operation without hardware TPM
From
Date
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

\
 
 \ /
  Last update: 2019-03-20 03:20    [W:0.209 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site