[PATCH 0/5][RFC] Encryption and authentication for hibernate snapshot image

[PATCH 0/5][RFC] Encryption and authentication for hibernate snapshot image

[Lee, Chun-Yi]

Hi,

This patchset is the implementation of encryption and authentication
for hibernate snapshot image. The image will be encrypted by AES and
authenticated by HMAC.

The hibernate function can be used to snapshot memory pages to an image,
then kernel restores the image to memory space in a appropriate time.
There have secrets in snapshot image and cracker may modifies it for
hacking system. Encryption and authentication of snapshot image can protect
the system.

Hibernate function requests the master key through key retention service.
The snapshot master key can be a trusted key or a user defined key. The
name of snapshot master key is fixed to "swsusp-kmk". User should loads
swsusp-kmk to kernel by keyctl tool before the hibernation resume.
e.g. The swsusp-kmk must be loaded before systemd-hibernate-resume

The TPM trusted key type is preferred to be the master key. But user
defined key can also be used for testing or when the platform doesn't
have TPM. User must be aware that the security of user key relies on
user space. If the root account be compromised, then the user key will
easy to be grabbed.

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>

Lee, Chun-Yi (5):
  PM / hibernate: Create snapshot keys handler
  PM / hibernate: Generate and verify signature for snapshot image
  PM / hibernate: Encrypt snapshot image
  PM / hibernate: Erase the snapshot master key in snapshot pages
  PM / hibernate: An option to request that snapshot image must be
    authenticated

 Documentation/admin-guide/kernel-parameters.txt |   6 +
 include/linux/kernel.h                          |   3 +-
 kernel/panic.c                                  |   1 +
 kernel/power/Kconfig                            |  25 +
 kernel/power/Makefile                           |   1 +
 kernel/power/hibernate.c                        |  62 ++-
 kernel/power/power.h                            |  59 +++
 kernel/power/snapshot.c                         | 576 +++++++++++++++++++++++-
 kernel/power/snapshot_key.c                     | 303 +++++++++++++
 kernel/power/swap.c                             |   6 +
 kernel/power/user.c                             |  12 +
 11 files changed, 1036 insertions(+), 18 deletions(-)
 create mode 100644 kernel/power/snapshot_key.c

-- 
2.13.6

[PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Lee, Chun-Yi]

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

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;
+}
-- 
2.13.6

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Randy Dunlap]

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.
> +


-- 
~Randy

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[joeyli]

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 

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Yu Chen]

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
> 

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[joeyli]

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

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Jann Horn]

+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
>
>

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Jann Horn]

+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
>
>

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[joeyli]

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 

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[joeyli]

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 

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Jann Horn]

+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?

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Jann Horn]

+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?

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Andy Lutomirski]

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.

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Andy Lutomirski]

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.

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[joeyli]

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

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[joeyli]

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

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[joeyli]

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

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Mimi Zohar]

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?
> 

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[Mimi Zohar]

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?
> 

Re: [PATCH 1/5] PM / hibernate: Create snapshot keys handler

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58609 bytes --]

[PATCH 2/5] PM / hibernate: Generate and verify signature for snapshot image

[Lee, Chun-Yi]

When producing memory snapshot image, hibernation uses HMAC-SHA512
with snapshot key (from TPM trusted key) to calculate the hash of
all data pages in snapshot image. The hash result will be kept in the
snapshot header as the image signature. Before hibernation restores
image, kernel executes HMAC-SHA512 again and compares the result with
the signature in the header to verify the integrity of snapshot image.

If the verification failed, the resume process will be stopped. Then
the snapshot image will be discarded and system will boot as normal.

On the other hand, a trampoline page be created in snapshot image
when hibernation. This trampoline page be used to forward the state of
snapshot key and the result of snapshot image verification from boot
kernel to image kernel when resuming. The trampoline page will also be
used to forward the snapshot key in the later patch.

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/hibernate.c |  18 ++-
 kernel/power/power.h     |  26 ++++
 kernel/power/snapshot.c  | 387 +++++++++++++++++++++++++++++++++++++++++++++--
 kernel/power/swap.c      |   6 +
 kernel/power/user.c      |  12 ++
 5 files changed, 432 insertions(+), 17 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 18d13cbf0591..871a05e4467c 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -271,10 +271,14 @@ static int create_image(int platform_mode)
 {
 	int error;
 
+	error = snapshot_prepare_hash(false);
+	if (error)
+		return error;
+
 	error = dpm_suspend_end(PMSG_FREEZE);
 	if (error) {
 		pr_err("Some devices failed to power down, aborting hibernation\n");
-		return error;
+		goto finish_hash;
 	}
 
 	error = platform_pre_snapshot(platform_mode);
@@ -331,6 +335,9 @@ static int create_image(int platform_mode)
 	dpm_resume_start(in_suspend ?
 		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
 
+ finish_hash:
+	snapshot_finish_hash();
+
 	return error;
 }
 
@@ -694,6 +701,14 @@ int hibernate(void)
 		return -EPERM;
 	}
 
+	error = snapshot_key_init();
+	if (error)
+		return error;
+
+	error = snapshot_create_trampoline();
+	if (error)
+		return error;
+
 	lock_system_sleep();
 	/* The snapshot device should not be opened while we're running */
 	if (!atomic_add_unless(&snapshot_device_available, -1, 0)) {
@@ -750,6 +765,7 @@ int hibernate(void)
 		pm_restore_gfp_mask();
 	} else {
 		pm_pr_dbg("Image restored successfully.\n");
+		snapshot_restore_trampoline();
 	}
 
  Free_bitmaps:
diff --git a/kernel/power/power.h b/kernel/power/power.h
index fe2dfa0d4d36..c614b0a294e3 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -11,6 +11,10 @@
 #define SNAPSHOT_KEY_SIZE SHA512_DIGEST_SIZE
 #define DERIVED_KEY_SIZE SHA512_DIGEST_SIZE
 
+/* HMAC algorithm for hibernate snapshot signature */
+#define SNAPSHOT_HMAC	"hmac(sha512)"
+#define SNAPSHOT_DIGEST_SIZE	SHA512_DIGEST_SIZE
+
 struct swsusp_info {
 	struct new_utsname	uts;
 	u32			version_code;
@@ -19,6 +23,17 @@ struct swsusp_info {
 	unsigned long		image_pages;
 	unsigned long		pages;
 	unsigned long		size;
+	unsigned long		trampoline_pfn;
+	u8			signature[SNAPSHOT_DIGEST_SIZE];
+} __aligned(PAGE_SIZE);
+
+/*
+ * The trampoline page is used to forward information
+ * from boot kernel to image kernel in restore stage.
+ */
+struct trampoline {
+	bool snapshot_key_valid;
+	int sig_verify_ret;
 } __aligned(PAGE_SIZE);
 
 #ifdef CONFIG_HIBERNATION
@@ -27,12 +42,19 @@ extern void __init hibernate_reserved_size_init(void);
 extern void __init hibernate_image_size_init(void);
 
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
+/* kernel/power/snapshot.c */
+extern int snapshot_image_verify_decrypt(void);
+extern int snapshot_prepare_hash(bool may_sleep);
+extern void snapshot_finish_hash(void);
 /* 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_image_verify_decrypt(void) { return 0; }
+static inline int snapshot_prepare_hash(bool may_sleep) { return 0; }
+static inline void snapshot_finish_hash(void) {}
 static inline int snapshot_key_init(void) { return 0; }
 #endif	/* !CONFIG_HIBERNATION_ENC_AUTH */
 
@@ -171,6 +193,10 @@ extern int snapshot_read_next(struct snapshot_handle *handle);
 extern int snapshot_write_next(struct snapshot_handle *handle);
 extern void snapshot_write_finalize(struct snapshot_handle *handle);
 extern int snapshot_image_loaded(struct snapshot_handle *handle);
+extern int snapshot_create_trampoline(void);
+extern void snapshot_init_trampoline(void);
+extern void snapshot_restore_trampoline(void);
+extern void snapshot_free_trampoline(void);
 
 /* If unset, the snapshot device cannot be open. */
 extern atomic_t snapshot_device_available;
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 3d37c279c090..949542ed5ffe 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -33,12 +33,16 @@
 #include <linux/compiler.h>
 #include <linux/ktime.h>
 #include <linux/set_memory.h>
+#include <linux/vmalloc.h>
 
 #include <linux/uaccess.h>
 #include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/tlbflush.h>
 #include <asm/io.h>
+#ifdef CONFIG_HIBERNATION_ENC_AUTH
+#include <crypto/hash.h>
+#endif
 
 #include "power.h"
 
@@ -79,6 +83,15 @@ static inline void hibernate_restore_protect_page(void *page_address) {}
 static inline void hibernate_restore_unprotect_page(void *page_address) {}
 #endif /* CONFIG_STRICT_KERNEL_RWX  && CONFIG_ARCH_HAS_SET_MEMORY */
 
+/* the trampoline is used by image kernel */
+static void *trampoline_virt;
+
+/* trampoline pfn from swsusp_info in snapshot for snapshot_write_next() */
+static unsigned long trampoline_pfn;
+
+/* Keep the buffer for foward page in snapshot_write_next() */
+static void *trampoline_buff;
+
 static int swsusp_page_is_free(struct page *);
 static void swsusp_set_page_forbidden(struct page *);
 static void swsusp_unset_page_forbidden(struct page *);
@@ -1392,8 +1405,246 @@ static inline void copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
 }
 #endif /* CONFIG_HIGHMEM */
 
-static void copy_data_pages(struct memory_bitmap *copy_bm,
-			    struct memory_bitmap *orig_bm)
+/* Total number of image pages */
+static unsigned int nr_copy_pages;
+
+/* Point array for collecting buffers' address in snapshot_write_next() */
+static void **h_buf;
+
+#ifdef CONFIG_HIBERNATION_ENC_AUTH
+/*
+ * Signature of snapshot image
+ */
+static u8 signature[SNAPSHOT_DIGEST_SIZE];
+
+/* Keep the signature verification result for trampoline */
+static int sig_verify_ret;
+
+/* keep the snapshot key status for trampoline */
+static bool snapshot_key_valid;
+
+static u8 *s4_verify_digest;
+static struct shash_desc *s4_verify_desc;
+
+int snapshot_prepare_hash(bool may_sleep)
+{
+	char auth_key[DERIVED_KEY_SIZE];
+	struct crypto_shash *tfm;
+	size_t digest_size, desc_size;
+	int ret;
+
+	ret = snapshot_get_auth_key(auth_key, may_sleep);
+	if (ret) {
+		pr_warn_once("auth key is invalid: %d\n", ret);
+		return -EINVAL;
+	}
+	snapshot_key_valid = true;
+
+	tfm = crypto_alloc_shash(SNAPSHOT_HMAC, 0, 0);
+	if (IS_ERR(tfm)) {
+		pr_err("Allocate HMAC failed: %ld\n", PTR_ERR(tfm));
+		return PTR_ERR(tfm);
+	}
+
+	ret = crypto_shash_setkey(tfm, auth_key, DERIVED_KEY_SIZE);
+	if (ret) {
+		pr_err("Set HMAC key failed\n");
+		goto error;
+	}
+
+	desc_size = crypto_shash_descsize(tfm) + sizeof(*s4_verify_desc);
+	digest_size = crypto_shash_digestsize(tfm);
+	s4_verify_digest = kzalloc(digest_size + desc_size,
+				   may_sleep ? GFP_KERNEL : GFP_ATOMIC);
+	if (!s4_verify_digest) {
+		pr_err("Allocate digest failed\n");
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	s4_verify_desc = (void *) s4_verify_digest + digest_size;
+	s4_verify_desc->tfm = tfm;
+	if (may_sleep)
+		s4_verify_desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+	ret = crypto_shash_init(s4_verify_desc);
+	if (ret < 0)
+		goto free_shash;
+
+	return 0;
+
+ free_shash:
+	kfree(s4_verify_digest);
+ error:
+	crypto_free_shash(tfm);
+	s4_verify_digest = NULL;
+	s4_verify_desc = NULL;
+	return ret;
+}
+
+void snapshot_finish_hash(void)
+{
+	if (s4_verify_desc)
+		crypto_free_shash(s4_verify_desc->tfm);
+	kfree(s4_verify_digest);
+	s4_verify_desc = NULL;
+	s4_verify_digest = NULL;
+}
+
+int snapshot_image_verify_decrypt(void)
+{
+	int ret, i;
+
+	if (!h_buf) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	ret = snapshot_key_init();
+	if (ret)
+		goto error_prep;
+
+	ret = snapshot_prepare_hash(true);
+	if (ret || !s4_verify_desc)
+		goto error_prep;
+
+	for (i = 0; i < nr_copy_pages; i++) {
+		ret = crypto_shash_update(s4_verify_desc, *(h_buf + i), PAGE_SIZE);
+		if (ret)
+			goto error_shash;
+	}
+
+	ret = crypto_shash_final(s4_verify_desc, s4_verify_digest);
+	if (ret)
+		goto error_shash;
+
+	pr_debug("Signature %*phN\n", SNAPSHOT_DIGEST_SIZE, signature);
+	pr_debug("Digest    %*phN\n", SNAPSHOT_DIGEST_SIZE, s4_verify_digest);
+	if (memcmp(signature, s4_verify_digest, SNAPSHOT_DIGEST_SIZE))
+		ret = -EKEYREJECTED;
+
+ error_shash:
+	snapshot_finish_hash();
+
+ error_prep:
+	vfree(h_buf);
+	if (ret)
+		pr_warn("Signature verification failed: %d\n", ret);
+ error:
+	sig_verify_ret = ret;
+	return ret;
+}
+
+static int
+__copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
+{
+	unsigned long pfn, dst_pfn;
+	struct page *d_page;
+	void *crypto_buffer = NULL;
+	int ret = 0;
+
+	memory_bm_position_reset(orig_bm);
+	memory_bm_position_reset(copy_bm);
+	for (;;) {
+		pfn = memory_bm_next_pfn(orig_bm);
+		if (unlikely(pfn == BM_END_OF_MAP))
+			break;
+		dst_pfn = memory_bm_next_pfn(copy_bm);
+		copy_data_page(dst_pfn, pfn);
+
+		/* Setup buffer */
+		d_page = pfn_to_page(dst_pfn);
+		if (PageHighMem(d_page)) {
+			void *kaddr = kmap_atomic(d_page);
+
+			copy_page(buffer, kaddr);
+			kunmap_atomic(kaddr);
+			crypto_buffer = buffer;
+		} else {
+			crypto_buffer = page_address(d_page);
+		}
+
+		/* Generate digest */
+		if (!s4_verify_desc)
+			continue;
+		ret = crypto_shash_update(s4_verify_desc, crypto_buffer,
+					  PAGE_SIZE);
+		if (ret)
+			return ret;
+	}
+
+	if (s4_verify_desc) {
+		ret = crypto_shash_final(s4_verify_desc, s4_verify_digest);
+		if (ret)
+			return ret;
+
+		memset(signature, 0, SNAPSHOT_DIGEST_SIZE);
+		memcpy(signature, s4_verify_digest, SNAPSHOT_DIGEST_SIZE);
+	}
+
+	return 0;
+}
+
+static void alloc_h_buf(void)
+{
+	h_buf = vmalloc(sizeof(void *) * nr_copy_pages);
+	if (!h_buf)
+		pr_err("Allocate buffer point array failed\n");
+}
+
+static void init_signature(struct swsusp_info *info)
+{
+	memcpy(info->signature, signature, SNAPSHOT_DIGEST_SIZE);
+}
+
+static void load_signature(struct swsusp_info *info)
+{
+	memset(signature, 0, SNAPSHOT_DIGEST_SIZE);
+	memcpy(signature, info->signature, SNAPSHOT_DIGEST_SIZE);
+}
+
+static void init_sig_verify(struct trampoline *t)
+{
+	t->sig_verify_ret = sig_verify_ret;
+	t->snapshot_key_valid = snapshot_key_valid;
+	sig_verify_ret = 0;
+	snapshot_key_valid = 0;
+}
+
+static void handle_sig_verify(struct trampoline *t)
+{
+	if (t->sig_verify_ret)
+		pr_warn("Signature verification failed: %d\n",
+			t->sig_verify_ret);
+	else if (t->snapshot_key_valid)
+		pr_info("Signature verification passed.\n");
+}
+#else
+static int
+__copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
+{
+	unsigned long pfn;
+
+	memory_bm_position_reset(orig_bm);
+	memory_bm_position_reset(copy_bm);
+	for (;;) {
+		pfn = memory_bm_next_pfn(orig_bm);
+		if (unlikely(pfn == BM_END_OF_MAP))
+			break;
+		copy_data_page(memory_bm_next_pfn(copy_bm), pfn);
+	}
+
+	return 0;
+}
+
+static inline void alloc_h_buf(void) {}
+static inline void init_signature(struct swsusp_info *info) {}
+static inline void load_signature(struct swsusp_info *info) {}
+static inline void init_sig_verify(struct trampoline *t) {}
+static inline void handle_sig_verify(struct trampoline *t) {}
+#endif /* !CONFIG_HIBERNATION_ENC_AUTH */
+
+static int copy_data_pages(struct memory_bitmap *copy_bm,
+			   struct memory_bitmap *orig_bm)
 {
 	struct zone *zone;
 	unsigned long pfn;
@@ -1407,18 +1658,9 @@ static void copy_data_pages(struct memory_bitmap *copy_bm,
 			if (page_is_saveable(zone, pfn))
 				memory_bm_set_bit(orig_bm, pfn);
 	}
-	memory_bm_position_reset(orig_bm);
-	memory_bm_position_reset(copy_bm);
-	for(;;) {
-		pfn = memory_bm_next_pfn(orig_bm);
-		if (unlikely(pfn == BM_END_OF_MAP))
-			break;
-		copy_data_page(memory_bm_next_pfn(copy_bm), pfn);
-	}
+	return __copy_data_pages(copy_bm, orig_bm);
 }
 
-/* Total number of image pages */
-static unsigned int nr_copy_pages;
 /* Number of pages needed for saving the original pfns of the image pages */
 static unsigned int nr_meta_pages;
 /*
@@ -1960,6 +2202,7 @@ static int swsusp_alloc(struct memory_bitmap *copy_bm,
 asmlinkage __visible int swsusp_save(void)
 {
 	unsigned int nr_pages, nr_highmem;
+	int ret;
 
 	pr_info("Creating hibernation image:\n");
 
@@ -1983,7 +2226,11 @@ asmlinkage __visible int swsusp_save(void)
 	 * Kill them.
 	 */
 	drain_local_pages(NULL);
-	copy_data_pages(&copy_bm, &orig_bm);
+	ret = copy_data_pages(&copy_bm, &orig_bm);
+	if (ret) {
+		pr_err("Copy data pages failed\n");
+		return ret;
+	}
 
 	/*
 	 * End of critical section. From now on, we can write to memory,
@@ -2037,10 +2284,98 @@ static int init_header(struct swsusp_info *info)
 	info->pages = snapshot_get_image_size();
 	info->size = info->pages;
 	info->size <<= PAGE_SHIFT;
+	info->trampoline_pfn = page_to_pfn(virt_to_page(trampoline_virt));
+	init_signature(info);
 	return init_header_complete(info);
 }
 
 /**
+ * create trampoline - Create a trampoline page before snapshot be created
+ * In hibernation process, this routine will be called by kernel before
+ * the snapshot image be created. It can be used in resuming process.
+ */
+int snapshot_create_trampoline(void)
+{
+	if (trampoline_virt) {
+		pr_warn("Tried to create trampoline again\n");
+		return 0;
+	}
+
+	trampoline_virt = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!trampoline_virt) {
+		pr_err("Allocate trampoline page failed\n");
+		return -ENOMEM;
+	}
+	trampoline_pfn = 0;
+	trampoline_buff = NULL;
+
+	return 0;
+}
+
+/**
+ * initial trampoline - Put data to trampoline buffer for target kernel
+ *
+ * In resuming process, this routine will be called by boot kernel before
+ * the target kernel be restored. The boot kernel uses trampoline buffer
+ * to transfer information to target kernel.
+ */
+void snapshot_init_trampoline(void)
+{
+	struct trampoline *t;
+
+	if (!trampoline_pfn || !trampoline_buff) {
+		pr_err("Did not find trampoline buffer, pfn: %ld\n",
+			trampoline_pfn);
+		return;
+	}
+
+	hibernate_restore_unprotect_page(trampoline_buff);
+	memset(trampoline_buff, 0, PAGE_SIZE);
+	t = (struct trampoline *)trampoline_buff;
+
+	init_sig_verify(t);
+
+	pr_info("Hibernation trampoline page prepared\n");
+}
+
+/**
+ * restore trampoline - Handle the data from boot kernel and free.
+ *
+ * In resuming process, this routine will be called by target kernel
+ * after target kernel is restored. The target kernel handles
+ * the data in trampoline that it is transferred from boot kernel.
+ */
+void snapshot_restore_trampoline(void)
+{
+	struct trampoline *t;
+
+	if (!trampoline_virt) {
+		pr_err("Doesn't have trampoline page\n");
+		return;
+	}
+
+	t = (struct trampoline *)trampoline_virt;
+
+	handle_sig_verify(t);
+	snapshot_free_trampoline();
+}
+
+void snapshot_free_trampoline(void)
+{
+	if (!trampoline_virt) {
+		pr_err("No trampoline page can be freed\n");
+		return;
+	}
+
+	trampoline_pfn = 0;
+	trampoline_buff = NULL;
+	memset(trampoline_virt, 0, PAGE_SIZE);
+	free_page((unsigned long)trampoline_virt);
+	trampoline_virt = NULL;
+	pr_info("Trampoline freed\n");
+}
+
+/**
  * pack_pfns - Prepare PFNs for saving.
  * @bm: Memory bitmap.
  * @buf: Memory buffer to store the PFNs in.
@@ -2187,6 +2522,8 @@ static int load_header(struct swsusp_info *info)
 	if (!error) {
 		nr_copy_pages = info->image_pages;
 		nr_meta_pages = info->pages - info->image_pages - 1;
+		trampoline_pfn = info->trampoline_pfn;
+		load_signature(info);
 	}
 	return error;
 }
@@ -2520,7 +2857,8 @@ static int prepare_image(struct memory_bitmap *new_bm, struct memory_bitmap *bm)
  * Get the address that snapshot_write_next() should return to its caller to
  * write to.
  */
-static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
+static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca,
+			unsigned long *pfn_out)
 {
 	struct pbe *pbe;
 	struct page *page;
@@ -2529,6 +2867,9 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
 	if (pfn == BM_END_OF_MAP)
 		return ERR_PTR(-EFAULT);
 
+	if (pfn_out)
+		*pfn_out = pfn;
+
 	page = pfn_to_page(pfn);
 	if (PageHighMem(page))
 		return get_highmem_page_buffer(page, ca);
@@ -2576,6 +2917,7 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
 int snapshot_write_next(struct snapshot_handle *handle)
 {
 	static struct chain_allocator ca;
+	unsigned long pfn;
 	int error = 0;
 
 	/* Check if we have already loaded the entire image */
@@ -2600,6 +2942,12 @@ int snapshot_write_next(struct snapshot_handle *handle)
 
 		safe_pages_list = NULL;
 
+		/* Allocate buffer point array for generating
+		 * digest to compare with signature.
+		 * h_buf will freed in snapshot_image_verify_decrypt().
+		 */
+		alloc_h_buf();
+
 		error = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
 		if (error)
 			return error;
@@ -2623,21 +2971,28 @@ int snapshot_write_next(struct snapshot_handle *handle)
 			chain_init(&ca, GFP_ATOMIC, PG_SAFE);
 			memory_bm_position_reset(&orig_bm);
 			restore_pblist = NULL;
-			handle->buffer = get_buffer(&orig_bm, &ca);
+			handle->buffer = get_buffer(&orig_bm, &ca, &pfn);
 			handle->sync_read = 0;
 			if (IS_ERR(handle->buffer))
 				return PTR_ERR(handle->buffer);
+			if (h_buf)
+				*h_buf = handle->buffer;
 		}
 	} else {
 		copy_last_highmem_page();
 		/* Restore page key for data page (s390 only). */
 		page_key_write(handle->buffer);
 		hibernate_restore_protect_page(handle->buffer);
-		handle->buffer = get_buffer(&orig_bm, &ca);
+		handle->buffer = get_buffer(&orig_bm, &ca, &pfn);
 		if (IS_ERR(handle->buffer))
 			return PTR_ERR(handle->buffer);
 		if (handle->buffer != buffer)
 			handle->sync_read = 0;
+		/* Capture the trampoline for transfer data */
+		if (pfn == trampoline_pfn && trampoline_pfn)
+			trampoline_buff = handle->buffer;
+		if (h_buf)
+			*(h_buf + (handle->cur - nr_meta_pages - 1)) = handle->buffer;
 	}
 	handle->cur++;
 	return PAGE_SIZE;
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index d7f6c1a288d3..2e669f589830 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -1095,6 +1095,9 @@ static int load_image(struct swap_map_handle *handle,
 		snapshot_write_finalize(snapshot);
 		if (!snapshot_image_loaded(snapshot))
 			ret = -ENODATA;
+		if (!ret)
+			ret = snapshot_image_verify_decrypt();
+		snapshot_init_trampoline();
 	}
 	swsusp_show_speed(start, stop, nr_to_read, "Read");
 	return ret;
@@ -1447,6 +1450,9 @@ static int load_image_lzo(struct swap_map_handle *handle,
 				}
 			}
 		}
+		if (!ret)
+			ret = snapshot_image_verify_decrypt();
+		snapshot_init_trampoline();
 	}
 	swsusp_show_speed(start, stop, nr_to_read, "Read");
 out_clean:
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 2d8b60a3c86b..d5c8f777e8d8 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -248,6 +248,7 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		if (!data->frozen || data->ready)
 			break;
 		pm_restore_gfp_mask();
+		snapshot_restore_trampoline();
 		free_basic_memory_bitmaps();
 		data->free_bitmaps = false;
 		thaw_processes();
@@ -259,6 +260,12 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 			error = -EPERM;
 			break;
 		}
+		error = snapshot_key_init();
+		if (error)
+			return error;
+		error = snapshot_create_trampoline();
+		if (error)
+			return error;
 		pm_restore_gfp_mask();
 		error = hibernation_snapshot(data->platform_support);
 		if (!error) {
@@ -275,6 +282,11 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 			error = -EPERM;
 			break;
 		}
+		if (snapshot_image_verify_decrypt()) {
+			error = -EPERM;
+			break;
+		}
+		snapshot_init_trampoline();
 		error = hibernation_restore(data->platform_support);
 		break;
 
-- 
2.13.6

[PATCH 3/5] PM / hibernate: Encrypt snapshot image

[Lee, Chun-Yi]

To protect the secret in memory snapshot image, this patch adds the
logic to encrypt snapshot pages by AES-CTR. Using AES-CTR is because
it's simple, fast and parallelizable. But this patch didn't implement
parallel encryption.

The encrypt key is derived from the snapshot key. And the initialization
vector will be kept in snapshot header for resuming.

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/hibernate.c |   8 ++-
 kernel/power/power.h     |   6 ++
 kernel/power/snapshot.c  | 154 ++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 164 insertions(+), 4 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 871a05e4467c..79f4db284126 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -275,10 +275,14 @@ static int create_image(int platform_mode)
 	if (error)
 		return error;
 
+	error = snapshot_prepare_crypto(false, true);
+	if (error)
+		goto finish_hash;
+
 	error = dpm_suspend_end(PMSG_FREEZE);
 	if (error) {
 		pr_err("Some devices failed to power down, aborting hibernation\n");
-		goto finish_hash;
+		goto finish_crypto;
 	}
 
 	error = platform_pre_snapshot(platform_mode);
@@ -335,6 +339,8 @@ static int create_image(int platform_mode)
 	dpm_resume_start(in_suspend ?
 		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
 
+ finish_crypto:
+	snapshot_finish_crypto();
  finish_hash:
 	snapshot_finish_hash();
 
diff --git a/kernel/power/power.h b/kernel/power/power.h
index c614b0a294e3..41263fdd3a54 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -5,6 +5,7 @@
 #include <linux/freezer.h>
 #include <linux/compiler.h>
 #include <crypto/sha.h>
+#include <crypto/aes.h>
 
 /* The max size of encrypted key blob */
 #define KEY_BLOB_BUFF_LEN 512
@@ -24,6 +25,7 @@ struct swsusp_info {
 	unsigned long		pages;
 	unsigned long		size;
 	unsigned long		trampoline_pfn;
+	u8			iv[AES_BLOCK_SIZE];
 	u8			signature[SNAPSHOT_DIGEST_SIZE];
 } __aligned(PAGE_SIZE);
 
@@ -44,6 +46,8 @@ extern void __init hibernate_image_size_init(void);
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
 /* kernel/power/snapshot.c */
 extern int snapshot_image_verify_decrypt(void);
+extern int snapshot_prepare_crypto(bool may_sleep, bool create_iv);
+extern void snapshot_finish_crypto(void);
 extern int snapshot_prepare_hash(bool may_sleep);
 extern void snapshot_finish_hash(void);
 /* kernel/power/snapshot_key.c */
@@ -53,6 +57,8 @@ 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_image_verify_decrypt(void) { return 0; }
+static inline int snapshot_prepare_crypto(bool may_sleep, bool create_iv) { return 0; }
+static inline void snapshot_finish_crypto(void) {}
 static inline int snapshot_prepare_hash(bool may_sleep) { return 0; }
 static inline void snapshot_finish_hash(void) {}
 static inline int snapshot_key_init(void) { return 0; }
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 949542ed5ffe..c9a6e4983571 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -41,7 +41,11 @@
 #include <asm/tlbflush.h>
 #include <asm/io.h>
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
+#include <linux/random.h>
+#include <linux/scatterlist.h>
+#include <crypto/aes.h>
 #include <crypto/hash.h>
+#include <crypto/skcipher.h>
 #endif
 
 #include "power.h"
@@ -1412,6 +1416,127 @@ static unsigned int nr_copy_pages;
 static void **h_buf;
 
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
+static struct skcipher_request *sk_req;
+static u8 iv[AES_BLOCK_SIZE];
+static void *c_buffer;
+
+static void init_iv(struct swsusp_info *info)
+{
+	memcpy(info->iv, iv, AES_BLOCK_SIZE);
+}
+
+static void load_iv(struct swsusp_info *info)
+{
+	memcpy(iv, info->iv, AES_BLOCK_SIZE);
+}
+
+int snapshot_prepare_crypto(bool may_sleep, bool create_iv)
+{
+	char enc_key[DERIVED_KEY_SIZE];
+	struct crypto_skcipher *tfm;
+	int ret = 0;
+
+	ret = snapshot_get_enc_key(enc_key, may_sleep);
+	if (ret) {
+		pr_warn_once("enc key is invalid\n");
+		return -EINVAL;
+	}
+
+	c_buffer = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!c_buffer) {
+		pr_err("Allocate crypto buffer page failed\n");
+		return -ENOMEM;
+	}
+
+	tfm = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);
+	if (IS_ERR(tfm)) {
+		ret = PTR_ERR(tfm);
+		pr_err("failed to allocate skcipher (%d)\n", ret);
+		goto alloc_fail;
+	}
+
+	ret = crypto_skcipher_setkey(tfm, enc_key, AES_MAX_KEY_SIZE);
+	if (ret) {
+		pr_err("failed to setkey (%d)\n", ret);
+		goto set_fail;
+	}
+
+	sk_req = skcipher_request_alloc(tfm, GFP_KERNEL);
+	if (!sk_req) {
+		pr_err("failed to allocate request\n");
+		ret = -ENOMEM;
+		goto set_fail;
+	}
+	if (may_sleep)
+		skcipher_request_set_callback(sk_req, CRYPTO_TFM_REQ_MAY_SLEEP,
+						NULL, NULL);
+	if (create_iv)
+		get_random_bytes(iv, AES_BLOCK_SIZE);
+
+	return 0;
+
+set_fail:
+	crypto_free_skcipher(tfm);
+alloc_fail:
+	__free_page(c_buffer);
+
+	return ret;
+}
+
+void snapshot_finish_crypto(void)
+{
+	struct crypto_skcipher *tfm;
+ 
+	if (!sk_req)
+		return;
+
+	tfm = crypto_skcipher_reqtfm(sk_req);
+	skcipher_request_zero(sk_req);
+	skcipher_request_free(sk_req);
+	crypto_free_skcipher(tfm);
+	__free_page(c_buffer);
+	sk_req = NULL;
+}
+
+static int encrypt_data_page(void *hash_buffer)
+{
+	struct scatterlist src[1], dst[1];
+	u8 iv_tmp[AES_BLOCK_SIZE];
+	int ret = 0;
+
+	if (!sk_req)
+		return 0;
+
+	memcpy(iv_tmp, iv, sizeof(iv));
+	sg_init_one(src, hash_buffer, PAGE_SIZE);
+	sg_init_one(dst, c_buffer, PAGE_SIZE);
+	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
+	ret = crypto_skcipher_encrypt(sk_req);
+
+	copy_page(hash_buffer, c_buffer);
+	memset(c_buffer, 0, PAGE_SIZE);
+
+	return ret;
+}
+
+static int decrypt_data_page(void *encrypted_page)
+{
+	struct scatterlist src[1], dst[1];
+	u8 iv_tmp[AES_BLOCK_SIZE];
+	int ret = 0;
+
+	memcpy(iv_tmp, iv, sizeof(iv));
+	sg_init_one(src, encrypted_page, PAGE_SIZE);
+	sg_init_one(dst, c_buffer, PAGE_SIZE);
+	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
+	ret = crypto_skcipher_decrypt(sk_req);
+
+	copy_page(encrypted_page, c_buffer);
+	memset(c_buffer, 0, PAGE_SIZE);
+
+	return ret;
+}
+
 /*
  * Signature of snapshot image
  */
@@ -1507,22 +1632,30 @@ int snapshot_image_verify_decrypt(void)
 	if (ret || !s4_verify_desc)
 		goto error_prep;
 
+	ret = snapshot_prepare_crypto(true, false);
+	if (ret)
+		goto error_prep;
+
 	for (i = 0; i < nr_copy_pages; i++) {
 		ret = crypto_shash_update(s4_verify_desc, *(h_buf + i), PAGE_SIZE);
 		if (ret)
-			goto error_shash;
+			goto error_shash_crypto;
+		ret = decrypt_data_page(*(h_buf + i));
+		if (ret)
+			goto error_shash_crypto;
 	}
 
 	ret = crypto_shash_final(s4_verify_desc, s4_verify_digest);
 	if (ret)
-		goto error_shash;
+		goto error_shash_crypto;
 
 	pr_debug("Signature %*phN\n", SNAPSHOT_DIGEST_SIZE, signature);
 	pr_debug("Digest    %*phN\n", SNAPSHOT_DIGEST_SIZE, s4_verify_digest);
 	if (memcmp(signature, s4_verify_digest, SNAPSHOT_DIGEST_SIZE))
 		ret = -EKEYREJECTED;
 
- error_shash:
+ error_shash_crypto:
+	snapshot_finish_crypto();
 	snapshot_finish_hash();
 
  error_prep:
@@ -1563,6 +1696,17 @@ __copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 			crypto_buffer = page_address(d_page);
 		}
 
+		/* Encrypt hashed page */
+		encrypt_data_page(crypto_buffer);
+
+		/* Copy encrypted buffer to destination page in high memory */
+		if (PageHighMem(d_page)) {
+			void *kaddr = kmap_atomic(d_page);
+
+			copy_page(kaddr, crypto_buffer);
+			kunmap_atomic(kaddr);
+		}
+
 		/* Generate digest */
 		if (!s4_verify_desc)
 			continue;
@@ -1637,6 +1781,8 @@ __copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 }
 
 static inline void alloc_h_buf(void) {}
+static inline void init_iv(struct swsusp_info *info) {}
+static inline void load_iv(struct swsusp_info *info) {}
 static inline void init_signature(struct swsusp_info *info) {}
 static inline void load_signature(struct swsusp_info *info) {}
 static inline void init_sig_verify(struct trampoline *t) {}
@@ -2285,6 +2431,7 @@ static int init_header(struct swsusp_info *info)
 	info->size = info->pages;
 	info->size <<= PAGE_SHIFT;
 	info->trampoline_pfn = page_to_pfn(virt_to_page(trampoline_virt));
+	init_iv(info);
 	init_signature(info);
 	return init_header_complete(info);
 }
@@ -2523,6 +2670,7 @@ static int load_header(struct swsusp_info *info)
 		nr_copy_pages = info->image_pages;
 		nr_meta_pages = info->pages - info->image_pages - 1;
 		trampoline_pfn = info->trampoline_pfn;
+		load_iv(info);
 		load_signature(info);
 	}
 	return error;
-- 
2.13.6

[PATCH 4/5] PM / hibernate: Erase the snapshot master key in snapshot pages

[Lee, Chun-Yi]

If the encryption key be guessed then the snapshot master key can
also be grabbed from snapshot image. Which means that the authentication
key can also be calculated. So kernel erases master key in snapshot
pages.

Because the master key in image kernel be erased, kernel uses the
trampoline page to forward snapshot master key to image kernel.

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/power.h        |  6 +++++
 kernel/power/snapshot.c     |  5 ++++
 kernel/power/snapshot_key.c | 66 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index 41263fdd3a54..d2fc73b2e200 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -36,6 +36,7 @@ struct swsusp_info {
 struct trampoline {
 	bool snapshot_key_valid;
 	int sig_verify_ret;
+	u8 snapshot_key[SNAPSHOT_KEY_SIZE];
 } __aligned(PAGE_SIZE);
 
 #ifdef CONFIG_HIBERNATION
@@ -55,6 +56,9 @@ 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);
+extern void snapshot_key_page_erase(unsigned long pfn, void *buff_addr);
+extern void snapshot_key_trampoline_backup(struct trampoline *t);
+extern void snapshot_key_trampoline_restore(struct trampoline *t);
 #else
 static inline int snapshot_image_verify_decrypt(void) { return 0; }
 static inline int snapshot_prepare_crypto(bool may_sleep, bool create_iv) { return 0; }
@@ -62,6 +66,8 @@ static inline void snapshot_finish_crypto(void) {}
 static inline int snapshot_prepare_hash(bool may_sleep) { return 0; }
 static inline void snapshot_finish_hash(void) {}
 static inline int snapshot_key_init(void) { return 0; }
+static inline void snapshot_key_trampoline_backup(struct trampoline *t) {}
+static inline void snapshot_key_trampoline_restore(struct trampoline *t) {}
 #endif	/* !CONFIG_HIBERNATION_ENC_AUTH */
 
 #ifdef CONFIG_ARCH_HIBERNATION_HEADER
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index c9a6e4983571..dd176df75d2b 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1696,6 +1696,9 @@ __copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 			crypto_buffer = page_address(d_page);
 		}
 
+		/* Erase key data in snapshot */
+		snapshot_key_page_erase(pfn, crypto_buffer);
+
 		/* Encrypt hashed page */
 		encrypt_data_page(crypto_buffer);
 
@@ -2481,6 +2484,7 @@ void snapshot_init_trampoline(void)
 	t = (struct trampoline *)trampoline_buff;
 
 	init_sig_verify(t);
+	snapshot_key_trampoline_backup(t);
 
 	pr_info("Hibernation trampoline page prepared\n");
 }
@@ -2504,6 +2508,7 @@ void snapshot_restore_trampoline(void)
 	t = (struct trampoline *)trampoline_virt;
 
 	handle_sig_verify(t);
+	snapshot_key_trampoline_restore(t);
 	snapshot_free_trampoline();
 }
 
diff --git a/kernel/power/snapshot_key.c b/kernel/power/snapshot_key.c
index 091f33929b47..c0f179283f9e 100644
--- a/kernel/power/snapshot_key.c
+++ b/kernel/power/snapshot_key.c
@@ -29,11 +29,26 @@ static struct snapshot_key {
 	const char *key_name;
 	bool initialized;
 	unsigned int key_len;
+	unsigned long pfn;			/* pfn of keyblob */
+	unsigned long addr_offset;		/* offset in page for keyblob */
 	u8 key[SNAPSHOT_KEY_SIZE];
+	u8 fingerprint[SHA512_DIGEST_SIZE];	/* fingerprint of keyblob */
 } skey = {
 	.key_name = "swsusp-kmk",
 };
 
+static void snapshot_key_clean(void)
+{
+	crypto_free_shash(hash_tfm);
+	hash_tfm = NULL;
+	skey.pfn = 0;
+	skey.key_len = 0;
+	skey.addr_offset = 0;
+	memzero_explicit(skey.key, SNAPSHOT_KEY_SIZE);
+	memzero_explicit(skey.fingerprint, SHA512_DIGEST_SIZE);
+	skey.initialized = false;
+}
+
 static int calc_hash(u8 *digest, const u8 *buf, unsigned int buflen,
 		     bool may_sleep)
 {
@@ -74,6 +89,53 @@ static int calc_key_hash(u8 *key, unsigned int key_len, const char *salt,
 	return ret;
 }
 
+static int get_key_fingerprint(u8 *fingerprint, u8 *key, unsigned int key_len,
+				bool may_sleep)
+{
+	return calc_key_hash(key, key_len, "FINGERPRINT", fingerprint, may_sleep);
+}
+
+void snapshot_key_page_erase(unsigned long pfn, void *buff_addr)
+{
+	if (!skey.initialized || pfn != skey.pfn)
+		return;
+
+	/* erase key data from snapshot buffer page */
+	if (!memcmp(skey.key, buff_addr + skey.addr_offset, skey.key_len)) {
+		memzero_explicit(buff_addr + skey.addr_offset, skey.key_len);
+		pr_info("Erased swsusp key in snapshot pages.\n");
+	}
+}
+
+/* this function may sleeps because snapshot_key_init() */
+void snapshot_key_trampoline_backup(struct trampoline *t)
+{
+	if (!t || snapshot_key_init())
+		return;
+
+	memcpy(t->snapshot_key, skey.key, skey.key_len);
+}
+
+/* Be called after snapshot image restored success */
+void snapshot_key_trampoline_restore(struct trampoline *t)
+{
+	u8 fingerprint[SHA512_DIGEST_SIZE];
+
+	if (!skey.initialized || !t)
+		return;
+
+	/* check key fingerprint before restore */
+	get_key_fingerprint(fingerprint, t->snapshot_key, skey.key_len, true);
+	if (memcmp(skey.fingerprint, fingerprint, SHA512_DIGEST_SIZE)) {
+		pr_warn("Restored swsusp key failed, fingerprint mismatch.\n");
+		snapshot_key_clean();
+		return;
+	}
+
+	memcpy(skey.key, t->snapshot_key, skey.key_len);
+	memzero_explicit(t->snapshot_key, SNAPSHOT_KEY_SIZE);
+}
+
 /* Derive authentication/encryption key */
 static int get_derived_key(u8 *derived_key, const char *derived_type_str,
 			   bool may_sleep)
@@ -223,9 +285,13 @@ int snapshot_key_init(void)
 	if (err)
 		goto key_fail;
 
+	skey.pfn = page_to_pfn(virt_to_page(skey.key));
+	skey.addr_offset = (unsigned long) skey.key & ~PAGE_MASK;
+	get_key_fingerprint(skey.fingerprint, skey.key, skey.key_len, true);
 	skey.initialized = true;
 
 	pr_info("Snapshot key is initialled.\n");
+	pr_debug("Fingerprint %*phN\n", SHA512_DIGEST_SIZE, skey.fingerprint);
 
 	return 0;
 
-- 
2.13.6

[PATCH 5/5] PM / hibernate: An option to request that snapshot image must be authenticated

[Lee, Chun-Yi]

This kernel option is similar to the option for kernel module signature
verification. When this option is unselected, kernel will be tainted by
restored from a snapshot image without (valid) signature.

When the option is selected, kernel will refuse the system to be restored
from a unauthenticated image. The hibernation resume process will be stopped
, the snapshot image will be discarded and system just boots as normal.

The hibernation can be triggered without snapshot master key when this
option is unselected. But kernel will be tainted after hibernation resume.

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>
---
 Documentation/admin-guide/kernel-parameters.txt |  6 ++++
 include/linux/kernel.h                          |  3 +-
 kernel/panic.c                                  |  1 +
 kernel/power/Kconfig                            | 11 +++++++
 kernel/power/hibernate.c                        |  8 +++--
 kernel/power/power.h                            |  5 ++++
 kernel/power/snapshot.c                         | 40 +++++++++++++++++++++++--
 kernel/power/user.c                             |  2 +-
 8 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 970d837bd57f..c4be29103865 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3941,6 +3941,12 @@
 		protect_image	Turn on image protection during restoration
 				(that will set all pages holding image data
 				during restoration read-only).
+		enforce_auth	When HIBERNATION_ENC_AUTH is set, this means
+				that snapshot image without (valid) signature
+				will fail to restore.
+				Note that if HIBERNATION_ENC_AUTH_FORCE is
+				set, that is always true, so this option does
+				nothing.
 
 	retain_initrd	[RAM] Keep initrd memory after extraction
 
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d6aac75b51ba..61714489cb57 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -598,7 +598,8 @@ extern enum system_states {
 #define TAINT_LIVEPATCH			15
 #define TAINT_AUX			16
 #define TAINT_RANDSTRUCT		17
-#define TAINT_FLAGS_COUNT		18
+#define TAINT_UNSAFE_HIBERNATE		18
+#define TAINT_FLAGS_COUNT		19
 
 struct taint_flag {
 	char c_true;	/* character printed when tainted */
diff --git a/kernel/panic.c b/kernel/panic.c
index 8b2e002d52eb..624eb1150361 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -327,6 +327,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
 	[ TAINT_LIVEPATCH ]		= { 'K', ' ', true },
 	[ TAINT_AUX ]			= { 'X', ' ', true },
 	[ TAINT_RANDSTRUCT ]		= { 'T', ' ', true },
+	[ TAINT_UNSAFE_HIBERNATE ]	= { 'H', ' ', true },
 };
 
 /**
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 7c5c30149dbc..3c998fd6dc4c 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -90,6 +90,17 @@ config HIBERNATION_ENC_AUTH
 	  master key of hibernation. The TPM trusted key depends on TPM. The
 	  security of user defined key relies on user space.
 
+config HIBERNATION_ENC_AUTH_FORCE
+	bool "Require hibernate snapshot image to be validly signed"
+	depends on HIBERNATION_ENC_AUTH
+	help
+	  This option will prevent that a snapshot image without (valid)
+	  signature be restored. Without this option, a unauthenticated
+	  snapshot image can be restored but the restored kernel will be
+	  tainted. Which also means that the hibernation can be triggered
+	  without snapshot key but kernel will be tainted without this
+	  option.
+
 config ARCH_SAVE_PAGE_KEYS
 	bool
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 79f4db284126..70a0d630a6c2 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -272,11 +272,11 @@ static int create_image(int platform_mode)
 	int error;
 
 	error = snapshot_prepare_hash(false);
-	if (error)
+	if (error && snapshot_is_enforce_auth())
 		return error;
 
 	error = snapshot_prepare_crypto(false, true);
-	if (error)
+	if (error && snapshot_is_enforce_auth())
 		goto finish_hash;
 
 	error = dpm_suspend_end(PMSG_FREEZE);
@@ -708,7 +708,7 @@ int hibernate(void)
 	}
 
 	error = snapshot_key_init();
-	if (error)
+	if (error && snapshot_is_enforce_auth())
 		return error;
 
 	error = snapshot_create_trampoline();
@@ -1251,6 +1251,8 @@ static int __init hibernate_setup(char *str)
 	} else if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)
 		   && !strncmp(str, "protect_image", 13)) {
 		enable_restore_image_protection();
+	} else if (!strncmp(str, "enforce_auth", 10)) {
+		snapshot_set_enforce_auth();
 	}
 	return 1;
 }
diff --git a/kernel/power/power.h b/kernel/power/power.h
index d2fc73b2e200..edb63991bcdc 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -36,6 +36,7 @@ struct swsusp_info {
 struct trampoline {
 	bool snapshot_key_valid;
 	int sig_verify_ret;
+	bool enforce_auth;
 	u8 snapshot_key[SNAPSHOT_KEY_SIZE];
 } __aligned(PAGE_SIZE);
 
@@ -51,6 +52,8 @@ extern int snapshot_prepare_crypto(bool may_sleep, bool create_iv);
 extern void snapshot_finish_crypto(void);
 extern int snapshot_prepare_hash(bool may_sleep);
 extern void snapshot_finish_hash(void);
+extern void snapshot_set_enforce_auth(void);
+extern int snapshot_is_enforce_auth(void);
 /* kernel/power/snapshot_key.c */
 extern int snapshot_key_init(void);
 extern bool snapshot_key_initialized(void);
@@ -65,6 +68,8 @@ static inline int snapshot_prepare_crypto(bool may_sleep, bool create_iv) { retu
 static inline void snapshot_finish_crypto(void) {}
 static inline int snapshot_prepare_hash(bool may_sleep) { return 0; }
 static inline void snapshot_finish_hash(void) {}
+static inline void snapshot_set_enforce_auth(void) {}
+static inline int snapshot_is_enforce_auth(void) { return 0; }
 static inline int snapshot_key_init(void) { return 0; }
 static inline void snapshot_key_trampoline_backup(struct trampoline *t) {}
 static inline void snapshot_key_trampoline_restore(struct trampoline *t) {}
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index dd176df75d2b..803340dfe8ef 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1537,6 +1537,23 @@ static int decrypt_data_page(void *encrypted_page)
 	return ret;
 }
 
+/* enforce the snapshot to be signed */
+#ifdef CONFIG_HIBERNATION_ENC_AUTH_FORCE
+static bool enforce_auth = true;
+#else
+static bool enforce_auth;
+#endif
+
+void snapshot_set_enforce_auth(void)
+{
+	enforce_auth = true;
+}
+
+int snapshot_is_enforce_auth(void)
+{
+	return enforce_auth;
+}
+
 /*
  * Signature of snapshot image
  */
@@ -1603,6 +1620,8 @@ int snapshot_prepare_hash(bool may_sleep)
 	crypto_free_shash(tfm);
 	s4_verify_digest = NULL;
 	s4_verify_desc = NULL;
+	if (!enforce_auth)
+		ret = 0;
 	return ret;
 }
 
@@ -1664,6 +1683,8 @@ int snapshot_image_verify_decrypt(void)
 		pr_warn("Signature verification failed: %d\n", ret);
  error:
 	sig_verify_ret = ret;
+	if (!enforce_auth)
+		ret = 0;
 	return ret;
 }
 
@@ -1751,6 +1772,7 @@ static void load_signature(struct swsusp_info *info)
 
 static void init_sig_verify(struct trampoline *t)
 {
+	t->enforce_auth = enforce_auth;
 	t->sig_verify_ret = sig_verify_ret;
 	t->snapshot_key_valid = snapshot_key_valid;
 	sig_verify_ret = 0;
@@ -1759,11 +1781,25 @@ static void init_sig_verify(struct trampoline *t)
 
 static void handle_sig_verify(struct trampoline *t)
 {
-	if (t->sig_verify_ret)
+	enforce_auth = t->enforce_auth;
+	if (enforce_auth)
+		pr_info("Enforce the snapshot to be validly signed\n");
+
+	if (t->sig_verify_ret) {
 		pr_warn("Signature verification failed: %d\n",
 			t->sig_verify_ret);
-	else if (t->snapshot_key_valid)
+		if (t->snapshot_key_valid)
+			pr_warn("Did not find valid snapshot key.\n");
+		/* taint kernel */
+		if (!enforce_auth) {
+			pr_warn("System resumed from unsafe snapshot - "
+				"tainting kernel\n");
+			add_taint(TAINT_UNSAFE_HIBERNATE, LOCKDEP_STILL_OK);
+			pr_info("%s\n", print_tainted());
+		}
+	} else if (t->snapshot_key_valid) {
 		pr_info("Signature verification passed.\n");
+	}
 }
 #else
 static int
diff --git a/kernel/power/user.c b/kernel/power/user.c
index d5c8f777e8d8..9597f48f01d0 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -261,7 +261,7 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 			break;
 		}
 		error = snapshot_key_init();
-		if (error)
+		if (error && snapshot_is_enforce_auth())
 			return error;
 		error = snapshot_create_trampoline();
 		if (error)
-- 
2.13.6

Re: [PATCH 5/5] PM / hibernate: An option to request that snapshot image must be authenticated

[Randy Dunlap]

Hi,

On 9/12/18 7:23 AM, Lee, Chun-Yi wrote:
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 7c5c30149dbc..3c998fd6dc4c 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -90,6 +90,17 @@ config HIBERNATION_ENC_AUTH
>  	  master key of hibernation. The TPM trusted key depends on TPM. The
>  	  security of user defined key relies on user space.
>  
> +config HIBERNATION_ENC_AUTH_FORCE
> +	bool "Require hibernate snapshot image to be validly signed"
> +	depends on HIBERNATION_ENC_AUTH
> +	help
> +	  This option will prevent that a snapshot image without (valid)
> +	  signature be restored. Without this option, a unauthenticated

	                                              an

> +	  snapshot image can be restored but the restored kernel will be
> +	  tainted. Which also means that the hibernation can be triggered

s/Which/This/

or like this:
	  tainted, which also

> +	  without snapshot key but kernel will be tainted without this
> +	  option.
> +


-- 
~Randy

Re: [PATCH 5/5] PM / hibernate: An option to request that snapshot image must be authenticated

[joeyli]

Hi Randy, 

On Wed, Sep 12, 2018 at 09:24:38AM -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 7c5c30149dbc..3c998fd6dc4c 100644
> > --- a/kernel/power/Kconfig
> > +++ b/kernel/power/Kconfig
> > @@ -90,6 +90,17 @@ config HIBERNATION_ENC_AUTH
> >  	  master key of hibernation. The TPM trusted key depends on TPM. The
> >  	  security of user defined key relies on user space.
> >  
> > +config HIBERNATION_ENC_AUTH_FORCE
> > +	bool "Require hibernate snapshot image to be validly signed"
> > +	depends on HIBERNATION_ENC_AUTH
> > +	help
> > +	  This option will prevent that a snapshot image without (valid)
> > +	  signature be restored. Without this option, a unauthenticated
> 
> 	                                              an
> 
> > +	  snapshot image can be restored but the restored kernel will be
> > +	  tainted. Which also means that the hibernation can be triggered
> 
> s/Which/This/
> 
> or like this:
> 	  tainted, which also
>

Thanks for your review and suggestion! I will update the description in
next version.

Regards
Joey Lee

[PATCH 0/5 v2][RFC] Encryption and authentication for hibernate snapshot image

[Lee, Chun-Yi]

Hi,

This patchset is the implementation of encryption and authentication
for hibernate snapshot image. The image will be encrypted by AES and
authenticated by HMAC.

The hibernate function can be used to snapshot memory pages to an image,
then kernel restores the image to memory space in a appropriate time.
There have secrets in snapshot image and cracker may modifies it for
hacking system. Encryption and authentication of snapshot image can protect
the system.

Hibernate function requests the master key through key retention service.
The snapshot master key can be a trusted key or a user defined key. The
name of snapshot master key is fixed to "swsusp-kmk". User should loads
swsusp-kmk to kernel by keyctl tool before the hibernation resume.
e.g. The swsusp-kmk must be loaded before systemd-hibernate-resume

The TPM trusted key type is preferred to be the master key. But user
defined key can also be used for testing or when the platform doesn't
have TPM. User must be aware that the security of user key relies on
user space. If the root account be compromised, then the user key will
easy to be grabbed.

v2:
- Fixed bug of trusted_key_init's return value.
- Fixed wording in Kconfig
- Removed VLA usage
- Removed the checking of capability for writing disk_kmk.
- Fixed Kconfig, select trusted key.
- Add memory barrier before setting key initialized flag.
- Add memory barrier after cleaning key initialized flag.

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>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>

Lee, Chun-Yi (5):
  PM / hibernate: Create snapshot keys handler
  PM / hibernate: Generate and verify signature for snapshot image
  PM / hibernate: Encrypt snapshot image
  PM / hibernate: Erase the snapshot master key in snapshot pages
  PM / hibernate: An option to request that snapshot image must be
    authenticated

 Documentation/admin-guide/kernel-parameters.txt |   6 +
 include/linux/kernel.h                          |   3 +-
 kernel/panic.c                                  |   1 +
 kernel/power/Kconfig                            |  25 +
 kernel/power/Makefile                           |   1 +
 kernel/power/hibernate.c                        |  59 ++-
 kernel/power/power.h                            |  59 +++
 kernel/power/snapshot.c                         | 576 +++++++++++++++++++++++-
 kernel/power/snapshot_key.c                     | 312 +++++++++++++
 kernel/power/swap.c                             |   6 +
 kernel/power/user.c                             |  12 +
 11 files changed, 1042 insertions(+), 18 deletions(-)
 create mode 100644 kernel/power/snapshot_key.c

-- 
2.13.6

[PATCH 3/5] PM / hibernate: Encrypt snapshot image

[Lee, Chun-Yi]

To protect the secret in memory snapshot image, this patch adds the
logic to encrypt snapshot pages by AES-CTR. Using AES-CTR is because
it's simple, fast and parallelizable. But this patch didn't implement
parallel encryption.

The encrypt key is derived from the snapshot key. And the initialization
vector will be kept in snapshot header for resuming.

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>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 kernel/power/hibernate.c |   8 ++-
 kernel/power/power.h     |   6 ++
 kernel/power/snapshot.c  | 154 ++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 164 insertions(+), 4 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 0dda6a9f0af1..5ac2ab6f4a0e 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -275,10 +275,14 @@ static int create_image(int platform_mode)
 	if (error)
 		return error;
 
+	error = snapshot_prepare_crypto(false, true);
+	if (error)
+		goto finish_hash;
+
 	error = dpm_suspend_end(PMSG_FREEZE);
 	if (error) {
 		pr_err("Some devices failed to power down, aborting hibernation\n");
-		goto finish_hash;
+		goto finish_crypto;
 	}
 
 	error = platform_pre_snapshot(platform_mode);
@@ -335,6 +339,8 @@ static int create_image(int platform_mode)
 	dpm_resume_start(in_suspend ?
 		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
 
+ finish_crypto:
+	snapshot_finish_crypto();
  finish_hash:
 	snapshot_finish_hash();
 
diff --git a/kernel/power/power.h b/kernel/power/power.h
index c614b0a294e3..41263fdd3a54 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -5,6 +5,7 @@
 #include <linux/freezer.h>
 #include <linux/compiler.h>
 #include <crypto/sha.h>
+#include <crypto/aes.h>
 
 /* The max size of encrypted key blob */
 #define KEY_BLOB_BUFF_LEN 512
@@ -24,6 +25,7 @@ struct swsusp_info {
 	unsigned long		pages;
 	unsigned long		size;
 	unsigned long		trampoline_pfn;
+	u8			iv[AES_BLOCK_SIZE];
 	u8			signature[SNAPSHOT_DIGEST_SIZE];
 } __aligned(PAGE_SIZE);
 
@@ -44,6 +46,8 @@ extern void __init hibernate_image_size_init(void);
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
 /* kernel/power/snapshot.c */
 extern int snapshot_image_verify_decrypt(void);
+extern int snapshot_prepare_crypto(bool may_sleep, bool create_iv);
+extern void snapshot_finish_crypto(void);
 extern int snapshot_prepare_hash(bool may_sleep);
 extern void snapshot_finish_hash(void);
 /* kernel/power/snapshot_key.c */
@@ -53,6 +57,8 @@ 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_image_verify_decrypt(void) { return 0; }
+static inline int snapshot_prepare_crypto(bool may_sleep, bool create_iv) { return 0; }
+static inline void snapshot_finish_crypto(void) {}
 static inline int snapshot_prepare_hash(bool may_sleep) { return 0; }
 static inline void snapshot_finish_hash(void) {}
 static inline int snapshot_key_init(void) { return 0; }
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index e817c035f378..cd10ab5e4850 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -41,7 +41,11 @@
 #include <asm/tlbflush.h>
 #include <asm/io.h>
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
+#include <linux/random.h>
+#include <linux/scatterlist.h>
+#include <crypto/aes.h>
 #include <crypto/hash.h>
+#include <crypto/skcipher.h>
 #endif
 
 #include "power.h"
@@ -1413,6 +1417,127 @@ static unsigned int nr_copy_pages;
 static void **h_buf;
 
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
+static struct skcipher_request *sk_req;
+static u8 iv[AES_BLOCK_SIZE];
+static void *c_buffer;
+
+static void init_iv(struct swsusp_info *info)
+{
+	memcpy(info->iv, iv, AES_BLOCK_SIZE);
+}
+
+static void load_iv(struct swsusp_info *info)
+{
+	memcpy(iv, info->iv, AES_BLOCK_SIZE);
+}
+
+int snapshot_prepare_crypto(bool may_sleep, bool create_iv)
+{
+	char enc_key[DERIVED_KEY_SIZE];
+	struct crypto_skcipher *tfm;
+	int ret = 0;
+
+	ret = snapshot_get_enc_key(enc_key, may_sleep);
+	if (ret) {
+		pr_warn_once("enc key is invalid\n");
+		return -EINVAL;
+	}
+
+	c_buffer = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!c_buffer) {
+		pr_err("Allocate crypto buffer page failed\n");
+		return -ENOMEM;
+	}
+
+	tfm = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);
+	if (IS_ERR(tfm)) {
+		ret = PTR_ERR(tfm);
+		pr_err("failed to allocate skcipher (%d)\n", ret);
+		goto alloc_fail;
+	}
+
+	ret = crypto_skcipher_setkey(tfm, enc_key, AES_MAX_KEY_SIZE);
+	if (ret) {
+		pr_err("failed to setkey (%d)\n", ret);
+		goto set_fail;
+	}
+
+	sk_req = skcipher_request_alloc(tfm, GFP_KERNEL);
+	if (!sk_req) {
+		pr_err("failed to allocate request\n");
+		ret = -ENOMEM;
+		goto set_fail;
+	}
+	if (may_sleep)
+		skcipher_request_set_callback(sk_req, CRYPTO_TFM_REQ_MAY_SLEEP,
+						NULL, NULL);
+	if (create_iv)
+		get_random_bytes(iv, AES_BLOCK_SIZE);
+
+	return 0;
+
+set_fail:
+	crypto_free_skcipher(tfm);
+alloc_fail:
+	__free_page(c_buffer);
+
+	return ret;
+}
+
+void snapshot_finish_crypto(void)
+{
+	struct crypto_skcipher *tfm;
+ 
+	if (!sk_req)
+		return;
+
+	tfm = crypto_skcipher_reqtfm(sk_req);
+	skcipher_request_zero(sk_req);
+	skcipher_request_free(sk_req);
+	crypto_free_skcipher(tfm);
+	__free_page(c_buffer);
+	sk_req = NULL;
+}
+
+static int encrypt_data_page(void *hash_buffer)
+{
+	struct scatterlist src[1], dst[1];
+	u8 iv_tmp[AES_BLOCK_SIZE];
+	int ret = 0;
+
+	if (!sk_req)
+		return 0;
+
+	memcpy(iv_tmp, iv, sizeof(iv));
+	sg_init_one(src, hash_buffer, PAGE_SIZE);
+	sg_init_one(dst, c_buffer, PAGE_SIZE);
+	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
+	ret = crypto_skcipher_encrypt(sk_req);
+
+	copy_page(hash_buffer, c_buffer);
+	memset(c_buffer, 0, PAGE_SIZE);
+
+	return ret;
+}
+
+static int decrypt_data_page(void *encrypted_page)
+{
+	struct scatterlist src[1], dst[1];
+	u8 iv_tmp[AES_BLOCK_SIZE];
+	int ret = 0;
+
+	memcpy(iv_tmp, iv, sizeof(iv));
+	sg_init_one(src, encrypted_page, PAGE_SIZE);
+	sg_init_one(dst, c_buffer, PAGE_SIZE);
+	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
+	ret = crypto_skcipher_decrypt(sk_req);
+
+	copy_page(encrypted_page, c_buffer);
+	memset(c_buffer, 0, PAGE_SIZE);
+
+	return ret;
+}
+
 /*
  * Signature of snapshot image
  */
@@ -1508,22 +1633,30 @@ int snapshot_image_verify_decrypt(void)
 	if (ret || !s4_verify_desc)
 		goto error_prep;
 
+	ret = snapshot_prepare_crypto(true, false);
+	if (ret)
+		goto error_prep;
+
 	for (i = 0; i < nr_copy_pages; i++) {
 		ret = crypto_shash_update(s4_verify_desc, *(h_buf + i), PAGE_SIZE);
 		if (ret)
-			goto error_shash;
+			goto error_shash_crypto;
+		ret = decrypt_data_page(*(h_buf + i));
+		if (ret)
+			goto error_shash_crypto;
 	}
 
 	ret = crypto_shash_final(s4_verify_desc, s4_verify_digest);
 	if (ret)
-		goto error_shash;
+		goto error_shash_crypto;
 
 	pr_debug("Signature %*phN\n", SNAPSHOT_DIGEST_SIZE, signature);
 	pr_debug("Digest    %*phN\n", SNAPSHOT_DIGEST_SIZE, s4_verify_digest);
 	if (memcmp(signature, s4_verify_digest, SNAPSHOT_DIGEST_SIZE))
 		ret = -EKEYREJECTED;
 
- error_shash:
+ error_shash_crypto:
+	snapshot_finish_crypto();
 	snapshot_finish_hash();
 
  error_prep:
@@ -1564,6 +1697,17 @@ __copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 			crypto_buffer = page_address(d_page);
 		}
 
+		/* Encrypt hashed page */
+		encrypt_data_page(crypto_buffer);
+
+		/* Copy encrypted buffer to destination page in high memory */
+		if (PageHighMem(d_page)) {
+			void *kaddr = kmap_atomic(d_page);
+
+			copy_page(kaddr, crypto_buffer);
+			kunmap_atomic(kaddr);
+		}
+
 		/* Generate digest */
 		if (!s4_verify_desc)
 			continue;
@@ -1638,6 +1782,8 @@ __copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 }
 
 static inline void alloc_h_buf(void) {}
+static inline void init_iv(struct swsusp_info *info) {}
+static inline void load_iv(struct swsusp_info *info) {}
 static inline void init_signature(struct swsusp_info *info) {}
 static inline void load_signature(struct swsusp_info *info) {}
 static inline void init_sig_verify(struct trampoline *t) {}
@@ -2286,6 +2432,7 @@ static int init_header(struct swsusp_info *info)
 	info->size = info->pages;
 	info->size <<= PAGE_SHIFT;
 	info->trampoline_pfn = page_to_pfn(virt_to_page(trampoline_virt));
+	init_iv(info);
 	init_signature(info);
 	return init_header_complete(info);
 }
@@ -2524,6 +2671,7 @@ static int load_header(struct swsusp_info *info)
 		nr_copy_pages = info->image_pages;
 		nr_meta_pages = info->pages - info->image_pages - 1;
 		trampoline_pfn = info->trampoline_pfn;
+		load_iv(info);
 		load_signature(info);
 	}
 	return error;
-- 
2.13.6

[PATCH 3/5] PM / hibernate: Encrypt snapshot image

[Lee, Chun-Yi]

To protect the secret in memory snapshot image, this patch adds the
logic to encrypt snapshot pages by AES-CTR. Using AES-CTR is because
it's simple, fast and parallelizable. But this patch didn't implement
parallel encryption.

The encrypt key is derived from the snapshot key. And the initialization
vector will be kept in snapshot header for resuming.

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>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 kernel/power/hibernate.c |   8 ++-
 kernel/power/power.h     |   6 ++
 kernel/power/snapshot.c  | 154 ++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 164 insertions(+), 4 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 0dda6a9f0af1..5ac2ab6f4a0e 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -275,10 +275,14 @@ static int create_image(int platform_mode)
 	if (error)
 		return error;
 
+	error = snapshot_prepare_crypto(false, true);
+	if (error)
+		goto finish_hash;
+
 	error = dpm_suspend_end(PMSG_FREEZE);
 	if (error) {
 		pr_err("Some devices failed to power down, aborting hibernation\n");
-		goto finish_hash;
+		goto finish_crypto;
 	}
 
 	error = platform_pre_snapshot(platform_mode);
@@ -335,6 +339,8 @@ static int create_image(int platform_mode)
 	dpm_resume_start(in_suspend ?
 		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
 
+ finish_crypto:
+	snapshot_finish_crypto();
  finish_hash:
 	snapshot_finish_hash();
 
diff --git a/kernel/power/power.h b/kernel/power/power.h
index c614b0a294e3..41263fdd3a54 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -5,6 +5,7 @@
 #include <linux/freezer.h>
 #include <linux/compiler.h>
 #include <crypto/sha.h>
+#include <crypto/aes.h>
 
 /* The max size of encrypted key blob */
 #define KEY_BLOB_BUFF_LEN 512
@@ -24,6 +25,7 @@ struct swsusp_info {
 	unsigned long		pages;
 	unsigned long		size;
 	unsigned long		trampoline_pfn;
+	u8			iv[AES_BLOCK_SIZE];
 	u8			signature[SNAPSHOT_DIGEST_SIZE];
 } __aligned(PAGE_SIZE);
 
@@ -44,6 +46,8 @@ extern void __init hibernate_image_size_init(void);
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
 /* kernel/power/snapshot.c */
 extern int snapshot_image_verify_decrypt(void);
+extern int snapshot_prepare_crypto(bool may_sleep, bool create_iv);
+extern void snapshot_finish_crypto(void);
 extern int snapshot_prepare_hash(bool may_sleep);
 extern void snapshot_finish_hash(void);
 /* kernel/power/snapshot_key.c */
@@ -53,6 +57,8 @@ 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_image_verify_decrypt(void) { return 0; }
+static inline int snapshot_prepare_crypto(bool may_sleep, bool create_iv) { return 0; }
+static inline void snapshot_finish_crypto(void) {}
 static inline int snapshot_prepare_hash(bool may_sleep) { return 0; }
 static inline void snapshot_finish_hash(void) {}
 static inline int snapshot_key_init(void) { return 0; }
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index e817c035f378..cd10ab5e4850 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -41,7 +41,11 @@
 #include <asm/tlbflush.h>
 #include <asm/io.h>
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
+#include <linux/random.h>
+#include <linux/scatterlist.h>
+#include <crypto/aes.h>
 #include <crypto/hash.h>
+#include <crypto/skcipher.h>
 #endif
 
 #include "power.h"
@@ -1413,6 +1417,127 @@ static unsigned int nr_copy_pages;
 static void **h_buf;
 
 #ifdef CONFIG_HIBERNATION_ENC_AUTH
+static struct skcipher_request *sk_req;
+static u8 iv[AES_BLOCK_SIZE];
+static void *c_buffer;
+
+static void init_iv(struct swsusp_info *info)
+{
+	memcpy(info->iv, iv, AES_BLOCK_SIZE);
+}
+
+static void load_iv(struct swsusp_info *info)
+{
+	memcpy(iv, info->iv, AES_BLOCK_SIZE);
+}
+
+int snapshot_prepare_crypto(bool may_sleep, bool create_iv)
+{
+	char enc_key[DERIVED_KEY_SIZE];
+	struct crypto_skcipher *tfm;
+	int ret = 0;
+
+	ret = snapshot_get_enc_key(enc_key, may_sleep);
+	if (ret) {
+		pr_warn_once("enc key is invalid\n");
+		return -EINVAL;
+	}
+
+	c_buffer = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!c_buffer) {
+		pr_err("Allocate crypto buffer page failed\n");
+		return -ENOMEM;
+	}
+
+	tfm = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);
+	if (IS_ERR(tfm)) {
+		ret = PTR_ERR(tfm);
+		pr_err("failed to allocate skcipher (%d)\n", ret);
+		goto alloc_fail;
+	}
+
+	ret = crypto_skcipher_setkey(tfm, enc_key, AES_MAX_KEY_SIZE);
+	if (ret) {
+		pr_err("failed to setkey (%d)\n", ret);
+		goto set_fail;
+	}
+
+	sk_req = skcipher_request_alloc(tfm, GFP_KERNEL);
+	if (!sk_req) {
+		pr_err("failed to allocate request\n");
+		ret = -ENOMEM;
+		goto set_fail;
+	}
+	if (may_sleep)
+		skcipher_request_set_callback(sk_req, CRYPTO_TFM_REQ_MAY_SLEEP,
+						NULL, NULL);
+	if (create_iv)
+		get_random_bytes(iv, AES_BLOCK_SIZE);
+
+	return 0;
+
+set_fail:
+	crypto_free_skcipher(tfm);
+alloc_fail:
+	__free_page(c_buffer);
+
+	return ret;
+}
+
+void snapshot_finish_crypto(void)
+{
+	struct crypto_skcipher *tfm;
+ 
+	if (!sk_req)
+		return;
+
+	tfm = crypto_skcipher_reqtfm(sk_req);
+	skcipher_request_zero(sk_req);
+	skcipher_request_free(sk_req);
+	crypto_free_skcipher(tfm);
+	__free_page(c_buffer);
+	sk_req = NULL;
+}
+
+static int encrypt_data_page(void *hash_buffer)
+{
+	struct scatterlist src[1], dst[1];
+	u8 iv_tmp[AES_BLOCK_SIZE];
+	int ret = 0;
+
+	if (!sk_req)
+		return 0;
+
+	memcpy(iv_tmp, iv, sizeof(iv));
+	sg_init_one(src, hash_buffer, PAGE_SIZE);
+	sg_init_one(dst, c_buffer, PAGE_SIZE);
+	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
+	ret = crypto_skcipher_encrypt(sk_req);
+
+	copy_page(hash_buffer, c_buffer);
+	memset(c_buffer, 0, PAGE_SIZE);
+
+	return ret;
+}
+
+static int decrypt_data_page(void *encrypted_page)
+{
+	struct scatterlist src[1], dst[1];
+	u8 iv_tmp[AES_BLOCK_SIZE];
+	int ret = 0;
+
+	memcpy(iv_tmp, iv, sizeof(iv));
+	sg_init_one(src, encrypted_page, PAGE_SIZE);
+	sg_init_one(dst, c_buffer, PAGE_SIZE);
+	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
+	ret = crypto_skcipher_decrypt(sk_req);
+
+	copy_page(encrypted_page, c_buffer);
+	memset(c_buffer, 0, PAGE_SIZE);
+
+	return ret;
+}
+
 /*
  * Signature of snapshot image
  */
@@ -1508,22 +1633,30 @@ int snapshot_image_verify_decrypt(void)
 	if (ret || !s4_verify_desc)
 		goto error_prep;
 
+	ret = snapshot_prepare_crypto(true, false);
+	if (ret)
+		goto error_prep;
+
 	for (i = 0; i < nr_copy_pages; i++) {
 		ret = crypto_shash_update(s4_verify_desc, *(h_buf + i), PAGE_SIZE);
 		if (ret)
-			goto error_shash;
+			goto error_shash_crypto;
+		ret = decrypt_data_page(*(h_buf + i));
+		if (ret)
+			goto error_shash_crypto;
 	}
 
 	ret = crypto_shash_final(s4_verify_desc, s4_verify_digest);
 	if (ret)
-		goto error_shash;
+		goto error_shash_crypto;
 
 	pr_debug("Signature %*phN\n", SNAPSHOT_DIGEST_SIZE, signature);
 	pr_debug("Digest    %*phN\n", SNAPSHOT_DIGEST_SIZE, s4_verify_digest);
 	if (memcmp(signature, s4_verify_digest, SNAPSHOT_DIGEST_SIZE))
 		ret = -EKEYREJECTED;
 
- error_shash:
+ error_shash_crypto:
+	snapshot_finish_crypto();
 	snapshot_finish_hash();
 
  error_prep:
@@ -1564,6 +1697,17 @@ __copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 			crypto_buffer = page_address(d_page);
 		}
 
+		/* Encrypt hashed page */
+		encrypt_data_page(crypto_buffer);
+
+		/* Copy encrypted buffer to destination page in high memory */
+		if (PageHighMem(d_page)) {
+			void *kaddr = kmap_atomic(d_page);
+
+			copy_page(kaddr, crypto_buffer);
+			kunmap_atomic(kaddr);
+		}
+
 		/* Generate digest */
 		if (!s4_verify_desc)
 			continue;
@@ -1638,6 +1782,8 @@ __copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 }
 
 static inline void alloc_h_buf(void) {}
+static inline void init_iv(struct swsusp_info *info) {}
+static inline void load_iv(struct swsusp_info *info) {}
 static inline void init_signature(struct swsusp_info *info) {}
 static inline void load_signature(struct swsusp_info *info) {}
 static inline void init_sig_verify(struct trampoline *t) {}
@@ -2286,6 +2432,7 @@ static int init_header(struct swsusp_info *info)
 	info->size = info->pages;
 	info->size <<= PAGE_SHIFT;
 	info->trampoline_pfn = page_to_pfn(virt_to_page(trampoline_virt));
+	init_iv(info);
 	init_signature(info);
 	return init_header_complete(info);
 }
@@ -2524,6 +2671,7 @@ static int load_header(struct swsusp_info *info)
 		nr_copy_pages = info->image_pages;
 		nr_meta_pages = info->pages - info->image_pages - 1;
 		trampoline_pfn = info->trampoline_pfn;
+		load_iv(info);
 		load_signature(info);
 	}
 	return error;
-- 
2.13.6

Re: [PATCH 3/5] PM / hibernate: Encrypt snapshot image

[Stephan Mueller]

Am Donnerstag, 3. Januar 2019, 15:32:25 CET schrieb Lee, Chun-Yi:

Hi Chun,

> To protect the secret in memory snapshot image, this patch adds the
> logic to encrypt snapshot pages by AES-CTR. Using AES-CTR is because
> it's simple, fast and parallelizable. But this patch didn't implement
> parallel encryption.
> 
> The encrypt key is derived from the snapshot key. And the initialization
> vector will be kept in snapshot header for resuming.
> 
> 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>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
>  kernel/power/hibernate.c |   8 ++-
>  kernel/power/power.h     |   6 ++
>  kernel/power/snapshot.c  | 154
> ++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 164
> insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 0dda6a9f0af1..5ac2ab6f4a0e 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -275,10 +275,14 @@ static int create_image(int platform_mode)
>  	if (error)
>  		return error;
> 
> +	error = snapshot_prepare_crypto(false, true);
> +	if (error)
> +		goto finish_hash;
> +
>  	error = dpm_suspend_end(PMSG_FREEZE);
>  	if (error) {
>  		pr_err("Some devices failed to power down, aborting hibernation\n");
> -		goto finish_hash;
> +		goto finish_crypto;
>  	}
> 
>  	error = platform_pre_snapshot(platform_mode);
> @@ -335,6 +339,8 @@ static int create_image(int platform_mode)
>  	dpm_resume_start(in_suspend ?
>  		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
> 
> + finish_crypto:
> +	snapshot_finish_crypto();
>   finish_hash:
>  	snapshot_finish_hash();
> 
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index c614b0a294e3..41263fdd3a54 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -5,6 +5,7 @@
>  #include <linux/freezer.h>
>  #include <linux/compiler.h>
>  #include <crypto/sha.h>
> +#include <crypto/aes.h>
> 
>  /* The max size of encrypted key blob */
>  #define KEY_BLOB_BUFF_LEN 512
> @@ -24,6 +25,7 @@ struct swsusp_info {
>  	unsigned long		pages;
>  	unsigned long		size;
>  	unsigned long		trampoline_pfn;
> +	u8			iv[AES_BLOCK_SIZE];
>  	u8			signature[SNAPSHOT_DIGEST_SIZE];
>  } __aligned(PAGE_SIZE);
> 
> @@ -44,6 +46,8 @@ extern void __init hibernate_image_size_init(void);
>  #ifdef CONFIG_HIBERNATION_ENC_AUTH
>  /* kernel/power/snapshot.c */
>  extern int snapshot_image_verify_decrypt(void);
> +extern int snapshot_prepare_crypto(bool may_sleep, bool create_iv);
> +extern void snapshot_finish_crypto(void);
>  extern int snapshot_prepare_hash(bool may_sleep);
>  extern void snapshot_finish_hash(void);
>  /* kernel/power/snapshot_key.c */
> @@ -53,6 +57,8 @@ 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_image_verify_decrypt(void) { return 0; }
> +static inline int snapshot_prepare_crypto(bool may_sleep, bool create_iv) {
> return 0; } +static inline void snapshot_finish_crypto(void) {}
>  static inline int snapshot_prepare_hash(bool may_sleep) { return 0; }
>  static inline void snapshot_finish_hash(void) {}
>  static inline int snapshot_key_init(void) { return 0; }
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index e817c035f378..cd10ab5e4850 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -41,7 +41,11 @@
>  #include <asm/tlbflush.h>
>  #include <asm/io.h>
>  #ifdef CONFIG_HIBERNATION_ENC_AUTH
> +#include <linux/random.h>
> +#include <linux/scatterlist.h>
> +#include <crypto/aes.h>
>  #include <crypto/hash.h>
> +#include <crypto/skcipher.h>
>  #endif
> 
>  #include "power.h"
> @@ -1413,6 +1417,127 @@ static unsigned int nr_copy_pages;
>  static void **h_buf;
> 
>  #ifdef CONFIG_HIBERNATION_ENC_AUTH
> +static struct skcipher_request *sk_req;
> +static u8 iv[AES_BLOCK_SIZE];

May I ask for a different name here? The variable iv is used throughout the 
kernel crypto API and it is always a challenge when doing code reviews to 
trace the right variable when using common names :-)

> +static void *c_buffer;
> +
> +static void init_iv(struct swsusp_info *info)
> +{
> +	memcpy(info->iv, iv, AES_BLOCK_SIZE);
> +}
> +
> +static void load_iv(struct swsusp_info *info)
> +{
> +	memcpy(iv, info->iv, AES_BLOCK_SIZE);
> +}
> +
> +int snapshot_prepare_crypto(bool may_sleep, bool create_iv)
> +{
> +	char enc_key[DERIVED_KEY_SIZE];
> +	struct crypto_skcipher *tfm;
> +	int ret = 0;
> +
> +	ret = snapshot_get_enc_key(enc_key, may_sleep);
> +	if (ret) {
> +		pr_warn_once("enc key is invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	c_buffer = (void *)get_zeroed_page(GFP_KERNEL);
> +	if (!c_buffer) {
> +		pr_err("Allocate crypto buffer page failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	tfm = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);

What is the reason for choosing CTR-AES to store data on disk?

> +	if (IS_ERR(tfm)) {
> +		ret = PTR_ERR(tfm);
> +		pr_err("failed to allocate skcipher (%d)\n", ret);
> +		goto alloc_fail;
> +	}
> +
> +	ret = crypto_skcipher_setkey(tfm, enc_key, AES_MAX_KEY_SIZE);
> +	if (ret) {
> +		pr_err("failed to setkey (%d)\n", ret);
> +		goto set_fail;
> +	}
> +
> +	sk_req = skcipher_request_alloc(tfm, GFP_KERNEL);
> +	if (!sk_req) {
> +		pr_err("failed to allocate request\n");
> +		ret = -ENOMEM;
> +		goto set_fail;
> +	}
> +	if (may_sleep)
> +		skcipher_request_set_callback(sk_req, CRYPTO_TFM_REQ_MAY_SLEEP,
> +						NULL, NULL);
> +	if (create_iv)
> +		get_random_bytes(iv, AES_BLOCK_SIZE);
> +
> +	return 0;
> +
> +set_fail:
> +	crypto_free_skcipher(tfm);
> +alloc_fail:
> +	__free_page(c_buffer);

May I recommend to memzero_explicit(enc_key)?

> +
> +	return ret;
> +}
> +
> +void snapshot_finish_crypto(void)
> +{
> +	struct crypto_skcipher *tfm;
> +
> +	if (!sk_req)
> +		return;
> +
> +	tfm = crypto_skcipher_reqtfm(sk_req);
> +	skcipher_request_zero(sk_req);
> +	skcipher_request_free(sk_req);
> +	crypto_free_skcipher(tfm);
> +	__free_page(c_buffer);
> +	sk_req = NULL;
> +}
> +
> +static int encrypt_data_page(void *hash_buffer)
> +{
> +	struct scatterlist src[1], dst[1];
> +	u8 iv_tmp[AES_BLOCK_SIZE];
> +	int ret = 0;
> +
> +	if (!sk_req)
> +		return 0;
> +
> +	memcpy(iv_tmp, iv, sizeof(iv));

Why do you copy the IV? If I see that right, we would have a key/counter 
collision as follows:

1. you copy the IV into a tmp variable

2. CTR AES is invoked which updates iv_tmp

3. iv_tmp is discarded

4. a repeated invocation of this function would again use the initially set IV 
to copy it into iv_tmp which means that the subsequent cipher operation uses 
yet again the same IV.

If my hunch is correct, the cryptographic strength of the cipher is defeated.

> +	sg_init_one(src, hash_buffer, PAGE_SIZE);
> +	sg_init_one(dst, c_buffer, PAGE_SIZE);
> +	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
> +	ret = crypto_skcipher_encrypt(sk_req);
> +
> +	copy_page(hash_buffer, c_buffer);
> +	memset(c_buffer, 0, PAGE_SIZE);
> +
> +	return ret;
> +}
> +
> +static int decrypt_data_page(void *encrypted_page)

This function looks almost identical to encrypt_data_page - may I suggest to 
collapse it into one function?

> +{
> +	struct scatterlist src[1], dst[1];
> +	u8 iv_tmp[AES_BLOCK_SIZE];
> +	int ret = 0;
> +
> +	memcpy(iv_tmp, iv, sizeof(iv));
> +	sg_init_one(src, encrypted_page, PAGE_SIZE);
> +	sg_init_one(dst, c_buffer, PAGE_SIZE);
> +	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
> +	ret = crypto_skcipher_decrypt(sk_req);
> +
> +	copy_page(encrypted_page, c_buffer);
> +	memset(c_buffer, 0, PAGE_SIZE);
> +
> +	return ret;
> +}
> +
>  /*
>   * Signature of snapshot image
>   */
> @@ -1508,22 +1633,30 @@ int snapshot_image_verify_decrypt(void)
>  	if (ret || !s4_verify_desc)
>  		goto error_prep;
> 
> +	ret = snapshot_prepare_crypto(true, false);
> +	if (ret)
> +		goto error_prep;
> +
>  	for (i = 0; i < nr_copy_pages; i++) {
>  		ret = crypto_shash_update(s4_verify_desc, *(h_buf + i), PAGE_SIZE);
>  		if (ret)
> -			goto error_shash;
> +			goto error_shash_crypto;
> +		ret = decrypt_data_page(*(h_buf + i));
> +		if (ret)
> +			goto error_shash_crypto;
>  	}
> 
>  	ret = crypto_shash_final(s4_verify_desc, s4_verify_digest);
>  	if (ret)
> -		goto error_shash;
> +		goto error_shash_crypto;
> 
>  	pr_debug("Signature %*phN\n", SNAPSHOT_DIGEST_SIZE, signature);
>  	pr_debug("Digest    %*phN\n", SNAPSHOT_DIGEST_SIZE, s4_verify_digest);
>  	if (memcmp(signature, s4_verify_digest, SNAPSHOT_DIGEST_SIZE))
>  		ret = -EKEYREJECTED;
> 
> - error_shash:
> + error_shash_crypto:
> +	snapshot_finish_crypto();
>  	snapshot_finish_hash();
> 
>   error_prep:
> @@ -1564,6 +1697,17 @@ __copy_data_pages(struct memory_bitmap *copy_bm,
> struct memory_bitmap *orig_bm) crypto_buffer = page_address(d_page);
>  		}
> 
> +		/* Encrypt hashed page */
> +		encrypt_data_page(crypto_buffer);
> +
> +		/* Copy encrypted buffer to destination page in high memory */
> +		if (PageHighMem(d_page)) {
> +			void *kaddr = kmap_atomic(d_page);
> +
> +			copy_page(kaddr, crypto_buffer);
> +			kunmap_atomic(kaddr);
> +		}
> +
>  		/* Generate digest */
>  		if (!s4_verify_desc)
>  			continue;
> @@ -1638,6 +1782,8 @@ __copy_data_pages(struct memory_bitmap *copy_bm,
> struct memory_bitmap *orig_bm) }
> 
>  static inline void alloc_h_buf(void) {}
> +static inline void init_iv(struct swsusp_info *info) {}
> +static inline void load_iv(struct swsusp_info *info) {}
>  static inline void init_signature(struct swsusp_info *info) {}
>  static inline void load_signature(struct swsusp_info *info) {}
>  static inline void init_sig_verify(struct trampoline *t) {}
> @@ -2286,6 +2432,7 @@ static int init_header(struct swsusp_info *info)
>  	info->size = info->pages;
>  	info->size <<= PAGE_SHIFT;
>  	info->trampoline_pfn = page_to_pfn(virt_to_page(trampoline_virt));
> +	init_iv(info);
>  	init_signature(info);
>  	return init_header_complete(info);
>  }
> @@ -2524,6 +2671,7 @@ static int load_header(struct swsusp_info *info)
>  		nr_copy_pages = info->image_pages;
>  		nr_meta_pages = info->pages - info->image_pages - 1;
>  		trampoline_pfn = info->trampoline_pfn;
> +		load_iv(info);
>  		load_signature(info);
>  	}
>  	return error;



Ciao
Stephan

Re: [PATCH 3/5] PM / hibernate: Encrypt snapshot image

[Stephan Mueller]

Am Donnerstag, 3. Januar 2019, 15:32:25 CET schrieb Lee, Chun-Yi:

Hi Chun,

> To protect the secret in memory snapshot image, this patch adds the
> logic to encrypt snapshot pages by AES-CTR. Using AES-CTR is because
> it's simple, fast and parallelizable. But this patch didn't implement
> parallel encryption.
> 
> The encrypt key is derived from the snapshot key. And the initialization
> vector will be kept in snapshot header for resuming.
> 
> 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>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> ---
>  kernel/power/hibernate.c |   8 ++-
>  kernel/power/power.h     |   6 ++
>  kernel/power/snapshot.c  | 154
> ++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 164
> insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 0dda6a9f0af1..5ac2ab6f4a0e 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -275,10 +275,14 @@ static int create_image(int platform_mode)
>  	if (error)
>  		return error;
> 
> +	error = snapshot_prepare_crypto(false, true);
> +	if (error)
> +		goto finish_hash;
> +
>  	error = dpm_suspend_end(PMSG_FREEZE);
>  	if (error) {
>  		pr_err("Some devices failed to power down, aborting hibernation\n");
> -		goto finish_hash;
> +		goto finish_crypto;
>  	}
> 
>  	error = platform_pre_snapshot(platform_mode);
> @@ -335,6 +339,8 @@ static int create_image(int platform_mode)
>  	dpm_resume_start(in_suspend ?
>  		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
> 
> + finish_crypto:
> +	snapshot_finish_crypto();
>   finish_hash:
>  	snapshot_finish_hash();
> 
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index c614b0a294e3..41263fdd3a54 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -5,6 +5,7 @@
>  #include <linux/freezer.h>
>  #include <linux/compiler.h>
>  #include <crypto/sha.h>
> +#include <crypto/aes.h>
> 
>  /* The max size of encrypted key blob */
>  #define KEY_BLOB_BUFF_LEN 512
> @@ -24,6 +25,7 @@ struct swsusp_info {
>  	unsigned long		pages;
>  	unsigned long		size;
>  	unsigned long		trampoline_pfn;
> +	u8			iv[AES_BLOCK_SIZE];
>  	u8			signature[SNAPSHOT_DIGEST_SIZE];
>  } __aligned(PAGE_SIZE);
> 
> @@ -44,6 +46,8 @@ extern void __init hibernate_image_size_init(void);
>  #ifdef CONFIG_HIBERNATION_ENC_AUTH
>  /* kernel/power/snapshot.c */
>  extern int snapshot_image_verify_decrypt(void);
> +extern int snapshot_prepare_crypto(bool may_sleep, bool create_iv);
> +extern void snapshot_finish_crypto(void);
>  extern int snapshot_prepare_hash(bool may_sleep);
>  extern void snapshot_finish_hash(void);
>  /* kernel/power/snapshot_key.c */
> @@ -53,6 +57,8 @@ 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_image_verify_decrypt(void) { return 0; }
> +static inline int snapshot_prepare_crypto(bool may_sleep, bool create_iv) {
> return 0; } +static inline void snapshot_finish_crypto(void) {}
>  static inline int snapshot_prepare_hash(bool may_sleep) { return 0; }
>  static inline void snapshot_finish_hash(void) {}
>  static inline int snapshot_key_init(void) { return 0; }
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index e817c035f378..cd10ab5e4850 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -41,7 +41,11 @@
>  #include <asm/tlbflush.h>
>  #include <asm/io.h>
>  #ifdef CONFIG_HIBERNATION_ENC_AUTH
> +#include <linux/random.h>
> +#include <linux/scatterlist.h>
> +#include <crypto/aes.h>
>  #include <crypto/hash.h>
> +#include <crypto/skcipher.h>
>  #endif
> 
>  #include "power.h"
> @@ -1413,6 +1417,127 @@ static unsigned int nr_copy_pages;
>  static void **h_buf;
> 
>  #ifdef CONFIG_HIBERNATION_ENC_AUTH
> +static struct skcipher_request *sk_req;
> +static u8 iv[AES_BLOCK_SIZE];

May I ask for a different name here? The variable iv is used throughout the 
kernel crypto API and it is always a challenge when doing code reviews to 
trace the right variable when using common names :-)

> +static void *c_buffer;
> +
> +static void init_iv(struct swsusp_info *info)
> +{
> +	memcpy(info->iv, iv, AES_BLOCK_SIZE);
> +}
> +
> +static void load_iv(struct swsusp_info *info)
> +{
> +	memcpy(iv, info->iv, AES_BLOCK_SIZE);
> +}
> +
> +int snapshot_prepare_crypto(bool may_sleep, bool create_iv)
> +{
> +	char enc_key[DERIVED_KEY_SIZE];
> +	struct crypto_skcipher *tfm;
> +	int ret = 0;
> +
> +	ret = snapshot_get_enc_key(enc_key, may_sleep);
> +	if (ret) {
> +		pr_warn_once("enc key is invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	c_buffer = (void *)get_zeroed_page(GFP_KERNEL);
> +	if (!c_buffer) {
> +		pr_err("Allocate crypto buffer page failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	tfm = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);

What is the reason for choosing CTR-AES to store data on disk?

> +	if (IS_ERR(tfm)) {
> +		ret = PTR_ERR(tfm);
> +		pr_err("failed to allocate skcipher (%d)\n", ret);
> +		goto alloc_fail;
> +	}
> +
> +	ret = crypto_skcipher_setkey(tfm, enc_key, AES_MAX_KEY_SIZE);
> +	if (ret) {
> +		pr_err("failed to setkey (%d)\n", ret);
> +		goto set_fail;
> +	}
> +
> +	sk_req = skcipher_request_alloc(tfm, GFP_KERNEL);
> +	if (!sk_req) {
> +		pr_err("failed to allocate request\n");
> +		ret = -ENOMEM;
> +		goto set_fail;
> +	}
> +	if (may_sleep)
> +		skcipher_request_set_callback(sk_req, CRYPTO_TFM_REQ_MAY_SLEEP,
> +						NULL, NULL);
> +	if (create_iv)
> +		get_random_bytes(iv, AES_BLOCK_SIZE);
> +
> +	return 0;
> +
> +set_fail:
> +	crypto_free_skcipher(tfm);
> +alloc_fail:
> +	__free_page(c_buffer);

May I recommend to memzero_explicit(enc_key)?

> +
> +	return ret;
> +}
> +
> +void snapshot_finish_crypto(void)
> +{
> +	struct crypto_skcipher *tfm;
> +
> +	if (!sk_req)
> +		return;
> +
> +	tfm = crypto_skcipher_reqtfm(sk_req);
> +	skcipher_request_zero(sk_req);
> +	skcipher_request_free(sk_req);
> +	crypto_free_skcipher(tfm);
> +	__free_page(c_buffer);
> +	sk_req = NULL;
> +}
> +
> +static int encrypt_data_page(void *hash_buffer)
> +{
> +	struct scatterlist src[1], dst[1];
> +	u8 iv_tmp[AES_BLOCK_SIZE];
> +	int ret = 0;
> +
> +	if (!sk_req)
> +		return 0;
> +
> +	memcpy(iv_tmp, iv, sizeof(iv));

Why do you copy the IV? If I see that right, we would have a key/counter 
collision as follows:

1. you copy the IV into a tmp variable

2. CTR AES is invoked which updates iv_tmp

3. iv_tmp is discarded

4. a repeated invocation of this function would again use the initially set IV 
to copy it into iv_tmp which means that the subsequent cipher operation uses 
yet again the same IV.

If my hunch is correct, the cryptographic strength of the cipher is defeated.

> +	sg_init_one(src, hash_buffer, PAGE_SIZE);
> +	sg_init_one(dst, c_buffer, PAGE_SIZE);
> +	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
> +	ret = crypto_skcipher_encrypt(sk_req);
> +
> +	copy_page(hash_buffer, c_buffer);
> +	memset(c_buffer, 0, PAGE_SIZE);
> +
> +	return ret;
> +}
> +
> +static int decrypt_data_page(void *encrypted_page)

This function looks almost identical to encrypt_data_page - may I suggest to 
collapse it into one function?

> +{
> +	struct scatterlist src[1], dst[1];
> +	u8 iv_tmp[AES_BLOCK_SIZE];
> +	int ret = 0;
> +
> +	memcpy(iv_tmp, iv, sizeof(iv));
> +	sg_init_one(src, encrypted_page, PAGE_SIZE);
> +	sg_init_one(dst, c_buffer, PAGE_SIZE);
> +	skcipher_request_set_crypt(sk_req, src, dst, PAGE_SIZE, iv_tmp);
> +	ret = crypto_skcipher_decrypt(sk_req);
> +
> +	copy_page(encrypted_page, c_buffer);
> +	memset(c_buffer, 0, PAGE_SIZE);
> +
> +	return ret;
> +}
> +
>  /*
>   * Signature of snapshot image
>   */
> @@ -1508,22 +1633,30 @@ int snapshot_image_verify_decrypt(void)
>  	if (ret || !s4_verify_desc)
>  		goto error_prep;
> 
> +	ret = snapshot_prepare_crypto(true, false);
> +	if (ret)
> +		goto error_prep;
> +
>  	for (i = 0; i < nr_copy_pages; i++) {
>  		ret = crypto_shash_update(s4_verify_desc, *(h_buf + i), PAGE_SIZE);
>  		if (ret)
> -			goto error_shash;
> +			goto error_shash_crypto;
> +		ret = decrypt_data_page(*(h_buf + i));
> +		if (ret)
> +			goto error_shash_crypto;
>  	}
> 
>  	ret = crypto_shash_final(s4_verify_desc, s4_verify_digest);
>  	if (ret)
> -		goto error_shash;
> +		goto error_shash_crypto;
> 
>  	pr_debug("Signature %*phN\n", SNAPSHOT_DIGEST_SIZE, signature);
>  	pr_debug("Digest    %*phN\n", SNAPSHOT_DIGEST_SIZE, s4_verify_digest);
>  	if (memcmp(signature, s4_verify_digest, SNAPSHOT_DIGEST_SIZE))
>  		ret = -EKEYREJECTED;
> 
> - error_shash:
> + error_shash_crypto:
> +	snapshot_finish_crypto();
>  	snapshot_finish_hash();
> 
>   error_prep:
> @@ -1564,6 +1697,17 @@ __copy_data_pages(struct memory_bitmap *copy_bm,
> struct memory_bitmap *orig_bm) crypto_buffer = page_address(d_page);
>  		}
> 
> +		/* Encrypt hashed page */
> +		encrypt_data_page(crypto_buffer);
> +
> +		/* Copy encrypted buffer to destination page in high memory */
> +		if (PageHighMem(d_page)) {
> +			void *kaddr = kmap_atomic(d_page);
> +
> +			copy_page(kaddr, crypto_buffer);
> +			kunmap_atomic(kaddr);
> +		}
> +
>  		/* Generate digest */
>  		if (!s4_verify_desc)
>  			continue;
> @@ -1638,6 +1782,8 @@ __copy_data_pages(struct memory_bitmap *copy_bm,
> struct memory_bitmap *orig_bm) }
> 
>  static inline void alloc_h_buf(void) {}
> +static inline void init_iv(struct swsusp_info *info) {}
> +static inline void load_iv(struct swsusp_info *info) {}
>  static inline void init_signature(struct swsusp_info *info) {}
>  static inline void load_signature(struct swsusp_info *info) {}
>  static inline void init_sig_verify(struct trampoline *t) {}
> @@ -2286,6 +2432,7 @@ static int init_header(struct swsusp_info *info)
>  	info->size = info->pages;
>  	info->size <<= PAGE_SHIFT;
>  	info->trampoline_pfn = page_to_pfn(virt_to_page(trampoline_virt));
> +	init_iv(info);
>  	init_signature(info);
>  	return init_header_complete(info);
>  }
> @@ -2524,6 +2671,7 @@ static int load_header(struct swsusp_info *info)
>  		nr_copy_pages = info->image_pages;
>  		nr_meta_pages = info->pages - info->image_pages - 1;
>  		trampoline_pfn = info->trampoline_pfn;
> +		load_iv(info);
>  		load_signature(info);
>  	}
>  	return error;



Ciao
Stephan