| Message ID | 155297558570.2276575.11731393787282486177.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security/keys/encrypted: Break module dependency chain | expand |
On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote: < snip > > +/* > + * request_trusted_key - request the trusted key > + * > + * Trusted keys are sealed to PCRs and other metadata. Although userspace > + * manages both trusted/encrypted key-types, like the encrypted key type > + * data, trusted key type data is not visible decrypted from userspace. > + */ > +static struct key *request_trusted_key(const char *trusted_desc, > + const u8 **master_key, size_t *master_keylen) > +{ > + struct trusted_key_payload *tpayload; > + struct key_type *type; > + struct key *tkey; > + > + type = key_type_lookup("trusted"); The associated key_type_put() will need to be called. > + if (IS_ERR(type)) { > + tkey = (struct key *)type; > + goto error; > + } > + tkey = request_key(type, trusted_desc, NULL); > + if (IS_ERR(tkey)) > + goto error; > + > + down_read(&tkey->sem); > + tpayload = tkey->payload.data[0]; > + *master_key = tpayload->key; > + *master_keylen = tpayload->key_len; > +error: > + return tkey; > +} > + > diff --git a/security/keys/key.c b/security/keys/key.c > index 696f1c092c50..9045b62afb04 100644 > --- a/security/keys/key.c > +++ b/security/keys/key.c > @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type) > found_kernel_type: > return ktype; > } > +EXPORT_SYMBOL_GPL(key_type_lookup); Only the kernel is calling key_type_lookup(). Why does key_type_lookup() need to be exported? Mimi > > void key_set_timeout(struct key *key, unsigned timeout) > { >
On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote: > > < snip > > > > +/* > > + * request_trusted_key - request the trusted key > > + * > > + * Trusted keys are sealed to PCRs and other metadata. Although userspace > > + * manages both trusted/encrypted key-types, like the encrypted key type > > + * data, trusted key type data is not visible decrypted from userspace. > > + */ > > +static struct key *request_trusted_key(const char *trusted_desc, > > + const u8 **master_key, size_t *master_keylen) > > +{ > > + struct trusted_key_payload *tpayload; > > + struct key_type *type; > > + struct key *tkey; > > + > > + type = key_type_lookup("trusted"); > > The associated key_type_put() will need to be called. Yes. > > > + if (IS_ERR(type)) { > > + tkey = (struct key *)type; > > + goto error; > > + } > > + tkey = request_key(type, trusted_desc, NULL); > > + if (IS_ERR(tkey)) > > + goto error; > > + > > + down_read(&tkey->sem); > > + tpayload = tkey->payload.data[0]; > > + *master_key = tpayload->key; > > + *master_keylen = tpayload->key_len; > > +error: > > + return tkey; > > +} > > + > > > > > diff --git a/security/keys/key.c b/security/keys/key.c > > index 696f1c092c50..9045b62afb04 100644 > > --- a/security/keys/key.c > > +++ b/security/keys/key.c > > @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type) > > found_kernel_type: > > return ktype; > > } > > +EXPORT_SYMBOL_GPL(key_type_lookup); This needs to be moved to patch1. > Only the kernel is calling key_type_lookup(). Why does > key_type_lookup() need to be exported? This patch series adds several new callers outside of keys-subsystem core that need this export, the first one being encrypted-keys itself in patch1. drivers/nvdimm/security.c:57: type = key_type_lookup("encrypted"); fs/ecryptfs/keystore.c:1627: type = key_type_lookup("encrypted"); security/integrity/evm/evm_crypto.c:361: type = key_type_lookup("encrypted"); security/keys/encrypted-keys/encrypted.c:440: type = key_type_lookup("trusted");
On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote: > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote: > > > > diff --git a/security/keys/key.c b/security/keys/key.c > > > index 696f1c092c50..9045b62afb04 100644 > > > --- a/security/keys/key.c > > > +++ b/security/keys/key.c > > > @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type) > > > found_kernel_type: > > > return ktype; > > > } > > > +EXPORT_SYMBOL_GPL(key_type_lookup); > > This needs to be moved to patch1. > > > Only the kernel is calling key_type_lookup(). Why does > > key_type_lookup() need to be exported? > > This patch series adds several new callers outside of keys-subsystem > core that need this export, the first one being encrypted-keys itself > in patch1. It's needed, because they could be compiled as kernel modules, not builtin (eg. EVM). Mimi > > drivers/nvdimm/security.c:57: type = key_type_lookup("encrypted"); > fs/ecryptfs/keystore.c:1627: type = key_type_lookup("encrypted"); > security/integrity/evm/evm_crypto.c:361: type = > key_type_lookup("encrypted"); > security/keys/encrypted-keys/encrypted.c:440: type = > key_type_lookup("trusted"); >
On Tue, Mar 19, 2019 at 6:11 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote: > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote: > > > > > > diff --git a/security/keys/key.c b/security/keys/key.c > > > > index 696f1c092c50..9045b62afb04 100644 > > > > --- a/security/keys/key.c > > > > +++ b/security/keys/key.c > > > > @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type) > > > > found_kernel_type: > > > > return ktype; > > > > } > > > > +EXPORT_SYMBOL_GPL(key_type_lookup); > > > > This needs to be moved to patch1. > > > > > Only the kernel is calling key_type_lookup(). Why does > > > key_type_lookup() need to be exported? > > > > This patch series adds several new callers outside of keys-subsystem > > core that need this export, the first one being encrypted-keys itself > > in patch1. > > It's needed, because they could be compiled as kernel modules, not > builtin (eg. EVM). > Right, but now I realize a problem. The side effect of having direct module dependencies to the key_type_{encrypted,trusted} symbols is that module reference counting is handled automatically. So, I need to respin this with some explicit try_module_get() and module_put() added otherwise the encrypted_keys facility can be removed while active keys are instantiated. > > drivers/nvdimm/security.c:57: type = key_type_lookup("encrypted"); > > fs/ecryptfs/keystore.c:1627: type = key_type_lookup("encrypted"); > > security/integrity/evm/evm_crypto.c:361: type = > > key_type_lookup("encrypted"); > > security/keys/encrypted-keys/encrypted.c:440: type = > > key_type_lookup("trusted"); > > >
On Tue, Mar 19, 2019 at 6:34 PM Dan Williams <dan.j.williams@intel.com> wrote: > > On Tue, Mar 19, 2019 at 6:11 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote: > > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote: > > > > > > > > diff --git a/security/keys/key.c b/security/keys/key.c > > > > > index 696f1c092c50..9045b62afb04 100644 > > > > > --- a/security/keys/key.c > > > > > +++ b/security/keys/key.c > > > > > @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type) > > > > > found_kernel_type: > > > > > return ktype; > > > > > } > > > > > +EXPORT_SYMBOL_GPL(key_type_lookup); > > > > > > This needs to be moved to patch1. > > > > > > > Only the kernel is calling key_type_lookup(). Why does > > > > key_type_lookup() need to be exported? > > > > > > This patch series adds several new callers outside of keys-subsystem > > > core that need this export, the first one being encrypted-keys itself > > > in patch1. > > > > It's needed, because they could be compiled as kernel modules, not > > builtin (eg. EVM). > > > > Right, but now I realize a problem. The side effect of having direct > module dependencies to the key_type_{encrypted,trusted} symbols is > that module reference counting is handled automatically. > > So, I need to respin this with some explicit try_module_get() and > module_put() added otherwise the encrypted_keys facility can be > removed while active keys are instantiated. ...and now I'm wondering if this refactoring is getting too big and should wait for v5.2. In the meantime apply my small fix for v5.1 https://patchwork.kernel.org/patch/10858649/
On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote: > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote: > > > > < snip > > > > > > +/* > > > + * request_trusted_key - request the trusted key > > > + * > > > + * Trusted keys are sealed to PCRs and other metadata. Although userspace > > > + * manages both trusted/encrypted key-types, like the encrypted key type > > > + * data, trusted key type data is not visible decrypted from userspace. > > > + */ > > > +static struct key *request_trusted_key(const char *trusted_desc, > > > + const u8 **master_key, size_t *master_keylen) > > > +{ > > > + struct trusted_key_payload *tpayload; > > > + struct key_type *type; > > > + struct key *tkey; > > > + > > > + type = key_type_lookup("trusted"); > > > > The associated key_type_put() will need to be called. > > Yes. I don't know if defining a key_type_lookup() wrapper, perhaps named is_key_type_available(), would help. Both key_type_lookup() and key_type_put() would be called. The existing code could then remain the same. Mimi > > > > > > + if (IS_ERR(type)) { > > > + tkey = (struct key *)type; > > > + goto error; > > > + } > > > + tkey = request_key(type, trusted_desc, NULL); > > > + if (IS_ERR(tkey)) > > > + goto error; > > > + > > > + down_read(&tkey->sem); > > > + tpayload = tkey->payload.data[0]; > > > + *master_key = tpayload->key; > > > + *master_keylen = tpayload->key_len; > > > +error: > > > + return tkey; > > > +} > > > + > >
On Tue, Mar 19, 2019 at 7:36 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote: > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote: > > > > > > < snip > > > > > > > > +/* > > > > + * request_trusted_key - request the trusted key > > > > + * > > > > + * Trusted keys are sealed to PCRs and other metadata. Although userspace > > > > + * manages both trusted/encrypted key-types, like the encrypted key type > > > > + * data, trusted key type data is not visible decrypted from userspace. > > > > + */ > > > > +static struct key *request_trusted_key(const char *trusted_desc, > > > > + const u8 **master_key, size_t *master_keylen) > > > > +{ > > > > + struct trusted_key_payload *tpayload; > > > > + struct key_type *type; > > > > + struct key *tkey; > > > > + > > > > + type = key_type_lookup("trusted"); > > > > > > The associated key_type_put() will need to be called. > > > > Yes. > > I don't know if defining a key_type_lookup() wrapper, perhaps named > is_key_type_available(), would help. Both key_type_lookup() and > key_type_put() would be called. The existing code could then remain > the same. > Maybe, but something still needs to pin the hosting module. I think this means that the first call to key_type->instantiate() pins the hosting module, and the ->destroy() of the last key for the key_type unpins the module. It does mean that the ->destroy() method is no longer optional.
On Tue, 2019-03-19 at 22:48 -0700, Dan Williams wrote: > On Tue, Mar 19, 2019 at 7:36 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote: > > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote: > > > > > > > > < snip > > > > > > > > > > +/* > > > > > + * request_trusted_key - request the trusted key > > > > > + * > > > > > + * Trusted keys are sealed to PCRs and other metadata. Although userspace > > > > > + * manages both trusted/encrypted key-types, like the encrypted key type > > > > > + * data, trusted key type data is not visible decrypted from userspace. > > > > > + */ > > > > > +static struct key *request_trusted_key(const char *trusted_desc, > > > > > + const u8 **master_key, size_t *master_keylen) > > > > > +{ > > > > > + struct trusted_key_payload *tpayload; > > > > > + struct key_type *type; > > > > > + struct key *tkey; > > > > > + > > > > > + type = key_type_lookup("trusted"); > > > > > > > > The associated key_type_put() will need to be called. > > > > > > Yes. > > > > I don't know if defining a key_type_lookup() wrapper, perhaps named > > is_key_type_available(), would help. Both key_type_lookup() and > > key_type_put() would be called. The existing code could then remain > > the same. > > > > Maybe, but something still needs to pin the hosting module. I think > this means that the first call to key_type->instantiate() pins the > hosting module, and the ->destroy() of the last key for the key_type > unpins the module. It does mean that the ->destroy() method is no > longer optional. This sounds like it isn't a new problem. Both issues need to be addressed, but I think we should differentiate between them and address them separately. In terms of the original nvdimm encrypted/trusted key problem, the above suggestion requires the least amount of change. For v5.2, I would replace it with the full updated patch set. Mimi
On Wed, Mar 20, 2019 at 5:07 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2019-03-19 at 22:48 -0700, Dan Williams wrote: > > On Tue, Mar 19, 2019 at 7:36 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote: > > > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > > > > > On Mon, 2019-03-18 at 23:06 -0700, Dan Williams wrote: > > > > > > > > > > < snip > > > > > > > > > > > > +/* > > > > > > + * request_trusted_key - request the trusted key > > > > > > + * > > > > > > + * Trusted keys are sealed to PCRs and other metadata. Although userspace > > > > > > + * manages both trusted/encrypted key-types, like the encrypted key type > > > > > > + * data, trusted key type data is not visible decrypted from userspace. > > > > > > + */ > > > > > > +static struct key *request_trusted_key(const char *trusted_desc, > > > > > > + const u8 **master_key, size_t *master_keylen) > > > > > > +{ > > > > > > + struct trusted_key_payload *tpayload; > > > > > > + struct key_type *type; > > > > > > + struct key *tkey; > > > > > > + > > > > > > + type = key_type_lookup("trusted"); > > > > > > > > > > The associated key_type_put() will need to be called. > > > > > > > > Yes. > > > > > > I don't know if defining a key_type_lookup() wrapper, perhaps named > > > is_key_type_available(), would help. Both key_type_lookup() and > > > key_type_put() would be called. The existing code could then remain > > > the same. > > > > > > > Maybe, but something still needs to pin the hosting module. I think > > this means that the first call to key_type->instantiate() pins the > > hosting module, and the ->destroy() of the last key for the key_type > > unpins the module. It does mean that the ->destroy() method is no > > longer optional. > > This sounds like it isn't a new problem. Both issues need to be > addressed, but I think we should differentiate between them and > address them separately. > > In terms of the original nvdimm encrypted/trusted key problem, the > above suggestion requires the least amount of change. For v5.2, I > would replace it with the full updated patch set. I believe smallest amount of change is this single patch: https://patchwork.kernel.org/patch/10858649/
diff --git a/include/linux/key.h b/include/linux/key.h index 7099985e35a9..e7bfd037d26f 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -270,6 +270,7 @@ static inline void key_ref_put(key_ref_t key_ref) key_put(key_ref_to_ptr(key_ref)); } +extern struct key_type *key_type_lookup(const char *type); extern struct key *request_key(struct key_type *type, const char *description, const char *callout_info); diff --git a/security/keys/encrypted-keys/Makefile b/security/keys/encrypted-keys/Makefile index 7a44dce6f69d..d42487bb3d8a 100644 --- a/security/keys/encrypted-keys/Makefile +++ b/security/keys/encrypted-keys/Makefile @@ -6,6 +6,3 @@ obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys.o encrypted-keys-y := encrypted.o ecryptfs_format.o -masterkey-$(CONFIG_TRUSTED_KEYS) := masterkey_trusted.o -masterkey-$(CONFIG_TRUSTED_KEYS)-$(CONFIG_ENCRYPTED_KEYS) := masterkey_trusted.o -encrypted-keys-y += $(masterkey-y) $(masterkey-m-m) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 347108f660a1..06925d3b30c9 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -423,6 +423,37 @@ static struct skcipher_request *init_skcipher_req(const u8 *key, return req; } +/* + * request_trusted_key - request the trusted key + * + * Trusted keys are sealed to PCRs and other metadata. Although userspace + * manages both trusted/encrypted key-types, like the encrypted key type + * data, trusted key type data is not visible decrypted from userspace. + */ +static struct key *request_trusted_key(const char *trusted_desc, + const u8 **master_key, size_t *master_keylen) +{ + struct trusted_key_payload *tpayload; + struct key_type *type; + struct key *tkey; + + 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; + + down_read(&tkey->sem); + tpayload = tkey->payload.data[0]; + *master_key = tpayload->key; + *master_keylen = tpayload->key_len; +error: + return tkey; +} + static struct key *request_master_key(struct encrypted_key_payload *epayload, const u8 **master_key, size_t *master_keylen) { @@ -1025,3 +1056,4 @@ late_initcall(init_encrypted); module_exit(cleanup_encrypted); MODULE_LICENSE("GPL"); +MODULE_SOFTDEP("pre: trusted"); diff --git a/security/keys/encrypted-keys/encrypted.h b/security/keys/encrypted-keys/encrypted.h index 1809995db452..0ae67824a24a 100644 --- a/security/keys/encrypted-keys/encrypted.h +++ b/security/keys/encrypted-keys/encrypted.h @@ -3,18 +3,6 @@ #define __ENCRYPTED_KEY_H #define ENCRYPTED_DEBUG 0 -#if defined(CONFIG_TRUSTED_KEYS) || \ - (defined(CONFIG_TRUSTED_KEYS_MODULE) && defined(CONFIG_ENCRYPTED_KEYS_MODULE)) -extern struct key *request_trusted_key(const char *trusted_desc, - const u8 **master_key, size_t *master_keylen); -#else -static inline struct key *request_trusted_key(const char *trusted_desc, - const u8 **master_key, - size_t *master_keylen) -{ - return ERR_PTR(-EOPNOTSUPP); -} -#endif #if ENCRYPTED_DEBUG static inline void dump_master_key(const u8 *master_key, size_t master_keylen) diff --git a/security/keys/encrypted-keys/masterkey_trusted.c b/security/keys/encrypted-keys/masterkey_trusted.c deleted file mode 100644 index 7560aea6438d..000000000000 --- a/security/keys/encrypted-keys/masterkey_trusted.c +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright (C) 2010 IBM Corporation - * Copyright (C) 2010 Politecnico di Torino, Italy - * TORSEC group -- http://security.polito.it - * - * Authors: - * Mimi Zohar <zohar@us.ibm.com> - * Roberto Sassu <roberto.sassu@polito.it> - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, version 2 of the License. - * - * See Documentation/security/keys/trusted-encrypted.rst - */ - -#include <linux/uaccess.h> -#include <linux/err.h> -#include <keys/trusted-type.h> -#include <keys/encrypted-type.h> -#include "encrypted.h" -#include "../internal.h" - -/* - * request_trusted_key - request the trusted key - * - * Trusted keys are sealed to PCRs and other metadata. Although userspace - * manages both trusted/encrypted key-types, like the encrypted key type - * data, trusted key type data is not visible decrypted from userspace. - */ -struct key *request_trusted_key(const char *trusted_desc, - const u8 **master_key, size_t *master_keylen) -{ - struct trusted_key_payload *tpayload; - struct key_type *type; - struct key *tkey; - - 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; - - down_read(&tkey->sem); - tpayload = tkey->payload.data[0]; - *master_key = tpayload->key; - *master_keylen = tpayload->key_len; -error: - return tkey; -} - -MODULE_SOFTDEP("pre: trusted"); diff --git a/security/keys/internal.h b/security/keys/internal.h index 8f533c81aa8d..ea2eb78459bf 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -89,8 +89,6 @@ extern spinlock_t key_serial_lock; extern struct mutex key_construction_mutex; extern wait_queue_head_t request_key_conswq; - -extern struct key_type *key_type_lookup(const char *type); extern void key_type_put(struct key_type *ktype); extern int __key_link_begin(struct key *keyring, diff --git a/security/keys/key.c b/security/keys/key.c index 696f1c092c50..9045b62afb04 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -706,6 +706,7 @@ struct key_type *key_type_lookup(const char *type) found_kernel_type: return ktype; } +EXPORT_SYMBOL_GPL(key_type_lookup); void key_set_timeout(struct key *key, unsigned timeout) {
Now that the trusted key type is looked up by name rather than direct symbol there is no need to play games with detecting the build configuration. Make request_trusted_key() a static facility internal to the encrypted-keys implementation. Suggested-by: James Bottomley <jejb@linux.ibm.com> Cc: Mimi Zohar <zohar@linux.ibm.com> Cc: David Howells <dhowells@redhat.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- include/linux/key.h | 1 security/keys/encrypted-keys/Makefile | 3 - security/keys/encrypted-keys/encrypted.c | 32 +++++++++++++ security/keys/encrypted-keys/encrypted.h | 12 ----- security/keys/encrypted-keys/masterkey_trusted.c | 55 ---------------------- security/keys/internal.h | 2 - security/keys/key.c | 1 7 files changed, 34 insertions(+), 72 deletions(-) delete mode 100644 security/keys/encrypted-keys/masterkey_trusted.c