Messages in this thread | | | Subject | Re: [PATCH] security/keys/trusted: Allow operation without hardware TPM | From | James Bottomley <> | Date | Tue, 19 Mar 2019 19:19:00 -0700 |
| |
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
| |