| 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
| ||
|
Message-ID: <531b27a8-6e99-40ca-9d74-f94a3b8c638e@linux.ibm.com>
Date: Wed, 23 Oct 2024 14:18:02 -0400
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Jarkko Sakkinen <jarkko@...nel.org>, linux-integrity@...r.kernel.org,
Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>,
James Bottomley <James.Bottomley@...senPartnership.com>
Cc: David Howells <dhowells@...hat.com>, Mimi Zohar <zohar@...ux.ibm.com>,
Roberto Sassu <roberto.sassu@...wei.com>, linux-kernel@...r.kernel.org,
stable@...r.kernel.org, Pengyu Ma <mapengyu@...il.com>
Subject: Re: [PATCH v7 3/5] tpm: flush the null key only when /dev/tpm0 is
accessed
On 10/21/24 1:39 AM, Jarkko Sakkinen wrote:
> Instead of flushing and reloading the null key for every single auth
> session, flush it only when:
>
> 1. User space needs to access /dev/tpm{rm}0.
> 2. When going to sleep.
> 3. When unregistering the chip.
>
> This removes the need to load and swap the null key between TPM and
> regular memory per transaction, when the user space is not using the
> chip.
>
> Cc: stable@...r.kernel.org # v6.10+
> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> Tested-by: Pengyu Ma <mapengyu@...il.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>
> ---
> v5:
> - No changes.
> v4:
> - Changed to bug fix as not having the patch there is a major hit
> to bootup times.
> v3:
> - Unchanged.
> v2:
> - Refined the commit message.
> - Added tested-by from Pengyu Ma <mapengyu@...il.com>.
> - Removed spurious pr_info() statement.
> ---
> drivers/char/tpm/tpm-chip.c | 13 +++++++++++++
> drivers/char/tpm/tpm-dev-common.c | 7 +++++++
> drivers/char/tpm/tpm-interface.c | 9 +++++++--
> drivers/char/tpm/tpm2-cmd.c | 3 +++
> drivers/char/tpm/tpm2-sessions.c | 17 ++++++++++++++---
> include/linux/tpm.h | 2 ++
> 6 files changed, 46 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 854546000c92..0ea00e32f575 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -674,6 +674,19 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
> */
> void tpm_chip_unregister(struct tpm_chip *chip)
> {
> +#ifdef CONFIG_TCG_TPM2_HMAC
> + int rc;
> +
> + rc = tpm_try_get_ops(chip);
> + if (!rc) {
> + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> + tpm2_flush_context(chip, chip->null_key);
If chip->null_key is already 0, the above function will not do anything
good, but you could avoid this whole block then by checking for 0 before
tpm_try_get_ops().
> + chip->null_key = 0;
> + }
> + tpm_put_ops(chip);
> + }
> +#endif
> +
> tpm_del_legacy_sysfs(chip);
> if (tpm_is_hwrng_enabled(chip))
> hwrng_unregister(&chip->hwrng);
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 30b4c288c1bb..4eaa8e05c291 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -27,6 +27,13 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
> struct tpm_header *header = (void *)buf;
> ssize_t ret, len;
>
> +#ifdef CONFIG_TCG_TPM2_HMAC
> + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> + tpm2_flush_context(chip, chip->null_key);
> + chip->null_key = 0;
> + }
> +#endif
> +
> ret = tpm2_prepare_space(chip, space, buf, bufsiz);
> /* If the command is not implemented by the TPM, synthesize a
> * response with a TPM2_RC_COMMAND_CODE return for user-space.
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 5da134f12c9a..bfa47d48b0f2 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -379,10 +379,15 @@ int tpm_pm_suspend(struct device *dev)
>
> rc = tpm_try_get_ops(chip);
> if (!rc) {
> - if (chip->flags & TPM_CHIP_FLAG_TPM2)
> + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +#ifdef CONFIG_TCG_TPM2_HMAC
> + tpm2_flush_context(chip, chip->null_key);
> + chip->null_key = 0;
> +#endif
Worth using an inline on this repeating pattern? Up to you.
static inline void tpm2_flush_null_key(struct tpm_chip *chip)
{
#ifdef CONFIG_TCG_TPM2_HMAC
if (chip->flags & TPM_CHIP_FLAG_TPM2 && chip->null_key) {
tpm2_flush_context(chip, chip->null_key);
chip->null_key = 0;
}
#endif
}
> tpm2_shutdown(chip, TPM2_SU_STATE);
> - else
> + } else {
> rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> + }
>
> tpm_put_ops(chip);
> }
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 1e856259219e..aba024cbe7c5 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -364,6 +364,9 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
> struct tpm_buf buf;
> int rc;
>
> + if (!handle)
> + return;
> +
wouldn't be necessary with inline.
> rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
> if (rc) {
> dev_warn(&chip->dev, "0x%08x was not flushed, out of memory\n",
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index bdac11964b55..78c650ce4c9f 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -920,11 +920,19 @@ static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
> u32 tmp_null_key;
> int rc;
>
> + /* fast path */
> + if (chip->null_key) {
> + *null_key = chip->null_key;
> + return 0;
> + }
> +
> rc = tpm2_load_context(chip, chip->null_key_context, &offset,
> &tmp_null_key);
> if (rc != -EINVAL) {
> - if (!rc)
> + if (!rc) {
> + chip->null_key = tmp_null_key;
> *null_key = tmp_null_key;
> + }
> goto err;
> }
>
> @@ -934,6 +942,7 @@ static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
>
> /* Return the null key if the name has not been changed: */
> if (memcmp(name, chip->null_key_name, sizeof(name)) == 0) {
> + chip->null_key = tmp_null_key;
> *null_key = tmp_null_key;
> return 0;
> }
> @@ -1006,7 +1015,6 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
> tpm_buf_append_u16(&buf, TPM_ALG_SHA256);
>
> rc = tpm_transmit_cmd(chip, &buf, 0, "start auth session");
> - tpm2_flush_context(chip, null_key);
>
> if (rc == TPM2_RC_SUCCESS)
> rc = tpm2_parse_start_auth_session(auth, &buf);
> @@ -1338,7 +1346,10 @@ static int tpm2_create_null_primary(struct tpm_chip *chip)
>
> rc = tpm2_save_context(chip, null_key, chip->null_key_context,
> sizeof(chip->null_key_context), &offset);
> - tpm2_flush_context(chip, null_key);
> + if (rc)
> + tpm2_flush_context(chip, null_key);
> + else
> + chip->null_key = null_key;
> }
>
> return rc;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index e93ee8d936a9..4eb39db80e05 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -205,6 +205,8 @@ struct tpm_chip {
> #ifdef CONFIG_TCG_TPM2_HMAC
> /* details for communication security via sessions */
>
> + /* loaded null key */
nit: handle of loaded null key
> + u32 null_key;
> /* saved context for NULL seed */
> u8 null_key_context[TPM2_MAX_CONTEXT_SIZE];
> /* name of NULL seed */
Powered by blists - more mailing lists