[PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
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
Cc: Mimi Zohar
Cc: David Howells
Signed-off-by: Dan Williams
---
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
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..
--- 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
- * Roberto Sassu
Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
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)
> {
>
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar 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");
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar 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");
>
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
On Tue, Mar 19, 2019 at 6:11 PM Mimi Zohar wrote:
>
> On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar 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");
> >
>
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
On Tue, Mar 19, 2019 at 6:34 PM Dan Williams wrote:
>
> On Tue, Mar 19, 2019 at 6:11 PM Mimi Zohar wrote:
> >
> > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar 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/
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar 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;
> > > +}
> > > +
> >
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
On Tue, Mar 19, 2019 at 7:36 PM Mimi Zohar wrote:
>
> On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar 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.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
On Tue, 2019-03-19 at 22:48 -0700, Dan Williams wrote:
> On Tue, Mar 19, 2019 at 7:36 PM Mimi Zohar wrote:
> >
> > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar 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
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/6] security/keys/encrypted: Clean up request_trusted_key()
On Wed, Mar 20, 2019 at 5:07 AM Mimi Zohar wrote:
>
> On Tue, 2019-03-19 at 22:48 -0700, Dan Williams wrote:
> > On Tue, Mar 19, 2019 at 7:36 PM Mimi Zohar wrote:
> > >
> > > On Tue, 2019-03-19 at 17:20 -0700, Dan Williams wrote:
> > > > On Tue, Mar 19, 2019 at 5:07 PM Mimi Zohar 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/
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
