Date:   Sun, 06 Jan 2019 09:01:27 +0100
From:   Stephan Mueller <smueller@...onox.de>
To:     "Lee, Chun-Yi" <joeyli.kernel@...il.com>
Cc:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, keyrings@...r.kernel.org,
        "Lee, Chun-Yi" <jlee@...e.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Chen Yu <yu.c.chen@...el.com>,
        Oliver Neukum <oneukum@...e.com>,
        Ryan Chen <yu.chen.surf@...il.com>,
        David Howells <dhowells@...hat.com>,
        Giovanni Gherdovich <ggherdovich@...e.cz>,
        Randy Dunlap <rdunlap@...radead.org>,
        Jann Horn <jannh@...gle.com>, Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

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

Hi Chun,

> 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.
> 
> 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.
> 
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
> Cc: Pavel Machek <pavel@....cz>
> Cc: Chen Yu <yu.c.chen@...el.com>
> Cc: Oliver Neukum <oneukum@...e.com>
> Cc: Ryan Chen <yu.chen.surf@...il.com>
> Cc: David Howells <dhowells@...hat.com>
> Cc: Giovanni Gherdovich <ggherdovich@...e.cz>
> Cc: Randy Dunlap <rdunlap@...radead.org>
> Cc: Jann Horn <jannh@...gle.com>
> Cc: Andy Lutomirski <luto@...nel.org>
> Signed-off-by: "Lee, Chun-Yi" <jlee@...e.com>
> ---
>  kernel/power/Kconfig        |  14 +++
>  kernel/power/Makefile       |   1 +
>  kernel/power/hibernate.c    |  33 ++++++
>  kernel/power/power.h        |  16 +++
>  kernel/power/snapshot_key.c | 245
> ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 309
> insertions(+)
>  create mode 100644 kernel/power/snapshot_key.c
> 
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index f8fe57d1022e..506a3c5a7a0d 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
> +	select 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 arbitrarily
> +	  modified. A user can use the TPM's 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 e7e47d9be1e5..d949adbaf580 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..ecc31e8e40d0 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -1034,6 +1034,36 @@ 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;
> +
> +	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 +1168,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..3a569b505d8d
> --- /dev/null
> +++ b/kernel/power/snapshot_key.c
> @@ -0,0 +1,245 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/* snapshot keys handler
> + *
> + * Copyright (C) 2018 Lee, Chun-Yi <jlee@...e.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)
> +{
> +	struct shash_desc *desc;
> +	int err;
> +
> +	desc = kzalloc(sizeof(struct shash_desc) +
> +		       crypto_shash_descsize(hash_tfm),
> +		       may_sleep ? GFP_KERNEL : GFP_ATOMIC);

Why not using SHASH_DESC_ON_STACK?

> +	if (!desc)
> +		return -ENOMEM;
> +
> +	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);
> +	kzfree(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;

strlen on binary data? I guess that will not work. May I suggest to hand down 
the length of salt to this function?

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

This function looks very much like a key derivation. May I strongly propose to 
use an official KDF type like SP800-108 or HKDF?

You find the counter-KDF according to SP800-108 in security/keys/dh.c (search 
for functions *kdf*).

Or we may start pulling in KDF support into the kernel crypto API via the 
patches along the line of [1].

[1] http://www.chronox.de/kdf.html

> +
> +/* 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 = 0;
> +
> +	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)

This function and trusted_key_init look very similar - could they be collapsed 
into one function?

> +{
> +	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);

Where would skey.key be destroyed again?

> +	/* 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;
> +
> +	barrier();
> +	skey.initialized = true;
> +
> +	pr_info("Snapshot key is initialled.\n");
> +
> +	return 0;
> +
> +key_fail:
> +	crypto_free_shash(hash_tfm);
> +	hash_tfm = NULL;
> +
> +	return err;
> +}



Ciao
Stephan

Powered by blists - more mailing lists