linux-kernel - Re: [PATCH v5 08/11] PM: hibernate: Use TPM-backed keys to encrypt image

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y3F+rj59Q5uzGI5i@sol.localdomain>
Date:   Sun, 13 Nov 2022 15:33:02 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Evan Green <evgreen@...omium.org>
Cc:     linux-kernel@...r.kernel.org, corbet@....net,
        linux-integrity@...r.kernel.org, gwendal@...omium.org,
        dianders@...omium.org, apronin@...omium.org,
        Pavel Machek <pavel@....cz>, Ben Boeckel <me@...boeckel.net>,
        rjw@...ysocki.net, jejb@...ux.ibm.com,
        Kees Cook <keescook@...omium.org>, dlunev@...gle.com,
        zohar@...ux.ibm.com, Matthew Garrett <mgarrett@...ora.tech>,
        jarkko@...nel.org, linux-pm@...r.kernel.org,
        Matthew Garrett <mjg59@...gle.com>,
        Len Brown <len.brown@...el.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH v5 08/11] PM: hibernate: Use TPM-backed keys to encrypt
 image

On Fri, Nov 11, 2022 at 03:16:33PM -0800, Evan Green wrote:
> 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.

This patch doesn't really make sense without the later patch
"PM: hibernate: seal the encryption key with a PCR policy".

Maybe they should be merged into one patch?

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

This can be const.

> +static int snapshot_create_kernel_key(struct snapshot_data *data)
> +{
> +	/* Create a key sealed by the SRK. */
> +	char *keyinfo = "new\t32\tkeyhandle=0x81000000";
> +	const struct cred *cred = current_cred();
> +	struct tpm_digest *digests = NULL;
> +	struct key *key = NULL;
> +	struct tpm_chip *chip;
> +	int ret, i;
> +
> +	chip = tpm_default_chip();
> +	if (!chip)
> +		return -ENODEV;
> +
> +	if (!(tpm_is_tpm2(chip))) {
> +		ret = -ENODEV;
> +		goto out_dev;
> +	}

tpm_is_tpm2() can return an error value.

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

So, PCR 23 will not actually be extended properly if there is not a SHA-256
bank?  Wouldn't it make more sense to return an error in that case?

> +static int snapshot_load_kernel_key(struct snapshot_data *data,
> +				    struct uswsusp_key_blob *blob)
> +{
> +
> +	char *keytemplate = "load\t%s\tkeyhandle=0x81000000";

Make this const.

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

There's no need for bin2hex().  Just use the %*phN format specifier instead:

	const char *keytemplate = "load\t%*phN\tkeyhandle=0x81000000";

	keyinfo = kasprintf(GFP_KERNEL, keytemplate, blob->blob_len, blob->blob);

- Eric