[PATCH v5 0/5] Lazy flush for the auth session

[PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

This patch set aims to fix:
https://bugzilla.kernel.org/show_bug.cgi?id=219229.

The baseline for the series is the v6.11 tag.

v4:
https://lore.kernel.org/linux-integrity/20240918203559.192605-1-jarkko@kernel.org/
v3:
https://lore.kernel.org/linux-integrity/20240917154444.702370-1-jarkko@kernel.org/
v2:
https://lore.kernel.org/linux-integrity/20240916110714.1396407-1-jarkko@kernel.org/
v1:
https://lore.kernel.org/linux-integrity/20240915180448.2030115-1-jarkko@kernel.org/

Jarkko Sakkinen (5):
  tpm: Return on tpm2_create_null_primary() failure
  tpm: Implement tpm2_load_null() rollback
  tpm: flush the null key only when /dev/tpm0 is accessed
  tpm: Allocate chip->auth in tpm2_start_auth_session()
  tpm: flush the auth session only when /dev/tpm0 is open

 drivers/char/tpm/tpm-chip.c       |  14 ++++
 drivers/char/tpm/tpm-dev-common.c |   8 +++
 drivers/char/tpm/tpm-interface.c  |  10 ++-
 drivers/char/tpm/tpm2-cmd.c       |   3 +
 drivers/char/tpm/tpm2-sessions.c  | 109 ++++++++++++++++++------------
 include/linux/tpm.h               |   2 +
 6 files changed, 102 insertions(+), 44 deletions(-)

-- 
2.46.1

[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

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Paul Menzel]

Dear Jarkko,


Thank you for working on this and your patches.

Am 21.09.24 um 14:08 schrieb Jarkko Sakkinen:
> This patch set aims to fix:
> https://bugzilla.kernel.org/show_bug.cgi?id=219229.

If I am not mistaken this is about reducing the boot time, right? It’d 
be great if you documented the numbers in the commit messages.


Kind regards,

Paul

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Sat Sep 21, 2024 at 3:36 PM EEST, Paul Menzel wrote:
> Dear Jarkko,
>
>
> Thank you for working on this and your patches.
>
> Am 21.09.24 um 14:08 schrieb Jarkko Sakkinen:
> > This patch set aims to fix:
> > https://bugzilla.kernel.org/show_bug.cgi?id=219229.
>
> If I am not mistaken this is about reducing the boot time, right? It’d 
> be great if you documented the numbers in the commit messages.

So what you're asking is already documented by:

1. https://bugzilla.kernel.org/show_bug.cgi?id=219229
2. Tested-by

I added lore link to the bugzilla bug, in order to address your comment.

I don't think my "QEMU numbers" will bring any value here tbh.

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Sat Sep 21, 2024 at 4:13 PM EEST, Jarkko Sakkinen wrote:
> On Sat Sep 21, 2024 at 3:36 PM EEST, Paul Menzel wrote:
> > Dear Jarkko,
> >
> >
> > Thank you for working on this and your patches.
> >
> > Am 21.09.24 um 14:08 schrieb Jarkko Sakkinen:
> > > This patch set aims to fix:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=219229.
> >
> > If I am not mistaken this is about reducing the boot time, right? It’d 
> > be great if you documented the numbers in the commit messages.
>
> So what you're asking is already documented by:
>
> 1. https://bugzilla.kernel.org/show_bug.cgi?id=219229
> 2. Tested-by
>
> I added lore link to the bugzilla bug, in order to address your comment.

... to the email which contains the figures.

20 s -> 8.7 s

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Sat Sep 21, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> This patch set aims to fix:
> https://bugzilla.kernel.org/show_bug.cgi?id=219229.
>
> The baseline for the series is the v6.11 tag.
>
> v4:
> https://lore.kernel.org/linux-integrity/20240918203559.192605-1-jarkko@kernel.org/
> v3:
> https://lore.kernel.org/linux-integrity/20240917154444.702370-1-jarkko@kernel.org/
> v2:
> https://lore.kernel.org/linux-integrity/20240916110714.1396407-1-jarkko@kernel.org/
> v1:
> https://lore.kernel.org/linux-integrity/20240915180448.2030115-1-jarkko@kernel.org/
>
> Jarkko Sakkinen (5):
>   tpm: Return on tpm2_create_null_primary() failure
>   tpm: Implement tpm2_load_null() rollback
>   tpm: flush the null key only when /dev/tpm0 is accessed
>   tpm: Allocate chip->auth in tpm2_start_auth_session()
>   tpm: flush the auth session only when /dev/tpm0 is open
>
>  drivers/char/tpm/tpm-chip.c       |  14 ++++
>  drivers/char/tpm/tpm-dev-common.c |   8 +++
>  drivers/char/tpm/tpm-interface.c  |  10 ++-
>  drivers/char/tpm/tpm2-cmd.c       |   3 +
>  drivers/char/tpm/tpm2-sessions.c  | 109 ++++++++++++++++++------------
>  include/linux/tpm.h               |   2 +
>  6 files changed, 102 insertions(+), 44 deletions(-)


Roberto, James, speaking of digest cache. This patch set has no aim to
fix those issues but I do believe that it should improve also that 
feature.

If I don't get soon patch reviews for the patch set, I'll pick the 2nd
best option: disable bus encryption on all architectures including x86
and ARM64 (being by default on).

It's a force majeure situation. I know this would sort out the issue
but I really cannot send these as a pull request with zero reviewe-by's.

I expect this to be closed by tomorrow.

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[James Bottomley]

On Sun, 2024-09-22 at 20:51 +0300, Jarkko Sakkinen wrote:
> On Sat Sep 21, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> > This patch set aims to fix:
> > https://bugzilla.kernel.org/show_bug.cgi?id=219229.
> > 
> > The baseline for the series is the v6.11 tag.
> > 
> > v4:
> > https://lore.kernel.org/linux-integrity/20240918203559.192605-1-jarkko@kernel.org/
> > v3:
> > https://lore.kernel.org/linux-integrity/20240917154444.702370-1-jarkko@kernel.org/
> > v2:
> > https://lore.kernel.org/linux-integrity/20240916110714.1396407-1-jarkko@kernel.org/
> > v1:
> > https://lore.kernel.org/linux-integrity/20240915180448.2030115-1-jarkko@kernel.org/
> > 
> > Jarkko Sakkinen (5):
> >   tpm: Return on tpm2_create_null_primary() failure
> >   tpm: Implement tpm2_load_null() rollback
> >   tpm: flush the null key only when /dev/tpm0 is accessed
> >   tpm: Allocate chip->auth in tpm2_start_auth_session()
> >   tpm: flush the auth session only when /dev/tpm0 is open
> > 
> >  drivers/char/tpm/tpm-chip.c       |  14 ++++
> >  drivers/char/tpm/tpm-dev-common.c |   8 +++
> >  drivers/char/tpm/tpm-interface.c  |  10 ++-
> >  drivers/char/tpm/tpm2-cmd.c       |   3 +
> >  drivers/char/tpm/tpm2-sessions.c  | 109 ++++++++++++++++++--------
> > ----
> >  include/linux/tpm.h               |   2 +
> >  6 files changed, 102 insertions(+), 44 deletions(-)
> 
> 
> Roberto, James, speaking of digest cache. This patch set has no aim
> to fix those issues but I do believe that it should improve also that
> feature.
> 
> If I don't get soon patch reviews for the patch set, I'll pick the
> 2nd best option: disable bus encryption on all architectures
> including x86 and ARM64 (being by default on).
> 
> It's a force majeure situation. I know this would sort out the issue
> but I really cannot send these as a pull request with zero reviewe-
> by's.
> 
> I expect this to be closed by tomorrow.

Hey come on, you knew I was running plumbers last week so I had all the
lead up and teardown stuff to do as well.  I'm only just digging
through accumulated email.

Patches 1-2 are fully irrelevant to the bug, so I ignored them on the
grounds that improvement to the error flow could be done through the
normal patch process

Patch 3 is completely unnecessary: the null key is only used to salt
the session and is not required to be resident while the session is
used (so can be flushed after session creation) therefore keeping it
around serves no purpose once the session is created and simply
clutters up the TPM volatile handle slots. (I don't know of a case
where we use all the slots in a kernel operation, but since we don't
need it lets not find out when we get one).  So I advise dropping patch
3.

I've reviewed 4 and 5.

Regards,

James

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:
> On Sun, 2024-09-22 at 20:51 +0300, Jarkko Sakkinen wrote:
> > On Sat Sep 21, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> > > This patch set aims to fix:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=219229.
> > > 
> > > The baseline for the series is the v6.11 tag.
> > > 
> > > v4:
> > > https://lore.kernel.org/linux-integrity/20240918203559.192605-1-jarkko@kernel.org/
> > > v3:
> > > https://lore.kernel.org/linux-integrity/20240917154444.702370-1-jarkko@kernel.org/
> > > v2:
> > > https://lore.kernel.org/linux-integrity/20240916110714.1396407-1-jarkko@kernel.org/
> > > v1:
> > > https://lore.kernel.org/linux-integrity/20240915180448.2030115-1-jarkko@kernel.org/
> > > 
> > > Jarkko Sakkinen (5):
> > >   tpm: Return on tpm2_create_null_primary() failure
> > >   tpm: Implement tpm2_load_null() rollback
> > >   tpm: flush the null key only when /dev/tpm0 is accessed
> > >   tpm: Allocate chip->auth in tpm2_start_auth_session()
> > >   tpm: flush the auth session only when /dev/tpm0 is open
> > > 
> > >  drivers/char/tpm/tpm-chip.c       |  14 ++++
> > >  drivers/char/tpm/tpm-dev-common.c |   8 +++
> > >  drivers/char/tpm/tpm-interface.c  |  10 ++-
> > >  drivers/char/tpm/tpm2-cmd.c       |   3 +
> > >  drivers/char/tpm/tpm2-sessions.c  | 109 ++++++++++++++++++--------
> > > ----
> > >  include/linux/tpm.h               |   2 +
> > >  6 files changed, 102 insertions(+), 44 deletions(-)
> > 
> > 
> > Roberto, James, speaking of digest cache. This patch set has no aim
> > to fix those issues but I do believe that it should improve also that
> > feature.
> > 
> > If I don't get soon patch reviews for the patch set, I'll pick the
> > 2nd best option: disable bus encryption on all architectures
> > including x86 and ARM64 (being by default on).
> > 
> > It's a force majeure situation. I know this would sort out the issue
> > but I really cannot send these as a pull request with zero reviewe-
> > by's.
> > 
> > I expect this to be closed by tomorrow.
>
> Hey come on, you knew I was running plumbers last week so I had all the
> lead up and teardown stuff to do as well.  I'm only just digging
> through accumulated email.

Fair enough, I actually do not want to disable the feature. That
was my main concern here. Now if we get this fixed we might be
able to revisit earlier decisions on defconfig and widen the
support eventually, not shrink it.


>
> Patches 1-2 are fully irrelevant to the bug, so I ignored them on the
> grounds that improvement to the error flow could be done through the
> normal patch process

Hmm.. I'll revisit this for v6. Not sure what to say on this yet
because I need to address the other remarks and based on that
reflect. So might drop or keep them but not 100% sure yet.


> Patch 3 is completely unnecessary: the null key is only used to salt
> the session and is not required to be resident while the session is
> used (so can be flushed after session creation) therefore keeping it
> around serves no purpose once the session is created and simply
> clutters up the TPM volatile handle slots. (I don't know of a case
> where we use all the slots in a kernel operation, but since we don't
> need it lets not find out when we get one).  So I advise dropping patch
> 3.

Let's go this through just to check I'm understanding.

Holding null key had radical effect on boot time: it cut it down by
5 secons down to 15 seconds:

https://lore.kernel.org/linux-integrity/CALSz7m1WG7fZ9UuO0URgCZEDG7r_wB4Ev_4mOHJThH_d1Ed1nw@mail.gmail.com/

Then in subsequent version I implemented lazy auth session and boot
time went down to 9.7 seconds.

So is the point you're trying to make that since auth session is 
already held as long as we can and they flushed in synchronous
point too, I can just as well drop patch 3?

I think I reach your point but just want to check that I do it
for the matching reasons. It is evolutionary cruft in the patch
set :-)

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[James Bottomley]

On Tue, 2024-09-24 at 19:29 +0300, Jarkko Sakkinen wrote:
> On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:
[...]
> > Patch 3 is completely unnecessary: the null key is only used to
> > salt the session and is not required to be resident while the
> > session is used (so can be flushed after session creation)
> > therefore keeping it around serves no purpose once the session is
> > created and simply clutters up the TPM volatile handle slots. (I
> > don't know of a case where we use all the slots in a kernel
> > operation, but since we don't need it lets not find out when we get
> > one).  So I advise dropping patch 3.
> 
> Let's go this through just to check I'm understanding.
> 
> Holding null key had radical effect on boot time: it cut it down by
> 5 secons down to 15 seconds:
> 
> https://lore.kernel.org/linux-integrity/CALSz7m1WG7fZ9UuO0URgCZEDG7r_wB4Ev_4mOHJThH_d1Ed1nw@mail.gmail.com/
> 
> Then in subsequent version I implemented lazy auth session and boot
> time went down to 9.7 seconds.
> 
> So is the point you're trying to make that since auth session is 
> already held as long as we can and they flushed in synchronous
> point too, I can just as well drop patch 3?

Yes, because the null key is only used in session generation which is
now lazy, it adds or subtracts nothing from the timings.  When you're
forced to flush the session, the null key goes too, so you again have
to restore it from the context.  When you can keep the session you
don't need the null key because you're not regenerating it.

> I think I reach your point but just want to check that I do it
> for the matching reasons. It is evolutionary cruft in the patch
> set :-)

Regards,

James

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 7:33 PM EEST, James Bottomley wrote:
> On Tue, 2024-09-24 at 19:29 +0300, Jarkko Sakkinen wrote:
> > On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:
> [...]
> > > Patch 3 is completely unnecessary: the null key is only used to
> > > salt the session and is not required to be resident while the
> > > session is used (so can be flushed after session creation)
> > > therefore keeping it around serves no purpose once the session is
> > > created and simply clutters up the TPM volatile handle slots. (I
> > > don't know of a case where we use all the slots in a kernel
> > > operation, but since we don't need it lets not find out when we get
> > > one).  So I advise dropping patch 3.
> > 
> > Let's go this through just to check I'm understanding.
> > 
> > Holding null key had radical effect on boot time: it cut it down by
> > 5 secons down to 15 seconds:
> > 
> > https://lore.kernel.org/linux-integrity/CALSz7m1WG7fZ9UuO0URgCZEDG7r_wB4Ev_4mOHJThH_d1Ed1nw@mail.gmail.com/
> > 
> > Then in subsequent version I implemented lazy auth session and boot
> > time went down to 9.7 seconds.
> > 
> > So is the point you're trying to make that since auth session is 
> > already held as long as we can and they flushed in synchronous
> > point too, I can just as well drop patch 3?
>
> Yes, because the null key is only used in session generation which is
> now lazy, it adds or subtracts nothing from the timings.  When you're
> forced to flush the session, the null key goes too, so you again have
> to restore it from the context.  When you can keep the session you
> don't need the null key because you're not regenerating it.

Yeah, OK, then we're in sync with this. It's evolutionary cruft.

Just had to check that the logic matches how I projected your earlier
comment because these are sensitive changes.

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 7:36 PM EEST, Jarkko Sakkinen wrote:
> On Tue Sep 24, 2024 at 7:33 PM EEST, James Bottomley wrote:
> > On Tue, 2024-09-24 at 19:29 +0300, Jarkko Sakkinen wrote:
> > > On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:
> > [...]
> > > > Patch 3 is completely unnecessary: the null key is only used to
> > > > salt the session and is not required to be resident while the
> > > > session is used (so can be flushed after session creation)
> > > > therefore keeping it around serves no purpose once the session is
> > > > created and simply clutters up the TPM volatile handle slots. (I
> > > > don't know of a case where we use all the slots in a kernel
> > > > operation, but since we don't need it lets not find out when we get
> > > > one).  So I advise dropping patch 3.
> > > 
> > > Let's go this through just to check I'm understanding.
> > > 
> > > Holding null key had radical effect on boot time: it cut it down by
> > > 5 secons down to 15 seconds:
> > > 
> > > https://lore.kernel.org/linux-integrity/CALSz7m1WG7fZ9UuO0URgCZEDG7r_wB4Ev_4mOHJThH_d1Ed1nw@mail.gmail.com/
> > > 
> > > Then in subsequent version I implemented lazy auth session and boot
> > > time went down to 9.7 seconds.
> > > 
> > > So is the point you're trying to make that since auth session is 
> > > already held as long as we can and they flushed in synchronous
> > > point too, I can just as well drop patch 3?
> >
> > Yes, because the null key is only used in session generation which is
> > now lazy, it adds or subtracts nothing from the timings.  When you're
> > forced to flush the session, the null key goes too, so you again have
> > to restore it from the context.  When you can keep the session you
> > don't need the null key because you're not regenerating it.
>
> Yeah, OK, then we're in sync with this. It's evolutionary cruft.
>
> Just had to check that the logic matches how I projected your earlier
> comment because these are sensitive changes.

I'm definitely going keeep 1/5 and 2/5 as they are still bug fixes.

So they will appear in v6 unchanged and perf fixes (which are not
functional fixes) should not be built on top of broken code.

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 8:26 PM EEST, Jarkko Sakkinen wrote:
> On Tue Sep 24, 2024 at 7:36 PM EEST, Jarkko Sakkinen wrote:
> > On Tue Sep 24, 2024 at 7:33 PM EEST, James Bottomley wrote:
> > > On Tue, 2024-09-24 at 19:29 +0300, Jarkko Sakkinen wrote:
> > > > On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:
> > > [...]
> > > > > Patch 3 is completely unnecessary: the null key is only used to
> > > > > salt the session and is not required to be resident while the
> > > > > session is used (so can be flushed after session creation)
> > > > > therefore keeping it around serves no purpose once the session is
> > > > > created and simply clutters up the TPM volatile handle slots. (I
> > > > > don't know of a case where we use all the slots in a kernel
> > > > > operation, but since we don't need it lets not find out when we get
> > > > > one).  So I advise dropping patch 3.
> > > > 
> > > > Let's go this through just to check I'm understanding.
> > > > 
> > > > Holding null key had radical effect on boot time: it cut it down by
> > > > 5 secons down to 15 seconds:
> > > > 
> > > > https://lore.kernel.org/linux-integrity/CALSz7m1WG7fZ9UuO0URgCZEDG7r_wB4Ev_4mOHJThH_d1Ed1nw@mail.gmail.com/
> > > > 
> > > > Then in subsequent version I implemented lazy auth session and boot
> > > > time went down to 9.7 seconds.
> > > > 
> > > > So is the point you're trying to make that since auth session is 
> > > > already held as long as we can and they flushed in synchronous
> > > > point too, I can just as well drop patch 3?
> > >
> > > Yes, because the null key is only used in session generation which is
> > > now lazy, it adds or subtracts nothing from the timings.  When you're
> > > forced to flush the session, the null key goes too, so you again have
> > > to restore it from the context.  When you can keep the session you
> > > don't need the null key because you're not regenerating it.
> >
> > Yeah, OK, then we're in sync with this. It's evolutionary cruft.
> >
> > Just had to check that the logic matches how I projected your earlier
> > comment because these are sensitive changes.
>
> I'm definitely going keeep 1/5 and 2/5 as they are still bug fixes.
>
> So they will appear in v6 unchanged and perf fixes (which are not
> functional fixes) should not be built on top of broken code.

And 3/5 is actually required because it saves of doing flush during
the boot if nothing else.

We are wasting more time so I don't want to waste it for nothing.

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Tue Sep 24, 2024 at 8:28 PM EEST, Jarkko Sakkinen wrote:
> On Tue Sep 24, 2024 at 8:26 PM EEST, Jarkko Sakkinen wrote:
> > On Tue Sep 24, 2024 at 7:36 PM EEST, Jarkko Sakkinen wrote:
> > > On Tue Sep 24, 2024 at 7:33 PM EEST, James Bottomley wrote:
> > > > On Tue, 2024-09-24 at 19:29 +0300, Jarkko Sakkinen wrote:
> > > > > On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:
> > > > [...]
> > > > > > Patch 3 is completely unnecessary: the null key is only used to
> > > > > > salt the session and is not required to be resident while the
> > > > > > session is used (so can be flushed after session creation)
> > > > > > therefore keeping it around serves no purpose once the session is
> > > > > > created and simply clutters up the TPM volatile handle slots. (I
> > > > > > don't know of a case where we use all the slots in a kernel
> > > > > > operation, but since we don't need it lets not find out when we get
> > > > > > one).  So I advise dropping patch 3.
> > > > > 
> > > > > Let's go this through just to check I'm understanding.
> > > > > 
> > > > > Holding null key had radical effect on boot time: it cut it down by
> > > > > 5 secons down to 15 seconds:
> > > > > 
> > > > > https://lore.kernel.org/linux-integrity/CALSz7m1WG7fZ9UuO0URgCZEDG7r_wB4Ev_4mOHJThH_d1Ed1nw@mail.gmail.com/
> > > > > 
> > > > > Then in subsequent version I implemented lazy auth session and boot
> > > > > time went down to 9.7 seconds.
> > > > > 
> > > > > So is the point you're trying to make that since auth session is 
> > > > > already held as long as we can and they flushed in synchronous
> > > > > point too, I can just as well drop patch 3?
> > > >
> > > > Yes, because the null key is only used in session generation which is
> > > > now lazy, it adds or subtracts nothing from the timings.  When you're
> > > > forced to flush the session, the null key goes too, so you again have
> > > > to restore it from the context.  When you can keep the session you
> > > > don't need the null key because you're not regenerating it.
> > >
> > > Yeah, OK, then we're in sync with this. It's evolutionary cruft.
> > >
> > > Just had to check that the logic matches how I projected your earlier
> > > comment because these are sensitive changes.
> >
> > I'm definitely going keeep 1/5 and 2/5 as they are still bug fixes.
> >
> > So they will appear in v6 unchanged and perf fixes (which are not
> > functional fixes) should not be built on top of broken code.
>
> And 3/5 is actually required because it saves of doing flush during
> the boot if nothing else.
>
> We are wasting more time so I don't want to waste it for nothing.

Anything beyong 50 ms matters and that flush certainly costs more than
that. As I already stated in earlier version, we need to find more of
these "50 ms and 100 ms there sites.

The functional fixes are required because perf fix is always *less
critical* than perf fix.

Please pay more attention to proper error rollback next time, that's
all I can say on that. It's not my fault that it is broken.

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Mimi Zohar]

On Sun, 2024-09-22 at 20:51 +0300, Jarkko Sakkinen wrote:
> On Sat Sep 21, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> > This patch set aims to fix:
> > https://bugzilla.kernel.org/show_bug.cgi?id=219229.
> > 
> > The baseline for the series is the v6.11 tag.
> > 
> > v4:
> > https://lore.kernel.org/linux-integrity/20240918203559.192605-1-jarkko@kernel.org/
> > v3:
> > https://lore.kernel.org/linux-integrity/20240917154444.702370-1-jarkko@kernel.org/
> > v2:
> > https://lore.kernel.org/linux-integrity/20240916110714.1396407-1-jarkko@kernel.org/
> > v1:
> > https://lore.kernel.org/linux-integrity/20240915180448.2030115-1-jarkko@kernel.org/
> > 
> > Jarkko Sakkinen (5):
> >   tpm: Return on tpm2_create_null_primary() failure
> >   tpm: Implement tpm2_load_null() rollback
> >   tpm: flush the null key only when /dev/tpm0 is accessed
> >   tpm: Allocate chip->auth in tpm2_start_auth_session()
> >   tpm: flush the auth session only when /dev/tpm0 is open
> > 
> >  drivers/char/tpm/tpm-chip.c       |  14 ++++
> >  drivers/char/tpm/tpm-dev-common.c |   8 +++
> >  drivers/char/tpm/tpm-interface.c  |  10 ++-
> >  drivers/char/tpm/tpm2-cmd.c       |   3 +
> >  drivers/char/tpm/tpm2-sessions.c  | 109 ++++++++++++++++++------------
> >  include/linux/tpm.h               |   2 +
> >  6 files changed, 102 insertions(+), 44 deletions(-)
> 
> 
> Roberto, James, speaking of digest cache. This patch set has no aim to
> fix those issues but I do believe that it should improve also that 
> feature.
> 
> If I don't get soon patch reviews for the patch set, I'll pick the 2nd
> best option: disable bus encryption on all architectures including x86
> and ARM64 (being by default on).
> 
> It's a force majeure situation. I know this would sort out the issue
> but I really cannot send these as a pull request with zero reviewe-by's.
> 
> I expect this to be closed by tomorrow.

Jarkko, sorry to be so late to this discussion.  The bus HMAC/encryption really
impacts IMA as well.  Even with this patch set, it's slow.  My preference would
be to disable bus encryption on all architectures until there is a boot/runtime
option allowing it to be disabled for IMA as discussed in the other thread.

In the other thread, I also mentioned that the Kconfig is incorrectly worded. 
The performance degradation is not limited to encryption, but the HMAC itself. 
Please change "Saying Y here adds some encryption overhead to all kernel to TPM
transactions." to "Saying Y here adds overhead to all kernel to TPM
transactions."

thanks,

Mimi

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Tue, 2024-10-01 at 14:10 -0400, Mimi Zohar wrote:
> On Sun, 2024-09-22 at 20:51 +0300, Jarkko Sakkinen wrote:
> > On Sat Sep 21, 2024 at 3:08 PM EEST, Jarkko Sakkinen wrote:
> > > This patch set aims to fix:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=219229.
> > > 
> > > The baseline for the series is the v6.11 tag.
> > > 
> > > v4:
> > > https://lore.kernel.org/linux-integrity/20240918203559.192605-1-jarkko@kernel.org/
> > > v3:
> > > https://lore.kernel.org/linux-integrity/20240917154444.702370-1-jarkko@kernel.org/
> > > v2:
> > > https://lore.kernel.org/linux-integrity/20240916110714.1396407-1-jarkko@kernel.org/
> > > v1:
> > > https://lore.kernel.org/linux-integrity/20240915180448.2030115-1-jarkko@kernel.org/
> > > 
> > > Jarkko Sakkinen (5):
> > >   tpm: Return on tpm2_create_null_primary() failure
> > >   tpm: Implement tpm2_load_null() rollback
> > >   tpm: flush the null key only when /dev/tpm0 is accessed
> > >   tpm: Allocate chip->auth in tpm2_start_auth_session()
> > >   tpm: flush the auth session only when /dev/tpm0 is open
> > > 
> > >  drivers/char/tpm/tpm-chip.c       |  14 ++++
> > >  drivers/char/tpm/tpm-dev-common.c |   8 +++
> > >  drivers/char/tpm/tpm-interface.c  |  10 ++-
> > >  drivers/char/tpm/tpm2-cmd.c       |   3 +
> > >  drivers/char/tpm/tpm2-sessions.c  | 109 ++++++++++++++++++------
> > > ------
> > >  include/linux/tpm.h               |   2 +
> > >  6 files changed, 102 insertions(+), 44 deletions(-)
> > 
> > 
> > Roberto, James, speaking of digest cache. This patch set has no aim
> > to
> > fix those issues but I do believe that it should improve also that 
> > feature.
> > 
> > If I don't get soon patch reviews for the patch set, I'll pick the
> > 2nd
> > best option: disable bus encryption on all architectures including
> > x86
> > and ARM64 (being by default on).
> > 
> > It's a force majeure situation. I know this would sort out the
> > issue
> > but I really cannot send these as a pull request with zero reviewe-
> > by's.
> > 
> > I expect this to be closed by tomorrow.
> 
> Jarkko, sorry to be so late to this discussion.  The bus
> HMAC/encryption really
> impacts IMA as well.  Even with this patch set, it's slow.  My
> preference would
> be to disable bus encryption on all architectures until there is a
> boot/runtime
> option allowing it to be disabled for IMA as discussed in the other
> thread.

No worries, I was getting nervous because of a job switch, now
I have time since cannot move this forward for week or two anyway
:-)

I'm totally +1 to make bus encyption opt-in instead of opt-out.
It's just not there yet.

My fixes fix one use case, i.e. the boot process for AMD, so in
that sense they are totally legit. But it is pretty clear by now
that tons of similar patches and small tweaks will be required.

As it is my 2nd work week, I can implement such patch, *next
week*. Up until that there is time to give any feedback.

> 
> In the other thread, I also mentioned that the Kconfig is incorrectly
> worded. 
> The performance degradation is not limited to encryption, but the
> HMAC itself. 
> Please change "Saying Y here adds some encryption overhead to all
> kernel to TPM
> transactions." to "Saying Y here adds overhead to all kernel to TPM
> transactions."

I'll keep this in mind!

I'd prefer to do a single patch set probably with my previous fixes
and this, as they are already tested by the reporter anyway and pile
this as a new patch on top. I.e. have basically all that I'd put to
the next PR.

> 
> thanks,
> 
> Mimi

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Stefan Berger]

On 9/21/24 8:08 AM, Jarkko Sakkinen wrote:
> This patch set aims to fix:
> https://bugzilla.kernel.org/show_bug.cgi?id=219229.
> 
> The baseline for the series is the v6.11 tag.

I was testing this with 6.12-rc1 on ppc64/kvm + vtpm boot time from 
pressing return on grub until login prompt appears while using an IMA 
measure policy:

with HMAC2: 36s
with HMAC2+this series: 29s
without HMAC2: 28s

Looks good to me, though using a hardware TPM would probably be more 
critical in this type of measurement.

> 
> v4:
> https://lore.kernel.org/linux-integrity/20240918203559.192605-1-jarkko@kernel.org/
> v3:
> https://lore.kernel.org/linux-integrity/20240917154444.702370-1-jarkko@kernel.org/
> v2:
> https://lore.kernel.org/linux-integrity/20240916110714.1396407-1-jarkko@kernel.org/
> v1:
> https://lore.kernel.org/linux-integrity/20240915180448.2030115-1-jarkko@kernel.org/
> 
> Jarkko Sakkinen (5):
>    tpm: Return on tpm2_create_null_primary() failure
>    tpm: Implement tpm2_load_null() rollback
>    tpm: flush the null key only when /dev/tpm0 is accessed
>    tpm: Allocate chip->auth in tpm2_start_auth_session()
>    tpm: flush the auth session only when /dev/tpm0 is open
> 
>   drivers/char/tpm/tpm-chip.c       |  14 ++++
>   drivers/char/tpm/tpm-dev-common.c |   8 +++
>   drivers/char/tpm/tpm-interface.c  |  10 ++-
>   drivers/char/tpm/tpm2-cmd.c       |   3 +
>   drivers/char/tpm/tpm2-sessions.c  | 109 ++++++++++++++++++------------
>   include/linux/tpm.h               |   2 +
>   6 files changed, 102 insertions(+), 44 deletions(-)
> 

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Thu, 2024-10-03 at 11:14 -0400, Stefan Berger wrote:
> 
> 
> On 9/21/24 8:08 AM, Jarkko Sakkinen wrote:
> > This patch set aims to fix:
> > https://bugzilla.kernel.org/show_bug.cgi?id=219229.
> > 
> > The baseline for the series is the v6.11 tag.
> 
> I was testing this with 6.12-rc1 on ppc64/kvm + vtpm boot time from 
> pressing return on grub until login prompt appears while using an IMA
> measure policy:
> 
> with HMAC2: 36s
> with HMAC2+this series: 29s
> without HMAC2: 28s
> 
> Looks good to me, though using a hardware TPM would probably be more 
> critical in this type of measurement.

Yep, exactly. QEMU fails here...

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> This patch set aims to fix:
> https://bugzilla.kernel.org/show_bug.cgi?id=219229.
> 
> The baseline for the series is the v6.11 tag.
> 
> v4:
> https://lore.kernel.org/linux-integrity/20240918203559.192605-1-jarkko@kernel.org/
> v3:
> https://lore.kernel.org/linux-integrity/20240917154444.702370-1-jarkko@kernel.org/
> v2:
> https://lore.kernel.org/linux-integrity/20240916110714.1396407-1-jarkko@kernel.org/
> v1:
> https://lore.kernel.org/linux-integrity/20240915180448.2030115-1-jarkko@kernel.org/
> 
> Jarkko Sakkinen (5):
>   tpm: Return on tpm2_create_null_primary() failure
>   tpm: Implement tpm2_load_null() rollback
>   tpm: flush the null key only when /dev/tpm0 is accessed
>   tpm: Allocate chip->auth in tpm2_start_auth_session()
>   tpm: flush the auth session only when /dev/tpm0 is open
> 
>  drivers/char/tpm/tpm-chip.c       |  14 ++++
>  drivers/char/tpm/tpm-dev-common.c |   8 +++
>  drivers/char/tpm/tpm-interface.c  |  10 ++-
>  drivers/char/tpm/tpm2-cmd.c       |   3 +
>  drivers/char/tpm/tpm2-sessions.c  | 109 ++++++++++++++++++----------
> --
>  include/linux/tpm.h               |   2 +
>  6 files changed, 102 insertions(+), 44 deletions(-)

The summarize some discussions:

1. I'll address Stefan's remarks.
2. We know that these patches address the desktop boot.
3. IMA is too slow => add a boot option for IMA default off. I.e.
   IMA will not use the feature unless you specifically ask.
4. Random generation can be optimized a lot with or without
   encryption. Not sure if  I have time to do ths right now
   but I have already patch planned for this.

What is blocking me is the James' request to not include
functional fixes. The problem with that is that if comply
to that request I will have to postpone all the performacne
fixes and send a patch set with only functional fixes and
go all review rounds with that before moving forward.

This is just how priorities go in kernel and doing by the
book. Is that really necessary?

Since I've just started in a new job any patches can be
expected earliest next week. That's why I was rushing with
the patch set in the first place because I knew that there
will be otherwise a few week delay but we'll get there :-)

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Roberto Sassu]

On Fri, 2024-10-11 at 17:06 +0300, Jarkko Sakkinen wrote:
> On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote:
> > This patch set aims to fix:
> > https://bugzilla.kernel.org/show_bug.cgi?id=219229.
> > 
> > The baseline for the series is the v6.11 tag.
> > 
> > v4:
> > https://lore.kernel.org/linux-integrity/20240918203559.192605-1-jarkko@kernel.org/
> > v3:
> > https://lore.kernel.org/linux-integrity/20240917154444.702370-1-jarkko@kernel.org/
> > v2:
> > https://lore.kernel.org/linux-integrity/20240916110714.1396407-1-jarkko@kernel.org/
> > v1:
> > https://lore.kernel.org/linux-integrity/20240915180448.2030115-1-jarkko@kernel.org/
> > 
> > Jarkko Sakkinen (5):
> >   tpm: Return on tpm2_create_null_primary() failure
> >   tpm: Implement tpm2_load_null() rollback
> >   tpm: flush the null key only when /dev/tpm0 is accessed
> >   tpm: Allocate chip->auth in tpm2_start_auth_session()
> >   tpm: flush the auth session only when /dev/tpm0 is open
> > 
> >  drivers/char/tpm/tpm-chip.c       |  14 ++++
> >  drivers/char/tpm/tpm-dev-common.c |   8 +++
> >  drivers/char/tpm/tpm-interface.c  |  10 ++-
> >  drivers/char/tpm/tpm2-cmd.c       |   3 +
> >  drivers/char/tpm/tpm2-sessions.c  | 109 ++++++++++++++++++----------
> > --
> >  include/linux/tpm.h               |   2 +
> >  6 files changed, 102 insertions(+), 44 deletions(-)
> 
> The summarize some discussions:
> 
> 1. I'll address Stefan's remarks.
> 2. We know that these patches address the desktop boot.
> 3. IMA is too slow => add a boot option for IMA default off. I.e.
>    IMA will not use the feature unless you specifically ask.

Initially, I thought that maybe it would not be good to have an event
log with unmodified and altered measurement entries. Then, I tried to
think if we can really prevent an active interposer from injecting
arbitrary PCR extends and pretending that those events actually
happened.

If I understood James's cover letter correctly, the kernel can detect
whether a TPM reset occurred, but not that a PCR extend occurred (maybe
with a shadow PCR?).

Second point, do we really want to take the responsibility to disable
the protection on behalf of users? Maybe a better choice is to let them
consciously disable HMAC protection.

So, maybe we should keep the HMAC protection enabled, and if the number
of PCR extends is above a certain threshold, we can print a warning
message in the kernel log.

Roberto

> 4. Random generation can be optimized a lot with or without
>    encryption. Not sure if  I have time to do ths right now
>    but I have already patch planned for this.
> 
> What is blocking me is the James' request to not include
> functional fixes. The problem with that is that if comply
> to that request I will have to postpone all the performacne
> fixes and send a patch set with only functional fixes and
> go all review rounds with that before moving forward.
> 
> This is just how priorities go in kernel and doing by the
> book. Is that really necessary?
> 
> Since I've just started in a new job any patches can be
> expected earliest next week. That's why I was rushing with
> the patch set in the first place because I knew that there
> will be otherwise a few week delay but we'll get there :-)
> 
> BR, Jarkko
> 

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Fri, 2024-10-11 at 18:10 +0200, Roberto Sassu wrote:
> Initially, I thought that maybe it would not be good to have an event
> log with unmodified and altered measurement entries. Then, I tried to
> think if we can really prevent an active interposer from injecting
> arbitrary PCR extends and pretending that those events actually
> happened.
> 
> If I understood James's cover letter correctly, the kernel can detect
> whether a TPM reset occurred, but not that a PCR extend occurred
> (maybe
> with a shadow PCR?).

We can detect TPM reset indirectly. I.e. null seed re-randomizes
per reset.

> 
> Second point, do we really want to take the responsibility to disable
> the protection on behalf of users? Maybe a better choice is to let
> them
> consciously disable HMAC protection.

So when IMA is not used already with these fixes we get good
results. And for tpm2_get_random() we can make the algorithm
smarter. All in all we have good path ongoing for "desktop
use case" that I would keep thing way there are or at least
postpone any major decisions just a bit.

For server/IMA use case I'll add a boot parameter it can be
either on or off by default, I will state that in the commit
message and we'll go from there.

> 
> So, maybe we should keep the HMAC protection enabled, and if the
> number
> of PCR extends is above a certain threshold, we can print a warning
> message in the kernel log.
> 
> Roberto

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Fri, 2024-10-11 at 19:25 +0300, Jarkko Sakkinen wrote:
> On Fri, 2024-10-11 at 18:10 +0200, Roberto Sassu wrote:
> > Initially, I thought that maybe it would not be good to have an
> > event
> > log with unmodified and altered measurement entries. Then, I tried
> > to
> > think if we can really prevent an active interposer from injecting
> > arbitrary PCR extends and pretending that those events actually
> > happened.
> > 
> > If I understood James's cover letter correctly, the kernel can
> > detect
> > whether a TPM reset occurred, but not that a PCR extend occurred
> > (maybe
> > with a shadow PCR?).
> 
> We can detect TPM reset indirectly. I.e. null seed re-randomizes
> per reset.
> 
> > 
> > Second point, do we really want to take the responsibility to
> > disable
> > the protection on behalf of users? Maybe a better choice is to let
> > them
> > consciously disable HMAC protection.
> 
> So when IMA is not used already with these fixes we get good
> results. And for tpm2_get_random() we can make the algorithm
> smarter. All in all we have good path ongoing for "desktop
> use case" that I would keep thing way there are or at least
> postpone any major decisions just a bit.
> 
> For server/IMA use case I'll add a boot parameter it can be
> either on or off by default, I will state that in the commit
> message and we'll go from there.

Up until legit fixes are place distributors can easily disable
the feature. It would be worse if TCG_TPM2_HMAC did not exist.

So I think it is better to focus on doing right things right,
since the feature itself is useful objectively, and make sure
that those fixes bring the wanted results.

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Mimi Zohar]

On Sat, 2024-10-12 at 13:56 +0300, Jarkko Sakkinen wrote:
> On Fri, 2024-10-11 at 19:25 +0300, Jarkko Sakkinen wrote:
> > On Fri, 2024-10-11 at 18:10 +0200, Roberto Sassu wrote:
> > > Initially, I thought that maybe it would not be good to have an
> > > event
> > > log with unmodified and altered measurement entries. Then, I tried
> > > to
> > > think if we can really prevent an active interposer from injecting
> > > arbitrary PCR extends and pretending that those events actually
> > > happened.
> > > 
> > > If I understood James's cover letter correctly, the kernel can
> > > detect
> > > whether a TPM reset occurred, but not that a PCR extend occurred
> > > (maybe
> > > with a shadow PCR?).
> > 
> > We can detect TPM reset indirectly. I.e. null seed re-randomizes
> > per reset.
> > 
> > > 
> > > Second point, do we really want to take the responsibility to
> > > disable
> > > the protection on behalf of users? Maybe a better choice is to let
> > > them
> > > consciously disable HMAC protection.
> > 
> > So when IMA is not used already with these fixes we get good
> > results. And for tpm2_get_random() we can make the algorithm
> > smarter. All in all we have good path ongoing for "desktop
> > use case" that I would keep thing way there are or at least
> > postpone any major decisions just a bit.
> > 
> > For server/IMA use case I'll add a boot parameter it can be
> > either on or off by default, I will state that in the commit
> > message and we'll go from there.

Sounds good.

> 
> Up until legit fixes are place distributors can easily disable
> the feature. It would be worse if TCG_TPM2_HMAC did not exist.
> 
> So I think it is better to focus on doing right things right,
> since the feature itself is useful objectively, and make sure
> that those fixes bring the wanted results.

Are you backtracking on having a boot parameter here specifically to turn on/off
HMAC encryption for IMA?

Mimi

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Mon, 2024-10-14 at 07:45 -0400, Mimi Zohar wrote:
> > > For server/IMA use case I'll add a boot parameter it can be
> > > either on or off by default, I will state that in the commit
> > > message and we'll go from there.
> 
> Sounds good.

But only after this patch set lands. I gave this a thought and since
this patch set is specifically for a specific Bugzilla bug that it
closes, I have no interest to increase its scope.

> 
> > 
> > Up until legit fixes are place distributors can easily disable
> > the feature. It would be worse if TCG_TPM2_HMAC did not exist.
> > 
> > So I think it is better to focus on doing right things right,
> > since the feature itself is useful objectively, and make sure
> > that those fixes bring the wanted results.
> 
> Are you backtracking on having a boot parameter here specifically to
> turn on/off
> HMAC encryption for IMA?

I'm not really sure yet but obviously any change goes through review.

Also fastest route is to send your own RFC's to IMA specific issue.
For me it will take some time (post this patch set).

> 
> Mimi
> 
> 

BR, Jarkko

BR, Jarkko

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Mimi Zohar]

On Mon, 2024-10-14 at 15:34 +0300, Jarkko Sakkinen wrote:
> On Mon, 2024-10-14 at 07:45 -0400, Mimi Zohar wrote:
> > > > For server/IMA use case I'll add a boot parameter it can be
> > > > either on or off by default, I will state that in the commit
> > > > message and we'll go from there.
> > 
> > Sounds good.
> 
> But only after this patch set lands. I gave this a thought and since
> this patch set is specifically for a specific Bugzilla bug that it
> closes, I have no interest to increase its scope.

Prior to your performance improvement patch set it took >10 minutes to boot,
when it succeeded booting.  Now on Fedora 40 with "ima_policy=tcb" on the boot
command line, it's taking ~3 minutes to boot.  Do you really think that is
acceptable?!

> > 
> > > 
> > > Up until legit fixes are place distributors can easily disable
> > > the feature. It would be worse if TCG_TPM2_HMAC did not exist.
> > > 
> > > So I think it is better to focus on doing right things right,
> > > since the feature itself is useful objectively, and make sure
> > > that those fixes bring the wanted results.

The right thing would have been to listen to my concerns when this was initially
being discussed.  The right thing wasn't enabling TCG_TPM2_HMAC by default.

> > 
> > Are you backtracking on having a boot parameter here specifically to
> > turn on/off
> > HMAC encryption for IMA?
> 
> I'm not really sure yet but obviously any change goes through review.
> 
> Also fastest route is to send your own RFC's to IMA specific issue.
> For me it will take some time (post this patch set).

Done.  The patch applies cleanly with/without the TPM performance improvement
patch set.
https://lore.kernel.org/linux-integrity/20241015193916.59964-1-zohar@linux.ibm.com/

Mimi

Re: [PATCH v5 0/5] Lazy flush for the auth session

[Jarkko Sakkinen]

On Tue Oct 15, 2024 at 11:08 PM EEST, Mimi Zohar wrote:
> > > > since the feature itself is useful objectively, and make sure
> > > > that those fixes bring the wanted results.
>
> The right thing would have been to listen to my concerns when this was initially
> being discussed.  The right thing wasn't enabling TCG_TPM2_HMAC by default.

This is debatable as for laptops and desktops having hard drive
encryption do benefit with this. If systemd hadn't added
systemd-cryptenroll I would agree with this. I learned about this
feature two years after its inception in that project, so we needed to
address this as a priority (I did not and will not follow systemd
development proactively, as don't have time for that).

I feel more safe using my laptop with the feature in place at least.

Besides, it is complicated feature enough that it would have been
impossible ever "zero glitch" land it. I don't think there is any
rigid "data centers first" rule really, except maybe for those
businesses that run data centers (and I'm not one of those
businesses).

BR, Jarkko