From: Jarkko Sakkinen <jarkko@kernel.org>
To: Evan Green <evgreen@chromium.org>
Cc: linux-kernel@vger.kernel.org, linux-integrity@vger.kernel.org,
apronin@chromium.org, dlunev@google.com,
Pavel Machek <pavel@ucw.cz>, Ben Boeckel <me@benboeckel.net>,
rjw@rjwysocki.net, corbet@lwn.net, linux-pm@vger.kernel.org,
zohar@linux.ibm.com, Kees Cook <keescook@chromium.org>,
Eric Biggers <ebiggers@kernel.org>,
jejb@linux.ibm.com, gwendal@chromium.org,
Matthew Garrett <mgarrett@aurora.tech>,
Len Brown <len.brown@intel.com>,
"Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH v3 08/11] PM: hibernate: Use TPM-backed keys to encrypt image
Date: Mon, 24 Oct 2022 00:55:07 +0300 [thread overview]
Message-ID: <Y1W4O2bQIQMyuT2+@kernel.org> (raw)
In-Reply-To: <CAE=gft45p4QFqe0E0X_1XGeRB2kLgH3p9ZfNNTvMk2H9GwbhMw@mail.gmail.com>
On Fri, Oct 21, 2022 at 12:56:50PM -0700, Evan Green wrote:
> On Fri, Sep 30, 2022 at 2:35 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Tue, Sep 27, 2022 at 09:49:19AM -0700, Evan Green wrote:
> > > When using encrypted hibernate images, have the TPM create a key for us
> > > and seal it. By handing back a sealed blob instead of the raw key, we
> > > prevent usermode from being able to decrypt and tamper with the
> > > hibernate image on a different machine.
> > >
> > > We'll also go through the motions of having PCR23 set to a known value at
> > > the time of key creation and unsealing. Currently there's nothing that
> > > enforces the contents of PCR23 as a condition to unseal the key blob,
> > > that will come in a later change.
> > >
> > > Sourced-from: Matthew Garrett <mjg59@google.com>
> > > Signed-off-by: Evan Green <evgreen@chromium.org>
> > >
> > > ---
> > > Matthew's incarnation of this patch is at:
> > > https://patchwork.kernel.org/project/linux-pm/patch/20210220013255.1083202-9-matthewgarrett@google.com/
> > >
> > > Changes in v3:
> > > - ENCRYPTED_HIBERNATION needs TRUSTED_KEYS builtin for
> > > key_type_trusted.
> > > - Remove KEYS dependency since it's covered by TRUSTED_KEYS (Kees)
> > >
> > > Changes in v2:
> > > - Rework load/create_kernel_key() to eliminate a label (Andrey)
> > > - Call put_device() needed from calling tpm_default_chip().
> > >
> > > kernel/power/Kconfig | 1 +
> > > kernel/power/snapenc.c | 207 +++++++++++++++++++++++++++++++++++++++--
> > > kernel/power/user.h | 1 +
> > > 3 files changed, 200 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> > > index cd574af0b43379..2f8acbd87b34dc 100644
> > > --- a/kernel/power/Kconfig
> > > +++ b/kernel/power/Kconfig
> > > @@ -96,6 +96,7 @@ config ENCRYPTED_HIBERNATION
> > > bool "Encryption support for userspace snapshots"
> > > depends on HIBERNATION_SNAPSHOT_DEV
> > > depends on CRYPTO_AEAD2=y
> > > + depends on TRUSTED_KEYS=y
> > > default n
> > > help
> > > Enable support for kernel-based encryption of hibernation snapshots
> > > diff --git a/kernel/power/snapenc.c b/kernel/power/snapenc.c
> > > index cb90692d6ab83a..90079f6d4f184b 100644
> > > --- a/kernel/power/snapenc.c
> > > +++ b/kernel/power/snapenc.c
> > > @@ -4,13 +4,23 @@
> > > #include <linux/crypto.h>
> > > #include <crypto/aead.h>
> > > #include <crypto/gcm.h>
> > > +#include <keys/trusted-type.h>
> > > +#include <linux/key-type.h>
> > > #include <linux/random.h>
> > > #include <linux/mm.h>
> > > +#include <linux/tpm.h>
> > > #include <linux/uaccess.h>
> > >
> > > #include "power.h"
> > > #include "user.h"
> > >
> > > +/* sha256("To sleep, perchance to dream") */
> > > +static struct tpm_digest known_digest = { .alg_id = TPM_ALG_SHA256,
> > > + .digest = {0x92, 0x78, 0x3d, 0x79, 0x2d, 0x00, 0x31, 0xb0, 0x55, 0xf9,
> > > + 0x1e, 0x0d, 0xce, 0x83, 0xde, 0x1d, 0xc4, 0xc5, 0x8e, 0x8c,
> > > + 0xf1, 0x22, 0x38, 0x6c, 0x33, 0xb1, 0x14, 0xb7, 0xec, 0x05,
> > > + 0x5f, 0x49}};
> > > +
> > > /* Encrypt more data from the snapshot into the staging area. */
> > > static int snapshot_encrypt_refill(struct snapshot_data *data)
> > > {
> > > @@ -313,6 +323,12 @@ void snapshot_teardown_encryption(struct snapshot_data *data)
> > > {
> > > int i;
> > >
> > > + if (data->key) {
> >
> > Would be a helpful to have perhaps inline comment before the check.
> >
> > Just stating this because I did not exactly follow why the null
> > check was needed (but do believe that there are good reasons to
> > do it).
> >
> > > + key_revoke(data->key);
> > > + key_put(data->key);
> > > + data->key = NULL;
> > > + }
> > > +
> > > if (data->aead_req) {
> > > aead_request_free(data->aead_req);
> > > data->aead_req = NULL;
> > > @@ -381,11 +397,83 @@ static int snapshot_setup_encryption_common(struct snapshot_data *data)
> > > return rc;
> > > }
> > >
> > > +static int snapshot_create_kernel_key(struct snapshot_data *data)
> > > +{
> > > + const struct cred *cred = current_cred();
> > > + struct tpm_digest *digests = NULL;
> > > + struct tpm_chip *chip;
> > > + struct key *key = NULL;
> > > + int ret, i;
> > > + /* Create a key sealed by the SRK. */
> > > + char *keyinfo = "new\t32\tkeyhandle=0x81000000";
> >
> > Again, I'd consider put this declaration as first.
> >
> > > +
> > > + chip = tpm_default_chip();
> > > + if (!chip)
> > > + return -ENODEV;
> > > +
> > > + if (!(tpm_is_tpm2(chip))) {
> > > + ret = -ENODEV;
> > > + goto out_dev;
> > > + }
> > > +
> > > + ret = tpm_pcr_reset(chip, 23);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + digests = kcalloc(chip->nr_allocated_banks, sizeof(struct tpm_digest),
> > > + GFP_KERNEL);
> > > + if (!digests) {
> > > + ret = -ENOMEM;
> > > + goto out;
> > > + }
> > > +
> > > + for (i = 0; i <= chip->nr_allocated_banks; i++) {
> > > + digests[i].alg_id = chip->allocated_banks[i].alg_id;
> > > + if (digests[i].alg_id == known_digest.alg_id)
> > > + memcpy(&digests[i], &known_digest, sizeof(known_digest));
> > > + }
> > > +
> > > + ret = tpm_pcr_extend(chip, 23, digests);
> > > + if (ret != 0)
> > > + goto out;
> > > +
> > > + key = key_alloc(&key_type_trusted, "swsusp", GLOBAL_ROOT_UID,
> > > + GLOBAL_ROOT_GID, cred, 0, KEY_ALLOC_NOT_IN_QUOTA,
> > > + NULL);
> > > +
> > > + if (IS_ERR(key)) {
> > > + ret = PTR_ERR(key);
> > > + key = NULL;
> > > + goto out;
> > > + }
> > > +
> > > + ret = key_instantiate_and_link(key, keyinfo, strlen(keyinfo) + 1, NULL,
> > > + NULL);
> >
> > Generally speaking, even if it somehow would be "safe", not strlen()
> > thank you.
> >
> > AFAIK, keyinfo is a constant so you could just as well use sizeof().
> > And then you would not need "+ 1".
>
> Ack, I'm changing this one to sizeof(keyinfo), but...
>
> >
> > > + if (ret != 0)
> > > + goto out;
> > > +
> > > + data->key = key;
> > > + key = NULL;
> > > +
> > > +out:
> > > + if (key) {
> > > + key_revoke(key);
> > > + key_put(key);
> > > + }
> > > +
> > > + kfree(digests);
> > > + tpm_pcr_reset(chip, 23);
> > > +
> > > +out_dev:
> > > + put_device(&chip->dev);
> > > + return ret;
> > > +}
> > > +
> > > int snapshot_get_encryption_key(struct snapshot_data *data,
> > > struct uswsusp_key_blob __user *key)
> > > {
> > > - u8 aead_key[SNAPSHOT_ENCRYPTION_KEY_SIZE];
> > > u8 nonce[USWSUSP_KEY_NONCE_SIZE];
> > > + struct trusted_key_payload *payload;
> > > int rc;
> > > /* Don't pull a random key from a world that can be reset. */
> > > if (data->ready)
> > > @@ -399,21 +487,28 @@ int snapshot_get_encryption_key(struct snapshot_data *data,
> > > get_random_bytes(nonce, sizeof(nonce));
> > > memcpy(&data->nonce_low, &nonce[0], sizeof(data->nonce_low));
> > > memcpy(&data->nonce_high, &nonce[8], sizeof(data->nonce_high));
> > > - /* Build a random key */
> > > - get_random_bytes(aead_key, sizeof(aead_key));
> > > - rc = crypto_aead_setkey(data->aead_tfm, aead_key, sizeof(aead_key));
> > > +
> > > + /* Create a kernel key, and set it. */
> > > + rc = snapshot_create_kernel_key(data);
> > > + if (rc)
> > > + goto fail;
> > > +
> > > + payload = data->key->payload.data[0];
> > > + /* Install the key */
> > > + rc = crypto_aead_setkey(data->aead_tfm, payload->key, SNAPSHOT_ENCRYPTION_KEY_SIZE);
> > > if (rc)
> > > goto fail;
> > >
> > > - /* Hand the key back to user mode (to be changed!) */
> > > - rc = put_user(sizeof(struct uswsusp_key_blob), &key->blob_len);
> > > + /* Hand the key back to user mode in sealed form. */
> > > + rc = put_user(payload->blob_len, &key->blob_len);
> > > if (rc)
> > > goto fail;
> > >
> > > - rc = copy_to_user(&key->blob, &aead_key, sizeof(aead_key));
> > > + rc = copy_to_user(&key->blob, &payload->blob, payload->blob_len);
> > > if (rc)
> > > goto fail;
> > >
> > > + /* The nonce just gets handed back in the clear. */
> > > rc = copy_to_user(&key->nonce, &nonce, sizeof(nonce));
> > > if (rc)
> > > goto fail;
> > > @@ -425,10 +520,99 @@ int snapshot_get_encryption_key(struct snapshot_data *data,
> > > return rc;
> > > }
> > >
> > > +static int snapshot_load_kernel_key(struct snapshot_data *data,
> > > + struct uswsusp_key_blob *blob)
> >
> > Bad alignment.
> >
> > > +{
> > > +
> > > + const struct cred *cred = current_cred();
> > > + char *keytemplate = "load\t%s\tkeyhandle=0x81000000";
> >
> > Ditto.
> >
> > > + struct tpm_digest *digests = NULL;
> > > + char *blobstring = NULL;
> > > + char *keyinfo = NULL;
> > > + struct tpm_chip *chip;
> > > + struct key *key = NULL;
> > > + int i, ret;
> > > +
> > > + chip = tpm_default_chip();
> > > + if (!chip)
> > > + return -ENODEV;
> > > +
> > > + if (!(tpm_is_tpm2(chip))) {
> > > + ret = -ENODEV;
> > > + goto out_dev;
> > > + }
> > > +
> > > + ret = tpm_pcr_reset(chip, 23);
> > > + if (ret)
> > > + goto out;
> > > +
> > > + digests = kcalloc(chip->nr_allocated_banks, sizeof(struct tpm_digest),
> > > + GFP_KERNEL);
> > > + if (!digests)
> > > + goto out;
> > > +
> > > + for (i = 0; i <= chip->nr_allocated_banks; i++) {
> > > + digests[i].alg_id = chip->allocated_banks[i].alg_id;
> > > + if (digests[i].alg_id == known_digest.alg_id)
> > > + memcpy(&digests[i], &known_digest, sizeof(known_digest));
> > > + }
> > > +
> > > + ret = tpm_pcr_extend(chip, 23, digests);
> > > + if (ret != 0)
> > > + goto out;
> > > +
> > > + blobstring = kmalloc(blob->blob_len * 2, GFP_KERNEL);
> > > + if (!blobstring) {
> > > + ret = -ENOMEM;
> > > + goto out;
> > > + }
> > > +
> > > + bin2hex(blobstring, blob->blob, blob->blob_len);
> > > + keyinfo = kasprintf(GFP_KERNEL, keytemplate, blobstring);
> > > + if (!keyinfo) {
> > > + ret = -ENOMEM;
> > > + goto out;
> > > + }
> > > +
> > > + key = key_alloc(&key_type_trusted, "swsusp", GLOBAL_ROOT_UID,
> > > + GLOBAL_ROOT_GID, cred, 0, KEY_ALLOC_NOT_IN_QUOTA,
> > > + NULL);
> > > +
> > > + if (IS_ERR(key)) {
> > > + ret = PTR_ERR(key);
> > > + key = NULL;
> > > + goto out;
> > > + }
> > > +
> > > + ret = key_instantiate_and_link(key, keyinfo, strlen(keyinfo) + 1, NULL,
> > > + NULL);
> >
> > Ditto.
>
> ... I can't change this one to sizeof. Since this came out of
> kasprintf() and we already checked against null, strlen() seemed safe
> here. Is there a different pattern I should be following?
You're right. Let's strlen() here givent that as long as kasprintf()
is working correctly there's no risks involved.
BR, Jarkko
next prev parent reply other threads:[~2022-10-23 21:55 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-27 16:49 [PATCH v3 00/11] Encrypted Hibernation Evan Green
2022-09-27 16:49 ` [PATCH v3 01/11] tpm: Add support for in-kernel resetting of PCRs Evan Green
2022-09-30 20:52 ` Jarkko Sakkinen
2022-09-27 16:49 ` [PATCH v3 02/11] tpm: Export and rename tpm2_find_and_validate_cc() Evan Green
2022-09-27 16:49 ` [PATCH v3 03/11] tpm: Allow PCR 23 to be restricted to kernel-only use Evan Green
2022-09-30 20:57 ` Jarkko Sakkinen
2022-09-27 16:49 ` [PATCH v3 04/11] security: keys: trusted: Include TPM2 creation data Evan Green
2022-09-27 16:49 ` [PATCH v3 05/11] security: keys: trusted: Allow storage of PCR values in " Evan Green
2022-09-27 16:58 ` Ben Boeckel
2022-09-27 16:49 ` [PATCH v3 06/11] security: keys: trusted: Verify " Evan Green
2022-09-27 16:49 ` [PATCH v3 07/11] PM: hibernate: Add kernel-based encryption Evan Green
2022-09-30 21:30 ` Jarkko Sakkinen
2022-09-27 16:49 ` [PATCH v3 08/11] PM: hibernate: Use TPM-backed keys to encrypt image Evan Green
2022-09-30 21:35 ` Jarkko Sakkinen
2022-10-21 19:56 ` Evan Green
2022-10-23 21:55 ` Jarkko Sakkinen [this message]
2022-09-27 16:49 ` [PATCH v3 09/11] PM: hibernate: Mix user key in encrypted hibernate Evan Green
2022-09-27 16:49 ` [PATCH v3 10/11] PM: hibernate: Verify the digest encryption key Evan Green
2022-09-27 16:49 ` [PATCH v3 11/11] PM: hibernate: seal the encryption key with a PCR policy Evan Green
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y1W4O2bQIQMyuT2+@kernel.org \
--to=jarkko@kernel.org \
--cc=apronin@chromium.org \
--cc=corbet@lwn.net \
--cc=dlunev@google.com \
--cc=ebiggers@kernel.org \
--cc=evgreen@chromium.org \
--cc=gwendal@chromium.org \
--cc=jejb@linux.ibm.com \
--cc=keescook@chromium.org \
--cc=len.brown@intel.com \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=me@benboeckel.net \
--cc=mgarrett@aurora.tech \
--cc=pavel@ucw.cz \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.