Lazy flush for the auth session

tpm2_sessions_init() does not ignores the result of

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

Cc [email protected] # v6.10+

Fixes d2add27cf2b8 ("tpm: Add NULL primary creation")

Signed-off-by Jarkko Sakkinen <[email protected]>

---
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(-)

drivers/char/tpm/tpm2-sessions.c +0 -0

index d3521aadd43e..0f09ac33ae99 100644

drivers/char/tpm/tpm2-sessions.c +1 -1

  }
 
  /**

@@ -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)

Quoted 1 line · Jarkko Sakkinen

tpm2_sessions_init() does not ignores the result of

s/ignores/ignore

Quoted 3 lines

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.

Quoted 32 lines

Cc [email protected] # v6.10+

Fixes d2add27cf2b8 ("tpm: Add NULL primary creation")

Signed-off-by Jarkko Sakkinen <[email protected]>

---
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(-)

drivers/char/tpm/tpm2-sessions.c +0 -0

index d3521aadd43e..0f09ac33ae99 100644
@@ -1338,7 +1338,8 @@ static int tpm2_create_null_primary(struct
 tpm_chip *chip)
   		tpm2_flush_context(chip, null_key);
   	}

drivers/char/tpm/tpm2-sessions.c static int tpm2_create_null_primary(struct tpm_chip *chip) · line 1342

Quoted 3 lines

drivers/char/tpm/tpm2-sessions.c +2 -1

-	return rc;
+	/* Map all errors to -ENODEV: */
+	return rc ? -ENODEV : rc;

return rc ? -ENODEV : 0;

Quoted 10 lines

  }

  /**

drivers/char/tpm/tpm2-sessions.c +0 -0

@@ -1354,7 +1355,7 @@ int tpm2_sessions_init(struct tpm_chip *chip)

   rc = tpm2_create_null_primary(chip);
   if (rc)

drivers/char/tpm/tpm2-sessions.c +0 -1

-		dev_err(&chip->dev, "TPM: security failed (NULL

seed derivation): %d\n", rc);

drivers/char/tpm/tpm2-sessions.c +1 -0

+		return rc;

Quoted 3 lines

   chip->auth = kmalloc(sizeof(*chip->auth), GFP_KERNEL); if (!chip->auth)

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 [email protected] # v6.10+

Fixes eb24c9788cd9 ("tpm: disable the TPM if NULL name changes")

Signed-off-by Jarkko Sakkinen <[email protected]>

---
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(-)

drivers/char/tpm/tpm2-sessions.c +10 -18

index 0f09ac33ae99..a856adef18d3 100644
@@ -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)

drivers/char/tpm/tpm2-sessions.c +0 -0

  }
 
  /**

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 [email protected] # v6.10+

Fixes 699e3efd6c64 ("tpm: Add HMAC session start and end functions")

Signed-off-by Jarkko Sakkinen <[email protected]>

---
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(-)

drivers/char/tpm/tpm2-sessions.c +0 -0

index 1aef5b1f9c90..a8d3d5d52178 100644
@@ -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;

drivers/char/tpm/tpm2-sessions.c +1 -1

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

drivers/char/tpm/tpm2-sessions.c +0 -0

@@ -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-

Quoted 1 line

salt);

  out:
        crypto_free_kpp(kpp);

drivers/char/tpm/tpm2-sessions.c +0 -0

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

drivers/char/tpm/tpm2-sessions.c +0 -0

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

drivers/char/tpm/tpm2-sessions.c +2 -2

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

drivers/char/tpm/tpm2-sessions.c +0 -0

@@ -1000,7 +1008,7 @@ int tpm2_start_auth_session(struct tpm_chip
 *chip)

        tpm_buf_append(&buf, auth->our_nonce, sizeof(auth-

Quoted 1 line

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

drivers/char/tpm/tpm2-sessions.c +0 -0

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

drivers/char/tpm/tpm2-sessions.c +0 -0

@@ -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 */

Quoted 30 lines · 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 [email protected] # v6.10+

Fixes 699e3efd6c64 ("tpm: Add HMAC session start and end functions")

Signed-off-by Jarkko Sakkinen <[email protected]>

---
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(-)

drivers/char/tpm/tpm2-sessions.c +0 -0

index 1aef5b1f9c90..a8d3d5d52178 100644
@@ -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);
  }

Quoted 6 lines

-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

Quoted 120 lines

 {
        struct crypto_kpp *kpp;
        struct kpp_request *req;

drivers/char/tpm/tpm2-sessions.c +1 -1

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

drivers/char/tpm/tpm2-sessions.c +0 -0

@@ -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-

Quoted 1 line

salt);

  out:
        crypto_free_kpp(kpp);

drivers/char/tpm/tpm2-sessions.c +0 -0

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

drivers/char/tpm/tpm2-sessions.c +0 -0

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

drivers/char/tpm/tpm2-sessions.c +2 -2

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

drivers/char/tpm/tpm2-sessions.c +0 -0

@@ -1000,7 +1008,7 @@ int tpm2_start_auth_session(struct tpm_chip
 *chip)

        tpm_buf_append(&buf, auth->our_nonce, sizeof(auth-

Quoted 1 line

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

drivers/char/tpm/tpm2-sessions.c +0 -0

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

drivers/char/tpm/tpm2-sessions.c +0 -0

@@ -1371,10 +1382,6 @@ int tpm2_sessions_init(struct tpm_chip *chip)
         if (rc)
                 return rc;

Quoted 8 lines

-       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 <[email protected]>

Quoted 30 lines · 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 [email protected] # v6.10+

Fixes 699e3efd6c64 ("tpm: Add HMAC session start and end functions")

Signed-off-by Jarkko Sakkinen <[email protected]>

---
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(-)

drivers/char/tpm/tpm2-sessions.c +0 -0

index 1aef5b1f9c90..a8d3d5d52178 100644
@@ -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);
  }

Quoted 6 lines

-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

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 <[email protected]>

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

Cc [email protected] # v6.10+

Fixes 7ca110f2679b ("tpm: Address !chip->auth in

tpm_buf_append_hmac_session*()")

Tested-by Pengyu Ma <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

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

drivers/char/tpm/tpm- renamed from drivers/char/tpm/tpm-chip.c +0 -0

chip.c
index 0ea00e32f575..7a6bb30d1f32 100644

drivers/char/tpm/tpm-chip.c +1 -0

@@ -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;
                 }

drivers/char/tpm/tpm-dev-common.c +0 -0

index 4eaa8e05c291..a3ed7a99a394 100644
@@ -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;
        }

drivers/char/tpm/tpm- renamed from drivers/char/tpm/tpm-interface.c +0 -0

interface.c
index bfa47d48b0f2..2363018fa8fb 100644

drivers/char/tpm/tpm-interface.c +1 -0

@@ -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

drivers/char/tpm/tpm2-sessions.c +0 -0

index a8d3d5d52178..38b92ad9e75f 100644
@@ -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

Quoted 4 lines · 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.

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>

Quoted 78 lines

Reported-by Pengyu Ma <[email protected]>

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

Cc [email protected] # v6.10+

Fixes 7ca110f2679b ("tpm: Address !chip->auth in

tpm_buf_append_hmac_session*()")

Tested-by Pengyu Ma <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

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

drivers/char/tpm/tpm- renamed from drivers/char/tpm/tpm-chip.c +0 -0

chip.c
index 0ea00e32f575..7a6bb30d1f32 100644

drivers/char/tpm/tpm-chip.c +1 -0

@@ -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;
                 }

drivers/char/tpm/tpm-dev-common.c +0 -0

index 4eaa8e05c291..a3ed7a99a394 100644
@@ -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;
        }

drivers/char/tpm/tpm- renamed from drivers/char/tpm/tpm-interface.c +0 -0

interface.c
index bfa47d48b0f2..2363018fa8fb 100644

drivers/char/tpm/tpm-interface.c +1 -0

@@ -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

drivers/char/tpm/tpm2-sessions.c +0 -0

index a8d3d5d52178..38b92ad9e75f 100644
@@ -333,6 +333,9 @@ void tpm_buf_append_hmac_session(struct tpm_chip
 *chip, struct tpm_buf *buf,
         }

Quoted 8 lines

 #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 <[email protected]>

Quoted 4 lines · 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.

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.

Quoted 16 lines · James Bottomley

Quoted 5 lines · 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.

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

Quoted 10 lines

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.

Quoted 28 lines · Jarkko Sakkinen

Quoted 16 lines · James Bottomley

Quoted 5 lines · 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.

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

Quoted 10 lines

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.

Quoted 6 lines

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.

Quoted 40 lines · James Bottomley

Quoted 29 lines · Jarkko Sakkinen

Quoted 16 lines · James Bottomley

Quoted 5 lines · 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.

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

Quoted 11 lines

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.

Quoted 6 lines

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:

Quoted 3 lines

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.

Quoted 41 lines · Jarkko Sakkinen

Quoted 40 lines · James Bottomley

Quoted 29 lines · Jarkko Sakkinen

Quoted 16 lines · James Bottomley

Quoted 5 lines · 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.

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

Quoted 11 lines

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.

Quoted 6 lines

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:

Quoted 7 lines

Quoted 3 lines

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.

Quoted 2 lines

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

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().

Quoted 6 lines · Jarkko Sakkinen

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.

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

Dear Jarkko,

Thank you for working on this and your patches.

Am 21.09.24 um 14:08 schrieb Jarkko Sakkinen:

Quoted 2 lines

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.

Quoted 11 lines · Paul Menzel

Dear Jarkko,

Thank you for working on this and your patches.

Am 21.09.24 um 14:08 schrieb Jarkko Sakkinen:

Quoted 2 lines

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.

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/[email protected]/
v3:
https://lore.kernel.org/linux-integrity/[email protected]/
v2:
https://lore.kernel.org/linux-integrity/[email protected]/
v1:
https://lore.kernel.org/linux-integrity/[email protected]/

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(-)

Quoted 21 lines · 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/[email protected]/
v3:
https://lore.kernel.org/linux-integrity/[email protected]/
v2:
https://lore.kernel.org/linux-integrity/[email protected]/
v1:
https://lore.kernel.org/linux-integrity/[email protected]/

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

Quoted 8 lines

 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.

Quoted 44 lines · Jarkko Sakkinen

Quoted 21 lines · 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/[email protected]/
v3:
https://lore.kernel.org/linux-integrity/[email protected]/
v2:
https://lore.kernel.org/linux-integrity/[email protected]/
v1:
https://lore.kernel.org/linux-integrity/[email protected]/

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

Quoted 8 lines

 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.

Quoted 1 line

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.

On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:

Quoted 8 lines

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 :-)

Quoted 1 line · Jarkko Sakkinen

On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:

[...]

Quoted 14 lines

Quoted 8 lines

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:

Quoted 8 lines

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.

Quoted 26 lines · James Bottomley

Quoted 1 line · Jarkko Sakkinen

On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:

[...]

Quoted 14 lines

Quoted 8 lines

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:

Quoted 8 lines

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?

Quoted 5 lines

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.

Quoted 35 lines · Jarkko Sakkinen

Quoted 26 lines · James Bottomley

Quoted 1 line · Jarkko Sakkinen

On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:

[...]

Quoted 14 lines

Quoted 8 lines

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:

Quoted 8 lines

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?

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 41 lines · Jarkko Sakkinen

Quoted 33 lines · Jarkko Sakkinen

Quoted 26 lines · James Bottomley

Quoted 1 line · Jarkko Sakkinen

On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:

[...]

Quoted 14 lines

Quoted 8 lines

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:

Quoted 8 lines

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?

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 21 lines · 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/[email protected]/
v3:
https://lore.kernel.org/linux-integrity/[email protected]/
v2:
https://lore.kernel.org/linux-integrity/[email protected]/
v1:
https://lore.kernel.org/linux-integrity/[email protected]/

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

Quoted 7 lines

 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.

Quoted 43 lines · Jarkko Sakkinen

Quoted 21 lines · 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/[email protected]/
v3:
https://lore.kernel.org/linux-integrity/[email protected]/
v2:
https://lore.kernel.org/linux-integrity/[email protected]/
v1:
https://lore.kernel.org/linux-integrity/[email protected]/

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

Quoted 8 lines

 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).

Quoted 6 lines

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

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/[email protected]/
v3:
https://lore.kernel.org/linux-integrity/[email protected]/
v2:
https://lore.kernel.org/linux-integrity/[email protected]/
v1:
https://lore.kernel.org/linux-integrity/[email protected]/

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(-)

Quoted 4 lines · 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.

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.

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/[email protected]/
v3:
https://lore.kernel.org/linux-integrity/[email protected]/
v2:
https://lore.kernel.org/linux-integrity/[email protected]/
v1:
https://lore.kernel.org/linux-integrity/[email protected]/

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(-)

Quoted 21 lines · 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/[email protected]/
v3:
https://lore.kernel.org/linux-integrity/[email protected]/
v2:
https://lore.kernel.org/linux-integrity/[email protected]/
v1:
https://lore.kernel.org/linux-integrity/[email protected]/

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

Quoted 8 lines

 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

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

Quoted 8 lines · Roberto Sassu

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.

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 31 lines · Jarkko Sakkinen

Quoted 8 lines · Roberto Sassu

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.

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 3 lines

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

Quoted 5 lines · Mimi Zohar

Quoted 3 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 8 lines

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.

Quoted 4 lines

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).

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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.