public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] tpm: lazy flush for the session null key
@ 2024-09-15 18:04 Jarkko Sakkinen
  2024-09-15 18:04 ` [PATCH 1/4] tpm: remove file header documentation from tpm2-sessions.c Jarkko Sakkinen
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-09-15 18:04 UTC (permalink / raw)
  To: linux-integrity
  Cc: James.Bottomley, roberto.sassu, mapengyu, Jarkko Sakkinen,
	Peter Huewe, Jason Gunthorpe, open list

There is no load and flush the null key for every transaction. It only
needs to be flushed when user space accesses TPM. This postpones the
flush up to that point.

The goal is to take the first step addressing [1]. Other performance
improvements are needed too but this is the most obvious one and
easiest to address.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=219229

Jarkko Sakkinen (4):
  tpm: remove file header documentation from tpm2-sessions.c
  tpm: address tpm2_create_null_primary() return value
  tpm: address tpm2_create_primary() failure
  tpm: flush the session null key only when required

 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  | 115 ++++++++++--------------------
 include/linux/tpm.h               |   2 +
 6 files changed, 68 insertions(+), 81 deletions(-)

-- 
2.46.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/4] tpm: remove file header documentation from tpm2-sessions.c
  2024-09-15 18:04 [PATCH 0/4] tpm: lazy flush for the session null key Jarkko Sakkinen
@ 2024-09-15 18:04 ` Jarkko Sakkinen
  2024-09-15 18:04 ` [PATCH 2/4] tpm: address tpm2_create_null_primary() return value Jarkko Sakkinen
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-09-15 18:04 UTC (permalink / raw)
  To: linux-integrity
  Cc: James.Bottomley, roberto.sassu, mapengyu, Jarkko Sakkinen,
	Peter Huewe, Jason Gunthorpe, open list

The documentation in the file header is duplicate documentation, which
is already addressed elsewhere (tpm-security.rs and function associated
documentations). In addition remove the invalid newline character after
the SPDX tag.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm2-sessions.c | 65 --------------------------------
 1 file changed, 65 deletions(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 44f60730cff4..6cc1ea81c57c 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -1,71 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
-
 /*
  * Copyright (C) 2018 James.Bottomley@HansenPartnership.com
- *
- * Cryptographic helper routines for handling TPM2 sessions for
- * authorization HMAC and request response encryption.
- *
- * The idea is to ensure that every TPM command is HMAC protected by a
- * session, meaning in-flight tampering would be detected and in
- * addition all sensitive inputs and responses should be encrypted.
- *
- * The basic way this works is to use a TPM feature called salted
- * sessions where a random secret used in session construction is
- * encrypted to the public part of a known TPM key.  The problem is we
- * have no known keys, so initially a primary Elliptic Curve key is
- * derived from the NULL seed (we use EC because most TPMs generate
- * these keys much faster than RSA ones).  The curve used is NIST_P256
- * because that's now mandated to be present in 'TCG TPM v2.0
- * Provisioning Guidance'
- *
- * Threat problems: the initial TPM2_CreatePrimary is not (and cannot
- * be) session protected, so a clever Man in the Middle could return a
- * public key they control to this command and from there intercept
- * and decode all subsequent session based transactions.  The kernel
- * cannot mitigate this threat but, after boot, userspace can get
- * proof this has not happened by asking the TPM to certify the NULL
- * key.  This certification would chain back to the TPM Endorsement
- * Certificate and prove the NULL seed primary had not been tampered
- * with and thus all sessions must have been cryptographically secure.
- * To assist with this, the initial NULL seed public key name is made
- * available in a sysfs file.
- *
- * Use of these functions:
- *
- * The design is all the crypto, hash and hmac gunk is confined in this
- * file and never needs to be seen even by the kernel internal user.  To
- * the user there's an init function tpm2_sessions_init() that needs to
- * be called once per TPM which generates the NULL seed primary key.
- *
- * These are the usage functions:
- *
- * tpm2_start_auth_session() which allocates the opaque auth structure
- *	and gets a session from the TPM.  This must be called before
- *	any of the following functions.  The session is protected by a
- *	session_key which is derived from a random salt value
- *	encrypted to the NULL seed.
- * tpm2_end_auth_session() kills the session and frees the resources.
- *	Under normal operation this function is done by
- *	tpm_buf_check_hmac_response(), so this is only to be used on
- *	error legs where the latter is not executed.
- * tpm_buf_append_name() to add a handle to the buffer.  This must be
- *	used in place of the usual tpm_buf_append_u32() for adding
- *	handles because handles have to be processed specially when
- *	calculating the HMAC.  In particular, for NV, volatile and
- *	permanent objects you now need to provide the name.
- * tpm_buf_append_hmac_session() which appends the hmac session to the
- *	buf in the same way tpm_buf_append_auth does().
- * tpm_buf_fill_hmac_session() This calculates the correct hash and
- *	places it in the buffer.  It must be called after the complete
- *	command buffer is finalized so it can fill in the correct HMAC
- *	based on the parameters.
- * tpm_buf_check_hmac_response() which checks the session response in
- *	the buffer and calculates what it should be.  If there's a
- *	mismatch it will log a warning and return an error.  If
- *	tpm_buf_append_hmac_session() did not specify
- *	TPM_SA_CONTINUE_SESSION then the session will be closed (if it
- *	hasn't been consumed) and the auth structure freed.
  */
 
 #include "tpm.h"
-- 
2.46.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/4] tpm: address tpm2_create_null_primary() return value
  2024-09-15 18:04 [PATCH 0/4] tpm: lazy flush for the session null key Jarkko Sakkinen
  2024-09-15 18:04 ` [PATCH 1/4] tpm: remove file header documentation from tpm2-sessions.c Jarkko Sakkinen
@ 2024-09-15 18:04 ` Jarkko Sakkinen
  2024-09-15 18:04 ` [PATCH 3/4] tpm: address tpm2_create_primary() failure Jarkko Sakkinen
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-09-15 18:04 UTC (permalink / raw)
  To: linux-integrity
  Cc: James.Bottomley, roberto.sassu, mapengyu, Jarkko Sakkinen,
	Peter Huewe, Jason Gunthorpe, open list

tpm2_sessions_init() does not check the return value of
tpm2_create_null_primary(). Return on failure.

Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm2-sessions.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 6cc1ea81c57c..d63510ad44ab 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -1288,8 +1288,10 @@ int tpm2_sessions_init(struct tpm_chip *chip)
 	int rc;
 
 	rc = tpm2_create_null_primary(chip);
-	if (rc)
+	if (rc) {
 		dev_err(&chip->dev, "TPM: security failed (NULL seed derivation): %d\n", rc);
+		return rc;
+	}
 
 	chip->auth = kmalloc(sizeof(*chip->auth), GFP_KERNEL);
 	if (!chip->auth)
-- 
2.46.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/4] tpm: address tpm2_create_primary() failure
  2024-09-15 18:04 [PATCH 0/4] tpm: lazy flush for the session null key Jarkko Sakkinen
  2024-09-15 18:04 ` [PATCH 1/4] tpm: remove file header documentation from tpm2-sessions.c Jarkko Sakkinen
  2024-09-15 18:04 ` [PATCH 2/4] tpm: address tpm2_create_null_primary() return value Jarkko Sakkinen
@ 2024-09-15 18:04 ` Jarkko Sakkinen
  2024-09-15 18:04 ` [PATCH 4/4] tpm: flush the session null key only when required Jarkko Sakkinen
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-09-15 18:04 UTC (permalink / raw)
  To: linux-integrity
  Cc: James.Bottomley, roberto.sassu, mapengyu, Jarkko Sakkinen,
	Peter Huewe, Jason Gunthorpe, open list

tpm2_load_null() ignores the return value of tpm2_create_primary(). Return
instead on failure. On success when the null key name has not been changed
heal appropriately, and return the created key handle instead of flushing
it.

Fixes: eb24c9788cd9 ("tpm: disable the TPM if NULL name changes")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm2-sessions.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index d63510ad44ab..34ce0d9d4577 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -850,9 +850,10 @@ static int tpm2_parse_start_auth_session(struct tpm2_auth *auth,
 
 static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
 {
-	int rc;
 	unsigned int offset = 0; /* dummy offset for null seed context */
 	u8 name[SHA256_DIGEST_SIZE + 2];
+	u32 tmp_null_key;
+	int rc;
 
 	rc = tpm2_load_context(chip, chip->null_key_context, &offset,
 			       null_key);
@@ -861,11 +862,17 @@ static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
 
 	/* an integrity failure may mean the TPM has been reset */
 	dev_err(&chip->dev, "NULL key integrity failure!\n");
-	/* check the null name against what we know */
-	tpm2_create_primary(chip, TPM2_RH_NULL, NULL, name);
-	if (memcmp(name, chip->null_key_name, sizeof(name)) == 0)
-		/* name unchanged, assume transient integrity failure */
+
+	rc = tpm2_create_primary(chip, TPM2_RH_NULL, &tmp_null_key, name);
+	if (rc)
 		return rc;
+
+	/* Return the null key if the name has not been changed: */
+	if (memcmp(name, chip->null_key_name, sizeof(name)) == 0) {
+		*null_key = tmp_null_key;
+		return 0;
+	}
+
 	/*
 	 * Fatal TPM failure: the NULL seed has actually changed, so
 	 * the TPM must have been illegally reset.  All in-kernel TPM
@@ -874,6 +881,7 @@ static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
 	 * userspace programmes can't be compromised by it.
 	 */
 	dev_err(&chip->dev, "NULL name has changed, disabling TPM due to interference\n");
+	tpm2_flush_context(chip, tmp_null_key);
 	chip->flags |= TPM_CHIP_FLAG_DISABLE;
 
 	return rc;
@@ -991,10 +999,6 @@ static int tpm2_parse_create_primary(struct tpm_chip *chip, struct tpm_buf *buf,
 	u32 val, param_len, keyhandle;
 
 	keyhandle = tpm_buf_read_u32(buf, &offset_r);
-	if (handle)
-		*handle = keyhandle;
-	else
-		tpm2_flush_context(chip, keyhandle);
 
 	param_len = tpm_buf_read_u32(buf, &offset_r);
 	/*
@@ -1135,6 +1139,7 @@ static int tpm2_parse_create_primary(struct tpm_chip *chip, struct tpm_buf *buf,
 		return -EINVAL;
 	}
 
+	*handle = keyhandle;
 	return 0;
 }
 
-- 
2.46.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/4] tpm: flush the session null key only when required
  2024-09-15 18:04 [PATCH 0/4] tpm: lazy flush for the session null key Jarkko Sakkinen
                   ` (2 preceding siblings ...)
  2024-09-15 18:04 ` [PATCH 3/4] tpm: address tpm2_create_primary() failure Jarkko Sakkinen
@ 2024-09-15 18:04 ` Jarkko Sakkinen
  2024-09-15 18:12 ` [PATCH 0/4] tpm: lazy flush for the session null key Jarkko Sakkinen
       [not found] ` <CALSz7m0ehXM+dU3z0xYPLQkHbyfyMjoCOoMLdBgRcUu1pnT_ww@mail.gmail.com>
  5 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-09-15 18:04 UTC (permalink / raw)
  To: linux-integrity
  Cc: James.Bottomley, roberto.sassu, mapengyu, Jarkko Sakkinen,
	Peter Huewe, Jason Gunthorpe, Stefan Berger, Ard Biesheuvel,
	open list

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.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 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  | 23 +++++++++++++++++++----
 include/linux/tpm.h               |  2 ++
 6 files changed, 51 insertions(+), 6 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);
+			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 c3fbbf4d3db7..4bc07963e260 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
 			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;
+
 	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 34ce0d9d4577..8d7b708ce566 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -855,10 +855,21 @@ 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,
-			       null_key);
-	if (rc != -EINVAL)
+			       &tmp_null_key);
+	if (rc != -EINVAL) {
+		if (!rc) {
+			chip->null_key = tmp_null_key;
+			*null_key = tmp_null_key;
+		}
 		return rc;
+	}
 
 	/* an integrity failure may mean the TPM has been reset */
 	dev_err(&chip->dev, "NULL key integrity failure!\n");
@@ -869,6 +880,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;
 	}
@@ -946,7 +958,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);
@@ -1275,7 +1286,11 @@ 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);
+		pr_info("%s: rc=0x%08x\n", __func__, rc);
+		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 */
+	u32 null_key;
 	/* saved context for NULL seed */
 	u8 null_key_context[TPM2_MAX_CONTEXT_SIZE];
 	 /* name of NULL seed */
-- 
2.46.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] tpm: lazy flush for the session null key
  2024-09-15 18:04 [PATCH 0/4] tpm: lazy flush for the session null key Jarkko Sakkinen
                   ` (3 preceding siblings ...)
  2024-09-15 18:04 ` [PATCH 4/4] tpm: flush the session null key only when required Jarkko Sakkinen
@ 2024-09-15 18:12 ` Jarkko Sakkinen
       [not found] ` <CALSz7m0ehXM+dU3z0xYPLQkHbyfyMjoCOoMLdBgRcUu1pnT_ww@mail.gmail.com>
  5 siblings, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-09-15 18:12 UTC (permalink / raw)
  To: Jarkko Sakkinen, linux-integrity
  Cc: James.Bottomley, roberto.sassu, mapengyu, Peter Huewe,
	Jason Gunthorpe, open list

On Sun Sep 15, 2024 at 9:04 PM EEST, Jarkko Sakkinen wrote:
> There is no load and flush the null key for every transaction. It only
> needs to be flushed when user space accesses TPM. This postpones the
> flush up to that point.
>
> The goal is to take the first step addressing [1]. Other performance
> improvements are needed too but this is the most obvious one and
> easiest to address.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=219229
>
> Jarkko Sakkinen (4):
>   tpm: remove file header documentation from tpm2-sessions.c
>   tpm: address tpm2_create_null_primary() return value
>   tpm: address tpm2_create_primary() failure
>   tpm: flush the session null key only when required
>
>  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  | 115 ++++++++++--------------------
>  include/linux/tpm.h               |   2 +
>  6 files changed, 68 insertions(+), 81 deletions(-)

I did not take any benchmarks yet but I did run this through
run-tests.sh in [1] to make sure that it does not break anything.

Looking at pseude-code of ContextSave from [2] fixing this is
orthogonal from any possible context gap issues as null key
is just plain transient object.

I would fix the obvious first and then look what can be done
to sessions (e.g. global LRU tracking of sessions or similar
approach). I don't expect over the top performance improvement
with this patch set.

[1] https://codeberg.org/jarkko/linux-tpmdd-test
[2] https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-3-Commands-Code.pdf

BR, Jarkko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] tpm: lazy flush for the session null key
       [not found] ` <CALSz7m0ehXM+dU3z0xYPLQkHbyfyMjoCOoMLdBgRcUu1pnT_ww@mail.gmail.com>
@ 2024-09-16  2:33   ` Pengyu Ma
  2024-09-16  5:16     ` Jarkko Sakkinen
  0 siblings, 1 reply; 9+ messages in thread
From: Pengyu Ma @ 2024-09-16  2:33 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-integrity, James.Bottomley, roberto.sassu, Peter Huewe,
	Jason Gunthorpe, open list

After applied your patches, the boot time is ~15 seconds.
Less than 20 sec, but still much more than 7 sec when disabling HMAC.

Send again in text mode.



On Mon, Sep 16, 2024 at 10:25 AM Pengyu Ma <mapengyu@gmail.com> wrote:
>
> Hi Jarkko,
>
> After applied your patches, the boot time is ~15 seconds.
> Less than 20 sec, but still much more than 7 sec when disabling HMAC.
>
> Thanks,
> Aaron
>
>
> On Mon, Sep 16, 2024 at 2:04 AM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>
>> There is no load and flush the null key for every transaction. It only
>> needs to be flushed when user space accesses TPM. This postpones the
>> flush up to that point.
>>
>> The goal is to take the first step addressing [1]. Other performance
>> improvements are needed too but this is the most obvious one and
>> easiest to address.
>>
>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=219229
>>
>> Jarkko Sakkinen (4):
>>   tpm: remove file header documentation from tpm2-sessions.c
>>   tpm: address tpm2_create_null_primary() return value
>>   tpm: address tpm2_create_primary() failure
>>   tpm: flush the session null key only when required
>>
>>  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  | 115 ++++++++++--------------------
>>  include/linux/tpm.h               |   2 +
>>  6 files changed, 68 insertions(+), 81 deletions(-)
>>
>> --
>> 2.46.0
>>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] tpm: lazy flush for the session null key
  2024-09-16  2:33   ` Pengyu Ma
@ 2024-09-16  5:16     ` Jarkko Sakkinen
  2024-09-16  5:53       ` Pengyu Ma
  0 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-09-16  5:16 UTC (permalink / raw)
  To: Pengyu Ma
  Cc: linux-integrity, James.Bottomley, roberto.sassu, Peter Huewe,
	Jason Gunthorpe, open list

On Mon Sep 16, 2024 at 5:33 AM EEST, Pengyu Ma wrote:
> After applied your patches, the boot time is ~15 seconds.
> Less than 20 sec, but still much more than 7 sec when disabling HMAC.

Great, and thank you for testing this. I did expect it to fully address
the issue but it is on the direct path. It took me few days to get my
testing environment right before moving forward [1], mainly to get
bpftrace included, thus the delay.

Do you mind if I add tested-by for the for this one?

Before the patch set the in-kernel TPM sequences were along the lines
of:

1. Load the null key.
2. Load the auth session.
3. Do stuff with overhead from encryption.
4. Save the session.
5. Save the null key.

With the changes:

1. Load the session.
2. Do stuff with overhead from encryption.
3. Save the session.

Each swapped session gets an increasing count. If the count grows over
treshold measured by the difference of the count in the latest loaded
session and the session currently being saved, then TPM throws out 
a context gap error. It has a limited resolution for this.

As long as /dev/tpm0 is not opened by any process, there is only one
session open (or at least fixed pre-determined number moving forward).
This means that context gap error cannot occur, as the only session
saved is the auth session.

I'll implement a patch on top of this, which does exactly this: track
the number of open /dev/tpm{rm0}. Only when the device is open, the
auth session is flushed.

With this change the sequence reduces to:

1. Do stuff with overhead from encryption.

Since the results are promising (thanks to you), I create a new version
of this patch set with this additional fix. There's no chance to reach
the same exact boot-up time as without encryption but I think we might
be able to reach a reasonable cost.

[1] https://codeberg.org/jarkko/linux-tpmdd-test

BR, Jarkko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] tpm: lazy flush for the session null key
  2024-09-16  5:16     ` Jarkko Sakkinen
@ 2024-09-16  5:53       ` Pengyu Ma
  0 siblings, 0 replies; 9+ messages in thread
From: Pengyu Ma @ 2024-09-16  5:53 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: linux-integrity, James.Bottomley, roberto.sassu, Peter Huewe,
	Jason Gunthorpe, open list

On Mon, Sep 16, 2024 at 1:16 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Mon Sep 16, 2024 at 5:33 AM EEST, Pengyu Ma wrote:
> > After applied your patches, the boot time is ~15 seconds.
> > Less than 20 sec, but still much more than 7 sec when disabling HMAC.
>
> Great, and thank you for testing this. I did expect it to fully address
> the issue but it is on the direct path. It took me few days to get my
> testing environment right before moving forward [1], mainly to get
> bpftrace included, thus the delay.
>
> Do you mind if I add tested-by for the for this one?
>

Yes, please feel free to add it.
And thanks for the effort and details.

BR,
Pengyu.


> Before the patch set the in-kernel TPM sequences were along the lines
> of:
>
> 1. Load the null key.
> 2. Load the auth session.
> 3. Do stuff with overhead from encryption.
> 4. Save the session.
> 5. Save the null key.
>
> With the changes:
>
> 1. Load the session.
> 2. Do stuff with overhead from encryption.
> 3. Save the session.
>
> Each swapped session gets an increasing count. If the count grows over
> treshold measured by the difference of the count in the latest loaded
> session and the session currently being saved, then TPM throws out
> a context gap error. It has a limited resolution for this.
>
> As long as /dev/tpm0 is not opened by any process, there is only one
> session open (or at least fixed pre-determined number moving forward).
> This means that context gap error cannot occur, as the only session
> saved is the auth session.
>
> I'll implement a patch on top of this, which does exactly this: track
> the number of open /dev/tpm{rm0}. Only when the device is open, the
> auth session is flushed.
>
> With this change the sequence reduces to:
>
> 1. Do stuff with overhead from encryption.
>
> Since the results are promising (thanks to you), I create a new version
> of this patch set with this additional fix. There's no chance to reach
> the same exact boot-up time as without encryption but I think we might
> be able to reach a reasonable cost.
>
> [1] https://codeberg.org/jarkko/linux-tpmdd-test
>
> BR, Jarkko

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-09-16  5:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-15 18:04 [PATCH 0/4] tpm: lazy flush for the session null key Jarkko Sakkinen
2024-09-15 18:04 ` [PATCH 1/4] tpm: remove file header documentation from tpm2-sessions.c Jarkko Sakkinen
2024-09-15 18:04 ` [PATCH 2/4] tpm: address tpm2_create_null_primary() return value Jarkko Sakkinen
2024-09-15 18:04 ` [PATCH 3/4] tpm: address tpm2_create_primary() failure Jarkko Sakkinen
2024-09-15 18:04 ` [PATCH 4/4] tpm: flush the session null key only when required Jarkko Sakkinen
2024-09-15 18:12 ` [PATCH 0/4] tpm: lazy flush for the session null key Jarkko Sakkinen
     [not found] ` <CALSz7m0ehXM+dU3z0xYPLQkHbyfyMjoCOoMLdBgRcUu1pnT_ww@mail.gmail.com>
2024-09-16  2:33   ` Pengyu Ma
2024-09-16  5:16     ` Jarkko Sakkinen
2024-09-16  5:53       ` Pengyu Ma

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox