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

joeyli
2019-01-07T15:37:15Z

Hi Stephan,

First, thanks for your review!

On Sun, Jan 06, 2019 at 09:01:27AM +0100, Stephan Mueller wrote:
> 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.
> >
[...snip]
> > +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?
>

Because security concern and bad runtime performance. Please looking at
c2cd0b08e1e patch for hibernation. And reference:

https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com/T/#u
https://lwn.net/Articles/749064/

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

hm... The salt is actually a "salt string" that's gave from
snapshot_get_auth_key() or snapshot_get_enc_key(). So I use
strlen() here. I will change the name to salt_string to avoid
confusion.

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

Actually key derivation function is modified from the get_derived_key() from
the encrypted.c file in encrypted key.

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

Thanks for your suggestion. I didn't touch any key derivation standard
before. I will study it.

But I still want to use my original function currently. Because the same
logic is also used in trusted key. I will happy to move to SP800-108 or
HKDF when it's available in kernel.

> > +
> > +/* Derive authentication/encryption key */
> > +static int get_derived_key(u8 *derived_key, const char *derived_type_str,
> > + bool may_sleep)
[...snip]
> > +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?
>

The data structure is different between trusted key with user key. I will try to
extract the duplicate part but may not collapse into one.

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

Yes, you saw it in later patch.

Thanks a lot!
Joey Lee