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

From: Stephan Mueller
Date: Sun Jan 06 2019 - 03:24:37 EST

Next message: Stephan Mueller: "Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler"
Previous message: Eric Wong: "[PATCH] rtc: cmos: ignore bogus century byte"
In reply to: Lee, Chun-Yi: "[PATCH 3/5] PM / hibernate: Encrypt snapshot image"
Next in thread: Lee, Chun-Yi: "[PATCH 4/5 v2] PM / hibernate: Erase the snapshot master key in snapshot pages"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> Cc: Pavel Machek <pavel@xxxxxx>
> Cc: Chen Yu <yu.c.chen@xxxxxxxxx>
> Cc: Oliver Neukum <oneukum@xxxxxxxx>
> Cc: Ryan Chen <yu.chen.surf@xxxxxxxxx>
> Cc: David Howells <dhowells@xxxxxxxxxx>
> Cc: Giovanni Gherdovich <ggherdovich@xxxxxxx>
> Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> Cc: Jann Horn <jannh@xxxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> Signed-off-by: "Lee, Chun-Yi" <jlee@xxxxxxxx>
> ---
> 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

Next message: Stephan Mueller: "Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler"
Previous message: Eric Wong: "[PATCH] rtc: cmos: ignore bogus century byte"
In reply to: Lee, Chun-Yi: "[PATCH 3/5] PM / hibernate: Encrypt snapshot image"
Next in thread: Lee, Chun-Yi: "[PATCH 4/5 v2] PM / hibernate: Erase the snapshot master key in snapshot pages"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]