| Message ID | 20180912142337.21955-2-jlee@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Encryption and authentication for hibernate snapshot image | expand |
Hi, On 9/12/18 7:23 AM, Lee, Chun-Yi wrote: > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig > index 3a6c2f87699e..7c5c30149dbc 100644 > --- a/kernel/power/Kconfig > +++ b/kernel/power/Kconfig > @@ -76,6 +76,20 @@ config HIBERNATION > > For more information take a look at <file:Documentation/power/swsusp.txt>. > > +config HIBERNATION_ENC_AUTH > + bool "Hibernation encryption and authentication" > + depends on HIBERNATION > + depends on TRUSTED_KEYS > + select CRYPTO_AES > + select CRYPTO_HMAC > + select CRYPTO_SHA512 > + help > + This option will encrypt and authenticate the memory snapshot image > + of hibernation. It prevents that the snapshot image be arbitrary arbitrarily > + modified. User can use TPMs trusted key or user defined key as the The user or A user can use the TPM's trusted key > + master key of hibernation. The TPM trusted key depends on TPM. The > + security of user defined key relies on user space. > +
Hi Randy, On Wed, Sep 12, 2018 at 09:27:27AM -0700, Randy Dunlap wrote: > Hi, > > On 9/12/18 7:23 AM, Lee, Chun-Yi wrote: > > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig > > index 3a6c2f87699e..7c5c30149dbc 100644 > > --- a/kernel/power/Kconfig > > +++ b/kernel/power/Kconfig > > @@ -76,6 +76,20 @@ config HIBERNATION > > > > For more information take a look at <file:Documentation/power/swsusp.txt>. > > > > +config HIBERNATION_ENC_AUTH > > + bool "Hibernation encryption and authentication" > > + depends on HIBERNATION > > + depends on TRUSTED_KEYS > > + select CRYPTO_AES > > + select CRYPTO_HMAC > > + select CRYPTO_SHA512 > > + help > > + This option will encrypt and authenticate the memory snapshot image > > + of hibernation. It prevents that the snapshot image be arbitrary > > arbitrarily > > > + modified. User can use TPMs trusted key or user defined key as the > > The user > or A user can use the TPM's trusted key > Thanks for your review! I will update it in next version. Joey Lee
On Wed, Sep 12, 2018 at 10:23:33PM +0800, Lee, Chun-Yi wrote: > This patch adds a snapshot keys handler for using the key retention > service api to create keys for snapshot image encryption and > authentication. > > This handler uses TPM trusted key as the snapshot master key, and the > encryption key and authentication key are derived from the snapshot > key. The user defined key can also be used as the snapshot master key > , but user must be aware that the security of user key relies on user > space. > In case the kernel provides mechanism to generate key from passphase, the master key could also be generated in kernel space other than TPM. It seems than snapshot_key_init() is easy to add the interface for that, right? > The name of snapshot key is fixed to "swsusp-kmk". User should use > the keyctl tool to load the key blob to root's user keyring. e.g. > > # /bin/keyctl add trusted swsusp-kmk "load `cat swsusp-kmk.blob`" @u > > or create a new user key. e.g. > > # /bin/keyctl add user swsusp-kmk password @u > > Then the disk_kmk sysfs file can be used to trigger the initialization > of snapshot key: > > # echo 1 > /sys/power/disk_kmk > > After the initialization be triggered, the secret in the payload of > swsusp-key will be copied by hibernation and be erased. Then user can > use keyctl to remove swsusp-kmk key from root's keyring. > > If user does not trigger the initialization by disk_kmk file after > swsusp-kmk be loaded to kernel. Then the snapshot key will be > initialled when hibernation be triggered. > > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Chen Yu <yu.c.chen@intel.com> > Cc: Oliver Neukum <oneukum@suse.com> > Cc: Ryan Chen <yu.chen.surf@gmail.com> > Cc: David Howells <dhowells@redhat.com> > Cc: Giovanni Gherdovich <ggherdovich@suse.cz> > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > --- > kernel/power/Kconfig | 14 +++ > kernel/power/Makefile | 1 + > kernel/power/hibernate.c | 36 +++++++ > kernel/power/power.h | 16 +++ > kernel/power/snapshot_key.c | 237 ++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 304 insertions(+) > create mode 100644 kernel/power/snapshot_key.c > > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig > index 3a6c2f87699e..7c5c30149dbc 100644 > --- a/kernel/power/Kconfig > +++ b/kernel/power/Kconfig > @@ -76,6 +76,20 @@ config HIBERNATION > > For more information take a look at <file:Documentation/power/swsusp.txt>. > > +config HIBERNATION_ENC_AUTH > + bool "Hibernation encryption and authentication" > + depends on HIBERNATION > + depends on TRUSTED_KEYS > + select CRYPTO_AES > + select CRYPTO_HMAC > + select CRYPTO_SHA512 > + help > + This option will encrypt and authenticate the memory snapshot image > + of hibernation. It prevents that the snapshot image be arbitrary > + modified. User can use TPMs trusted key or user defined key as the > + master key of hibernation. The TPM trusted key depends on TPM. The > + security of user defined key relies on user space. > + > config ARCH_SAVE_PAGE_KEYS > bool > > diff --git a/kernel/power/Makefile b/kernel/power/Makefile > index a3f79f0eef36..bddca7b79a28 100644 > --- a/kernel/power/Makefile > +++ b/kernel/power/Makefile > @@ -11,6 +11,7 @@ obj-$(CONFIG_FREEZER) += process.o > obj-$(CONFIG_SUSPEND) += suspend.o > obj-$(CONFIG_PM_TEST_SUSPEND) += suspend_test.o > obj-$(CONFIG_HIBERNATION) += hibernate.o snapshot.o swap.o user.o > +obj-$(CONFIG_HIBERNATION_ENC_AUTH) += snapshot_key.o > obj-$(CONFIG_PM_AUTOSLEEP) += autosleep.o > obj-$(CONFIG_PM_WAKELOCKS) += wakelock.o > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > index abef759de7c8..18d13cbf0591 100644 > --- a/kernel/power/hibernate.c > +++ b/kernel/power/hibernate.c > @@ -1034,6 +1034,39 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr, > > power_attr(disk); > > +#ifdef CONFIG_HIBERNATION_ENC_AUTH > +static ssize_t disk_kmk_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + if (snapshot_key_initialized()) > + return sprintf(buf, "initialized\n"); > + else > + return sprintf(buf, "uninitialized\n"); > +} > + > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, > + const char *buf, size_t n) > +{ Does kmk mean kernel master key? It might looks unclear from first glance, how about disk_genkey_store()? > + int error = 0; > + char *p; > + int len; > + > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > + p = memchr(buf, '\n', n); > + len = p ? p - buf : n; > + if (strncmp(buf, "1", len)) > + return -EINVAL; Why user is not allowed to disable(remove) it by echo 0 ? > + > + error = snapshot_key_init(); > + > + return error ? error : n; > +} > + > +power_attr(disk_kmk); > +#endif /* !CONFIG_HIBERNATION_ENC_AUTH */ > + > static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr, > char *buf) > { > @@ -1138,6 +1171,9 @@ power_attr(reserved_size); > > static struct attribute * g[] = { > &disk_attr.attr, > +#ifdef CONFIG_HIBERNATION_ENC_AUTH > + &disk_kmk_attr.attr, > +#endif > &resume_offset_attr.attr, > &resume_attr.attr, > &image_size_attr.attr, > diff --git a/kernel/power/power.h b/kernel/power/power.h > index 9e58bdc8a562..fe2dfa0d4d36 100644 > --- a/kernel/power/power.h > +++ b/kernel/power/power.h > @@ -4,6 +4,12 @@ > #include <linux/utsname.h> > #include <linux/freezer.h> > #include <linux/compiler.h> > +#include <crypto/sha.h> > + > +/* The max size of encrypted key blob */ > +#define KEY_BLOB_BUFF_LEN 512 > +#define SNAPSHOT_KEY_SIZE SHA512_DIGEST_SIZE > +#define DERIVED_KEY_SIZE SHA512_DIGEST_SIZE > > struct swsusp_info { > struct new_utsname uts; > @@ -20,6 +26,16 @@ struct swsusp_info { > extern void __init hibernate_reserved_size_init(void); > extern void __init hibernate_image_size_init(void); > > +#ifdef CONFIG_HIBERNATION_ENC_AUTH > +/* kernel/power/snapshot_key.c */ > +extern int snapshot_key_init(void); > +extern bool snapshot_key_initialized(void); > +extern int snapshot_get_auth_key(u8 *auth_key, bool may_sleep); > +extern int snapshot_get_enc_key(u8 *enc_key, bool may_sleep); > +#else > +static inline int snapshot_key_init(void) { return 0; } > +#endif /* !CONFIG_HIBERNATION_ENC_AUTH */ > + > #ifdef CONFIG_ARCH_HIBERNATION_HEADER > /* Maximum size of architecture specific data in a hibernation header */ > #define MAX_ARCH_HEADER_SIZE (sizeof(struct new_utsname) + 4) > diff --git a/kernel/power/snapshot_key.c b/kernel/power/snapshot_key.c > new file mode 100644 > index 000000000000..091f33929b47 > --- /dev/null > +++ b/kernel/power/snapshot_key.c > @@ -0,0 +1,237 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* snapshot keys handler > + * > + * Copyright (C) 2018 Lee, Chun-Yi <jlee@suse.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public Licence > + * as published by the Free Software Foundation; either version > + * 2 of the Licence, or (at your option) any later version. > + */ > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/cred.h> > +#include <linux/key-type.h> > +#include <linux/slab.h> > +#include <crypto/hash.h> > +#include <crypto/sha.h> > +#include <keys/trusted-type.h> > +#include <keys/user-type.h> > + > +#include "power.h" > + > +static const char hash_alg[] = "sha512"; > +static struct crypto_shash *hash_tfm; > + > +/* The master key of snapshot */ > +static struct snapshot_key { > + const char *key_name; > + bool initialized; > + unsigned int key_len; > + u8 key[SNAPSHOT_KEY_SIZE]; > +} skey = { > + .key_name = "swsusp-kmk", > +}; > + > +static int calc_hash(u8 *digest, const u8 *buf, unsigned int buflen, > + bool may_sleep) calc_hash() is used for both signature and encryption, could it be integrated in snapshot_key_init() thus the code could be re-used? > +{ > + SHASH_DESC_ON_STACK(desc, hash_tfm); Per commit c2cd0b08e1efd9ee58d09049a6c77e5efa0ef627 SHASH_DESC_ON_STACK() should not be used. > + int err; > + > + desc->tfm = hash_tfm; > + desc->flags = may_sleep ? CRYPTO_TFM_REQ_MAY_SLEEP : 0; > + > + err = crypto_shash_digest(desc, buf, buflen, digest); Check the err? > + shash_desc_zero(desc); > + return err; > +} > + > +static int calc_key_hash(u8 *key, unsigned int key_len, const char *salt, > + u8 *hash, bool may_sleep) > +{ > + unsigned int salted_buf_len; > + u8 *salted_buf; > + int ret; > + > + if (!key || !hash_tfm || !hash) > + return -EINVAL; > + > + salted_buf_len = strlen(salt) + 1 + SNAPSHOT_KEY_SIZE; > + salted_buf = kzalloc(salted_buf_len, > + may_sleep ? GFP_KERNEL : GFP_ATOMIC); > + if (!salted_buf) > + return -ENOMEM; > + > + strcpy(salted_buf, salt); > + memcpy(salted_buf + strlen(salted_buf) + 1, key, key_len); > + > + ret = calc_hash(hash, salted_buf, salted_buf_len, may_sleep); > + memzero_explicit(salted_buf, salted_buf_len); > + kzfree(salted_buf); > + > + return ret; > +} > + > +/* Derive authentication/encryption key */ > +static int get_derived_key(u8 *derived_key, const char *derived_type_str, > + bool may_sleep) > +{ > + int ret; > + > + if (!skey.initialized || !hash_tfm) > + return -EINVAL; > + > + ret = calc_key_hash(skey.key, skey.key_len, derived_type_str, > + derived_key, may_sleep); > + > + return ret; > +} > + > +int snapshot_get_auth_key(u8 *auth_key, bool may_sleep) > +{ > + return get_derived_key(auth_key, "AUTH_KEY", may_sleep); > +} > + > +int snapshot_get_enc_key(u8 *enc_key, bool may_sleep) > +{ > + return get_derived_key(enc_key, "ENC_KEY", may_sleep); > +} > + > +bool snapshot_key_initialized(void) > +{ > + return skey.initialized; > +} > + > +static bool invalid_key(u8 *key, unsigned int key_len) > +{ > + int i; > + > + if (!key || !key_len) > + return true; > + > + if (key_len > SNAPSHOT_KEY_SIZE) { > + pr_warn("Size of swsusp key more than: %d.\n", > + SNAPSHOT_KEY_SIZE); > + return true; > + } > + > + /* zero keyblob is invalid key */ > + for (i = 0; i < key_len; i++) { > + if (key[i] != 0) > + return false; > + } > + pr_warn("The swsusp key should not be zero.\n"); > + > + return true; > +} > + > +static int trusted_key_init(void) > +{ > + struct trusted_key_payload *tkp; > + struct key *key; > + int err; > + > + pr_debug("%s\n", __func__); > + > + /* find out swsusp-key */ > + key = request_key(&key_type_trusted, skey.key_name, NULL); > + if (IS_ERR(key)) { > + pr_err("Request key error: %ld\n", PTR_ERR(key)); > + err = PTR_ERR(key); > + return err; > + } > + > + down_write(&key->sem); > + tkp = key->payload.data[0]; > + if (invalid_key(tkp->key, tkp->key_len)) { > + err = -EINVAL; > + goto key_invalid; > + } > + skey.key_len = tkp->key_len; > + memcpy(skey.key, tkp->key, tkp->key_len); > + /* burn the original key contents */ > + memzero_explicit(tkp->key, tkp->key_len); > + > +key_invalid: > + up_write(&key->sem); > + key_put(key); > + > + return err; > +} > + > +static int user_key_init(void) > +{ > + struct user_key_payload *ukp; > + struct key *key; > + int err = 0; > + > + pr_debug("%s\n", __func__); > + > + /* find out swsusp-key */ > + key = request_key(&key_type_user, skey.key_name, NULL); > + if (IS_ERR(key)) { > + pr_err("Request key error: %ld\n", PTR_ERR(key)); > + err = PTR_ERR(key); > + return err; > + } > + > + down_write(&key->sem); > + ukp = user_key_payload_locked(key); > + if (!ukp) { > + /* key was revoked before we acquired its semaphore */ > + err = -EKEYREVOKED; > + goto key_invalid; > + } > + if (invalid_key(ukp->data, ukp->datalen)) { > + err = -EINVAL; > + goto key_invalid; > + } > + skey.key_len = ukp->datalen; > + memcpy(skey.key, ukp->data, ukp->datalen); > + /* burn the original key contents */ > + memzero_explicit(ukp->data, ukp->datalen); > + > +key_invalid: > + up_write(&key->sem); > + key_put(key); > + > + return err; > +} > + > +/* this function may sleeps */ > +int snapshot_key_init(void) > +{ > + int err; > + > + pr_debug("%s\n", __func__); > + > + if (skey.initialized) > + return 0; > + > + hash_tfm = crypto_alloc_shash(hash_alg, 0, CRYPTO_ALG_ASYNC); > + if (IS_ERR(hash_tfm)) { > + pr_err("Can't allocate %s transform: %ld\n", > + hash_alg, PTR_ERR(hash_tfm)); > + return PTR_ERR(hash_tfm); > + } > + > + err = trusted_key_init(); > + if (err) > + err = user_key_init(); > + if (err) > + goto key_fail; > + > + skey.initialized = true; > + > + pr_info("Snapshot key is initialled.\n"); > + > + return 0; > + > +key_fail: > + crypto_free_shash(hash_tfm); > + hash_tfm = NULL; > + > + return err; > +} > -- > 2.13.6 >
+cc keyrings list On Thu, Sep 13, 2018 at 4:08 PM Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > This patch adds a snapshot keys handler for using the key retention > service api to create keys for snapshot image encryption and > authentication. > > This handler uses TPM trusted key as the snapshot master key, and the > encryption key and authentication key are derived from the snapshot > key. The user defined key can also be used as the snapshot master key > , but user must be aware that the security of user key relies on user > space. > > The name of snapshot key is fixed to "swsusp-kmk". User should use > the keyctl tool to load the key blob to root's user keyring. e.g. > > # /bin/keyctl add trusted swsusp-kmk "load `cat swsusp-kmk.blob`" @u > > or create a new user key. e.g. > > # /bin/keyctl add user swsusp-kmk password @u > > Then the disk_kmk sysfs file can be used to trigger the initialization > of snapshot key: > > # echo 1 > /sys/power/disk_kmk > > After the initialization be triggered, the secret in the payload of > swsusp-key will be copied by hibernation and be erased. Then user can > use keyctl to remove swsusp-kmk key from root's keyring. > > If user does not trigger the initialization by disk_kmk file after > swsusp-kmk be loaded to kernel. Then the snapshot key will be > initialled when hibernation be triggered. [...] > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, > + const char *buf, size_t n) > +{ > + int error = 0; > + char *p; > + int len; > + > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; This is wrong, you can't use capable() in a write handler. You'd have to use file_ns_capable(), and I think sysfs currently doesn't give you a pointer to the struct file. If you want to do this in a write handler, you'll have to either get rid of this check or plumb through the cred struct pointer. Alternatively, you could use some interface that doesn't go through a write handler. > + p = memchr(buf, '\n', n); > + len = p ? p - buf : n; > + if (strncmp(buf, "1", len)) > + return -EINVAL; > + > + error = snapshot_key_init(); > + > + return error ? error : n; > +} > + [...] > + > +static int user_key_init(void) > +{ > + struct user_key_payload *ukp; > + struct key *key; > + int err = 0; > + > + pr_debug("%s\n", __func__); > + > + /* find out swsusp-key */ > + key = request_key(&key_type_user, skey.key_name, NULL); request_key() looks at current's keyring. That shouldn't happen in a write handler. > + if (IS_ERR(key)) { > + pr_err("Request key error: %ld\n", PTR_ERR(key)); > + err = PTR_ERR(key); > + return err; > + } > + > + down_write(&key->sem); > + ukp = user_key_payload_locked(key); > + if (!ukp) { > + /* key was revoked before we acquired its semaphore */ > + err = -EKEYREVOKED; > + goto key_invalid; > + } > + if (invalid_key(ukp->data, ukp->datalen)) { > + err = -EINVAL; > + goto key_invalid; > + } > + skey.key_len = ukp->datalen; > + memcpy(skey.key, ukp->data, ukp->datalen); > + /* burn the original key contents */ > + memzero_explicit(ukp->data, ukp->datalen); You just zero out the contents of the supplied key? That seems very unidiomatic for the keys subsystem, and makes me wonder why you're using the keys subsystem for this in the first place. It doesn't look like normal use of the keys subsystem. > +key_invalid: > + up_write(&key->sem); > + key_put(key); > + > + return err; > +} > + > +/* this function may sleeps */ > +int snapshot_key_init(void) > +{ > + int err; > + > + pr_debug("%s\n", __func__); > + > + if (skey.initialized) > + return 0; > + > + hash_tfm = crypto_alloc_shash(hash_alg, 0, CRYPTO_ALG_ASYNC); > + if (IS_ERR(hash_tfm)) { > + pr_err("Can't allocate %s transform: %ld\n", > + hash_alg, PTR_ERR(hash_tfm)); > + return PTR_ERR(hash_tfm); > + } > + > + err = trusted_key_init(); > + if (err) > + err = user_key_init(); > + if (err) > + goto key_fail; > + > + skey.initialized = true; Does this need a memory barrier to prevent reordering of the "skey.initialized = true" assignment before the key is fully initialized? > + > + pr_info("Snapshot key is initialled.\n"); > + > + return 0; > + > +key_fail: > + crypto_free_shash(hash_tfm); > + hash_tfm = NULL; > + > + return err; > +} > -- > 2.13.6 > >
Hi Chun-Yi,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.19-rc3 next-20180913]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Lee-Chun-Yi/Encryption-and-authentication-for-hibernate-snapshot-image/20180914-031757
config: powerpc64-allmodconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=powerpc64
All errors (new ones prefixed by >>):
powerpc64-linux-gnu-ld: warning: orphan section `.gnu.hash' from `linker stubs' being placed in section `.gnu.hash'.
>> kernel/power/snapshot_key.o:(.toc+0x0): undefined reference to `key_type_trusted'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Yu Chen, Thanks for your review and very sorry for my delay! On Thu, Sep 13, 2018 at 09:58:32PM +0800, Yu Chen wrote: > On Wed, Sep 12, 2018 at 10:23:33PM +0800, Lee, Chun-Yi wrote: > > This patch adds a snapshot keys handler for using the key retention > > service api to create keys for snapshot image encryption and > > authentication. > > > > This handler uses TPM trusted key as the snapshot master key, and the > > encryption key and authentication key are derived from the snapshot > > key. The user defined key can also be used as the snapshot master key > > , but user must be aware that the security of user key relies on user > > space. > > > In case the kernel provides mechanism to generate key from passphase, > the master key could also be generated in kernel space other than TPM. > It seems than snapshot_key_init() is easy to add the interface for that, > right? The user defined key can be used to carry the blob from user space. User sapce can use keyctl tool to enroll the blob. We can design a structure of blob that it carries the hash of passphase, salt... so on. Then kernel parses the blob to generate the key for encryption/authentication. > > The name of snapshot key is fixed to "swsusp-kmk". User should use > > the keyctl tool to load the key blob to root's user keyring. e.g. > > > > # /bin/keyctl add trusted swsusp-kmk "load `cat swsusp-kmk.blob`" @u > > > > or create a new user key. e.g. > > > > # /bin/keyctl add user swsusp-kmk password @u The above password can be a blob. The user defined key can carries the blob to kernel. We can design the blob with userland tool later. > > > > Then the disk_kmk sysfs file can be used to trigger the initialization > > of snapshot key: > > > > # echo 1 > /sys/power/disk_kmk > > > > After the initialization be triggered, the secret in the payload of > > swsusp-key will be copied by hibernation and be erased. Then user can > > use keyctl to remove swsusp-kmk key from root's keyring. > > > > If user does not trigger the initialization by disk_kmk file after > > swsusp-kmk be loaded to kernel. Then the snapshot key will be > > initialled when hibernation be triggered. > > > > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> > > Cc: Pavel Machek <pavel@ucw.cz> > > Cc: Chen Yu <yu.c.chen@intel.com> > > Cc: Oliver Neukum <oneukum@suse.com> > > Cc: Ryan Chen <yu.chen.surf@gmail.com> > > Cc: David Howells <dhowells@redhat.com> > > Cc: Giovanni Gherdovich <ggherdovich@suse.cz> > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > > --- [...snip] > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > > index abef759de7c8..18d13cbf0591 100644 > > --- a/kernel/power/hibernate.c > > +++ b/kernel/power/hibernate.c > > @@ -1034,6 +1034,39 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr, > > > > power_attr(disk); > > > > +#ifdef CONFIG_HIBERNATION_ENC_AUTH > > +static ssize_t disk_kmk_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf) > > +{ > > + if (snapshot_key_initialized()) > > + return sprintf(buf, "initialized\n"); > > + else > > + return sprintf(buf, "uninitialized\n"); > > +} > > + > > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, > > + const char *buf, size_t n) > > +{ > Does kmk mean kernel master key? It might looks unclear from first glance, > how about disk_genkey_store()? Yes, it's the kernel maskter key for disk (hibernate). The sysfs interfaces is used to load the master key from keyring, then kernel parses the encrypted key or user defined key to grab the plaintext key. So sysfs triggered the initial process of the master key. Even user space didn't trigger the process through sysfs, kernel will still runs the initial process when hibernation be triggered. Then kernel uses the master key to derive encrypte key and authenticate key. I prefer to keep the name of sysfs and the function name. > > + int error = 0; > > + char *p; > > + int len; > > + > > + if (!capable(CAP_SYS_ADMIN)) > > + return -EPERM; > > + > > + p = memchr(buf, '\n', n); > > + len = p ? p - buf : n; > > + if (strncmp(buf, "1", len)) > > + return -EINVAL; > Why user is not allowed to disable(remove) it by > echo 0 ? Similar with evm, this sysfs interface gives user space a chance to ask kernel to initial master key. Once the master key be initialled and loaded, kernel doesn't want the key to be removed because the key is unsealed by TPM. The PCRs may already capped then there have no other master key can be unsealed and enrolled. So, I prefer that do not give user space the function to remove an enrolled master key. Once the master key be loaded, kernel will use the key to encrypt snapshot image. > > + > > + error = snapshot_key_init(); > > + > > + return error ? error : n; > > +} > > + > > +power_attr(disk_kmk); > > +#endif /* !CONFIG_HIBERNATION_ENC_AUTH */ [...snip] > > diff --git a/kernel/power/snapshot_key.c b/kernel/power/snapshot_key.c > > new file mode 100644 > > index 000000000000..091f33929b47 > > --- /dev/null > > +++ b/kernel/power/snapshot_key.c [...snip] > > + > > +static int calc_hash(u8 *digest, const u8 *buf, unsigned int buflen, > > + bool may_sleep) > calc_hash() is used for both signature and encryption, > could it be integrated in snapshot_key_init() thus > the code could be re-used? The snapshot_key_init() is used to parse the blob of master key for grabbing the plaintext of key. It doesn't relate to encryption key and authentication key. The snapshot_key_init() reuses calc_hash() to calculate the fingerprint of master key in 0004 patch. The get_key_fingerprint() is a wrapper of calc_hash(). > > +{ > > + SHASH_DESC_ON_STACK(desc, hash_tfm); > Per commit c2cd0b08e1efd9ee58d09049a6c77e5efa0ef627 > SHASH_DESC_ON_STACK() should not be used. Thank you for point out! I will avoid to use VLA in next version. > > + int err; > > + > > + desc->tfm = hash_tfm; > > + desc->flags = may_sleep ? CRYPTO_TFM_REQ_MAY_SLEEP : 0; > > + > > + err = crypto_shash_digest(desc, buf, buflen, digest); > Check the err? I will check the err, thanks! > > + shash_desc_zero(desc); > > + return err; > > +} > > + [...snip] Thanks a lot! Joey Lee
Hi Jann, Thanks for your review and very sorry for my delay! On Thu, Sep 13, 2018 at 04:31:18PM +0200, Jann Horn wrote: > +cc keyrings list > > On Thu, Sep 13, 2018 at 4:08 PM Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > > This patch adds a snapshot keys handler for using the key retention > > service api to create keys for snapshot image encryption and > > authentication. [...snip] > > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, > > + const char *buf, size_t n) > > +{ > > + int error = 0; > > + char *p; > > + int len; > > + > > + if (!capable(CAP_SYS_ADMIN)) > > + return -EPERM; > > This is wrong, you can't use capable() in a write handler. You'd have > to use file_ns_capable(), and I think sysfs currently doesn't give you > a pointer to the struct file. > If you want to do this in a write handler, you'll have to either get > rid of this check or plumb through the cred struct pointer. > Alternatively, you could use some interface that doesn't go through a > write handler. > Thank you for point out this problem. Actually the evm_write_key() is the example for my code. The difference is that evm creates interface file on securityfs, but my implementation is on sysfs: security/integrity/evm/evm_secfs.c static ssize_t evm_write_key(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { int i, ret; if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_SETUP)) return -EPERM; ... On the other hand, the writing handler of /sys/power/wake_lock also uses capable() to check the CAP_BLOCK_SUSPEND capability: kernel/power/main.c static ssize_t wake_lock_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t n) { int error = pm_wake_lock(buf); return error ? error : n; } power_attr(wake_lock); kernel/power/wakelock.c int pm_wake_lock(const char *buf) { ... if (!capable(CAP_BLOCK_SUSPEND)) return -EPERM; ... So I confused for when can capable() be used in sysfs interface? Is capable() only allowed in reading handler? Why the writing handler of securityfs can use capable()? > > + > > +static int user_key_init(void) > > +{ > > + struct user_key_payload *ukp; > > + struct key *key; > > + int err = 0; > > + > > + pr_debug("%s\n", __func__); > > + > > + /* find out swsusp-key */ > > + key = request_key(&key_type_user, skey.key_name, NULL); > > request_key() looks at current's keyring. That shouldn't happen in a > write handler. > The evm_write_key() also uses request_key() but it's on securityfs. Should I move my sysfs interface to securityfs? > > + if (IS_ERR(key)) { > > + pr_err("Request key error: %ld\n", PTR_ERR(key)); > > + err = PTR_ERR(key); > > + return err; > > + } > > + > > + down_write(&key->sem); > > + ukp = user_key_payload_locked(key); > > + if (!ukp) { > > + /* key was revoked before we acquired its semaphore */ > > + err = -EKEYREVOKED; > > + goto key_invalid; > > + } > > + if (invalid_key(ukp->data, ukp->datalen)) { > > + err = -EINVAL; > > + goto key_invalid; > > + } > > + skey.key_len = ukp->datalen; > > + memcpy(skey.key, ukp->data, ukp->datalen); > > + /* burn the original key contents */ > > + memzero_explicit(ukp->data, ukp->datalen); > > You just zero out the contents of the supplied key? That seems very > unidiomatic for the keys subsystem, and makes me wonder why you're > using the keys subsystem for this in the first place. It doesn't look > like normal use of the keys subsystem. > Because I want that only one decrypted key in kernel memory. Then hibernation can handle the key more easy. In evm_init_key(), it also burned the key contents after evm key be initialled: security/integrity/evm/evm_crypto.c int evm_init_key(void) { [...snip] /* burn the original key contents */ memset(ekp->decrypted_data, 0, ekp->decrypted_datalen); up_read(&evm_key->sem); key_put(evm_key); return rc; } The keys subsystem already handles the interactive with userland and TPM. That's the reason for using keys subsystem in hibernation. > > +key_invalid: > > + up_write(&key->sem); > > + key_put(key); > > + > > + return err; > > +} > > + > > +/* this function may sleeps */ > > +int snapshot_key_init(void) > > +{ > > + int err; > > + > > + pr_debug("%s\n", __func__); > > + > > + if (skey.initialized) > > + return 0; > > + > > + hash_tfm = crypto_alloc_shash(hash_alg, 0, CRYPTO_ALG_ASYNC); > > + if (IS_ERR(hash_tfm)) { > > + pr_err("Can't allocate %s transform: %ld\n", > > + hash_alg, PTR_ERR(hash_tfm)); > > + return PTR_ERR(hash_tfm); > > + } > > + > > + err = trusted_key_init(); > > + if (err) > > + err = user_key_init(); > > + if (err) > > + goto key_fail; > > + > > + skey.initialized = true; > > Does this need a memory barrier to prevent reordering of the > "skey.initialized = true" assignment before the key is fully > initialized? > Thanks for your reminding. I will add memory barrier here. Thank a lot! Joey Lee
+Andy for opinions on things in write handlers +Mimi Zohar as EVM maintainer On Tue, Oct 2, 2018 at 9:55 AM joeyli <jlee@suse.com> wrote: > On Thu, Sep 13, 2018 at 04:31:18PM +0200, Jann Horn wrote: > > On Thu, Sep 13, 2018 at 4:08 PM Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > > > This patch adds a snapshot keys handler for using the key retention > > > service api to create keys for snapshot image encryption and > > > authentication. > [...snip] > > > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, > > > + const char *buf, size_t n) > > > +{ > > > + int error = 0; > > > + char *p; > > > + int len; > > > + > > > + if (!capable(CAP_SYS_ADMIN)) > > > + return -EPERM; > > > > This is wrong, you can't use capable() in a write handler. You'd have > > to use file_ns_capable(), and I think sysfs currently doesn't give you > > a pointer to the struct file. > > If you want to do this in a write handler, you'll have to either get > > rid of this check or plumb through the cred struct pointer. > > Alternatively, you could use some interface that doesn't go through a > > write handler. > > > > Thank you for point out this problem. > > Actually the evm_write_key() is the example for my code. The > difference is that evm creates interface file on securityfs, but my > implementation is on sysfs: > > security/integrity/evm/evm_secfs.c > > static ssize_t evm_write_key(struct file *file, const char __user *buf, > size_t count, loff_t *ppos) > { > int i, ret; > > if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_SETUP)) > return -EPERM; > ... > > On the other hand, the writing handler of /sys/power/wake_lock also > uses capable() to check the CAP_BLOCK_SUSPEND capability: > > kernel/power/main.c > static ssize_t wake_lock_store(struct kobject *kobj, > struct kobj_attribute *attr, > const char *buf, size_t n) > { > int error = pm_wake_lock(buf); > return error ? error : n; > } > power_attr(wake_lock); > > kernel/power/wakelock.c > int pm_wake_lock(const char *buf) > { > ... > if (!capable(CAP_BLOCK_SUSPEND)) > return -EPERM; > ... > > > So I confused for when can capable() be used in sysfs interface? Is > capable() only allowed in reading handler? Why the writing handler > of securityfs can use capable()? They can't, they're all wrong. :P All of these capable() checks in write handlers have to be assumed to be ineffective. But it's not a big deal because you still need UID 0 to access these files. > > > +static int user_key_init(void) > > > +{ > > > + struct user_key_payload *ukp; > > > + struct key *key; > > > + int err = 0; > > > + > > > + pr_debug("%s\n", __func__); > > > + > > > + /* find out swsusp-key */ > > > + key = request_key(&key_type_user, skey.key_name, NULL); > > > > request_key() looks at current's keyring. That shouldn't happen in a > > write handler. > > > > The evm_write_key() also uses request_key() but it's on securityfs. Should > I move my sysfs interface to securityfs? I don't think you should be doing this in the context of any filesystem. If EVM does that, EVM is doing it wrong. > > > + if (IS_ERR(key)) { > > > + pr_err("Request key error: %ld\n", PTR_ERR(key)); > > > + err = PTR_ERR(key); > > > + return err; > > > + } > > > + > > > + down_write(&key->sem); > > > + ukp = user_key_payload_locked(key); > > > + if (!ukp) { > > > + /* key was revoked before we acquired its semaphore */ > > > + err = -EKEYREVOKED; > > > + goto key_invalid; > > > + } > > > + if (invalid_key(ukp->data, ukp->datalen)) { > > > + err = -EINVAL; > > > + goto key_invalid; > > > + } > > > + skey.key_len = ukp->datalen; > > > + memcpy(skey.key, ukp->data, ukp->datalen); > > > + /* burn the original key contents */ > > > + memzero_explicit(ukp->data, ukp->datalen); > > > > You just zero out the contents of the supplied key? That seems very > > unidiomatic for the keys subsystem, and makes me wonder why you're > > using the keys subsystem for this in the first place. It doesn't look > > like normal use of the keys subsystem. > > > > Because I want that only one decrypted key in kernel memory. Then hibernation > can handle the key more easy. In evm_init_key(), it also burned the key > contents after evm key be initialled: > > security/integrity/evm/evm_crypto.c > int evm_init_key(void) > { > [...snip] > /* burn the original key contents */ > memset(ekp->decrypted_data, 0, ekp->decrypted_datalen); > up_read(&evm_key->sem); > key_put(evm_key); > return rc; > } > > The keys subsystem already handles the interactive with userland and TPM. > That's the reason for using keys subsystem in hibernation. How do you guarantee that the user is allowed to overwrite that key? I don't understand the keys subsystem very well - could this be a key on the trusted keyring, or something like that?
On Tue, Oct 2, 2018 at 12:36 PM Jann Horn <jannh@google.com> wrote: > > +Andy for opinions on things in write handlers > +Mimi Zohar as EVM maintainer > > On Tue, Oct 2, 2018 at 9:55 AM joeyli <jlee@suse.com> wrote: > > On Thu, Sep 13, 2018 at 04:31:18PM +0200, Jann Horn wrote: > > > On Thu, Sep 13, 2018 at 4:08 PM Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > > > > This patch adds a snapshot keys handler for using the key retention > > > > service api to create keys for snapshot image encryption and > > > > authentication. > > [...snip] > > > > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, > > > > + const char *buf, size_t n) > > > > +{ > > > > + int error = 0; > > > > + char *p; > > > > + int len; > > > > + > > > > + if (!capable(CAP_SYS_ADMIN)) > > > > + return -EPERM; > > > > > > This is wrong, you can't use capable() in a write handler. You'd have > > > to use file_ns_capable(), and I think sysfs currently doesn't give you > > > a pointer to the struct file. > > > If you want to do this in a write handler, you'll have to either get > > > rid of this check or plumb through the cred struct pointer. > > > Alternatively, you could use some interface that doesn't go through a > > > write handler. > > > > > > > Thank you for point out this problem. > > > > Actually the evm_write_key() is the example for my code. The > > difference is that evm creates interface file on securityfs, but my > > implementation is on sysfs: > > > > security/integrity/evm/evm_secfs.c > > > > static ssize_t evm_write_key(struct file *file, const char __user *buf, > > size_t count, loff_t *ppos) > > { > > int i, ret; > > > > if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_SETUP)) > > return -EPERM; Yeah, that's a bug. > > ... > > > > On the other hand, the writing handler of /sys/power/wake_lock also > > uses capable() to check the CAP_BLOCK_SUSPEND capability: > > > > kernel/power/main.c > > static ssize_t wake_lock_store(struct kobject *kobj, > > struct kobj_attribute *attr, > > const char *buf, size_t n) > > { > > int error = pm_wake_lock(buf); > > return error ? error : n; > > } > > power_attr(wake_lock); > > > > kernel/power/wakelock.c > > int pm_wake_lock(const char *buf) > > { > > ... > > if (!capable(CAP_BLOCK_SUSPEND)) > > return -EPERM; > > ... Also a bug. > > > > > > So I confused for when can capable() be used in sysfs interface? Is > > capable() only allowed in reading handler? Why the writing handler > > of securityfs can use capable()? > > They can't, they're all wrong. :P All of these capable() checks in > write handlers have to be assumed to be ineffective. But it's not a > big deal because you still need UID 0 to access these files. Why are there capability checks at all? > > > > > +static int user_key_init(void) > > > > +{ > > > > + struct user_key_payload *ukp; > > > > + struct key *key; > > > > + int err = 0; > > > > + > > > > + pr_debug("%s\n", __func__); > > > > + > > > > + /* find out swsusp-key */ > > > > + key = request_key(&key_type_user, skey.key_name, NULL); > > > > > > request_key() looks at current's keyring. That shouldn't happen in a > > > write handler. > > > > > > > The evm_write_key() also uses request_key() but it's on securityfs. Should > > I move my sysfs interface to securityfs? > > I don't think you should be doing this in the context of any > filesystem. If EVM does that, EVM is doing it wrong. EVM sounds buggy. In general if you look at current *at all* in an implementation of write() *in any filesystem*, you are doing it wrong.
On Tue, 2018-10-02 at 21:36 +0200, Jann Horn wrote: > +Andy for opinions on things in write handlers > +Mimi Zohar as EVM maintainer > > On Tue, Oct 2, 2018 at 9:55 AM joeyli <jlee@suse.com> wrote: > > On Thu, Sep 13, 2018 at 04:31:18PM +0200, Jann Horn wrote: > > > On Thu, Sep 13, 2018 at 4:08 PM Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > > > > This patch adds a snapshot keys handler for using the key retention > > > > service api to create keys for snapshot image encryption and > > > > authentication. > > [...snip] > > > > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, > > > > + const char *buf, size_t n) > > > > +{ > > > > + int error = 0; > > > > + char *p; > > > > + int len; > > > > + > > > > + if (!capable(CAP_SYS_ADMIN)) > > > > + return -EPERM; > > > > > > This is wrong, you can't use capable() in a write handler. You'd have > > > to use file_ns_capable(), and I think sysfs currently doesn't give you > > > a pointer to the struct file. > > > If you want to do this in a write handler, you'll have to either get > > > rid of this check or plumb through the cred struct pointer. > > > Alternatively, you could use some interface that doesn't go through a > > > write handler. > > > > > > > Thank you for point out this problem. > > > > Actually the evm_write_key() is the example for my code. The > > difference is that evm creates interface file on securityfs, but my > > implementation is on sysfs: > > > > security/integrity/evm/evm_secfs.c > > > > static ssize_t evm_write_key(struct file *file, const char __user *buf, > > size_t count, loff_t *ppos) > > { > > int i, ret; > > > > if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_SETUP)) > > return -EPERM; > > ... > > > > On the other hand, the writing handler of /sys/power/wake_lock also > > uses capable() to check the CAP_BLOCK_SUSPEND capability: > > > > kernel/power/main.c > > static ssize_t wake_lock_store(struct kobject *kobj, > > struct kobj_attribute *attr, > > const char *buf, size_t n) > > { > > int error = pm_wake_lock(buf); > > return error ? error : n; > > } > > power_attr(wake_lock); > > > > kernel/power/wakelock.c > > int pm_wake_lock(const char *buf) > > { > > ... > > if (!capable(CAP_BLOCK_SUSPEND)) > > return -EPERM; > > ... > > > > > > So I confused for when can capable() be used in sysfs interface? Is > > capable() only allowed in reading handler? Why the writing handler > > of securityfs can use capable()? > > They can't, they're all wrong. :P All of these capable() checks in > write handlers have to be assumed to be ineffective. But it's not a > big deal because you still need UID 0 to access these files. Thanks, Jann. At least EVM should be updated to replace the existing "capable" check with "if (file_ns_capable(file, &init_user_ns, CAP_SYS_ADMIN))". > > > > > +static int user_key_init(void) > > > > +{ > > > > + struct user_key_payload *ukp; > > > > + struct key *key; > > > > + int err = 0; > > > > + > > > > + pr_debug("%s\n", __func__); > > > > + > > > > + /* find out swsusp-key */ > > > > + key = request_key(&key_type_user, skey.key_name, NULL); > > > > > > request_key() looks at current's keyring. That shouldn't happen in a > > > write handler. > > > > > > > The evm_write_key() also uses request_key() but it's on securityfs. Should > > I move my sysfs interface to securityfs? > > I don't think you should be doing this in the context of any > filesystem. If EVM does that, EVM is doing it wrong. This is being executed in the initramfs before pivoting root. > > > > > + if (IS_ERR(key)) { > > > > + pr_err("Request key error: %ld\n", PTR_ERR(key)); > > > > + err = PTR_ERR(key); > > > > + return err; > > > > + } > > > > + > > > > + down_write(&key->sem); > > > > + ukp = user_key_payload_locked(key); > > > > + if (!ukp) { > > > > + /* key was revoked before we acquired its semaphore */ > > > > + err = -EKEYREVOKED; > > > > + goto key_invalid; > > > > + } > > > > + if (invalid_key(ukp->data, ukp->datalen)) { > > > > + err = -EINVAL; > > > > + goto key_invalid; > > > > + } > > > > + skey.key_len = ukp->datalen; > > > > + memcpy(skey.key, ukp->data, ukp->datalen); > > > > + /* burn the original key contents */ > > > > + memzero_explicit(ukp->data, ukp->datalen); > > > > > > You just zero out the contents of the supplied key? That seems very > > > unidiomatic for the keys subsystem, and makes me wonder why you're > > > using the keys subsystem for this in the first place. It doesn't look > > > like normal use of the keys subsystem. > > > Right, evm_write_key() is normally called from the initramfs to signal the kernel that the EVM encrypted key has been loaded onto root's keyring. The decrypted EVM key is then copied to kernel memory. Before returning, the decrypted EVM key on root's keyring is cleared. The initramfs then removes the EVM key from the keyring. For more details about trusted/encrypted keys refer to Documentation/security/keys/trusted-encrypted.rst. Mimi > > Because I want that only one decrypted key in kernel memory. Then hibernation > > can handle the key more easy. In evm_init_key(), it also burned the key > > contents after evm key be initialled: > > > > security/integrity/evm/evm_crypto.c > > int evm_init_key(void) > > { > > [...snip] > > /* burn the original key contents */ > > memset(ekp->decrypted_data, 0, ekp->decrypted_datalen); > > up_read(&evm_key->sem); > > key_put(evm_key); > > return rc; > > } > > > > The keys subsystem already handles the interactive with userland and TPM. > > That's the reason for using keys subsystem in hibernation. > > How do you guarantee that the user is allowed to overwrite that key? I > don't understand the keys subsystem very well - could this be a key on > the trusted keyring, or something like that? >
Hi Any, Jann, On Wed, Oct 03, 2018 at 03:08:12PM -0700, Andy Lutomirski wrote: > On Tue, Oct 2, 2018 at 12:36 PM Jann Horn <jannh@google.com> wrote: > > > > +Andy for opinions on things in write handlers > > +Mimi Zohar as EVM maintainer > > > > On Tue, Oct 2, 2018 at 9:55 AM joeyli <jlee@suse.com> wrote: > > > On Thu, Sep 13, 2018 at 04:31:18PM +0200, Jann Horn wrote: > > > > On Thu, Sep 13, 2018 at 4:08 PM Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote: > > > > > This patch adds a snapshot keys handler for using the key retention > > > > > service api to create keys for snapshot image encryption and > > > > > authentication. > > > [...snip] > > > > > +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, > > > > > + const char *buf, size_t n) > > > > > +{ > > > > > + int error = 0; > > > > > + char *p; > > > > > + int len; > > > > > + > > > > > + if (!capable(CAP_SYS_ADMIN)) > > > > > + return -EPERM; > > > > > > > > This is wrong, you can't use capable() in a write handler. You'd have > > > > to use file_ns_capable(), and I think sysfs currently doesn't give you > > > > a pointer to the struct file. > > > > If you want to do this in a write handler, you'll have to either get > > > > rid of this check or plumb through the cred struct pointer. > > > > Alternatively, you could use some interface that doesn't go through a > > > > write handler. > > > > > > > > > > Thank you for point out this problem. > > > > > > Actually the evm_write_key() is the example for my code. The > > > difference is that evm creates interface file on securityfs, but my > > > implementation is on sysfs: > > > > > > security/integrity/evm/evm_secfs.c > > > > > > static ssize_t evm_write_key(struct file *file, const char __user *buf, > > > size_t count, loff_t *ppos) > > > { > > > int i, ret; > > > > > > if (!capable(CAP_SYS_ADMIN) || (evm_initialized & EVM_SETUP)) > > > return -EPERM; > > Yeah, that's a bug. > > > > ... > > > > > > On the other hand, the writing handler of /sys/power/wake_lock also > > > uses capable() to check the CAP_BLOCK_SUSPEND capability: > > > > > > kernel/power/main.c > > > static ssize_t wake_lock_store(struct kobject *kobj, > > > struct kobj_attribute *attr, > > > const char *buf, size_t n) > > > { > > > int error = pm_wake_lock(buf); > > > return error ? error : n; > > > } > > > power_attr(wake_lock); > > > > > > kernel/power/wakelock.c > > > int pm_wake_lock(const char *buf) > > > { > > > ... > > > if (!capable(CAP_BLOCK_SUSPEND)) > > > return -EPERM; > > > ... > > Also a bug. > > > > > > > > > > So I confused for when can capable() be used in sysfs interface? Is > > > capable() only allowed in reading handler? Why the writing handler > > > of securityfs can use capable()? > > > > They can't, they're all wrong. :P All of these capable() checks in > > write handlers have to be assumed to be ineffective. But it's not a > > big deal because you still need UID 0 to access these files. > > Why are there capability checks at all? > > > > > > > > +static int user_key_init(void) > > > > > +{ > > > > > + struct user_key_payload *ukp; > > > > > + struct key *key; > > > > > + int err = 0; > > > > > + > > > > > + pr_debug("%s\n", __func__); > > > > > + > > > > > + /* find out swsusp-key */ > > > > > + key = request_key(&key_type_user, skey.key_name, NULL); > > > > > > > > request_key() looks at current's keyring. That shouldn't happen in a > > > > write handler. > > > > > > > > > > The evm_write_key() also uses request_key() but it's on securityfs. Should > > > I move my sysfs interface to securityfs? > > > > I don't think you should be doing this in the context of any > > filesystem. If EVM does that, EVM is doing it wrong. > > EVM sounds buggy. > > In general if you look at current *at all* in an implementation of > write() *in any filesystem*, you are doing it wrong. I have read CVE-2013-1959... Thanks for Jann and Andy's review. I will create the sysfs interface through other way, then using file_ns_capable() for capability checking in next version. Thanks a lot! Joey Lee
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig index 3a6c2f87699e..7c5c30149dbc 100644 --- a/kernel/power/Kconfig +++ b/kernel/power/Kconfig @@ -76,6 +76,20 @@ config HIBERNATION For more information take a look at <file:Documentation/power/swsusp.txt>. +config HIBERNATION_ENC_AUTH + bool "Hibernation encryption and authentication" + depends on HIBERNATION + depends on TRUSTED_KEYS + select CRYPTO_AES + select CRYPTO_HMAC + select CRYPTO_SHA512 + help + This option will encrypt and authenticate the memory snapshot image + of hibernation. It prevents that the snapshot image be arbitrary + modified. User can use TPMs trusted key or user defined key as the + master key of hibernation. The TPM trusted key depends on TPM. The + security of user defined key relies on user space. + config ARCH_SAVE_PAGE_KEYS bool diff --git a/kernel/power/Makefile b/kernel/power/Makefile index a3f79f0eef36..bddca7b79a28 100644 --- a/kernel/power/Makefile +++ b/kernel/power/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_FREEZER) += process.o obj-$(CONFIG_SUSPEND) += suspend.o obj-$(CONFIG_PM_TEST_SUSPEND) += suspend_test.o obj-$(CONFIG_HIBERNATION) += hibernate.o snapshot.o swap.o user.o +obj-$(CONFIG_HIBERNATION_ENC_AUTH) += snapshot_key.o obj-$(CONFIG_PM_AUTOSLEEP) += autosleep.o obj-$(CONFIG_PM_WAKELOCKS) += wakelock.o diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index abef759de7c8..18d13cbf0591 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -1034,6 +1034,39 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr, power_attr(disk); +#ifdef CONFIG_HIBERNATION_ENC_AUTH +static ssize_t disk_kmk_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + if (snapshot_key_initialized()) + return sprintf(buf, "initialized\n"); + else + return sprintf(buf, "uninitialized\n"); +} + +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute *attr, + const char *buf, size_t n) +{ + int error = 0; + char *p; + int len; + + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + p = memchr(buf, '\n', n); + len = p ? p - buf : n; + if (strncmp(buf, "1", len)) + return -EINVAL; + + error = snapshot_key_init(); + + return error ? error : n; +} + +power_attr(disk_kmk); +#endif /* !CONFIG_HIBERNATION_ENC_AUTH */ + static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) { @@ -1138,6 +1171,9 @@ power_attr(reserved_size); static struct attribute * g[] = { &disk_attr.attr, +#ifdef CONFIG_HIBERNATION_ENC_AUTH + &disk_kmk_attr.attr, +#endif &resume_offset_attr.attr, &resume_attr.attr, &image_size_attr.attr, diff --git a/kernel/power/power.h b/kernel/power/power.h index 9e58bdc8a562..fe2dfa0d4d36 100644 --- a/kernel/power/power.h +++ b/kernel/power/power.h @@ -4,6 +4,12 @@ #include <linux/utsname.h> #include <linux/freezer.h> #include <linux/compiler.h> +#include <crypto/sha.h> + +/* The max size of encrypted key blob */ +#define KEY_BLOB_BUFF_LEN 512 +#define SNAPSHOT_KEY_SIZE SHA512_DIGEST_SIZE +#define DERIVED_KEY_SIZE SHA512_DIGEST_SIZE struct swsusp_info { struct new_utsname uts; @@ -20,6 +26,16 @@ struct swsusp_info { extern void __init hibernate_reserved_size_init(void); extern void __init hibernate_image_size_init(void); +#ifdef CONFIG_HIBERNATION_ENC_AUTH +/* kernel/power/snapshot_key.c */ +extern int snapshot_key_init(void); +extern bool snapshot_key_initialized(void); +extern int snapshot_get_auth_key(u8 *auth_key, bool may_sleep); +extern int snapshot_get_enc_key(u8 *enc_key, bool may_sleep); +#else +static inline int snapshot_key_init(void) { return 0; } +#endif /* !CONFIG_HIBERNATION_ENC_AUTH */ + #ifdef CONFIG_ARCH_HIBERNATION_HEADER /* Maximum size of architecture specific data in a hibernation header */ #define MAX_ARCH_HEADER_SIZE (sizeof(struct new_utsname) + 4) diff --git a/kernel/power/snapshot_key.c b/kernel/power/snapshot_key.c new file mode 100644 index 000000000000..091f33929b47 --- /dev/null +++ b/kernel/power/snapshot_key.c @@ -0,0 +1,237 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* snapshot keys handler + * + * Copyright (C) 2018 Lee, Chun-Yi <jlee@suse.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public Licence + * as published by the Free Software Foundation; either version + * 2 of the Licence, or (at your option) any later version. + */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/cred.h> +#include <linux/key-type.h> +#include <linux/slab.h> +#include <crypto/hash.h> +#include <crypto/sha.h> +#include <keys/trusted-type.h> +#include <keys/user-type.h> + +#include "power.h" + +static const char hash_alg[] = "sha512"; +static struct crypto_shash *hash_tfm; + +/* The master key of snapshot */ +static struct snapshot_key { + const char *key_name; + bool initialized; + unsigned int key_len; + u8 key[SNAPSHOT_KEY_SIZE]; +} skey = { + .key_name = "swsusp-kmk", +}; + +static int calc_hash(u8 *digest, const u8 *buf, unsigned int buflen, + bool may_sleep) +{ + SHASH_DESC_ON_STACK(desc, hash_tfm); + int err; + + desc->tfm = hash_tfm; + desc->flags = may_sleep ? CRYPTO_TFM_REQ_MAY_SLEEP : 0; + + err = crypto_shash_digest(desc, buf, buflen, digest); + shash_desc_zero(desc); + return err; +} + +static int calc_key_hash(u8 *key, unsigned int key_len, const char *salt, + u8 *hash, bool may_sleep) +{ + unsigned int salted_buf_len; + u8 *salted_buf; + int ret; + + if (!key || !hash_tfm || !hash) + return -EINVAL; + + salted_buf_len = strlen(salt) + 1 + SNAPSHOT_KEY_SIZE; + salted_buf = kzalloc(salted_buf_len, + may_sleep ? GFP_KERNEL : GFP_ATOMIC); + if (!salted_buf) + return -ENOMEM; + + strcpy(salted_buf, salt); + memcpy(salted_buf + strlen(salted_buf) + 1, key, key_len); + + ret = calc_hash(hash, salted_buf, salted_buf_len, may_sleep); + memzero_explicit(salted_buf, salted_buf_len); + kzfree(salted_buf); + + return ret; +} + +/* Derive authentication/encryption key */ +static int get_derived_key(u8 *derived_key, const char *derived_type_str, + bool may_sleep) +{ + int ret; + + if (!skey.initialized || !hash_tfm) + return -EINVAL; + + ret = calc_key_hash(skey.key, skey.key_len, derived_type_str, + derived_key, may_sleep); + + return ret; +} + +int snapshot_get_auth_key(u8 *auth_key, bool may_sleep) +{ + return get_derived_key(auth_key, "AUTH_KEY", may_sleep); +} + +int snapshot_get_enc_key(u8 *enc_key, bool may_sleep) +{ + return get_derived_key(enc_key, "ENC_KEY", may_sleep); +} + +bool snapshot_key_initialized(void) +{ + return skey.initialized; +} + +static bool invalid_key(u8 *key, unsigned int key_len) +{ + int i; + + if (!key || !key_len) + return true; + + if (key_len > SNAPSHOT_KEY_SIZE) { + pr_warn("Size of swsusp key more than: %d.\n", + SNAPSHOT_KEY_SIZE); + return true; + } + + /* zero keyblob is invalid key */ + for (i = 0; i < key_len; i++) { + if (key[i] != 0) + return false; + } + pr_warn("The swsusp key should not be zero.\n"); + + return true; +} + +static int trusted_key_init(void) +{ + struct trusted_key_payload *tkp; + struct key *key; + int err; + + pr_debug("%s\n", __func__); + + /* find out swsusp-key */ + key = request_key(&key_type_trusted, skey.key_name, NULL); + if (IS_ERR(key)) { + pr_err("Request key error: %ld\n", PTR_ERR(key)); + err = PTR_ERR(key); + return err; + } + + down_write(&key->sem); + tkp = key->payload.data[0]; + if (invalid_key(tkp->key, tkp->key_len)) { + err = -EINVAL; + goto key_invalid; + } + skey.key_len = tkp->key_len; + memcpy(skey.key, tkp->key, tkp->key_len); + /* burn the original key contents */ + memzero_explicit(tkp->key, tkp->key_len); + +key_invalid: + up_write(&key->sem); + key_put(key); + + return err; +} + +static int user_key_init(void) +{ + struct user_key_payload *ukp; + struct key *key; + int err = 0; + + pr_debug("%s\n", __func__); + + /* find out swsusp-key */ + key = request_key(&key_type_user, skey.key_name, NULL); + if (IS_ERR(key)) { + pr_err("Request key error: %ld\n", PTR_ERR(key)); + err = PTR_ERR(key); + return err; + } + + down_write(&key->sem); + ukp = user_key_payload_locked(key); + if (!ukp) { + /* key was revoked before we acquired its semaphore */ + err = -EKEYREVOKED; + goto key_invalid; + } + if (invalid_key(ukp->data, ukp->datalen)) { + err = -EINVAL; + goto key_invalid; + } + skey.key_len = ukp->datalen; + memcpy(skey.key, ukp->data, ukp->datalen); + /* burn the original key contents */ + memzero_explicit(ukp->data, ukp->datalen); + +key_invalid: + up_write(&key->sem); + key_put(key); + + return err; +} + +/* this function may sleeps */ +int snapshot_key_init(void) +{ + int err; + + pr_debug("%s\n", __func__); + + if (skey.initialized) + return 0; + + hash_tfm = crypto_alloc_shash(hash_alg, 0, CRYPTO_ALG_ASYNC); + if (IS_ERR(hash_tfm)) { + pr_err("Can't allocate %s transform: %ld\n", + hash_alg, PTR_ERR(hash_tfm)); + return PTR_ERR(hash_tfm); + } + + err = trusted_key_init(); + if (err) + err = user_key_init(); + if (err) + goto key_fail; + + skey.initialized = true; + + pr_info("Snapshot key is initialled.\n"); + + return 0; + +key_fail: + crypto_free_shash(hash_tfm); + hash_tfm = NULL; + + return err; +}
This patch adds a snapshot keys handler for using the key retention service api to create keys for snapshot image encryption and authentication. This handler uses TPM trusted key as the snapshot master key, and the encryption key and authentication key are derived from the snapshot key. The user defined key can also be used as the snapshot master key , but user must be aware that the security of user key relies on user space. The name of snapshot key is fixed to "swsusp-kmk". User should use the keyctl tool to load the key blob to root's user keyring. e.g. # /bin/keyctl add trusted swsusp-kmk "load `cat swsusp-kmk.blob`" @u or create a new user key. e.g. # /bin/keyctl add user swsusp-kmk password @u Then the disk_kmk sysfs file can be used to trigger the initialization of snapshot key: # echo 1 > /sys/power/disk_kmk After the initialization be triggered, the secret in the payload of swsusp-key will be copied by hibernation and be erased. Then user can use keyctl to remove swsusp-kmk key from root's keyring. If user does not trigger the initialization by disk_kmk file after swsusp-kmk be loaded to kernel. Then the snapshot key will be initialled when hibernation be triggered. Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> Cc: Pavel Machek <pavel@ucw.cz> Cc: Chen Yu <yu.c.chen@intel.com> Cc: Oliver Neukum <oneukum@suse.com> Cc: Ryan Chen <yu.chen.surf@gmail.com> Cc: David Howells <dhowells@redhat.com> Cc: Giovanni Gherdovich <ggherdovich@suse.cz> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> --- kernel/power/Kconfig | 14 +++ kernel/power/Makefile | 1 + kernel/power/hibernate.c | 36 +++++++ kernel/power/power.h | 16 +++ kernel/power/snapshot_key.c | 237 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 304 insertions(+) create mode 100644 kernel/power/snapshot_key.c