[PATCH v5 1/5] tpm: Return on tpm2_create_null_primary() failure

[PATCH v5 1/5] tpm: Return on tpm2_create_null_primary() failure

[Jarkko Sakkinen]

tpm2_sessions_init() does not ignores the result of
tpm2_create_null_primary(). Address this by returning -ENODEV to the
caller.

Cc: stable@vger.kernel.org # v6.10+
Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v5:
- Do not print klog messages on error, as tpm2_save_context() already
  takes care of this.
v4:
- Fixed up stable version.
v3:
- Handle TPM and POSIX error separately and return -ENODEV always back
  to the caller.
v2:
- Refined the commit message.
---
 drivers/char/tpm/tpm2-sessions.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index d3521aadd43e..0f09ac33ae99 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -1338,7 +1338,8 @@ static int tpm2_create_null_primary(struct tpm_chip *chip)
 		tpm2_flush_context(chip, null_key);
 	}
 
-	return rc;
+	/* Map all errors to -ENODEV: */
+	return rc ? -ENODEV : rc;
 }
 
 /**
@@ -1354,7 +1355,7 @@ int tpm2_sessions_init(struct tpm_chip *chip)
 
 	rc = tpm2_create_null_primary(chip);
 	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.1

Re: [PATCH v5 1/5] tpm: Return on tpm2_create_null_primary() failure

[Stefan Berger]

On 9/21/24 8:08 AM, Jarkko Sakkinen wrote:
> tpm2_sessions_init() does not ignores the result of

s/ignores/ignore

> tpm2_create_null_primary(). Address this by returning -ENODEV to the
> caller.

I am not sure why mapping all errors to -ENODEV resolves the fact that 
tpm2_sessions_init() does not ignore the result of 
tpm2_create_null_primary(). I think what you want is to return -ENODEV 
from tpm2_auto_startup.

> 
> Cc: stable@vger.kernel.org # v6.10+
> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v5:
> - Do not print klog messages on error, as tpm2_save_context() already
>    takes care of this.
> v4:
> - Fixed up stable version.
> v3:
> - Handle TPM and POSIX error separately and return -ENODEV always back
>    to the caller.
> v2:
> - Refined the commit message.
> ---
>   drivers/char/tpm/tpm2-sessions.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index d3521aadd43e..0f09ac33ae99 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -1338,7 +1338,8 @@ static int tpm2_create_null_primary(struct tpm_chip *chip)
>   		tpm2_flush_context(chip, null_key);
>   	}
>   
> -	return rc;
> +	/* Map all errors to -ENODEV: */
> +	return rc ? -ENODEV : rc;

return rc ? -ENODEV : 0;

>   }
>   
>   /**
> @@ -1354,7 +1355,7 @@ int tpm2_sessions_init(struct tpm_chip *chip)
>   
>   	rc = tpm2_create_null_primary(chip);
>   	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)

Re: [PATCH v5 1/5] tpm: Return on tpm2_create_null_primary() failure

[Jarkko Sakkinen]

On Thu, 2024-10-03 at 10:57 -0400, Stefan Berger wrote:
> 
> 
> On 9/21/24 8:08 AM, Jarkko Sakkinen wrote:
> > tpm2_sessions_init() does not ignores the result of
> 
> s/ignores/ignore
> 
> > tpm2_create_null_primary(). Address this by returning -ENODEV to
> > the
> > caller.
> 
> I am not sure why mapping all errors to -ENODEV resolves the fact
> that 
> tpm2_sessions_init() does not ignore the result of 
> tpm2_create_null_primary(). I think what you want is to return -
> ENODEV 
> from tpm2_auto_startup.

Fair point.

> 
> > 
> > Cc: stable@vger.kernel.org # v6.10+
> > Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v5:
> > - Do not print klog messages on error, as tpm2_save_context()
> > already
> >    takes care of this.
> > v4:
> > - Fixed up stable version.
> > v3:
> > - Handle TPM and POSIX error separately and return -ENODEV always
> > back
> >    to the caller.
> > v2:
> > - Refined the commit message.
> > ---
> >   drivers/char/tpm/tpm2-sessions.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2-sessions.c
> > b/drivers/char/tpm/tpm2-sessions.c
> > index d3521aadd43e..0f09ac33ae99 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -1338,7 +1338,8 @@ static int tpm2_create_null_primary(struct
> > tpm_chip *chip)
> >   		tpm2_flush_context(chip, null_key);
> >   	}
> >   
> > -	return rc;
> > +	/* Map all errors to -ENODEV: */
> > +	return rc ? -ENODEV : rc;
> 
> return rc ? -ENODEV : 0;
> 
> >   }
> >   
> >   /**
> > @@ -1354,7 +1355,7 @@ int tpm2_sessions_init(struct tpm_chip *chip)
> >   
> >   	rc = tpm2_create_null_primary(chip);
> >   	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)

Thanks!


BR, Jarkko

[PATCH v5 2/5] tpm: Implement tpm2_load_null() rollback

[Jarkko Sakkinen]

tpm2_load_null() has weak and broken error handling:

- The return value of tpm2_create_primary() is ignored.
- Leaks TPM return codes from tpm2_load_context() to the caller.
- If the key name comparison succeeds returns previous error
  instead of zero to the caller.

Implement a proper error rollback.

Cc: stable@vger.kernel.org # v6.10+
Fixes: eb24c9788cd9 ("tpm: disable the TPM if NULL name changes")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v5:
- Fix the TPM error code leak from tpm2_load_context().
v4:
- No changes.
v3:
- Update log messages. Previously the log message incorrectly stated
  on load failure that integrity check had been failed, even tho the
  check is done *after* the load operation.
v2:
- Refined the commit message.
- Reverted tpm2_create_primary() changes. They are not required if
  tmp_null_key is used as the parameter.
---
 drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++---------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 0f09ac33ae99..a856adef18d3 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -915,33 +915,36 @@ 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);
-	if (rc != -EINVAL)
-		return rc;
+			       &tmp_null_key);
+	if (rc != -EINVAL) {
+		if (!rc)
+			*null_key = tmp_null_key;
+		goto err;
+	}
 
-	/* 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 */
-		return rc;
-	/*
-	 * Fatal TPM failure: the NULL seed has actually changed, so
-	 * the TPM must have been illegally reset.  All in-kernel TPM
-	 * operations will fail because the NULL primary can't be
-	 * loaded to salt the sessions, but disable the TPM anyway so
-	 * userspace programmes can't be compromised by it.
-	 */
-	dev_err(&chip->dev, "NULL name has changed, disabling TPM due to interference\n");
+	rc = tpm2_create_primary(chip, TPM2_RH_NULL, &tmp_null_key, name);
+	if (rc)
+		goto err;
+
+	/* 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;
+	}
+
+	/* Deduce from the name change TPM interference: */
+	dev_err(&chip->dev, "the null key integrity check failedh\n");
+	tpm2_flush_context(chip, tmp_null_key);
 	chip->flags |= TPM_CHIP_FLAG_DISABLE;
 
-	return rc;
+err:
+	return rc ? -ENODEV : rc;
 }
 
 /**
-- 
2.46.1

Re: [PATCH v5 2/5] tpm: Implement tpm2_load_null() rollback

[Stefan Berger]

On 9/21/24 8:08 AM, Jarkko Sakkinen wrote:
> tpm2_load_null() has weak and broken error handling:
> 
> - The return value of tpm2_create_primary() is ignored.
> - Leaks TPM return codes from tpm2_load_context() to the caller.
> - If the key name comparison succeeds returns previous error
>    instead of zero to the caller.
> 
> Implement a proper error rollback.
> 
> Cc: stable@vger.kernel.org # v6.10+
> Fixes: eb24c9788cd9 ("tpm: disable the TPM if NULL name changes")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v5:
> - Fix the TPM error code leak from tpm2_load_context().
> v4:
> - No changes.
> v3:
> - Update log messages. Previously the log message incorrectly stated
>    on load failure that integrity check had been failed, even tho the
>    check is done *after* the load operation.
> v2:
> - Refined the commit message.
> - Reverted tpm2_create_primary() changes. They are not required if
>    tmp_null_key is used as the parameter.
> ---
>   drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++---------------
>   1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index 0f09ac33ae99..a856adef18d3 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -915,33 +915,36 @@ 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);
> -	if (rc != -EINVAL)
> -		return rc;
> +			       &tmp_null_key);
> +	if (rc != -EINVAL) {
> +		if (!rc)
> +			*null_key = tmp_null_key;
> +		goto err;
> +	}
>   
> -	/* 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 */
> -		return rc;
> -	/*
> -	 * Fatal TPM failure: the NULL seed has actually changed, so
> -	 * the TPM must have been illegally reset.  All in-kernel TPM
> -	 * operations will fail because the NULL primary can't be
> -	 * loaded to salt the sessions, but disable the TPM anyway so
> -	 * userspace programmes can't be compromised by it. > -	 */
> -	dev_err(&chip->dev, "NULL name has changed, disabling TPM due to interference\n");
> +	rc = tpm2_create_primary(chip, TPM2_RH_NULL, &tmp_null_key, name);
> +	if (rc)
> +		goto err;
> +
> +	/* 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;
> +	}
> +
> +	/* Deduce from the name change TPM interference: */
> +	dev_err(&chip->dev, "the null key integrity check failedh\n");

s/failedh/failed

> +	tpm2_flush_context(chip, tmp_null_key);
>   	chip->flags |= TPM_CHIP_FLAG_DISABLE;
>   
> -	return rc;
> +err:
> +	return rc ? -ENODEV : rc;

return rc ? -ENODEV : 0;

>   }
>   
>   /**

[PATCH v5 3/5] tpm: flush the null key only when /dev/tpm0 is accessed

[Jarkko Sakkinen]

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@vger.kernel.org # v6.10+
Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
Tested-by: Pengyu Ma <mapengyu@gmail.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.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@gmail.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);
+			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
 			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 a856adef18d3..1aef5b1f9c90 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;
 	}
 
 	/* Map all errors to -ENODEV: */
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.1

[PATCH v5 4/5] tpm: Allocate chip->auth in tpm2_start_auth_session()

[Jarkko Sakkinen]

Move allocation of chip->auth to tpm2_start_auth_session() so that the
field can be used as flag to tell whether auth session is active or not.

Cc: stable@vger.kernel.org # v6.10+
Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v5:
- No changes.
v4:
- Change to bug.
v3:
- No changes.
v2:
- A new patch.
---
 drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 1aef5b1f9c90..a8d3d5d52178 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -484,7 +484,8 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8 *pt_v,
 	sha256_final(&sctx, out);
 }
 
-static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
+static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
+				struct tpm2_auth *auth)
 {
 	struct crypto_kpp *kpp;
 	struct kpp_request *req;
@@ -543,7 +544,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
 	sg_set_buf(&s[0], chip->null_ec_key_x, EC_PT_SZ);
 	sg_set_buf(&s[1], chip->null_ec_key_y, EC_PT_SZ);
 	kpp_request_set_input(req, s, EC_PT_SZ*2);
-	sg_init_one(d, chip->auth->salt, EC_PT_SZ);
+	sg_init_one(d, auth->salt, EC_PT_SZ);
 	kpp_request_set_output(req, d, EC_PT_SZ);
 	crypto_kpp_compute_shared_secret(req);
 	kpp_request_free(req);
@@ -554,8 +555,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
 	 * This works because KDFe fully consumes the secret before it
 	 * writes the salt
 	 */
-	tpm2_KDFe(chip->auth->salt, "SECRET", x, chip->null_ec_key_x,
-		  chip->auth->salt);
+	tpm2_KDFe(auth->salt, "SECRET", x, chip->null_ec_key_x, auth->salt);
 
  out:
 	crypto_free_kpp(kpp);
@@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
 			/* manually close the session if it wasn't consumed */
 			tpm2_flush_context(chip, auth->handle);
 		memzero_explicit(auth, sizeof(*auth));
+		kfree(auth);
+		chip->auth = NULL;
 	} else {
 		/* reset for next use  */
 		auth->session = TPM_HEADER_SIZE;
@@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
 
 	tpm2_flush_context(chip, auth->handle);
 	memzero_explicit(auth, sizeof(*auth));
+	kfree(auth);
+	chip->auth = NULL;
 }
 EXPORT_SYMBOL(tpm2_end_auth_session);
 
@@ -970,25 +974,29 @@ static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
  */
 int tpm2_start_auth_session(struct tpm_chip *chip)
 {
+	struct tpm2_auth *auth;
 	struct tpm_buf buf;
-	struct tpm2_auth *auth = chip->auth;
-	int rc;
 	u32 null_key;
+	int rc;
 
-	if (!auth) {
-		dev_warn_once(&chip->dev, "auth session is not active\n");
+	if (chip->auth) {
+		dev_warn_once(&chip->dev, "auth session is active\n");
 		return 0;
 	}
 
+	auth = kzalloc(sizeof(*auth), GFP_KERNEL);
+	if (!auth)
+		return -ENOMEM;
+
 	rc = tpm2_load_null(chip, &null_key);
 	if (rc)
-		goto out;
+		goto err;
 
 	auth->session = TPM_HEADER_SIZE;
 
 	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS);
 	if (rc)
-		goto out;
+		goto err;
 
 	/* salt key handle */
 	tpm_buf_append_u32(&buf, null_key);
@@ -1000,7 +1008,7 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
 	tpm_buf_append(&buf, auth->our_nonce, sizeof(auth->our_nonce));
 
 	/* append encrypted salt and squirrel away unencrypted in auth */
-	tpm_buf_append_salt(&buf, chip);
+	tpm_buf_append_salt(&buf, chip, auth);
 	/* session type (HMAC, audit or policy) */
 	tpm_buf_append_u8(&buf, TPM2_SE_HMAC);
 
@@ -1021,10 +1029,13 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
 
 	tpm_buf_destroy(&buf);
 
-	if (rc)
-		goto out;
+	if (rc == TPM2_RC_SUCCESS) {
+		chip->auth = auth;
+		return 0;
+	}
 
- out:
+err:
+	kfree(auth);
 	return rc;
 }
 EXPORT_SYMBOL(tpm2_start_auth_session);
@@ -1371,10 +1382,6 @@ int tpm2_sessions_init(struct tpm_chip *chip)
 	if (rc)
 		return rc;
 
-	chip->auth = kmalloc(sizeof(*chip->auth), GFP_KERNEL);
-	if (!chip->auth)
-		return -ENOMEM;
-
 	return rc;
 }
 #endif /* CONFIG_TCG_TPM2_HMAC */
-- 
2.46.1

Re: [PATCH v5 4/5] tpm: Allocate chip->auth in tpm2_start_auth_session()

[James Bottomley]

On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> Move allocation of chip->auth to tpm2_start_auth_session() so that
> the field can be used as flag to tell whether auth session is active
> or not.
> 
> Cc: stable@vger.kernel.org # v6.10+
> Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v5:
> - No changes.
> v4:
> - Change to bug.
> v3:
> - No changes.
> v2:
> - A new patch.
> ---
>  drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-----------
> --
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm2-sessions.c
> b/drivers/char/tpm/tpm2-sessions.c
> index 1aef5b1f9c90..a8d3d5d52178 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -484,7 +484,8 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char
> *str, u8 *pt_u, u8 *pt_v,
>         sha256_final(&sctx, out);
>  }
>  
> -static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip
> *chip)
> +static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip
> *chip,
> +                               struct tpm2_auth *auth)

This addition of auth as an argument is a bit unnecessary.  You can set
chip->auth before calling this and it will all function.  Since there's
no error leg in tpm2_start_auth_session unless the session creation
itself fails and the guarantee of the ops lock is single threading this
chip->auth can be nulled again in that error leg.

If you want to keep the flow proposed in the patch, the change from how
it works now to how it works with this patch needs documenting in the
change log

>  {
>         struct crypto_kpp *kpp;
>         struct kpp_request *req;
> @@ -543,7 +544,7 @@ static void tpm_buf_append_salt(struct tpm_buf
> *buf, struct tpm_chip *chip)
>         sg_set_buf(&s[0], chip->null_ec_key_x, EC_PT_SZ);
>         sg_set_buf(&s[1], chip->null_ec_key_y, EC_PT_SZ);
>         kpp_request_set_input(req, s, EC_PT_SZ*2);
> -       sg_init_one(d, chip->auth->salt, EC_PT_SZ);
> +       sg_init_one(d, auth->salt, EC_PT_SZ);
>         kpp_request_set_output(req, d, EC_PT_SZ);
>         crypto_kpp_compute_shared_secret(req);
>         kpp_request_free(req);
> @@ -554,8 +555,7 @@ static void tpm_buf_append_salt(struct tpm_buf
> *buf, struct tpm_chip *chip)
>          * This works because KDFe fully consumes the secret before
> it
>          * writes the salt
>          */
> -       tpm2_KDFe(chip->auth->salt, "SECRET", x, chip->null_ec_key_x,
> -                 chip->auth->salt);
> +       tpm2_KDFe(auth->salt, "SECRET", x, chip->null_ec_key_x, auth-
> >salt);
>  
>   out:
>         crypto_free_kpp(kpp);
> @@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip
> *chip, struct tpm_buf *buf,
>                         /* manually close the session if it wasn't
> consumed */
>                         tpm2_flush_context(chip, auth->handle);
>                 memzero_explicit(auth, sizeof(*auth));
> +               kfree(auth);
> +               chip->auth = NULL;
>         } else {
>                 /* reset for next use  */
>                 auth->session = TPM_HEADER_SIZE;
> @@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
>  
>         tpm2_flush_context(chip, auth->handle);
>         memzero_explicit(auth, sizeof(*auth));
> +       kfree(auth);
> +       chip->auth = NULL;
>  }
>  EXPORT_SYMBOL(tpm2_end_auth_session);
>  
> @@ -970,25 +974,29 @@ static int tpm2_load_null(struct tpm_chip
> *chip, u32 *null_key)
>   */
>  int tpm2_start_auth_session(struct tpm_chip *chip)
>  {
> +       struct tpm2_auth *auth;
>         struct tpm_buf buf;
> -       struct tpm2_auth *auth = chip->auth;
> -       int rc;
>         u32 null_key;
> +       int rc;
>  
> -       if (!auth) {
> -               dev_warn_once(&chip->dev, "auth session is not
> active\n");
> +       if (chip->auth) {
> +               dev_warn_once(&chip->dev, "auth session is
> active\n");
>                 return 0;
>         }
>  
> +       auth = kzalloc(sizeof(*auth), GFP_KERNEL);
> +       if (!auth)
> +               return -ENOMEM;
> +
>         rc = tpm2_load_null(chip, &null_key);
>         if (rc)
> -               goto out;
> +               goto err;
>  
>         auth->session = TPM_HEADER_SIZE;
>  
>         rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
> TPM2_CC_START_AUTH_SESS);
>         if (rc)
> -               goto out;
> +               goto err;
>  
>         /* salt key handle */
>         tpm_buf_append_u32(&buf, null_key);
> @@ -1000,7 +1008,7 @@ int tpm2_start_auth_session(struct tpm_chip
> *chip)
>         tpm_buf_append(&buf, auth->our_nonce, sizeof(auth-
> >our_nonce));
>  
>         /* append encrypted salt and squirrel away unencrypted in
> auth */
> -       tpm_buf_append_salt(&buf, chip);
> +       tpm_buf_append_salt(&buf, chip, auth);
>         /* session type (HMAC, audit or policy) */
>         tpm_buf_append_u8(&buf, TPM2_SE_HMAC);
>  
> @@ -1021,10 +1029,13 @@ int tpm2_start_auth_session(struct tpm_chip
> *chip)
>  
>         tpm_buf_destroy(&buf);
>  
> -       if (rc)
> -               goto out;
> +       if (rc == TPM2_RC_SUCCESS) {
> +               chip->auth = auth;
> +               return 0;
> +       }
>  
> - out:
> +err:
> +       kfree(auth);
>         return rc;
>  }
>  EXPORT_SYMBOL(tpm2_start_auth_session);
> @@ -1371,10 +1382,6 @@ int tpm2_sessions_init(struct tpm_chip *chip)
>         if (rc)
>                 return rc;
>  
> -       chip->auth = kmalloc(sizeof(*chip->auth), GFP_KERNEL);
> -       if (!chip->auth)
> -               return -ENOMEM;
> -
>         return rc;
>  }
>  #endif /* CONFIG_TCG_TPM2_HMAC */

Other than the comment above

Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Re: [PATCH v5 4/5] tpm: Allocate chip->auth in tpm2_start_auth_session()

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 4:33 PM EEST, James Bottomley wrote:
> On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > Move allocation of chip->auth to tpm2_start_auth_session() so that
> > the field can be used as flag to tell whether auth session is active
> > or not.
> > 
> > Cc: stable@vger.kernel.org # v6.10+
> > Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v5:
> > - No changes.
> > v4:
> > - Change to bug.
> > v3:
> > - No changes.
> > v2:
> > - A new patch.
> > ---
> >  drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-----------
> > --
> >  1 file changed, 25 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2-sessions.c
> > b/drivers/char/tpm/tpm2-sessions.c
> > index 1aef5b1f9c90..a8d3d5d52178 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -484,7 +484,8 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char
> > *str, u8 *pt_u, u8 *pt_v,
> >         sha256_final(&sctx, out);
> >  }
> >  
> > -static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip
> > *chip)
> > +static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip
> > *chip,
> > +                               struct tpm2_auth *auth)
>
> This addition of auth as an argument is a bit unnecessary.  You can set
> chip->auth before calling this and it will all function.  Since there's
> no error leg in tpm2_start_auth_session unless the session creation
> itself fails and the guarantee of the ops lock is single threading this
> chip->auth can be nulled again in that error leg.
>
> If you want to keep the flow proposed in the patch, the change from how
> it works now to how it works with this patch needs documenting in the
> change log

OK, I don't want to overgrow the diff so +1 for this.

>
> >  {
> >         struct crypto_kpp *kpp;
> >         struct kpp_request *req;
> > @@ -543,7 +544,7 @@ static void tpm_buf_append_salt(struct tpm_buf
> > *buf, struct tpm_chip *chip)
> >         sg_set_buf(&s[0], chip->null_ec_key_x, EC_PT_SZ);
> >         sg_set_buf(&s[1], chip->null_ec_key_y, EC_PT_SZ);
> >         kpp_request_set_input(req, s, EC_PT_SZ*2);
> > -       sg_init_one(d, chip->auth->salt, EC_PT_SZ);
> > +       sg_init_one(d, auth->salt, EC_PT_SZ);
> >         kpp_request_set_output(req, d, EC_PT_SZ);
> >         crypto_kpp_compute_shared_secret(req);
> >         kpp_request_free(req);
> > @@ -554,8 +555,7 @@ static void tpm_buf_append_salt(struct tpm_buf
> > *buf, struct tpm_chip *chip)
> >          * This works because KDFe fully consumes the secret before
> > it
> >          * writes the salt
> >          */
> > -       tpm2_KDFe(chip->auth->salt, "SECRET", x, chip->null_ec_key_x,
> > -                 chip->auth->salt);
> > +       tpm2_KDFe(auth->salt, "SECRET", x, chip->null_ec_key_x, auth-
> > >salt);
> >  
> >   out:
> >         crypto_free_kpp(kpp);
> > @@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip
> > *chip, struct tpm_buf *buf,
> >                         /* manually close the session if it wasn't
> > consumed */
> >                         tpm2_flush_context(chip, auth->handle);
> >                 memzero_explicit(auth, sizeof(*auth));
> > +               kfree(auth);
> > +               chip->auth = NULL;
> >         } else {
> >                 /* reset for next use  */
> >                 auth->session = TPM_HEADER_SIZE;
> > @@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
> >  
> >         tpm2_flush_context(chip, auth->handle);
> >         memzero_explicit(auth, sizeof(*auth));
> > +       kfree(auth);
> > +       chip->auth = NULL;
> >  }
> >  EXPORT_SYMBOL(tpm2_end_auth_session);
> >  
> > @@ -970,25 +974,29 @@ static int tpm2_load_null(struct tpm_chip
> > *chip, u32 *null_key)
> >   */
> >  int tpm2_start_auth_session(struct tpm_chip *chip)
> >  {
> > +       struct tpm2_auth *auth;
> >         struct tpm_buf buf;
> > -       struct tpm2_auth *auth = chip->auth;
> > -       int rc;
> >         u32 null_key;
> > +       int rc;
> >  
> > -       if (!auth) {
> > -               dev_warn_once(&chip->dev, "auth session is not
> > active\n");
> > +       if (chip->auth) {
> > +               dev_warn_once(&chip->dev, "auth session is
> > active\n");
> >                 return 0;
> >         }
> >  
> > +       auth = kzalloc(sizeof(*auth), GFP_KERNEL);
> > +       if (!auth)
> > +               return -ENOMEM;
> > +
> >         rc = tpm2_load_null(chip, &null_key);
> >         if (rc)
> > -               goto out;
> > +               goto err;
> >  
> >         auth->session = TPM_HEADER_SIZE;
> >  
> >         rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
> > TPM2_CC_START_AUTH_SESS);
> >         if (rc)
> > -               goto out;
> > +               goto err;
> >  
> >         /* salt key handle */
> >         tpm_buf_append_u32(&buf, null_key);
> > @@ -1000,7 +1008,7 @@ int tpm2_start_auth_session(struct tpm_chip
> > *chip)
> >         tpm_buf_append(&buf, auth->our_nonce, sizeof(auth-
> > >our_nonce));
> >  
> >         /* append encrypted salt and squirrel away unencrypted in
> > auth */
> > -       tpm_buf_append_salt(&buf, chip);
> > +       tpm_buf_append_salt(&buf, chip, auth);
> >         /* session type (HMAC, audit or policy) */
> >         tpm_buf_append_u8(&buf, TPM2_SE_HMAC);
> >  
> > @@ -1021,10 +1029,13 @@ int tpm2_start_auth_session(struct tpm_chip
> > *chip)
> >  
> >         tpm_buf_destroy(&buf);
> >  
> > -       if (rc)
> > -               goto out;
> > +       if (rc == TPM2_RC_SUCCESS) {
> > +               chip->auth = auth;
> > +               return 0;
> > +       }
> >  
> > - out:
> > +err:
> > +       kfree(auth);
> >         return rc;
> >  }
> >  EXPORT_SYMBOL(tpm2_start_auth_session);
> > @@ -1371,10 +1382,6 @@ int tpm2_sessions_init(struct tpm_chip *chip)
> >         if (rc)
> >                 return rc;
> >  
> > -       chip->auth = kmalloc(sizeof(*chip->auth), GFP_KERNEL);
> > -       if (!chip->auth)
> > -               return -ENOMEM;
> > -
> >         return rc;
> >  }
> >  #endif /* CONFIG_TCG_TPM2_HMAC */
>
> Other than the comment above
>
> Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Just in case, I'll address the comment so please check also v6.

BR, Jarkko

Re: [PATCH v5 4/5] tpm: Allocate chip->auth in tpm2_start_auth_session()

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 4:33 PM EEST, James Bottomley wrote:
> On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > Move allocation of chip->auth to tpm2_start_auth_session() so that
> > the field can be used as flag to tell whether auth session is active
> > or not.
> > 
> > Cc: stable@vger.kernel.org # v6.10+
> > Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v5:
> > - No changes.
> > v4:
> > - Change to bug.
> > v3:
> > - No changes.
> > v2:
> > - A new patch.
> > ---
> >  drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-----------
> > --
> >  1 file changed, 25 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2-sessions.c
> > b/drivers/char/tpm/tpm2-sessions.c
> > index 1aef5b1f9c90..a8d3d5d52178 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -484,7 +484,8 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char
> > *str, u8 *pt_u, u8 *pt_v,
> >         sha256_final(&sctx, out);
> >  }
> >  
> > -static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip
> > *chip)
> > +static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip
> > *chip,
> > +                               struct tpm2_auth *auth)
>
> This addition of auth as an argument is a bit unnecessary.  You can set
> chip->auth before calling this and it will all function.  Since there's
> no error leg in tpm2_start_auth_session unless the session creation
> itself fails and the guarantee of the ops lock is single threading this
> chip->auth can be nulled again in that error leg.
>
> If you want to keep the flow proposed in the patch, the change from how
> it works now to how it works with this patch needs documenting in the
> change log

I checked this through and have to disagree with it. We don't want
to set chip->auth before the whole start auth session is successful

BR, Jarkko

[PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[Jarkko Sakkinen]

Instead of flushing and reloading the auth session for every single
transaction, keep the session open unless /dev/tpm0 is used. In practice
this means applying TPM2_SA_CONTINUE_SESSION to the session attributes.
Flush the session always when /dev/tpm0 is written.

Reported-by: Pengyu Ma <mapengyu@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229
Cc: stable@vger.kernel.org # v6.10+
Fixes: 7ca110f2679b ("tpm: Address !chip->auth in tpm_buf_append_hmac_session*()")
Tested-by: Pengyu Ma <mapengyu@gmail.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v5:
- No changes.
v4:
- Changed as bug.
v3:
- Refined the commit message.
- Removed the conditional for applying TPM2_SA_CONTINUE_SESSION only when
  /dev/tpm0 is open. It is not required as the auth session is flushed,
  not saved.
v2:
- A new patch.
---
 drivers/char/tpm/tpm-chip.c       | 1 +
 drivers/char/tpm/tpm-dev-common.c | 1 +
 drivers/char/tpm/tpm-interface.c  | 1 +
 drivers/char/tpm/tpm2-sessions.c  | 3 +++
 4 files changed, 6 insertions(+)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0ea00e32f575..7a6bb30d1f32 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -680,6 +680,7 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 	rc = tpm_try_get_ops(chip);
 	if (!rc) {
 		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+			tpm2_end_auth_session(chip);
 			tpm2_flush_context(chip, chip->null_key);
 			chip->null_key = 0;
 		}
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 4eaa8e05c291..a3ed7a99a394 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -29,6 +29,7 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
 
 #ifdef CONFIG_TCG_TPM2_HMAC
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		tpm2_end_auth_session(chip);
 		tpm2_flush_context(chip, chip->null_key);
 		chip->null_key = 0;
 	}
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index bfa47d48b0f2..2363018fa8fb 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -381,6 +381,7 @@ int tpm_pm_suspend(struct device *dev)
 	if (!rc) {
 		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
 #ifdef CONFIG_TCG_TPM2_HMAC
+			tpm2_end_auth_session(chip);
 			tpm2_flush_context(chip, chip->null_key);
 			chip->null_key = 0;
 #endif
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index a8d3d5d52178..38b92ad9e75f 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -333,6 +333,9 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
 	}
 
 #ifdef CONFIG_TCG_TPM2_HMAC
+	/* The first write to /dev/tpm{rm0} will flush the session. */
+	attributes |= TPM2_SA_CONTINUE_SESSION;
+
 	/*
 	 * The Architecture Guide requires us to strip trailing zeros
 	 * before computing the HMAC
-- 
2.46.1

Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[James Bottomley]

On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> Instead of flushing and reloading the auth session for every single
> transaction, keep the session open unless /dev/tpm0 is used. In
> practice this means applying TPM2_SA_CONTINUE_SESSION to the session
> attributes. Flush the session always when /dev/tpm0 is written.

Patch looks fine but this description is way too terse to explain how
it works.

I would suggest:

Boot time elongation as a result of adding sessions has been reported
as an issue in https://bugzilla.kernel.org/show_bug.cgi?id=219229

The root cause is the addition of session overhead to
tpm2_pcr_extend().  This overhead can be reduced by not creating and
destroying a session for each invocation of the function.  Do this by
keeping a session resident in the TPM for reuse by any session based
TPM command.  The current flow of TPM commands in the kernel supports
this because tpm2_end_session() is only called for tpm errors because
most commands don't continue the session and expect the session to be
flushed on success.  Thus we can add the continue session flag to
session creation to ensure the session won't be flushed except on
error, which is a rare case.

Since the session consumes a slot in the TPM it must not be seen by
userspace but we can flush it whenever a command write occurs and re-
create it again on the next kernel session use.  Since TPM use in boot
is somewhat rare this allows considerable reuse of the in-kernel
session and shortens boot time:

<give figures>



> 
> Reported-by: Pengyu Ma <mapengyu@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229
> Cc: stable@vger.kernel.org # v6.10+
> Fixes: 7ca110f2679b ("tpm: Address !chip->auth in
> tpm_buf_append_hmac_session*()")
> Tested-by: Pengyu Ma <mapengyu@gmail.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v5:
> - No changes.
> v4:
> - Changed as bug.
> v3:
> - Refined the commit message.
> - Removed the conditional for applying TPM2_SA_CONTINUE_SESSION only
> when
>   /dev/tpm0 is open. It is not required as the auth session is
> flushed,
>   not saved.
> v2:
> - A new patch.
> ---
>  drivers/char/tpm/tpm-chip.c       | 1 +
>  drivers/char/tpm/tpm-dev-common.c | 1 +
>  drivers/char/tpm/tpm-interface.c  | 1 +
>  drivers/char/tpm/tpm2-sessions.c  | 3 +++
>  4 files changed, 6 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-
> chip.c
> index 0ea00e32f575..7a6bb30d1f32 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -680,6 +680,7 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>         rc = tpm_try_get_ops(chip);
>         if (!rc) {
>                 if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +                       tpm2_end_auth_session(chip);
>                         tpm2_flush_context(chip, chip->null_key);
>                         chip->null_key = 0;
>                 }
> diff --git a/drivers/char/tpm/tpm-dev-common.c
> b/drivers/char/tpm/tpm-dev-common.c
> index 4eaa8e05c291..a3ed7a99a394 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -29,6 +29,7 @@ static ssize_t tpm_dev_transmit(struct tpm_chip
> *chip, struct tpm_space *space,
>  
>  #ifdef CONFIG_TCG_TPM2_HMAC
>         if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +               tpm2_end_auth_session(chip);
>                 tpm2_flush_context(chip, chip->null_key);
>                 chip->null_key = 0;
>         }
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-
> interface.c
> index bfa47d48b0f2..2363018fa8fb 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -381,6 +381,7 @@ int tpm_pm_suspend(struct device *dev)
>         if (!rc) {
>                 if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>  #ifdef CONFIG_TCG_TPM2_HMAC
> +                       tpm2_end_auth_session(chip);
>                         tpm2_flush_context(chip, chip->null_key);
>                         chip->null_key = 0;
>  #endif
> diff --git a/drivers/char/tpm/tpm2-sessions.c
> b/drivers/char/tpm/tpm2-sessions.c
> index a8d3d5d52178..38b92ad9e75f 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -333,6 +333,9 @@ void tpm_buf_append_hmac_session(struct tpm_chip
> *chip, struct tpm_buf *buf,
>         }
>  
>  #ifdef CONFIG_TCG_TPM2_HMAC
> +       /* The first write to /dev/tpm{rm0} will flush the session.
> */
> +       attributes |= TPM2_SA_CONTINUE_SESSION;
> +
>         /*
>          * The Architecture Guide requires us to strip trailing zeros
>          * before computing the HMAC

Code is fine, with the change log update, you can add

Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote:
> On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > Instead of flushing and reloading the auth session for every single
> > transaction, keep the session open unless /dev/tpm0 is used. In
> > practice this means applying TPM2_SA_CONTINUE_SESSION to the session
> > attributes. Flush the session always when /dev/tpm0 is written.
>
> Patch looks fine but this description is way too terse to explain how
> it works.
>
> I would suggest:
>
> Boot time elongation as a result of adding sessions has been reported
> as an issue in https://bugzilla.kernel.org/show_bug.cgi?id=219229
>
> The root cause is the addition of session overhead to
> tpm2_pcr_extend().  This overhead can be reduced by not creating and
> destroying a session for each invocation of the function.  Do this by
> keeping a session resident in the TPM for reuse by any session based
> TPM command.  The current flow of TPM commands in the kernel supports
> this because tpm2_end_session() is only called for tpm errors because
> most commands don't continue the session and expect the session to be
> flushed on success.  Thus we can add the continue session flag to
> session creation to ensure the session won't be flushed except on
> error, which is a rare case.
>
> Since the session consumes a slot in the TPM it must not be seen by
> userspace but we can flush it whenever a command write occurs and re-
> create it again on the next kernel session use.  Since TPM use in boot
> is somewhat rare this allows considerable reuse of the in-kernel
> session and shortens boot time:
>
> <give figures>
>
>
>
> > 
> > Reported-by: Pengyu Ma <mapengyu@gmail.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229
> > Cc: stable@vger.kernel.org # v6.10+
> > Fixes: 7ca110f2679b ("tpm: Address !chip->auth in
> > tpm_buf_append_hmac_session*()")
> > Tested-by: Pengyu Ma <mapengyu@gmail.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v5:
> > - No changes.
> > v4:
> > - Changed as bug.
> > v3:
> > - Refined the commit message.
> > - Removed the conditional for applying TPM2_SA_CONTINUE_SESSION only
> > when
> >   /dev/tpm0 is open. It is not required as the auth session is
> > flushed,
> >   not saved.
> > v2:
> > - A new patch.
> > ---
> >  drivers/char/tpm/tpm-chip.c       | 1 +
> >  drivers/char/tpm/tpm-dev-common.c | 1 +
> >  drivers/char/tpm/tpm-interface.c  | 1 +
> >  drivers/char/tpm/tpm2-sessions.c  | 3 +++
> >  4 files changed, 6 insertions(+)
> > 
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-
> > chip.c
> > index 0ea00e32f575..7a6bb30d1f32 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -680,6 +680,7 @@ void tpm_chip_unregister(struct tpm_chip *chip)
> >         rc = tpm_try_get_ops(chip);
> >         if (!rc) {
> >                 if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > +                       tpm2_end_auth_session(chip);
> >                         tpm2_flush_context(chip, chip->null_key);
> >                         chip->null_key = 0;
> >                 }
> > diff --git a/drivers/char/tpm/tpm-dev-common.c
> > b/drivers/char/tpm/tpm-dev-common.c
> > index 4eaa8e05c291..a3ed7a99a394 100644
> > --- a/drivers/char/tpm/tpm-dev-common.c
> > +++ b/drivers/char/tpm/tpm-dev-common.c
> > @@ -29,6 +29,7 @@ static ssize_t tpm_dev_transmit(struct tpm_chip
> > *chip, struct tpm_space *space,
> >  
> >  #ifdef CONFIG_TCG_TPM2_HMAC
> >         if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > +               tpm2_end_auth_session(chip);
> >                 tpm2_flush_context(chip, chip->null_key);
> >                 chip->null_key = 0;
> >         }
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-
> > interface.c
> > index bfa47d48b0f2..2363018fa8fb 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -381,6 +381,7 @@ int tpm_pm_suspend(struct device *dev)
> >         if (!rc) {
> >                 if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> >  #ifdef CONFIG_TCG_TPM2_HMAC
> > +                       tpm2_end_auth_session(chip);
> >                         tpm2_flush_context(chip, chip->null_key);
> >                         chip->null_key = 0;
> >  #endif
> > diff --git a/drivers/char/tpm/tpm2-sessions.c
> > b/drivers/char/tpm/tpm2-sessions.c
> > index a8d3d5d52178..38b92ad9e75f 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -333,6 +333,9 @@ void tpm_buf_append_hmac_session(struct tpm_chip
> > *chip, struct tpm_buf *buf,
> >         }
> >  
> >  #ifdef CONFIG_TCG_TPM2_HMAC
> > +       /* The first write to /dev/tpm{rm0} will flush the session.
> > */
> > +       attributes |= TPM2_SA_CONTINUE_SESSION;
> > +
> >         /*
> >          * The Architecture Guide requires us to strip trailing zeros
> >          * before computing the HMAC
>
> Code is fine, with the change log update, you can add
>
> Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>

OK, thank you.

BR, Jarkko

Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote:
> On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > Instead of flushing and reloading the auth session for every single
> > transaction, keep the session open unless /dev/tpm0 is used. In
> > practice this means applying TPM2_SA_CONTINUE_SESSION to the session
> > attributes. Flush the session always when /dev/tpm0 is written.
>
> Patch looks fine but this description is way too terse to explain how
> it works.
>
> I would suggest:
>
> Boot time elongation as a result of adding sessions has been reported
> as an issue in https://bugzilla.kernel.org/show_bug.cgi?id=219229
>
> The root cause is the addition of session overhead to
> tpm2_pcr_extend().  This overhead can be reduced by not creating and
> destroying a session for each invocation of the function.  Do this by
> keeping a session resident in the TPM for reuse by any session based
> TPM command.  The current flow of TPM commands in the kernel supports
> this because tpm2_end_session() is only called for tpm errors because
> most commands don't continue the session and expect the session to be
> flushed on success.  Thus we can add the continue session flag to
> session creation to ensure the session won't be flushed except on
> error, which is a rare case.

I need to disagree on this as I don't even have PCR extends in my
boot sequence and it still adds overhead. Have you verified this
from the reporter?

There's bunch of things that use auth session, like trusted keys.
Making such claim that PCR extend is the reason is nonsense.

BR, Jarkko

Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[James Bottomley]

On Tue, 2024-09-24 at 21:07 +0300, Jarkko Sakkinen wrote:
> On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote:
> > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > > Instead of flushing and reloading the auth session for every
> > > single transaction, keep the session open unless /dev/tpm0 is
> > > used. In practice this means applying TPM2_SA_CONTINUE_SESSION to
> > > the session attributes. Flush the session always when /dev/tpm0
> > > is written.
> > 
> > Patch looks fine but this description is way too terse to explain
> > how it works.
> > 
> > I would suggest:
> > 
> > Boot time elongation as a result of adding sessions has been
> > reported as an issue in
> > https://bugzilla.kernel.org/show_bug.cgi?id=219229
> > 
> > The root cause is the addition of session overhead to
> > tpm2_pcr_extend().  This overhead can be reduced by not creating
> > and destroying a session for each invocation of the function.  Do
> > this by keeping a session resident in the TPM for reuse by any
> > session based TPM command.  The current flow of TPM commands in the
> > kernel supports this because tpm2_end_session() is only called for
> > tpm errors because most commands don't continue the session and
> > expect the session to be flushed on success.  Thus we can add the
> > continue session flag to session creation to ensure the session
> > won't be flushed except on error, which is a rare case.
> 
> I need to disagree on this as I don't even have PCR extends in my
> boot sequence and it still adds overhead. Have you verified this
> from the reporter?
> 
> There's bunch of things that use auth session, like trusted keys.
> Making such claim that PCR extend is the reason is nonsense.

Well, the bug report does say it's the commit adding sessions to the
PCR extends that causes the delay:

https://bugzilla.kernel.org/show_bug.cgi?id=219229#c5

I don't know what else to tell you.

James

Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 9:40 PM EEST, James Bottomley wrote:
> On Tue, 2024-09-24 at 21:07 +0300, Jarkko Sakkinen wrote:
> > On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote:
> > > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > > > Instead of flushing and reloading the auth session for every
> > > > single transaction, keep the session open unless /dev/tpm0 is
> > > > used. In practice this means applying TPM2_SA_CONTINUE_SESSION to
> > > > the session attributes. Flush the session always when /dev/tpm0
> > > > is written.
> > > 
> > > Patch looks fine but this description is way too terse to explain
> > > how it works.
> > > 
> > > I would suggest:
> > > 
> > > Boot time elongation as a result of adding sessions has been
> > > reported as an issue in
> > > https://bugzilla.kernel.org/show_bug.cgi?id=219229
> > > 
> > > The root cause is the addition of session overhead to
> > > tpm2_pcr_extend().  This overhead can be reduced by not creating
> > > and destroying a session for each invocation of the function.  Do
> > > this by keeping a session resident in the TPM for reuse by any
> > > session based TPM command.  The current flow of TPM commands in the
> > > kernel supports this because tpm2_end_session() is only called for
> > > tpm errors because most commands don't continue the session and
> > > expect the session to be flushed on success.  Thus we can add the
> > > continue session flag to session creation to ensure the session
> > > won't be flushed except on error, which is a rare case.
> > 
> > I need to disagree on this as I don't even have PCR extends in my
> > boot sequence and it still adds overhead. Have you verified this
> > from the reporter?
> > 
> > There's bunch of things that use auth session, like trusted keys.
> > Making such claim that PCR extend is the reason is nonsense.
>
> Well, the bug report does say it's the commit adding sessions to the
> PCR extends that causes the delay:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=219229#c5
>
> I don't know what else to tell you.

As far as I've tested this bug I've been able to generate similar costs
with anything using HMAC encryption. PCR extend op itself should have
same cost with or without encryption AFAIK.

I guess I need provide benchmarks on this to prove that PCR extend is
not the only site that is affected.

BR, Jarkko

Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[James Bottomley]

On Wed, 2024-09-25 at 00:35 +0300, Jarkko Sakkinen wrote:
> On Tue Sep 24, 2024 at 9:40 PM EEST, James Bottomley wrote:
> > On Tue, 2024-09-24 at 21:07 +0300, Jarkko Sakkinen wrote:
> > > On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote:
> > > > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > > > > Instead of flushing and reloading the auth session for every
> > > > > single transaction, keep the session open unless /dev/tpm0 is
> > > > > used. In practice this means applying
> > > > > TPM2_SA_CONTINUE_SESSION to the session attributes. Flush the
> > > > > session always when /dev/tpm0 is written.
> > > > 
> > > > Patch looks fine but this description is way too terse to
> > > > explain how it works.
> > > > 
> > > > I would suggest:
> > > > 
> > > > Boot time elongation as a result of adding sessions has been
> > > > reported as an issue in
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=219229
> > > > 
> > > > The root cause is the addition of session overhead to
> > > > tpm2_pcr_extend().  This overhead can be reduced by not
> > > > creating and destroying a session for each invocation of the
> > > > function. Do this by keeping a session resident in the TPM for
> > > > reuse by any session based TPM command.  The current flow of
> > > > TPM commands in the kernel supports this because
> > > > tpm2_end_session() is only called for tpm errors because most
> > > > commands don't continue the session and expect the session to
> > > > be flushed on success.  Thus we can add the continue session
> > > > flag to session creation to ensure the session won't be flushed
> > > > except on error, which is a rare case.
> > > 
> > > I need to disagree on this as I don't even have PCR extends in my
> > > boot sequence and it still adds overhead. Have you verified this
> > > from the reporter?
> > > 
> > > There's bunch of things that use auth session, like trusted keys.
> > > Making such claim that PCR extend is the reason is nonsense.
> > 
> > Well, the bug report does say it's the commit adding sessions to
> > the PCR extends that causes the delay:
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=219229#c5
> > 
> > I don't know what else to tell you.
> 
> As far as I've tested this bug I've been able to generate similar
> costs with anything using HMAC encryption. PCR extend op itself
> should have same cost with or without encryption AFAIK.

That's true, but the only significant TPM operation in the secure boot
path is the PCR extend for IMA.  The RNG stuff is there a bit, but
there are other significant delays in seeding the entropy pool.  During
boot with IMA enabled, you can do hundreds of binary measurements,
hence the slow down.

> I guess I need provide benchmarks on this to prove that PCR extend is
> not the only site that is affected.

Well, on the per operation figures, it's obviously not, a standard TPM
operation gets a significant overhead because of sessions. However, it
is the only site that causes a large boot slowdown because of the
number of the number of measurements IMA does on boot.

Regards,

James

Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[Jarkko Sakkinen]

On Wed Sep 25, 2024 at 12:51 AM EEST, James Bottomley wrote:
> On Wed, 2024-09-25 at 00:35 +0300, Jarkko Sakkinen wrote:
> > On Tue Sep 24, 2024 at 9:40 PM EEST, James Bottomley wrote:
> > > On Tue, 2024-09-24 at 21:07 +0300, Jarkko Sakkinen wrote:
> > > > On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote:
> > > > > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > > > > > Instead of flushing and reloading the auth session for every
> > > > > > single transaction, keep the session open unless /dev/tpm0 is
> > > > > > used. In practice this means applying
> > > > > > TPM2_SA_CONTINUE_SESSION to the session attributes. Flush the
> > > > > > session always when /dev/tpm0 is written.
> > > > > 
> > > > > Patch looks fine but this description is way too terse to
> > > > > explain how it works.
> > > > > 
> > > > > I would suggest:
> > > > > 
> > > > > Boot time elongation as a result of adding sessions has been
> > > > > reported as an issue in
> > > > > https://bugzilla.kernel.org/show_bug.cgi?id=219229
> > > > > 
> > > > > The root cause is the addition of session overhead to
> > > > > tpm2_pcr_extend().  This overhead can be reduced by not
> > > > > creating and destroying a session for each invocation of the
> > > > > function. Do this by keeping a session resident in the TPM for
> > > > > reuse by any session based TPM command.  The current flow of
> > > > > TPM commands in the kernel supports this because
> > > > > tpm2_end_session() is only called for tpm errors because most
> > > > > commands don't continue the session and expect the session to
> > > > > be flushed on success.  Thus we can add the continue session
> > > > > flag to session creation to ensure the session won't be flushed
> > > > > except on error, which is a rare case.
> > > > 
> > > > I need to disagree on this as I don't even have PCR extends in my
> > > > boot sequence and it still adds overhead. Have you verified this
> > > > from the reporter?
> > > > 
> > > > There's bunch of things that use auth session, like trusted keys.
> > > > Making such claim that PCR extend is the reason is nonsense.
> > > 
> > > Well, the bug report does say it's the commit adding sessions to
> > > the PCR extends that causes the delay:
> > > 
> > > https://bugzilla.kernel.org/show_bug.cgi?id=219229#c5
> > > 
> > > I don't know what else to tell you.
> > 
> > As far as I've tested this bug I've been able to generate similar
> > costs with anything using HMAC encryption. PCR extend op itself
> > should have same cost with or without encryption AFAIK.
>
> That's true, but the only significant TPM operation in the secure boot
> path is the PCR extend for IMA.  The RNG stuff is there a bit, but
> there are other significant delays in seeding the entropy pool.  During
> boot with IMA enabled, you can do hundreds of binary measurements,
> hence the slow down.
>
> > I guess I need provide benchmarks on this to prove that PCR extend is
> > not the only site that is affected.
>
> Well, on the per operation figures, it's obviously not, a standard TPM
> operation gets a significant overhead because of sessions. However, it
> is the only site that causes a large boot slowdown because of the
> number of the number of measurements IMA does on boot.

Fair enough. I can buy this.

I'll phrase it that (since it was mentioned in the bugzilla comment)
in the bug in question the root is in PCR extend but since in my own
tests I got overhead from trusted keys I also mention that it overally
affects also that and tpm2_get_random().

>
> Regards,
>
> James

BR, Jarkko

Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[Jarkko Sakkinen]

On Wed Sep 25, 2024 at 10:42 AM EEST, Jarkko Sakkinen wrote:
> Fair enough. I can buy this.
>
> I'll phrase it that (since it was mentioned in the bugzilla comment)
> in the bug in question the root is in PCR extend but since in my own
> tests I got overhead from trusted keys I also mention that it overally
> affects also that and tpm2_get_random().

I do not want to take null key flushing away although I got the
reasoning given the small amount of time is saved (maybe 25-50 ms
in my QEMU setup if I recall correctly) but it would make sense to
squash it auth session patch.

I'll also check 1/2 and 2/2 if I'm doing too much in them. Not
adding any tags to v6 and it really makes sense to develop 
benchmarks and not rush with the new version now that I got
also your feedback, since it is past rc1 timeline.

Good target rcX would be around rc3.

BR, Jarkko

Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[Jarkko Sakkinen]

On Wed Sep 25, 2024 at 10:46 AM EEST, Jarkko Sakkinen wrote:
> On Wed Sep 25, 2024 at 10:42 AM EEST, Jarkko Sakkinen wrote:
> > Fair enough. I can buy this.
> >
> > I'll phrase it that (since it was mentioned in the bugzilla comment)
> > in the bug in question the root is in PCR extend but since in my own
> > tests I got overhead from trusted keys I also mention that it overally
> > affects also that and tpm2_get_random().
>
> I do not want to take null key flushing away although I got the
> reasoning given the small amount of time is saved (maybe 25-50 ms
> in my QEMU setup if I recall correctly) but it would make sense to
> squash it auth session patch.
>
> I'll also check 1/2 and 2/2 if I'm doing too much in them. Not
> adding any tags to v6 and it really makes sense to develop 
> benchmarks and not rush with the new version now that I got
> also your feedback, since it is past rc1 timeline.
>
> Good target rcX would be around rc3.

I have to admit this: I had blind spot on that PCR extend comment
because I did not get hits on that when testing this so it definitely
needs to be documented. I spotted it only yesterday.

BR, Jarkko